-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[WIP] apparmor_events table in osquery #4982
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
I accepted the CLA |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
ok to test |
You can run make format_master to fix code audit. Failing FreeBSD should be our problem |
Sorry for that, I'd run Should I merge two commits in one? |
Feel free to push as many commits as you want to. While merging they will be squashed into the single commit |
ok to test |
To pass FreeBSD rebase is needed on new master |
Did rebase |
Any chance to have it reviewed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of notes/questions, largely nits. Overall looks pretty strait forward to me, I'd be curious if someone with a bit more experience with SELinux/AppArmor could take a look, as my knowledge of these things is limited.
std::string apparmor_status; | ||
if (!GetStringFieldFromMap( | ||
apparmor_status, audit_event_record.fields, "apparmor")) { | ||
VLOG(1) << "Malformed AUDIT_APPARMOR record received. The " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Try to keep status logging such as this to one sentence to fit on one line of text, perhaps something to the effect of VLOG(1) << "AUDIT_APPARMOR record did not contain apparmor field.";
or the like?
} | ||
|
||
Status AppArmorEventSubscriber::Callback(const ECRef& ec, const SCRef& sc) { | ||
std::vector<Row> emitted_row_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A QueryData
is precisely this, let's just use that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits but I'm also concerned about the casing in this file and it seems it was started by @alessandrogario .
@alessandrogario is there a reason for this? A convention for audit I'm missing or something of that nature?
Hi @luc-lynx, I really like the idea of having this table in osquery. I rebased this table onto our current master to help get this going again. There's work that still remains, for example reviewing our new CLA, adding the new files to our CMake build, and iterating on a few more code reviews. I am happy to help along the way, let me know if you have time. |
Thanks for your message. I'll definitely find time to carry on work on the PR. |
Just a little update that I've started to work on that and will get back with some updates shortly. |
The Linux jobs fail due to required code-format changes, run: |
@theopolis Yes, I'm trying to run it but I'm getting a huge bunch of complains about the code I didn't touch... Do you happen to know how it can be scoped only to my changeset? |
@luc-lynx So this process would be:
As you can see formatting after the fact is a bit more annoying. |
@theopolis could you please review the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up @alessandrogario.
Thanks for the review, I'll get back with the updates in a week. |
@theopolis could you please take a look at my comments in the review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If builds pass this LGTM
I've fixed imports and spec (turned out it was also missing). Should be fine now. |
Changes description:
This PR adds a new
apparmor_events
table that contains events from AppArmor which is a Mandatory Access Control System. AppArmor is considered to be more user friendly and easy to audit though it's not as feature-rich as SELinux.Motivation for the PR:
selinux_events
table so the table gets messyselinux_events
table the table contains raw messages that aren't parsed and tokenised. There's no way to run queries on AppArmor events.selinux_events
table.Technical details:
apparmor=
substring as it's always added to AppArmor messagesmessage
column inapparmor_events
columnselinux_event
table anymore