many: site local per snap AppArmor profile customisations

[alexmurray]

The main purpose of this PR is to try and provide a way for users to solve issues like https://github.com/snapcrafters/discord/issues/23 until we have a more complete solution within snapd itself.

FYI - The include if exists bits were resurrected from PR #11096 and cleaned up slightly.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.91%. Comparing base (a5a6458) to head (8b84a67). Report is 14 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #11619   +/-   ##
=======================================
  Coverage   78.90%   78.91%           
=======================================
  Files        1043     1043           
  Lines      134337   134364   +27     
=======================================
+ Hits       106004   106031   +27     
+ Misses      21721    21720    -1     
- Partials     6612     6613    +1     

Flag	Coverage Δ
unittests	78.91% <100.00%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[stolowski]

I would feel safer if the extra snippets were not manually written by the user, but came with the snap itself, and installed in a way that even non advanced users could activate them. This is probably a simplistic idea, but could we make a dirty-workarounds interface where the snap author could define apparmor and seccomp rules right in the plugs YAML and then the only operation that is left to the end user is just to connect it? It's not an equivalent to what you are proposing here, but I guess that the use-cases are about the same?

I fear that making this too easy for snaps could lead to snaps that "conveniently" have just one plug with all the snippets they need (completely opaque to users, with no promise about the contract made by connecting it anymore); such snippets (if shipped with snaps) would also be impossible to review by the store.

[jdstrand]

IMHO we should support an admin providing local customizations that aren't removed like they are now. We have had these conversations in the past and this was deemed outside of the philosophy because either the snap should be made to work or the snap sandbox should fixed. In the case of discord (but also several other snaps that I personally use), upstream isn't fixing it and we aren't fixing the sandbox (for years now).

In case it was missed, I said this in the https://github.com/snapcrafters/discord/issues/23#issuecomment-1086731458:

"The problem should be solved differently: a) discord should stop doing this (when appropriate snap interfaces aren't connected) or b) there should be a way to suppress the denials by the admin via a 'snap' command or c) there should be a mechanism for an admin to add additional rules to the profile.

Unfortunately, discord seems uninterested in 'a', the necessary kernel support still isn't there for 'b' and 'c' has historically been not part of the snap philosophy. I'm not sure why for 'c' since it is possible for people to add rules to /var/lib/snapd/apparmor/snap.... and reload the profile. They'll get removed periodically but that can be resolved through scripting (something I've had to resort to). People shouldn't have to jump through these hoops..."

I don't think this should be lifted up as a first class thing within snapd (whether as a snap apparmor command) or as a dirty-workaround interface for developers). We actually don't want regular folks doing this and we certainly don't want snap authors shipping break out rules. Instead, we want to allow a deliberate admin action to augment the policy.

Is it a support problem if an admin adds snippets? Possibly, but you could easily add something that the policy is tainted if specifying a profile snippet and then say 'undo that' before answering support questions.

The main historical concern, IIRC, is that people will copy/waste garbage rules and throw them into a file without thinking. Is this a real concern? Sure. But this can happen today:

1. people can today modify /var/lib/snap/apparmor/profiles/snap.* and reload the policy. This gets undone by snap refresh eventually
2. people can put policy into an apparmor abstraction that is used by the snap (eg, /etc/apparmor.d/abstraction/base) or similar. This doesn't get undone by snap refresh

'2' is a bit extreme but demonstrates that disallowing this to admins doesn't really help. '1' is easily worked around by deferring snap refresh and/or with some scripting (look for a needle in the profile, if it isn't there, put it in and your rules and reload). In fact, I've had to resort to this to workaround current firefox policy (I created a PR and it is merged but not in a stable snapd yet), current electron policy and https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/1849753/comments/22 (same problem applies to snap-confine; I created an RFC PR for this but it wasn't picked up: https://github.com/snapcore/snapd/pull/10029; btw, this problem is way worse than noisy logs-- it breaks my use of the golang classic snap under certain circumstances).

As for whether or not the rules compile at all, this is easily checked by snapd itself: if there are admin rules and the policy doesn't compile, remove the admin rules and try again.

I've always felt that an admin should have control of the policy because a) an admin should have control of the system and b) an admin can do it today (just in a roundabout way that is infuriating), but I was told 'no'. For 'a', in apparmor upstream, Ubuntu understood user concerns and frustrations with default policy and introduced the concept of 'Site-specific additions and overrides' through the local/README mechanism (ie, what @alexmurray was clearly inspired by). Millions of users today and for the last many years get the default policy as shipped in Ubuntu, Debian, SUSE, etc, but admins can add some rules to tweak it for their needs. Anecdotally, with my long tenure on the Ubuntu security team, I can honestly say that I cannot recall a bug report where I saw garbage rules in the profile (and I read a lot of bugs).

Perhaps I'm missing some historical details or have selective memory, but lifting up that 'snaps should be fixed' or 'we should fix the sandbox' only works if it actually happens. Discord isn't budging, but more importantly, there is no progress on a way to silence denials (not to mention some PRs/features aren't being picked up (PR #10029)).

Now, to be clear, I think that snapd absolutely should manage the policy generally and that snaps should work out of the box. I don't think that discord not doing something is a reason to accept this PR, but I do think that if there is no time to fix the sandbox, then taking the hard line that "admins can't augment the policy cause we've got you covered" really doesn't fly. Please understand that I'm not trying to point fingers or say that priorities are off or whatever. I'm just trying to point out that priorities being what they are, it's clear that there are sandbox issues that aren't being addressed after years of knowing about them. Whether or not you agree with me that by definition admins should have this ability, letting admins augment the policy would alleviate some frustration with no real downsides (support concerns can be addressed, policy loads can be checked, admins can workaround and do it today already).

ADDENDUM: "I've always felt that an admin should have control of the policy because a) an admin should have control of the system and b) an admin can do it today (just in a roundabout way that is infuriating)". There is a stronger argument for non-classic systems that this should be different. I would argue that it actually is not different since on a core system you want to control admin access, not what an admin can do when on the system. Eg, you could change this feature to only be on classic systems, but why? If someone can ssh into a core system, they can still add a systemd timer or unit to add and load workaround policy. The barrier should continue to be not allowing an admin on the core device (disable ssh, etc).

[alexmurray]

Thanks for the very detailed reasoning @jdstrand - I wholeheartedly agree - essentially admins can already change snap AppArmor profiles but it is clunky - so let's make it a little easier but not too easy such that it should lead to abuse.

Also re #10029 - the only reason this hasn't got picked up is resourcing, not some fundamental opposition AFAIK - I hope to try have another look at it myself once #11096 is done (unless someone from the snapd team gets there first :wink:)

[mardy]

Thanks @jdstrand and @alexmurray for explaining your reasoning. While I agree with all what your write, I would still prefer if before proceeding with this we sat down and wrote a list of the use-cases for admin snippets. I'm not really concerned about security here, because indeed a sysadmin can already alter the profiles and much more; my concern is that it may be taht for some use-cases AppArmor is only a little part of the picture, and then people will need a similar feature to control the seccomp rules, the mount namespace, the device cgroups, etc. So, if we are to support these use-cases (if they exist), we might want to spend some time on designing this first.

Back to the Discord snap issue and to the proposal of the dirty-workaround interface: yes, that's not such a great idea :-), but if the most pressing use-cases are about spamming the logs, what about having an interface just to suppress them? That is, an interface that would work like this:

plugs:
  - suppress-denials:
      - interface: dirty-workaround  # possibly another name :-)
      - apparmor-rules:
          # Each of these lines will be added to the profile with a "deny" in front of them
          - capability sys_ptrace
          - /var/run/my-socket r

Since this plug would just be adding deny rules, it could also be autoconnected.

[jdstrand]

Also re #10029 - the only reason this hasn't got picked up is resourcing, not some fundamental opposition AFAIK - I hope to try have another look at it myself once #11096 is done (unless someone from the snapd team gets there first 😉)

Thanks and sure; the general "fixing the sandbox" comments were about resourcing (which I understand). The admin rules comments specifically were about snap philosophy.

[jdstrand]

While I agree with all what your write, I would still prefer if before proceeding with this we sat down and wrote a list of the use-cases for admin snippets.

Obviously feel free to handle this how you like, but I will say that IMHO apparmor is the main one due to the potential for log noise. seccomp could in theory also be done, but a) in my experience it isn't very noisy (and when it is, it is more fundamental to the snap), b) there are no per-rule deny rules. The other backends don't produce log noise and IME when they aren't working right for the snap, the snap is more fundamentally broken.

So, IMHO, allowing this isn't so much about policy authoring (that is for snapd) but rather about small tweaks for site specific problem cases. Honestly, if we had per profile apparmor quieting, we probably wouldn't be having this conversation (but my comments wrt admin rules would still apply) so it makes sense for you to lift up deny rules as the use case. But apparmor quieting is still planned (timeline unknown, at least to me); I still don't care for the "deny rule interface" approach because a) it would constitute a workaround for what we eventually want but is elevating the workaround to a first class concept, b) deny rules are tricky to get right and snap authors may misapply them (we've avoided exposing apparmor directly to devs) and c) it requires that the snap author be responsive and predict what the site local changes might be. In the case of Discord, due to 'c', this probably wouldn't address user concerns since Discord has been unresponsive thus far, so why would they be now?

One could say that I'm being inconsistent by saying "let's not expose raw apparmor to devs" but advocating for exposing raw apparmor to admins. I think there is an important distinction: snap devs should always work within the system to adjust their apps and file bugs against the sandbox so they work properly out of the box. Admin rules are about letting an admin be an admin on their system (for something that is clearly providing friction) to allow tweaks and there is no expectation or requirement that admins use the feature. snapd could enforce that only deny rules be permitted in the snippet, and this would help people with the discord issue, but it would be artificially limiting since sometimes an admin will want to add an allow rule (as is the case for me with Firefox while I wait for the next snapd to come out).

Should we go all out and let an "admin be an admin" and allow changes to any part of the sandbox? Possibly but that is a radical departure with lots of complexity and foot guns. My argument for "let an admin be an admin" is not advocating for that. snapd should control it's sandbox; my point is let's just relieve some pressure on a known pain point by allowing an admin to do something they can do already without jumping through so many hoops.

[mardy]

I still don't care for the "deny rule interface" approach because a) it would constitute a workaround for what we eventually want but is elevating the workaround to a first class concept

Yeah, this point is hard to disagree with :-)

I'm personally fine with this PR; the biggest issue is that such a mechanism could be abused via "social engineering", where a user could be led into installing an override file that opens the door to a malicious snap. But then, as @jdstrand wrote, we already have a very similar (and actually even more dangerous, as it's not limited to a single snap) override mechanism in the AppArmor configuration itself (those local files under /etc), and anyway, if the user can be convinced to place untrusted files into system locations, that can already be exploited in innumerable ways.

[ernestl]

@alexmurray, should this be pursued further? If yes, is this appropriate and required for snapd 2.63 along with the shift to apparmor 4.0?

[zyga]

Please rebase/merge master to get accurate tests

[zyga]

I should have noticed the 🎂🎂

@alexmurray do you plan on rebasing this or should I take over?

[alexmurray]

I'm happy to rebase it. Will try take a look tomorrow.

[alexmurray]

Rebased on master.

[bboozzoo]

This will need documentation on how to use the feature. Specifically, the local snippet will likely be added only after the user notices issues, for which we'll need to provide clear explanation of what the steps to take are to activate it (AFAIU just restarting snapd.apparmor.service would do), and what the possible side effects are. I'm bit wary of how the users always end up copy pasting random snippets from the internet, because some random poster on reddit/ubuntu forums claimed this fixed their issues (but most of the time in most unreasonable way). We should be very explicit that it's easy to break things this way.

It'd probably be useful to extend debug-tools/snap-debug-info.sh script to include the content of any local snippets.