[apparmor] [PATCH v3] Convert aa-status to Python
Marc Deslauriers
marc.deslauriers at canonical.com
Tue May 31 18:52:02 UTC 2011
On Tue, 2011-05-31 at 01:16 -0700, Seth Arnold wrote:
> On Thu, May 26, 2011 at 5:37 PM, Marc Deslauriers
> <marc.deslauriers at canonical.com> wrote:
>
> Marc, thanks, I'm finally taking the time to give this a look, rather
> than just commenting on the overall idea. :)
Thanks!
Let me begin by saying the code is a direct port of the behaviour of the
original tool. Since the original tool was meant to be used in scripts,
and I don't know the extent of how it is being used, I preserved the
same return codes and error messages.
That being said, nothing prevents us from diverging from the original
behaviour in a new major release.
>
> > +def cmd_enabled():
> > + '''Returns error code if AppArmor is not enabled'''
> > + if get_profiles() == {}:
> > + sys.exit(2)
>
> I think this code makes the assumption that _no profiles loaded_ means
> _not enabled_. AppArmor might be enabled but have no profiles loaded.
> (One might wonder if AppArmor is _really enabled_ if it isn't confining
> anything, but there's a big difference between "enabled but doing
> nothing" and "module not loaded".)
get_profiles() itself returns a variety of error codes if AppArmor isn't
enabled. The empty profiles check returning 2 is to match behaviour with
the original aa-status's:
# we consider the case where no profiles are loaded to be "disabled" as
well
my $rc = (keys(%profiles) == 0) ? 2 : 0;
...and what is documented in the man page:
2 if apparmor is enabled but no policy is loaded.
>
> Some other mechanism to report enabled / empty would be nice.
Well, that would be return code 2.
>
> > +def cmd_profiled():
> > + '''Prints the number of loaded profiles'''
> > + profiles = get_profiles()
> > + sys.stdout.write("%d\n" % len(profiles))
> > + if profiles == {}:
> > + sys.exit(2)
>
> Here, as well... incidentally, exiting with a status code seems a bit
> abrupt for "no profiles loaded".
Again, this matches original behaviour.
>
> Some very dissapointing news:
> http://docs.python.org/release/3.0.1/whatsnew/3.0.html#pep-3101-a-new-approach-to-string-formatting
> The dirt-simple % format handling is being removed eventually. All these
> nice simple % formats should be replaced with:
>
> "{0}".format(len(profiles))
>
> or
>
> "{0} profiles are loaded.".format(len(profiles))
>
> Sigh. I always thought the % operator was one of Python's nicest
> features.
Gah! Me too!
Hmm...it appears the new format only works on Python 2.6+. I had tested
aa-status from 2.4 all the way to 3.2, so it doesn't look like it got
removed yet. I think we need to make a policy decision regarding Python
versions to support in AppArmor if future tools are to be written in
Python.
>
> > +def cmd_enforced():
> > + '''Prints the number of loaded enforcing profiles'''
> > + profiles = get_profiles()
> > + sys.stdout.write("%d\n" % len(filter_profiles(profiles, 'enforce')))
> > + if profiles == {}:
> > + sys.exit(2)
> > +
> > +def cmd_complaining():
> > + '''Prints the number of loaded non-enforcing profiles'''
> > + profiles = get_profiles()
> > + sys.stdout.write("%d\n" % len(filter_profiles(profiles, 'complain')))
> > + if profiles == {}:
> > + sys.exit(2)
> > +
> > +def cmd_verbose():
> > + '''Displays multiple data points about loaded profile set'''
> > + global verbose
> > + verbose = True
> > + profiles = get_profiles()
> > + processes = get_processes(profiles)
> > +
> > + stdmsg("%d profiles are loaded." % len(profiles))
> > + for status in ('enforce', 'complain'):
> > + filtered_profiles = filter_profiles(profiles, status)
> > + stdmsg("%d profiles are in %s mode." % (len(filtered_profiles), status))
> > + for item in filtered_profiles:
> > + stdmsg(" %s" % item)
> > +
> > + stdmsg("%d processes have profiles defined." % len(processes))
> > + for status in ('enforce', 'complain', 'unconfined'):
> > + filtered_processes = filter_processes(processes, status)
> > + if status == 'unconfined':
> > + stdmsg("%d processes are unconfined but have a profile defined." % len(filtered_processes))
> > + else:
> > + stdmsg("%d processes are in %s mode." % (len(filtered_processes), status))
> > + # Sort by name, and then by pid
> > + filtered_processes.sort(key=lambda x: int(x[0]))
> > + filtered_processes.sort(key=lambda x: x[1])
> > + for (pid, process) in filtered_processes:
> > + stdmsg(" %s (%s) " % (process, pid))
> > +
> > + if profiles == {}:
> > + sys.exit(2)
>
> ... and seeing this block repeated for each of the cmd_foo() handlers
> makes me think that it should be handled outside of the individual
> cmd_foo() functions themselves.
I agree.
>
> > +def get_profiles():
> > + '''Fetch loaded profiles'''
> > +
> > + profiles = {}
> > +
> > + if os.path.exists("/sys/module/apparmor"):
> > + stdmsg("apparmor module is loaded.")
> > + else:
> > + errormsg("apparmor module is not loaded.")
> > + sys.exit(1)
>
> And if sysfs isn't loaded, or isn't mounted here? I wish we had a more
> solid mechanism to determine if AppArmor is loaded.
I agree. John, any suggestions?
>
> > + apparmorfs = find_apparmorfs()
> > + if not apparmorfs:
> > + errormsg("apparmor filesystem is not mounted.")
> > + sys.exit(3)
>
> We _do_ pretty much need securityfs to be loaded, but this error message
> doesn't help the user figure out how to do that. I suggest:
>
> "Expected a securityfs filesystem to be mounted; /sys/kernel/security is common"
I agree.
>
> > + apparmor_profiles = os.path.join(apparmorfs, "profiles")
> > + if not os.access(apparmor_profiles, os.R_OK):
> > + errormsg("You do not have enough privilege to read the profile set.")
> > + sys.exit(4)
>
> Rather than exit, why not continue with the operations that _are_
> allowed to the user? `ps auxwZ` contains a wealth of information
> available to unprivileged users that the user might still wish to be
> summarized.
oh, interesting. We should probably print a warning, and then print
whatever info is available. Agreed.
>
> > + for p in open(apparmor_profiles).readlines():
> > + match = re.search("^([^\(]+)\s+\((\w+)\)$", p)
> > + profiles[match.group(1)] = match.group(2)
> > +
> > + return profiles
>
> This specifically is where I wish {} didn't mean two things. :)
>
> > +def get_processes(profiles):
> > + '''Fetch process list'''
> > + processes = {}
> > + contents = os.listdir("/proc")
> > + for filename in contents:
> > + if filename.isdigit():
> > + try:
> > + for p in open("/proc/%s/attr/current" % filename).readlines():
> > + match = re.search("^([^\(]+)\s+\((\w+)\)$", p)
> > + if match:
> > + processes[filename] = { 'profile' : match.group(1), \
> > + 'mode' : match.group(2) }
> > + elif os.path.realpath("/proc/%s/exe" % filename) in profiles:
> > + # keep only unconfined processes that have a profile defined
> > + processes[filename] = { 'profile' : os.path.realpath("/proc/%s/exe" % filename), \
> > + 'mode' : 'unconfined' }
> > + except:
> > + pass
> > + return processes
> > +
> > +def filter_profiles(profiles, status):
> > + '''Return a list of profiles that have a particular status'''
> > + filtered = []
> > + for key, value in list(profiles.items()):
> > + if value == status:
> > + filtered.append(key)
> > + filtered.sort()
> > + return filtered
> > +
> > +def filter_processes(processes, status):
> > + '''Return a list of processes that have a particular status'''
> > + filtered = []
> > + for key, value in list(processes.items()):
> > + if value['mode'] == status:
> > + filtered.append([key, value['profile']])
> > + return filtered
> > +
>
> Both these beg for list comprehensions. I'll let you decide if you think
> these are clearer or not:
>
> def filter_profiles(profiles, status):
> '''Return a list of profiles that have a particular status'''
> l = [k for k,v in profiles.items() if v == status]
> l.sort()
> return l
>
> def filter_processes(processes, status):
> '''Return a list of processes that have a particular status'''
> l = [[fn,stat['profile']] for fn,stat in processes.items() if
> stat['mode'] == status]
> return l
Ah, yes, much better.
>
> > +def find_apparmorfs():
> > + '''Finds AppArmor mount point'''
> > + for p in open("/proc/mounts").readlines():
> > + if p.split()[2] == "securityfs" and \
> > + os.path.exists(os.path.join(p.split()[1], "apparmor")):
> > + return os.path.join(p.split()[1], "apparmor")
> > + return False
> > +
> > +def errormsg(message):
> > + '''Prints to stderr if verbose mode is on'''
> > + global verbose
> > + if verbose:
> > + sys.stderr.write(message + "\n")
> > +
> > +def stdmsg(message):
> > + '''Prints to stdout if verbose mode is on'''
> > + global verbose
> > + if verbose:
> > + sys.stdout.write(message + "\n")
> > +
> > +def print_usage():
> > + '''Print usage information'''
> > + sys.stdout.write('''Usage: %s [OPTIONS]
> > +Displays various information about the currently loaded AppArmor policy.
> > +OPTIONS (one only):
> > + --enabled returns error code if AppArmor not enabled
> > + --profiled prints the number of loaded policies
> > + --enforced prints the number of loaded enforcing policies
> > + --complaining prints the number of loaded non-enforcing policies
> > + --verbose (default) displays multiple data points about loaded policy set
> > + --help this message
> > +''' % sys.argv[0])
> > +
> > +# Main
> > +global verbose
> > +verbose = False
> > +
> > +if len(sys.argv) > 2:
> > + sys.stderr.write("Error: Too many options.\n")
> > + print_usage()
> > + sys.exit(1)
> > +elif len(sys.argv) == 2:
> > + cmd = sys.argv.pop(1)
> > +else:
> > + cmd = '--verbose'
> > +
> > +# Command dispatch:
> > +commands = {
> > + '--enabled' : cmd_enabled,
> > + '--profiled' : cmd_profiled,
> > + '--enforced' : cmd_enforced,
> > + '--complaining' : cmd_complaining,
> > + '--verbose' : cmd_verbose,
> > + '-v' : cmd_verbose,
> > + '--help' : print_usage,
> > + '-h' : print_usage
> > +}
> > +
> > +if cmd in commands:
> > + commands[cmd]()
> > + sys.exit(0)
> > +else:
> > + sys.stderr.write("Error: Invalid command.\n")
> > + print_usage()
> > + sys.exit(1)
> > +
>
> Ooooh, it'd sure be nice if these were easy to translate. :) I know the
> Perl version wasn't i18ned, but it sure would be nice if Python's
> gettext made it as easy...
Yes, I agree. If we're going to modify the tool's behaviour, I wonder if
we should have a special option to print the info out in a format better
suited for parsing.
>
> Thanks again Marc!
Thanks for the review!
Marc.
More information about the AppArmor
mailing list