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

Add support of parsing authorized_keys file #7560

Merged
merged 13 commits into from
Aug 2, 2022

Conversation

iko1
Copy link
Contributor

@iko1 iko1 commented Apr 19, 2022

Currently, the authorized_keys table is populated with an empty column: "algorithm" and other key fields are added to the column "key" alongside the actual key.

I've taken a look at the file format to populate the table correctly:
The file format consists of keys on each line for public-key authentication, and the format is quite simple, unless few exceptions: line starts with '#' or the option zos-key-ring-label is specified.
The line syntax : (options) key-type base64-encoded key (comment)
Options field (optional) are comma-delimited. There are pre-set of options that can be specified. Spaces are not allowed unless within double quotes for example in the Option Command.
Key type field(required) is one of the following options "ssh-dss", "ssh-rsa", "ecdsa-sha2-nistp256", "ecdsa-sha2- nistp384", or "ecdsa-sha2-nistp521" the code consists that one of key type exists to parse the line when key type is unknown, the line is ignored.
Key field(required): base64-encoded key
Comment(optional): it's mainly used to recognize the key origin.

I've refactored the table schema according to the fields above, the parser code, and written some unit tests to check the code with some generic public-key definitions that I've found on the web.

Fixes #7544

@iko1 iko1 requested review from a team as code owners April 19, 2022 13:15
@iko1 iko1 changed the title Add Support of parsing authorized_keys file Add support of parsing authorized_keys file Apr 19, 2022
@mike-myers-tob
Copy link
Member

Can you explain what this changes or adds to the existing authorized_keys table? It looks like a couple of new columns, and adds tests that were missing before.

Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I'm finding it very hard to absorb how the loops and if blocks work together.

osquery/tables/system/posix/authorized_keys.cpp Outdated Show resolved Hide resolved
osquery/tables/system/posix/authorized_keys.cpp Outdated Show resolved Hide resolved
osquery/tables/system/posix/authorized_keys.cpp Outdated Show resolved Hide resolved
osquery/tables/system/posix/authorized_keys.cpp Outdated Show resolved Hide resolved
@iko1 iko1 requested a review from directionless May 5, 2022 17:54
@mike-myers-tob mike-myers-tob added ready for review Pull requests that are ready to be reviewed by a maintainer and removed needs response labels May 5, 2022
@iko1 iko1 force-pushed the fix/authorized-keys-table branch from bf5c07a to 39a9f52 Compare May 6, 2022 15:24
@directionless directionless added this to the 5.4.0 milestone May 24, 2022
@mike-myers-tob
Copy link
Member

@directionless is going to resolve conversations above and re-review/approve, but this is slipping to 5.5 milestone

@mike-myers-tob mike-myers-tob modified the milestones: 5.4.0, 5.5.0 Jul 5, 2022
@iko1 iko1 closed this Jul 29, 2022
@iko1 iko1 reopened this Jul 29, 2022
Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Code seems okay to me. Slightly hard to read. @Smjert ?

osquery/tables/system/posix/authorized_keys.cpp Outdated Show resolved Hide resolved
osquery/tables/system/posix/authorized_keys.cpp Outdated Show resolved Hide resolved
@mike-myers-tob mike-myers-tob changed the title Add support of parsing authorized_keys file Add support of parsing authorized_keys file Aug 1, 2022
@iko1
Copy link
Contributor Author

iko1 commented Aug 2, 2022

Code seems okay to me. Slightly hard to read. @Smjert ?

Would someone else like to take a look at this PR?
The code fixes the bug, but parsing the authorized_keys file could be a bit complicated.

Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

This generally seems like an improvement. And we can always extend. Let's do it

@mike-myers-tob mike-myers-tob merged commit be96cf0 into osquery:master Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linux macOS 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.

Column algorithm of table authorized_keys is empty
3 participants