[apparmor] Policy cache

[John Johansen]

On 11/10/2011 11:42 AM, Christian Boltz wrote:
> Hello,
> 
> Am Mittwoch, 9. November 2011 schrieb John Johansen:
>> On 11/09/2011 04:54 PM, Kees Cook wrote:
>>> On Wed, Nov 09, 2011 at 04:43:44PM -0800, Seth Arnold wrote:
>>>> On Wed, Nov 9, 2011 at 3:52 PM, Kees Cook <kees at ubuntu.com> wrote:
>>>>> On Wed, Nov 09, 2011 at 03:38:36PM -0800, John Johansen wrote:
>>>>>> With that said we should add an option to the
>>>>>> apparmor_parser to allow setting the cache location.  If an
>>>>>> alternate cache location is desired a new default could be
>>>>>> set in the /etc/apparmor.d/parser.conf file 
>>>>> I would recommend /lib/apparmor/cache as the new upstream
>>>>> default location. It should be an option, though, yes.
> 
> /lib is meant to be read-only from the FHS view, so we would replace a 
> FHS violation (cache files in /etc) with another one (dynamic files in 
> /lib) which is even worse IMHO. /etc is at least ment to be writeable.
> 
right, I don't like /lib either

>>>> I know /lib is usually available early, but I don't care for the
>>>> idea of considering it a "writable location". That's what /var
>>>> is there for -- per-service dynamic storage. 
> 
> Exactly - /var/cache is _the_ place for cache files.
> 
>>> Hrm. So /var by default? And people with /var not in / can
>>> relocated it as needed? 
> 
> Yes, /var/cache/apparmor would be the correct default.
> 
> If someone wants another location, he can create a symlink (similar to 
> the symlink /etc/apparmor.d/cache -> /var/cache/apparmor I'm using in 
> the openSUSE package ;-)
> 
right but a symlink for /var/cache/apparmor/ -> /crazy/location doesn't
work if /var isn't available because it hasn't been mounted yet

> If you really want, make it a config option - but please try to avoid 
> that AppArmor gets renamed to KAppArmor one day *eg*
> 
ugh kAppArmor yuk

>>> I'd almost rather keep it in /etc. More people more /var than /lib. 
>                                                    ^^^^
>                                                   s/r/v/ ?
> 
> Maybe nearly nobody puts /etc on a separate partition.
> OTOH: Do you have some numbers about how many people have a separate 
> /var partition? Would using /var/cache/ be a real problem for them?
> 
well beyond losing the cache, probably not

>>> For example, udev uses /etc for cached rules.
> 
> Sounds like a bug in udev ;-)
> 
> That said: What's the exact cache location? 
> /etc/udev on openSUSE looks quite clean - it has some files in rules.d 
> and an udev.conf. Well, some of the files in rules.d are placed there by 
> a package (which would probably better install them in 
> /lib/udev/rules.d/), but I don't see anything that looks like a cache.
> 
> And besides that: see signature ;-)
> 
>> This is exactly how the discussion have gone, /etc isn't right, should
>> be /var, but it might not be available, how about /lib, no /lib
>> shouldn't be writable that is what /var is for, and around and around
>> ...
> 
> ;-)
> 
>> And we keep coming back to /etc/ is the best generic default because
>> no where matches, but /etc/ works and its what we are using already
>>
>> I am not opposed to moving but we can't seem to agree where, hence
>> lets just make it configurable and be done with it.
> 
> Yes, I'm afraid it needs to be a config or compile time option, even if 
> I really don't like the idea to use anything except /var/cache/.
> 
> The problem with making it a config or a compile-time option is that it 
> will make the documentation funny. "Profiles are cached in 
> /etc/apparmor.d/cache, except if you use openSUSE which uses 
> /var/cache/apparmor, except if you use $foo_distro which uses 
> /lib/apparmor/cache, except if you use $bar_distro which uses 
> /var/cache/apparmor-profiles, except if you use $other_distro ..." ;-)
> 
hrmm I think we can do a little better than that, but yeah it would
sure be nice if there where only one location

> 
> If /var is not (yet) available, starting AppArmor will take some 
> additional seconds because of the non-available cache. That's not nice, 
> but totally different from "it will burn down your system" ;-)
> 
hehe, well it sure seems that way one management starts pointing at
you for slowing down their boot chart ;-)

> BTW: The openSUSE initscript (rc.apparmor.suse) contains "Should-Start: 
> $local_fs", which means AppArmor will be started after all partitions 
> are mounted.
> 
> The other rc.apparmor.* scripts don't contain something like that - 
> maybe that's something to fix?
> 
Actually that is problematic, in that Ubuntu needs to load policy
before some filesystems are mounted.  Right now the policy is split
between early and late boot

So lets just say the /var case not being present is a very real
possibility for Ubuntu.

>>>> The hash need not be this complicated: it could just be:
>>>>
>>>> /lib/apparmor/cache/3.2.0-5-generic/f3.1-c2.0-n1.0-ch1.5-cp1.1-n
>>>> s1.1-r1.1/ reduced to:
>>>> /lib/apparmor/cache/3.2.0-5-generic/31201015111111
> 
> Both variants mean you'll need a program to generate the hash for the 
> cleanup script. What about just using the same name that is used in 
> /lib/modules/?
> 
well that is if we want a per kernel cache, I would actually rather
have it be based off supported features as then it could be reused
by multiple kernels

>>> But that means we can't ever go from .9 to .10. I think a hash
>>> would be fine -- we're just looking for whatever matches.
>>
>> yes I think the hash will be adequate, especially if we continue with
>> the practice of putting .features file in the directory, so that if
>> there ever is a hash collision we can catch it.
> 
> Before we discuss about the method to name the directory - do we really 
> need cache directories for different kernels? I somehow doubt ;-)
> 
Need no, want likely.  Whether I like it or not boot speed is fairly
important from the Ubuntu end, and being able to precompute cache on
policy install for different kernels is a big win there.

> Let me start with a question: Does apparmor_parser notice a kernel 
> change and update the cache (if the "write-cache" option is enabled)? 
> [1]
> 
yes, in cache/ there is a .features file.  It compares it to the kernel's
features, and if they don't match the current cache isn't used.

If write-cache is set the newly generated policy will be written back
to the cache.

> My thoughts about having subdirectories for each kernel version:
> 
> + faster boot time for people who switch forth and back between 
>   different kernels (but seriously - who does that?)
devs mostly
> - something[tm] needs to cleanup/delete the cache for old kernel 
>   versions
yep
> - needs to be implemented in apparmor_parser
yep

I missed faster boot time after a new kernel install.  We can't
currently just create cache for the new kernel being installed,
because another package being installed might come along after and
cause cache rebuilds based on the current kernel.

> 
> IMHO the + argument is too small and a corner-case when compared to the 
> other arguments - having the cache only for one kernel would be fine 
> IMHO.
> 
generally I agree, I am not sure its worth the added complexity but
I fear it may be as more policy gets created and loaded.  Right now this
has a low priority for me which means it won't be done by me, but if
we start running into boot speed problems, I can tell you it will likely
get a large priority boost ;-)