Page MenuHomeVyOS Platform

ConfigTree() throws ValueError() if tagNode contains whitespaces
Closed, ResolvedPublicBUG

Description

@itspngu reported an error via Slack while migrating a config from 1.3 LTS -> 1.4-rolling. The error is a general one which can also be triggered in 1.3 presumably.

[email protected]# load /config/config.boot2
Loading configuration from '/config/config.boot2'
Traceback (most recent call last):
  File "/opt/vyatta/etc/config-migrate/migrate/flow-accounting/0-to-1", line 34, in <module>
    config = ConfigTree(config_file)
  File "/usr/lib/python3/dist-packages/vyos/configtree.py", line 142, in __init__
    raise ValueError("Failed to parse config: {0}".format(msg))
ValueError: Failed to parse config: Syntax error on line 337, character 39: Invalid syntax.

Migration script error: /opt/vyatta/etc/config-migrate/migrate/flow-accounting/0-to-1: Command '['/opt/vyatta/etc/config-migrate/migrate/flow-accounting/0-to-1', '/tmp/tmpcs26v53e']' returned non-zero exit status 1..

To reproduce you need a configuration that is beeing migrated

login {
    user vyos{
        authentication {
            plaintext-password vyos
            public-keys "foo:1 2 3" {
                key AAAAB3NzaC1yc2EAAAADAQABAAAAgQCt+jpJ6JBNHQDTPkeTNN37m1BFrxQ6jlXfPPiRv4rGeVR/LZRxu6u6vz33z8jddBbnwoYtL+8xlbntfzZsBYXvHuj9kf4fk9aWuUacKaIibYaRPsGoOQhYciEqoT+pGiLGMpx+4xoQmevN7DEeJgwF9iY34nsMEF/s8qPiYiamWQ==
                type ssh-rsa
            }
        }
        full-name vyos
    }
}

The error is with the line public-keys "foo:1 2 3" that is allowed from the CLI (regex does not prohibit whitespaces, but it also triggers the exception in ConfigTree().

This error may happen on every tagNode instance (most of them prevent whitespaces - SSH does not).

Details

Difficulty level
Normal (likely a few hours)
Version
1.3.0
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Config syntax change (migratable)
Issue type
Bug (incorrect behavior)

Event Timeline

I see the issue. Whitespace is fine in a tag node name as long as the name is quoted, however ConfigTree.to_string() does not re-quote the name, hence on the next migration script, parsing the config file will throw an error. I will investigate the proper solution.

Note that a fix for 1.4 will address the user's issue, as he is updating to 1.4-rolling, so the migration will take place upon booting into 1.4.

jestabro changed the task status from Open to On hold.Aug 19 2022, 11:46 PM

This is on hold, pending discussion on whether whitespace should be allowed in tag node names in 1.4.

Note that a fix for 1.4 will address the user's issue, as he is updating to 1.4-rolling, so the migration will take place upon booting into 1.4.

I've been able to finish the upgrade by removing the spaces in the tag node name and re-running the add system image command. I thought spaces in key identifiers for keys used by gnupg's ssh-agent drop-in replacement were the norm, but it seems that's not the case - in fact, the card reports its serial number without spaces, and the spaces only start showing up in the output of ssh-add -L:

$ gpg --card-status
Reader ...........: Yubico YubiKey OTP FIDO CCID 00 00
Application ID ...: <XXX>
Application type .: OpenPGP
Version ..........: 3.4
Manufacturer .....: Yubico
Serial number ....: 12312312
Name of cardholder: <XXX>
Login data .......: <XXX>
Signature PIN ....: not forced
Key attributes ...: rsa4096 rsa4096 rsa4096
Max. PIN lengths .: 127 127 127
PIN retry counter : 3 0 3
Signature counter : 225
KDF setting ......: off
UIF setting ......: Sign=off Decrypt=off Auth=off
Signature key ....: <XXX>
      created ....: 2022-07-05 17:32:19
Encryption key....: <XXX>
      created ....: 2022-07-05 17:34:07
Authentication key: <XXX>
      created ....: 2022-07-05 17:35:09
General key info..: sub  rsa4096/<XXX> 2022-07-05 <XXX> <XXX>
sec#  rsa4096/<XXX>  created: 2022-07-05  expires: never
ssb>  rsa4096/<XXX>  created: 2022-07-05  expires: 2022-10-03
                                  card-no: 0006 12312312
ssb>  rsa4096/<XXX>  created: 2022-07-05  expires: 2022-10-03
                                  card-no: 0006 12312312
ssb>  rsa4096/<XXX>  created: 2022-07-05  expires: 2022-10-03
                                  card-no: 0006 12312312
$ ssh-add -L
ssh-rsa AAAAB3N...s27pJBdQ== cardno:12 312 312

The latter then ends up on target systems when using ssh-copy-id. I've not run into problems with it up to now so it does seem to be acceptable standards-wise, which is not necessarily related to the question of wether spaces should be allowed in VyOS config tag node names but could be a problem that pops up again in the future. If the decision is made to not allow spaces in tag names, it might be desirable to implement some glue code that removes spaces in key identifiers when reading them in authorized_keys format.

@itspngu you might try tomorrows rolling release and upgrade again. The issue should be resolved - it also helps us to see of the fix works!

@c-po @itspngu , as mentioned above, we have held off on implementing the fix, as there is a compelling argument to disallow whitespace in tag node names, just as it is disallowed in node names in general; making an exception in the case of tag node names invites problems going forward. On the other hand, thanks to the details that you provided, @itspngu, we can implement a workaround for the case of ssh-copy-id; I know of no other instance of the problem. If we do find a necessary use case of whitespace in tag node names in the future, the simple fix can then be implemented.

jestabro changed the task status from On hold to In progress.Aug 29 2022, 8:48 PM

@c-po suggested the very good idea of using a migration script to remove whitespace in this case, however, upon closer inspection, we realized that this cannot work, as a result of the binding between Python and config_tree: namely, the translation uses a split_on_whitespace function which will lose access to tag node value names containing whitespace, so the name is not manipulable through standard migration techniques. One can replace split_on_whitespace with smarter translations, however, a global solution is preferable: following discussion, the suggestion is to add a check in the legacy cstore backend to reject node names containing whitespace; the future vyconf will have this check built-in. This is the approach we will investigate.

Resolved in T4664: merged in Sagitta; backport candidate for Equuleus 1.3.3.