[apparmor] AppArmor and /etc/

[John Johansen]

On 01/07/2018 07:22 AM, intrigeri wrote:
> Hi,
> 
> … and sorry for the delay!
> 
> John Johansen:
>> On 11/25/2017 08:16 AM, intrigeri wrote:
>>> Marco d'Itri:
>>>> Why are policies generally installed in /etc/ and not in 
>>>> /usr/share/apparmor/?
>>>
>> It actually depends on the distro, eg. ubuntu touch moved the text
>> policy to /var/lib/apparmor/ and the cache to /var/cache/apparmor
> 
> Interesting!
> 
> Then I'd like to try moving the cache to /var/cache on Debian and
> Ubuntu to start with. This seems like a realistic goal for the Buster
> development cycle.
> 
I'm not sure /var/cache is the right place, while the data certainly
can be regenerated, its getting to the point that we should stop
referring to it as cache. Its the binary version of policy and there
are several cases where its required and falling back to regeneration
will result in failure.

With that said /var/cache isn't terrible and is better that /etc/
for most modern linux systems. I will also throw in the proviso that
I don't mess with this area much and Jamie or Steve are better people
to comment on this.


> Apart of the early boot dependency issue (which I think is not really
> one in practice as we run After=local-fs.target on Debian), is there
> any foreseeable problem I should be aware of?
> Perhaps the Ubuntu folks have some insight to share based on their
> past experience doing similar things?
> 
the early boot dependency was a real issue that we did run into. But
with the systemd rework I am not so sure it is anymore.

> Moving the text policy itself seems more involved and I doubt it'll
> happen during the Buster development cycle.
> 
meh, that is actually pretty easy too. You have to leave the
/etc/apparmor/parser.conf in place but you can use it to override
the defaults.

It only becomes a mess when you do a split policy like Ubuntu did
with policy in /etc/ and in /var/lib/

>> the /etc/apparmor.d/ location is just the upstream project default
>> as its generic and should work on all distros, where /usr/ may not
>> be available during early boot etc.
> 
> Last time I checked, bits of the code we run when starting
> apparmor.service on Debian already depend on /usr. At least on Debian,
> /usr is mounted by the initramfs so IIRC this can break only systems
> that have a separate /usr filesystem *and* don't use an initramfs.
> Nobody reported a problem with this so far so until there's practical
> evidence I'm calling this a theoretical issue :)
> 

yeah systems have moved away from this, and I don't know that it is
a real concern any more.

>>> 2. Our local override mechanism is incomplete
>>>
>>>    Due to limitations in the policy language, some (rare) kinds of
>>>    rules cannot be overridden without editing the original rule, e.g.
>>>    https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/451422
>>>
>>> 3. Our local override mechanism is Debian-specific
>>>
>>>    AFAIK the "#include <local/$profile>" thing is the norm only on
>>>    Debian and derivatives. Christian, what do you do at OpenSUSE?
>>>
>> Local over rides are more of packaging issue, and never played into
>> the original decision. If you wanted to override something you would
>> modify the profile text.
> 
> Sure you can, but with dpkg such local changes will be overwritten by
> the next package upgrade unless the modified file is a conffile.
> This is why having a better overriding mechanism seems to be a blocker
> for moving the default policy out of /etc. Or did I miss something?
> 

hahaha, maybe, I have an extreme dislike of override files that
hide, and rewrite policy so its not what it appears when someone
opens it.

With that said, there will be better ability to do overrides, because
in practice its just needed. The exact form still needs to be determined
and of course the work still needs to be done.

In particular we need some boolean operations on pattern matching to
make certain things easier to express.

  deny /proc/** - /proc/@{pid}/* r,

and having a way to expose rule priorities would be good, basically
that is how deny works, deny rules have a higher priority than allow
rules.

Beyond that, what directories, or where policy snips reside that
allow overrides is I think at least partially a packaging issue,
where does a package throw and override, where does local policy
editing throw an override, what is a writable location on the
system...

we will also probably need to provide some configuration around
it, but I'd like to hear more from those who deal more with packaging
and policy than I do.

>>> It seems that /etc/apparmor.d/cache is created by the Debian
>>> packaging, so presumably I could easily ship a CACHEDIR.TAG file
>>> in there.
>>>
>>> What do others think?
> 
>> hrmmm, maybe cache files aren't always generated locally and we
>> have had cases of shipping precompiled policy.
> 
> I see. And then in this case, a backup of the cache would be needed to
> restore a system, so having a CACHEDIR.TAG file would be a problem.
> 

I should add that the shipped precompiled policy has been for
read-only images (the ubuntu phone), but you could do it for containers

>> Its certain is something that a distro could decide setting regardless
>> of what the upstream project does.
> 
> OK. Given Debian has never shipped pre-compiled policy yet, my
> assessment is that the benefits of shipping a CACHEDIR.TAG file
> greatly outweigh the risk of making backups incomplete. I'm prepared
> to do that but as discussed on https://bugs.debian.org/883584 (with
> this mailing list in Cc) it requires a change in the cache clearing
> C code, so moving the binary cache to /var/cache might actually be
> simpler in practice, in addition to being a more correct solution.
> So for now I'll focus on moving the cache out of /etc and will
> postpone the CACHEDIR.TAG matter.
> 

sure, but that doesn't mean we don't also improve cache writing and
clearing.

>>>> If there is no "#include if exists" directive that could be used for 
>>>> the /etc/apparmor.d/local/* files then I believe that maintainers should 
>>>> be encouraged to ship empty files instead of each repeating the same 
>>>> "you may modify this" comment (currently we have both styles).
>>>
>>> I don't think we have "#include if exists".
>>>
>> We sort of do. If you do a directory based include, like
>>   include <local/>
> 
>> files are only included if they exist, and there is no failure even if
>> the directory is empty.
> 
>> where this fails is you basically need an empty dir per profile
>> unless the files are to be shared.
> 
> Urgh, this would work but does not look much nicer to me than the
> current situation. I'd rather go for the "#include if exists" thing
> instead:
> 
its coming, I'm doing some testing on it. I will propose it for SRU

>> We don't currently have a file specific include that won't fail if
>> the file doesn't exist, but its an extension we could add.
> 
>>> If we had one, then I think we should indeed drop these files
>>> entirely. There's a README in that directory, that's just as
>>> discoverable by users as these local override files, and would even be
>>> more discoverable if it was the only file shipped in
>>> /etc/apparmor.d/local/ by default. What do others think? If you agree
>> I would disagree that its more discoverable, but its not like you can't
>> have a comment pointing to it in the profile.
> 
>>> it's a good idea, then I'll file a bug report on LP about adding one
>>> such directive (and will try to find someone whose C skills are better
>>> than me, in order to implement it).
>>>
>> yes allow a condition if it exists include makes sense
> 
> Cool. I've updated the bug we already have about it to record this new
> reason to implement this: https://bugs.launchpad.net/apparmor/+bug/1206742
> 
> I'll try to find someone who can write the needed C code. Pointers and
> description of non-obvious requirements would be welcome, as would be
> directive name suggestions :)
> 
how about review ;)

>>> Until we have one such directive, then I think we should indeed DRY,
>>> ship empty files in there, and rely on the README being the only
>>> non-empty file by default in /etc/apparmor.d/local/.
>>>
>>> In most cases this text is generated by dh_apparmor
>>> (/usr/share/debhelper/autoscripts/postinst-apparmor) so it's easy to
>>> fix this at the distro level: upload dh-apparmor,
> 
> Done in Vcs-Bzr, will be part of next upload of src:apparmor to Debian:
> https://alioth.debian.org/scm/loggerhead/collab-maint/apparmor/revision/1711
> 
>>> wait for all reverse-build-deps to be rebuilt, and at some point
>>> request binNMUs to get rid of the long tail. I can do the
>>> dh-apparmor upload. Marco, would you like to handle the next steps?
> 
> It's early enough in the Buster cycle so there are good chances most
> of the dh-apparmor reverse dependencies get source uploads before the
> freeze ⇒ I say let's not bother and let time do its job.
> 
> Cheers,
>