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

Fixes #23031 - parse server duid's with nested double quotes #571

Merged
merged 1 commit into from
Apr 5, 2018
Merged

Fixes #23031 - parse server duid's with nested double quotes #571

merged 1 commit into from
Apr 5, 2018

Conversation

dmitri-d
Copy link
Member

No description provided.

@@ -209,10 +209,11 @@ def server_duid
Rsec::Fail.reset
keyword = word('server-duid').fail 'keyword_server_duid'
anything = /[^;,{}\s]+/.r
literal = /"([^"]|\")*"/.r
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like ISC configuration parser treats all strings like that, isn't cleaner to use this in LITERAL constant?

https://github.com/42wim/isc-dhcp/blob/f54a146c7fe88889d60f0c1aa8e6f04707f95223/common/conflex.c#L512

@lzap
Copy link
Member

lzap commented Apr 4, 2018

If it won't break our tests, I think we should go with:

LITERAL = /"([^"]|\")*"/.r

Single quotes do not appear to be supported by the parser, if I am looking into the correct place in the codebase. https://lists.isc.org/pipermail/dhcp-users/2011-February/013135.html

@dmitri-d
Copy link
Member Author

dmitri-d commented Apr 4, 2018

I think the ultimate goal should be parsing strings the same way as dhcpd's parser does. Unfortunately, I don't think it is doable using a non-monstrous regex (if at all). The reason I didn't reuse the regex the way you are suggesting is that it will also match comma-separated strings as one string, which breaks the parser.

DUID is an outlier, as majority of the strings are host or option names and values, I think we are pretty safe with the fix in the PR. I'll add extending rsec parser with a custom string parser as a feature to smart-proxy's backlog.

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's good. Thanks.

@lzap lzap merged commit aaaa773 into theforeman:develop Apr 5, 2018
@mark-wagner
Copy link

I think a similar change will be needed for the uid within a lease.

@lzap
Copy link
Member

lzap commented Apr 25, 2018

I also think doing this for all strings won't hurt, the ISC parser does seem to work that way.

@alvinstarr
Copy link

I believe that literal should be literal = /"([^"]|\\")*"/.r
The patch you suggested is in the version of 1.17 that I have installed but I was still getting duid errors until I added the second .
I am really unfamiliar with ruby so I may well be wrong.

@mark-wagner
Copy link

@alvinstarr that regexp doesn't work for. Here is the uid that was tripping me up:

uid "\001X\357h}g\""

http://rubular.com/r/gqxRXXs1YZ

The regexp in https://stackoverflow.com/questions/249791/regex-for-quoted-string-with-escaping-quotes seems to work but may have other problems.

http://rubular.com/r/vRvdrzFYxe

@dmitri-d
Copy link
Member Author

dmitri-d commented Apr 27, 2018

Please see my comment above. Feel free to experiment, but please run tests before proposing a fix. Please also note complicated regular expressions will be rejected.

@alvinstarr
Copy link

This may be better
"([^\\"]|\\.)*"
I believe the string consists of:
char - a single character
### - 3 digits 0-7 for octal characters
\char - an escaped character
This of course only works for ASCII characters if my memories of C strings is correct.

@dmitri-d
Copy link
Member Author

I am locking this conversation. If you are seeing this issue, please update the redmine ticket referenced in this PR, or create a new one.

@theforeman theforeman locked as resolved and limited conversation to collaborators Apr 27, 2018
@lzap
Copy link
Member

lzap commented May 2, 2018

I am not sure if locking was necessary here, "this might be better" is appropriate comment although I do agree some of them can be useless when not properly tried and tested. However, I think leaving those comments "as-is" is probably preferred than locking discussion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants