[apparmor] [PATCH v3] Convert aa-status to Python

[John Johansen]

On 05/31/2011 11:52 AM, Marc Deslauriers wrote:
> 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.
> 
Well I think its certainly worth considering for this release.  It was
worth while mimicking the old behavior while developing/debugging, now
lets consider improving it.

<< snip >>

>>
>> 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.
> 
Sigh, why did they go and do something as brain dead and verbose as that?
Python breaking backwards compatibility is my big complaint with it, that
said if we are using python for tools we should try riding in the middle
supporting as many releases as we can.

So lets stick with using % formating for now, and we can change it when
we have to in the future.



>>
>>> +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?
> 
Well sysfs not being loaded on modern linux is pretty much impossible, but
it might not be available (container) or mounted at /sys/, nor do we actually
require it to function, securityfs can be mounted else where.

We can't use /proc/modules as apparmor is built in and won't show up there.

We could look at mounts to find where sysfs is mounted so that the /sys portion
isn't hard coded.

We could also just look for where securityfs is mounted, grab that and check
for the apparmor directory.

If securityfs/apparmor is present apparmor is available,
else if sysfs/module/apparmor is present apparmor is not the current LSM
else if sysfs is mounted apparmor is not built in to the kernel
else we can't tell whether apparmor is available

>>
>>> +    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.
It is the upstream blessed mount point but it isn't required.

> 
>>
>>> +    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.
>
That would be real nice to have especially as there are vague plans for user
defined profiles.

<< snip >>

>> 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.
> 
that could be handy, and I think now is the release to break things as we
are doing some of that else where too.