sandbox/apparmor: add GenerateAAREExclusionPatterns #11567
Conversation
This function is generic (and complex) enough to be able to handle all of the overlapping and wildcard behavior we need in docker-support, and it could also serve to replace numerous other places in the codebase where we need this sort of complex behavior. It is a generalization of the existing aareExclusionPatterns helper, though it's actually unclear if this exact implementation will currently be able to serve the use case from that helper directly or if more options/adjustments are needed to enable that use case as well. To keep the diff smaller, this patch does not actually change any of the profiles/interfaces, just TODO's are left for where to use it. Note that the generated rules are slightly more condensed in terms of number of rules but significantly more verbose in terms of alternations, not sharing more of repeated substrings between alternations inside the patterns. This was done explicitly to keep the generating code simpler and easier to understand, but it may prove to have performance effects, either detrimental or benevolent but that should be measured before deciding to make the generation code even more complex than it already is. Signed-off-by: Ian Johnson <[email protected]>
…er func Signed-off-by: Ian Johnson <[email protected]>
anonymouse64
added
the
Needs security review
Codecov Report
@@ Coverage Diff @@
## master #11567 +/- ##
==========================================
+ Coverage 78.00% 78.03% +0.02%
==========================================
Files 939 939
Lines 109526 110129 +603
==========================================
+ Hits 85441 85938 +497
- Misses 18796 18869 +73
- Partials 5289 5322 +33
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Just WOW :-)
I wish the long function could be split into a couple of smaller ones, but I don't see an obvious way to do it (without having to pass a lot of parameters...).
There's a little (but dangerous!) bug which would break the pattern when you have capital letters or numbers, but other than that this looks great!
| @@ -129,6 +129,9 @@ | |||
| # LD_PRELOAD to be set to a library which is in a directory configured by | |||
| # ld.so.conf, but access to those locations is mediated by this profile | |||
| # (which requires rules for specific locations). | |||
| # TODO: use GenerateAAREExclusionPatterns for this, though the first | |||
There was a problem hiding this comment.
AFAIU this apparmor profile is not generated at run-time, so I don't think we can use GenerateAAREExclusionPatterns unless we somehow hook it up in the Makefile.
There was a problem hiding this comment.
Yes but it would still be nice to have some way to use the Go code which does this to generate it instead like perhaps as a go generate thing, hence the TODO
| if len(stillPresentPatterns) == 1 { | ||
| // there is only one group left from here and we can do the simple | ||
| // generation with this pattern for the rest of the rules |
There was a problem hiding this comment.
I think we should be able to call GenerateAAREExclusionPatternsSingle here, if we add the common exclude pattern computed so far to the opts.Prefix (of course, we need to pass a copy of the opts variable, not to modify our own one)
There was a problem hiding this comment.
hmm so the issue with trying to implement this is that the loop for generateAAREExclusionPatternsSingle starts at 1 to skip the "/" in the normal exclude pattern case, however to re-use that here, we would need to start at 0 instead. I considered adding an option to not skip the first character / allow non-absolute filepaths but doing so would require more tests so I think I will just leave a TODO about this optimization instead
pedronis
added
the
Needs Samuele review
These were not meant to be exported, only the fully generic one is meant to be exported. Signed-off-by: Ian Johnson <[email protected]>
…lude patt Thanks to Alberto for spotting this :-) Signed-off-by: Ian Johnson <[email protected]>
|
(closed and reopened to see if that kicks github action into doing something ... ) |
I just couldn't resist the l33tcodeness of the problem of generating these rules with a helper so I fell into a hole and wrote this complex monster of a helper function.
Hope this helps! 😄
I only changed the docker-support interface to use this new helper and left TODO's elsewhere in the codebase that it could be used for future followups in the interest of time. Also happy to drop the docker-support changes to land this more quickly if reviewing it is too complex but I think it's also useful to see here how it's used in a prototypical manner.
This function is generic (and complex) enough to be able to handle all of the
overlapping and wildcard behavior we need in docker-support, and it could also
serve to replace numerous other places in the codebase where we need this sort
of complex behavior. It is a generalization of the existing
aareExclusionPatterns helper, though it's actually unclear if this exact
implementation will currently be able to serve the use case from that helper
directly or if more options/adjustments are needed to enable that use case as
well.
To keep the diff smaller, this patch does not actually change any of the
profiles/interfaces, just TODO's are left for where to use it.
Note that the generated rules are slightly more condensed in terms of number of
rules but significantly more verbose in terms of alternations, not sharing more
of repeated substrings between alternations inside the patterns. This was done
explicitly to keep the generating code simpler and easier to understand, but it
may prove to have performance effects, either detrimental or benevolent but
that should be measured before deciding to make the generation code even more
complex than it already is.