-
Notifications
You must be signed in to change notification settings - Fork 107
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
Adding first iteration on AppArmor profile generation to the recorder #1917
Conversation
|
Welcome @0xmilkmix! |
Hi @0xmilkmix. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1917 +/- ##
==========================================
- Coverage 47.32% 45.21% -2.11%
==========================================
Files 77 79 +2
Lines 7352 7714 +362
==========================================
+ Hits 3479 3488 +9
- Misses 3742 4090 +348
- Partials 131 136 +5 |
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.
@0xmilkmix Great work! I left a few comments.
I think we can improve a bit the code by reducing the duplication in the recorder.bpf.c
. Also in the bpfrecorder.go, I would use an interface to extract the parts for each recorder type instead of using an if recordingMode
everywhere. This would make the code a bit more clearer.
|
||
struct { | ||
__uint(type, BPF_MAP_TYPE_HASH); | ||
__uint(max_entries, 10240); |
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: why this max number?
It would be helpful to add some integration tests similar with this one https://github.com/kubernetes-sigs/security-profiles-operator/blob/main/test/tc_bpf_recorder_test.go. In this way, we can have some validation. |
I think you need to run |
@0xmilkmix may I ask you to rebase please? |
Thanks for the feedback. I'll try to work on them this week or next one. Best regards |
8a016c5
to
1003122
Compare
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.
Thanks for addressing the review comments.
@0xmilkmix I think there are some lint warnings which need to be fixed. |
Should be ok now, thanks. |
…ic recorders Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
… not setting it Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Sascha Grunert <sgrunert@redhat.com> Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
…vulnerability scaner
It seems that the image is not available for scanning after building.
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This is a PR adding features for AppArmor profiles generation to the recorder:
abstract
representation, allowing to generate simpler profiles (like Banespoc
CLI to generate AppArmor profiles (CRD and raw) from the command lineThis PR does not include the integration with the recorder controller for now. I wanted to first discussed the idea and first iteration with you. Would it be possible to have collaborations so that the code from this PR could then be integrated by you in the controller for in-cluster recording?
Also, note that using
open
/openat
tracing has some limitations. Namely, symlinks will not be resolved and this could lead to misconfigured AppArmor profiles. A LSM hook such assecurity_open
would be beneficial here.Other known limitations in the submitted version:
Sometime the recorded process could end and events would still be in the ring buffer, generating an incomplete profileDoes this PR have test?
N/A
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Yes