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 parsing issue when a backslash as the last character on sudoers file line #7440

Merged
merged 5 commits into from Feb 15, 2022

Conversation

iko1
Copy link
Contributor

@iko1 iko1 commented Jan 8, 2022

This PR fixes the bug reported at #7421. I handle the case that a line contains a backslash as last character.
The code aggregates multiple lines within a "rule_details" column. The aggregation logic starts when a line ends with backslash and end when EOF or the next line doesn't end with a backlash.
The code deletes the backslash character in favor of clearness.
Any feedback is more than welcome, Thanks!

@iko1 iko1 requested review from a team as code owners January 8, 2022 17:30
@iko1 iko1 force-pushed the fix/sudoers-parser-long-lines branch from 1b14fb6 to 2fb0ab1 Compare January 8, 2022 17:33
@iko1 iko1 requested a review from Smjert January 12, 2022 09:31
@directionless directionless added this to the 5.3.0 milestone Jan 18, 2022
@mike-myers-tob mike-myers-tob added the ready for review Pull requests that are ready to be reviewed by a maintainer label Jan 19, 2022
@mike-myers-tob
Copy link
Member

Looks good to me (I reviewed, but don't have the ability approve a PR)

mike-myers-tob
mike-myers-tob previously approved these changes Jan 22, 2022
Copy link
Member

@sharvilshah sharvilshah left a comment

Choose a reason for hiding this comment

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

Hey @iko1, thanks so much for this! Do you think we need to add/update the sudoers_test to handle this case?

@iko1
Copy link
Contributor Author

iko1 commented Jan 26, 2022

Hey @iko1, thanks so much for this! Do you think we need to add/update the sudoers_test to handle this case?

@sharvilshah, Indeed, the change should be covered in unit test. I'm thinking about adding the following tests:

  1. Trivial case when a long line exists in a line that doesn't start with '#'
  2. Line that starts with '#' and end with blackslash should be parsed as a single line.

If you have any other edge cases that should be covered, please elaborate them here

@sharvilshah
Copy link
Member

Hey @iko1, thanks so much for this! Do you think we need to add/update the sudoers_test to handle this case?

@sharvilshah, Indeed, the change should be covered in unit test. I'm thinking about adding the following tests:

  1. Trivial case when a long line exists in a line that doesn't start with '#'
  2. Line that starts with '#' and end with blackslash should be parsed as a single line.

If you have any other edge cases that should be covered, please elaborate them here

Those sound great, thanks!

Copy link
Member

@sharvilshah sharvilshah left a comment

Choose a reason for hiding this comment

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

Looks good, thanks

@Smjert Smjert merged commit 28769fa into osquery:master Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ready for review Pull requests that are ready to be reviewed by a maintainer virtual tables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants