Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix problematic parsing of keyspec #246

Merged
merged 1 commit into from
Sep 6, 2019
Merged

Conversation

EECOLOR
Copy link
Contributor

@EECOLOR EECOLOR commented Aug 22, 2019

The original had some flaws. The .* statement is an eager match and could eat spaces. An example:

options ssh-xyz key name with spaces would be matched as:

[
  "options ",
  "options",
  "ssh-xyz key name",
  "ssh",
  "with",
  "spaces"
]

The new regex would match it like this:

[
  "options ",
  "options",
  "ssh-xyz",
  "ssh",
  "key",
  "name with spaces"
]

Another example: ssh-xyz key ssh name with spaces

[
  "ssh-xyz key ",
  "ssh-xyz key",
  "ssh name",
  "ssh",
  "with",
  "spaces"
]

vs

[
  "",
  "",
  "ssh-xyz",
  "ssh",
  "key",
  "ssh name with spaces"
]

Workaround until a fix is released: carefully manage the spaces and do not use ssh or ecdsa-sha2 in unexpected places (certainly no spaces in the name).

Thanks to @ekoster for finding this.

The original had some flaws. The `.*` statement is an eager match and could eat spaces. An example:

`options ssh-xyz key name with spaces` would be matched as:

```
[
  "options ",
  "options",
  "ssh-xyz key name",
  "ssh",
  "with",
  "spaces"
]
```

The new regex would match it like this:
```
[
  "options ",
  "options",
  "ssh-xyz",
  "ssh",
  "key",
  "name with spaces"
]
```

Another example: `ssh-xyz key ssh name with spaces`
```
[
  "ssh-xyz key ",
  "ssh-xyz key",
  "ssh name",
  "ssh",
  "with",
  "spaces"
]
```
vs
```
[
  "",
  "",
  "ssh-xyz",
  "ssh",
  "key",
  "ssh name with spaces"
]
```

Workaround until a fix is released: carefully manage the spaces and do not use `ssh` or `ecdsa-sha2` in unexpected places (certainly no spaces in the name).

Thanks to @ekoster for finding this.
@florindragos
Copy link
Contributor

florindragos commented Aug 23, 2019

Hi @EECOLOR

There seems to be a problem when running on Puppet 6. Acceptance tests are passing on Puppet 5, but on Puppet 6 there are 3 failures.

Edit: looks like the master is also failing. Looking into it :-)

Failures:

  1) accounts invoke main tests creates groups of matching names, assigns non-matching group, manages homedir, manages other properties, gives key, makes dotfiles, managevim false
     On host `e8ea9d807155daeeb70a930853604cc9c590003730e0c0df5f2cb3f1ad9dd602'
     Failure/Error: expect(user('hunner').maximum_days_between_password_change).to match 60
       expected 99999 to match 60

     # ./spec/acceptance/accounts_spec.rb:320:in `block (3 levels) in <top (required)>'

  2) accounts invoke create duplicate users with same uid runs with no errors
     On host `e8ea9d807155daeeb70a930853604cc9c590003730e0c0df5f2cb3f1ad9dd602'
     Failure/Error: expect(user('duplicate_user1')).to exist
       expected User "duplicate_user1" to exist

     # ./spec/acceptance/accounts_spec.rb:477:in `block (3 levels) in <top (required)>'

  3) accounts::user define create duplicate users with same uid runs with no errors
     On host `e8ea9d807155daeeb70a930853604cc9c590003730e0c0df5f2cb3f1ad9dd602'
     Failure/Error: expect(user('duplicate_user1')).to exist
       expected User "duplicate_user1" to exist

     # ./spec/acceptance/user_spec.rb:356:in `block (3 levels) in <top (required)>'

@EECOLOR
Copy link
Contributor Author

EECOLOR commented Aug 23, 2019

The problem is related, the following would fail with both the old and proposed regular expression:

command="/bin/echo Hello",from="myhost.exapmle.com,192.168.1.1" ssh-rsa test_key vagrant2

https://github.com/puppetlabs/puppetlabs-accounts/blob/master/spec/acceptance/accounts_spec.rb#L49

@EECOLOR
Copy link
Contributor Author

EECOLOR commented Aug 23, 2019

If having a space inside the options should be supported there is no regular expression you could write to fix the problem.

A naive adaptation (^((.*?)\s+)?((ssh|ecdsa-sha2)[^\s]*)\s+([^\s]*)\s+(.*)$) would reveal it's problems with the following entry:

ssh-xyz key ssh name with spaces

Please note that the current version and the previous version (using split) have never supported spaces in options. The split version however did support having spaces in the name.

@florindragos
Copy link
Contributor

@EECOLOR I'm happy to merge this as soon as the build is passing. Unfortunately, there has been a regression introduced with the release of puppet 6.8.0 so we might have to wait a couple of days for the fix to be released.

@sheenaajay
Copy link
Contributor

@EECOLOR Thank you. Tests are running clean now.
Screen Shot 2019-09-06 at 14 39 31
Thanks for submitting the PR.

@sheenaajay sheenaajay merged commit a1e79bb into puppetlabs:master Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants