Change the tools to use merged profile names (`var['foo//bar']`) instead of the profile/hat layout (`var[profile][hat]`) in many places. Also storage gets moved to ProfileList instead of using a hasher.
Already changed places (in this MR) are parsing profiles, writing profiles, handling and storing of extra profiles, log handling and asking the user about profile additions.
Remaining usage of the `var[profile][hat]` layout are the `aa` and `original_aa` hashers, they'll be replaced in a separate MR.
See the individual commits for details. I'd also recommend to do the review on the individual commits, because the big diff is probably unreadable ;-)
While this is a big chain of changes, each commit contains working code, converting between the two storage layouts with `split_to_merged()` and `merged_to_split()` as needed, with merged layout "bubbling up" in more and more functions.
The long-term goal of these changes is to enable support for nested child profiles in the tools, but - one step after the other ;-)
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/736
Acked-by: John Johansen <john.johansen@canonical.com>
The function decides on the filename of a profile, therefore use
'filename' as variable name instead of the somewhat confusing 'profile'
and 'full_profilename'.
If an include file includes itsself (for example if local/foo has
'#include <local/foo>'), print a warning instead of calling
load_include() again and again.
This fixes a crash when hitting such a case:
RecursionError: maximum recursion depth exceeded while calling a Python object
Fixes: https://bugzilla.suse.com/show_bug.cgi?id=1184779 for the tools.
The parser will also need a fix.
if a profile doesn't have an attachment specified and the profile name
starts with '/', set the attachment to the profile name. This allows to
have one add_profile() call instead of two very similar ones.
exit early if profile_data is empty (which means we did read an empty
file). This allows to simplify the if conditions to "if active_profile:"
and "else:".
... and adjust all callers and the tests.
For bonus points ;-) this also removes a hasher usage, and extends the
test to check that only the expected profile gets created.
... instead of the 'extras' hasher.
Also adjust all code that previously used 'extras' to use
'extra_profiles'. This affects get_profile() and read_profile().
Add a prof_storage parameter to add_profile() to hand over the actual
profile data/rules as ProfileStorage.
Also adjust several tests to hand over a (dummy) ProfileStorage object.
Note: For now, the parameter is optional because it needs some more changes
in aa.py to be really useable. This will change in a later commit.
... instead of converting log_dict to traditional [profile][hat] layout
in do_logprof_pass().
A nice side effect is that we get sorting the main profile before its
hats for free and can remove the sorting code.
Also update a comment in ask_rule_questions().
Finally, adjust aa-mergeprof so that it hands over a merged log_dict (using
split_to_merged())
... instead of the old [profile][hat] structure.
This needs changes in do_logprof_pass() when calling ask_the_questions()
(using merged_to_split() for now).
Also adjust test-libapparmor-test_multi.py logfile_to_profile() to
expect the merged structure.
... and convert them back to the [profile][hat] layout at the end so
that callers still get the expected result.
As a side effect, log_dict no longer needs to be a hasher().
... instead of the old [profile][hat] structure.
This needs changes in read_profile() (now using the merged profile name)
and attach_profile_data() (using merged_to_split() for now).
Also adjust test-aa.py to expect the merged structure.
Change parse_profile_data() to internally use merged profile names
(`foo//bar`) instead of separate profile and hat, and only split it up
again to the [profile][hat] layout at the very end with
merged_to_split().
A nice side effect is that we get rid of a hasher() usage.
parse_profile_data() also gets changed to use `hat = None` (instead of
`hat = profile`) if not inside a child profile. As a result,
parse_profile_start() and one of its tests need a small change.
Besides that small change, calling code should not see a difference, and
the tests also stay working.
... by adding some new tests, and by marking two lines as "pragma: no
branch" because I didn't find a testcase that doesn't let them continue
with the next line.
Finally, remove severity.py from the "not 100% covered" list in
test/Makefile.
Add tests with invalid type to ensure error handling works as expected.
Merge branch 'cboltz/cboltz-profile-storage-tests'
[Fixed conflict with prior change to utils/test/test-profile-storage.py]
Acked-by: Steve Beattie <steve@nxnw.org>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/735
Also adjust the calling code to use get_header() instead of
write_header().
Finally, move the tests to test-profile-storage.py and do a few
adjustments needed by the change. Part of these adjustments is to hand
over empty params with the correct type instead of just "None".
... instead of profile_data\[profile\]\[profile\]\['name'\] which is not always correct.
Note: setting 'name' will be fixed in a later commit/MR, but don't hold your breath for it ;-)
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/733
Acked-by: John Johansen <john.johansen@canonical.com>
AUDIT events get skipped when parsing the log (they are not relevant for updating a profile), therefore stop returning an always-empty AUDIT section when reading the log.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/731
Acked-by: John Johansen <john.johansen@canonical.com>
... instead of profile_data[profile][profile]['name'] which is not
always correct.
Note: setting 'name' will be fixed in a later commit, but don't hold
your breath for it ;-)
AUDIT events get skipped when parsing the log (they are not relevant for
updating a profile), therefore stop returning an always-empty AUDIT
section when reading the log.
This needed several changes because so far data for all includes was
stored in include[]. However, preamble data for everything else gets
stored in active_profiles (and with this commit, preamble includes also
get stored in active_profiles).
The needed changes to store preamble includes in active_profiles are:
* include_list_recursive(): add and honor in_preamble flag
* add this flag at several places calling include_list_recursive() for
a preamble
* parse_profile_data(): call active_profiles.init_file() for all files.
Before, empty/comment-only files weren't (indirectly) added to
active_profiles because none of the add_$ruletype functions was
called, which could lead to KeyErrors for comment-only preamble include
files (prevented by the now-obsolete and removed check in
get_all_merged_variables()).
For this, we have to hand over in_preamble, and to do a slightly
different handling for preamble and profile rules.
For adding preamble rules to ProfileList, add a add_rule() function that
accepts the rule type as parameter. This is easier than using one of the
add_$type functions depending on the rule type.
With this change, match_line_against_rule_classes() handles nearly all
rule types that have a *Rule class, with the exception of include rules
which need some additional work.
in_preamble keeps track of the current parsing position.
It's True while parsing the preamble of a profile file, and when loading
an include file that is included in the preamble (typically tunables/*).
While inside a profile or parsing abstractions/*, it is False.
This commit only hands the information around and keeps in_preamble
updated, but it doesn't really get used yet.
Also adjust the tests to hand over the additional parameter to
parse_profile_data().
... to handle parsing of lines that are managed with a *Rule
This needs 'ruletypes' to also know about *Rule, not only about
*Ruleset.
Also switch over handling of most rules that live inside a profile from
parse_profile() to match_line_against_rule_classes() to make
parse_profile() more readable.
Exceptions are:
- include and abi, which can also exist in the preamble
- file rules, because they'd need to be handled later (after variable
definitions)
Since loadincludes() now only loads a specified list of subdirectories,
we no longer need a directory blacklist.
The only possibly remaining part are .git subdirectories (for example
tunables/.git or abstractions/.git). Since it's very unlikely that
someone would have only a subdirectory of /etc/apparmor.d/ in git, drop
that check.
Also split out parts of the function into loadincludes_dir() to keep the
code readable.
Note: This change might affect the list of includes proposed by
aa-logprof.
split off parse_profile_start_to_storage() from parse_profile_data()
See merge request apparmor/apparmor!710
Acked-by: Steve Beattie <steve.beattie@canonical.com>
in_contained_hat is needed to know if we are already in a profile or
not. (Simply checking if we are in a hat doesn't work, because something
like "profile foo//bar" will set profile and hat at once, and later
(wrongfully) expect another "}".
However, the way how this variable was set became too complicated.
To simplify the code, set in_contained_hat directly in
parse_profile_data() RE_PROFILE_START instead of returning it via
parse_profile_start() and parse_profile_start_to_storage()
Since this change removes a return value from two functions, also adjust
the tests accordingly.
parse_profile_start_to_storage() converts the result of
parse_profile_start() into a ProfileStorage object.
No functional change, but parse_profile_data() becomes more readable.
Also extend tests to cover parse_profile_start_to_storage().
... to find regressions or improvements in the python code coverage.
`make coverage-regression` will error out if a file looses its 100%
coverage, or if a file improved to 100% coverage.
Other coverage changes (for example 45% -> 47%) will be ignored.
