Merge aa-notify: Improve support for local profiles

This MR contains fixes and improvements for --local profiles in aa-notify

 - aa-notify: Make --local commandline option override use_local_profiles
 - utils: Move get_local_include to ProfileStorage
 - utils: Add tests for get_local_include
 - aa-notify gui: Fix undefined variable when ttkthemes is not installed

Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>

MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1770
Approved-by: Maxime Bélair <maxime.belair@canonical.com>
Merged-by: Maxime Bélair <maxime.belair@canonical.com>

utils/aa-notify

@@ -851,7 +851,7 @@ def main():
     parser.add_argument('-w', '--wait', type=int, metavar=('NUM'), help=_('wait NUM seconds before displaying notifications (with -p)'))
     parser.add_argument('-m', '--merge-notifications', action='store_true', help=_('Merge notification for improved readability (with -p)'))
     parser.add_argument('-F', '--foreground', action='store_true', help=_('Do not fork to the background'))
-    parser.add_argument('-L', '--local', nargs='?', const='yes', default='auto', choices=['yes', 'no', 'auto'], help=_('Add to local profile'))
+    parser.add_argument('-L', '--local', nargs='?', const='yes', choices=['yes', 'no', 'auto'], help=_('Add to local profile'))
     parser.add_argument('--prompt-filter', type=str, metavar=('PF'), help=_('kind of operations which display a popup prompt'))
     parser.add_argument('--debug', action='store_true', help=_('debug mode'))
     parser.add_argument('--configdir', type=str, help=argparse.SUPPRESS)
@@ -1073,6 +1073,8 @@ def main():
         keys_to_aggregate = config['']['keys_to_aggregate'].strip().split(',')
     else:
         keys_to_aggregate = {'operation', 'class', 'name', 'denied', 'target'}
+
+    if not args.local:
         if 'use_local_profiles' in config['']:
             if config['']['use_local_profiles'] in {'auto', 'yes', 'no'}:
                 args.local = config['']['use_local_profiles']
@@ -1082,6 +1084,8 @@ def main():
                 sys.exit(_('ERROR: using an invalid value for use_local_profiles in config {}\nSupported values: {}').format(
                     config['']['use_local_profiles'], ', '.join({'yes', 'auto', 'no'})
                 ))
+        else:
+            args.local = 'auto'
 
     if args.file:
         logfile = args.file

utils/aa-notify.pod

@@ -106,8 +106,8 @@ System-wide configuration for B<aa-notify> is done via
   # Binaries for which we ignore userns-related capability denials
   ignore_denied_capability="sudo,su"
 
-  # Write change to local profiles if enabled to preserve regular profiles and simplify upgrades
+  # Write change to local profiles if enabled to preserve regular profiles and simplify upgrades (yes, no, auto)
-  use_local_profiles
+  use_local_profiles="yes"
 
   # OPTIONAL - kind of operations which display a popup prompt.
   prompt_filter="userns"

utils/apparmor/aa.py

@@ -1703,17 +1703,16 @@ def read_profile(file, is_active_profile, read_error_fatal=False):
             extra_profiles.add_profile(filename, profile, attachment, profile_data[profile])
 
 
-def get_local_include(profile_name):
+# TODO: Split profiles' creating and saving.
-    # If a local profile already exists, we use it.
-    for rule in active_profiles[profile_name]['inc_ie'].rules:
-        if rule.path.startswith("local/"):
-            return rule.path
-    return None
-
-
 def create_local_profile_if_needed(profile_name):
-    base_profile = profile_name.split("/", 1)[0]
+    base_profile = profile_name
-    local_include = get_local_include(profile_name)
+    while True:
+        parent = active_profiles[base_profile].data.get('parent')
+        if parent == '':
+            break
+        base_profile = parent
+
+    local_include = active_profiles[profile_name].get_local_include()
 
     # Not found: we add a mention of the local profile in the main profile
     if not local_include:
@@ -1756,7 +1755,7 @@ def write_include(include_data, incfile, out_dir=None, include_metadata=True):
 
     include_string = serialize_include(include_data, include_metadata=include_metadata)
 
-    with NamedTemporaryFile('w', suffix='~', delete=False) as tmp:
+    with NamedTemporaryFile('w', suffix='~', delete=False, dir=profile_dir + "/local") as tmp:
         if os.path.exists(target_file):
             shutil.copymode(target_file, tmp.name)
         else:

utils/apparmor/gui.py

@@ -158,17 +158,18 @@ class ShowMoreGUIAggregated(GUI):
 
         self.text_display = tk.Text(self.label_frame, wrap='word', height=40, width=100, yscrollcommand=self.scrollbar.set)
 
+        kwargs = {
+            "height": self.text_display.winfo_reqheight() - 4,  # The border are *inside* the canvas but *outside* the textbox. I need to remove 4px (2*the size of the borders) to get the same size
+            "width": self.text_display.winfo_reqwidth() - 4,
+            "borderwidth": self.text_display['borderwidth'],
+            "relief": self.text_display['relief'],
+            "yscrollcommand": self.scrollbar.set,
+        }
+
         if ttkthemes:
             self.text_display.configure(background=self.bg_color, foreground=self.fg_color)
-        self.canvas = tk.Canvas(
+            kwargs['background'] = self.bg_color
-            self.label_frame,
+        self.canvas = tk.Canvas(self.label_frame, **kwargs)
-            background=self.bg_color,
-            height=self.text_display.winfo_reqheight() - 4,  # The border are *inside* the canvas but *outside* the textbox. I need to remove 4px (2*the size of the borders) to get the same size
-            width=self.text_display.winfo_reqwidth() - 4,
-            borderwidth=self.text_display['borderwidth'],
-            relief=self.text_display['relief'],
-            yscrollcommand=self.scrollbar.set
-        )
 
         self.inner_frame = ttk.Frame(self.canvas)
         self.canvas.create_window((2, 2), window=self.inner_frame, anchor='nw')

utils/apparmor/profile_storage.py

@@ -199,6 +199,21 @@ class ProfileStorage:
 
         return data
 
+    def get_local_include(self):
+        inc = None
+        preferred_inc = self.data['name']
+        if preferred_inc.startswith('/'):
+            preferred_inc = preferred_inc[1:]
+        preferred_inc = 'local/' + preferred_inc.replace('/', '.')
+
+        # If a local profile already exists, we use it.
+        for rule in self.data['inc_ie'].rules:
+            if rule.path.startswith("local/"):
+                inc = rule.path
+                if rule.path == preferred_inc:  # Prefer includes that matches the profile name.
+                    break
+        return inc
+
     @classmethod
     def parse(cls, line, file, lineno, profile, hat):
         """parse a profile start line (using parse_profile_startline()) and convert it to an instance of this class"""

utils/apparmor/update_profile.py

@@ -46,9 +46,9 @@ def add_to_profile(rule_obj, profile_name):
 
 
 def add_to_local_profile(rule_obj, profile_name):
-    inc_file = aa.create_local_profile_if_needed(profile_name, cleanup=True)
+    inc_file = aa.create_local_profile_if_needed(profile_name)
 
-    aa.include[inc_file][inc_file].data[rule_obj.rule_name].add(rule_obj)
+    aa.include[inc_file][inc_file].data[rule_obj.rule_name].add(rule_obj, cleanup=True)
     aa.write_include_ui_feedback(aa.include[inc_file][inc_file], inc_file)
 
 
@@ -66,7 +66,7 @@ def add_rule(mode, rule, profile_name):
     elif mode == 'no':
         add_to_profile(rule_obj, profile_name)
     elif mode == 'auto':
-        if aa.get_local_include(profile_name):
+        if aa.active_profiles[profile_name].get_local_include():
             add_to_local_profile(rule_obj, profile_name)
         else:
             add_to_profile(rule_obj, profile_name)

utils/test/test-profile-storage.py

@@ -14,6 +14,7 @@ import unittest
 from apparmor.common import AppArmorBug, AppArmorException
 from apparmor.profile_storage import ProfileStorage, add_or_remove_flag, split_flags, var_transform
 from apparmor.rule.capability import CapabilityRule
+from apparmor.rule.include import IncludeRule
 from common_test import AATest, setup_all_loops
 
 
@@ -313,6 +314,27 @@ class AaTest_var_transform(AATest):
         self.assertEqual(var_transform(params), expected)
 
 
+class AaTest_include(AATest):
+    tests = (
+        (('profile foo /foo {', []),                                        None),  # No include
+        (('profile foo /foo {', ['elsewhere/foo']),                         None),  # No include in local/
+        (('profile foo /foo {', ['local/foo']),                             "local/foo"),  # Single include, we pick it
+        (('profile foo /foo {', ['local/bar']),                             "local/bar"),  # Single include, we pick it
+        (('profile x//y /y {',  ['local/x..y', 'local/y']),                 "local/x..y"),  # Pick the include that matches the profile nam
+        (('profile foo /foo {', ['local/bar', 'local/foo', 'local/baz']),   "local/foo"),  # Pick the include that matches the profile name
+        (('/usr/bin/xx {',      ['local/usr.bin.xx', 'local/xx']),          "local/usr.bin.xx"),  # Pick the include that matches the profile name
+        (('profile foo /foo {', ['local/bar', 'local/baz', 'local/qux']),   "local/qux"),  # No match, pick the last one
+    )
+
+    def _run_test(self, params, expected):
+        (profile, hat, prof_storage) = ProfileStorage.parse(params[0], 'somefile', 1, None, None)
+
+        for inc in params[1]:
+            prof_storage.data['inc_ie'].add(IncludeRule(inc, True, True))
+
+        self.assertEqual(prof_storage.get_local_include(), expected)
+
+
 setup_all_loops(__name__)
 if __name__ == '__main__':
     unittest.main(verbosity=1)