We want to support versioned snap security profiles. This makes some problems simpler. When e.g. the format of a file changes in an incompatible way (like seccomp) we change the version and write the file out into a new place. When the core gets reverted the previous generation of the profile is still in place.
The design proposal is the following:
In order to generate the digest we hash the content of lexicographically sorted inputs. E.g.:
a: 1\n
b: 2\n
we use profile directories like /var/lib/snapd/profiles/<digest>/{seccomp,apparmor}/
the directory contains a file digest-keys.txt with the inputs that lead to the digest
snapd writes the profiles to this directory on snap install and also on snapd startup
snap-confine uses the same digest generator and load the profiles from that directory - if the digest directory does not exist (yet) snap-confine will wait for some seconds for it to appear. this may happen if e.g. a snap service starts before snapd starts and snapd has not generate the profiles just yet
we will also try to generate the profiles ahead of reboot, e.g. by calling a new helper snap-generate-profilesfrom the new core snap when installing a new core
A few complements, with points we didn’t manage to clarify over our call today:
The advantage of snap-generate-profiles (or is it snapd --generate-profiles?) compared to just generating the profiles when snapd itself starts is still a bit nebulous. Calling it externally means creating another race since that external snapd doesn’t own the data it depends upon and the main snapd may still change it.
We need to make snap-confine wait for some amount of time and warning about it if its required profile is not yet in place. That ensures we can handle some corner cases such as updates to the kernel that happen behind the back of snapd in a classic system.
Ideally profiles would be created and updated atomically, so that once we have a profile directory in place we know it’s complete.
Not sure if we should use digest-keys.txt or if perhaps just key would be enough.
That point about updating profiles atomically could mean using a slightly different structure for the storage of the profiles, since we can’t update a directory atomically. We’d need a symlink to the directory, so we can recreate the profile and then update the symlink.
This adds a bit of complexity to the implementation, for interface developers and for users/admins/auditors but I think this is likely acceptable.
The proposed implementation might leave snapd in the critical path if the profiles aren’t present and snapd fails to start, but this could be handled if either (or both) a health check was in place to automatically revert to a known good snapd or if snap-confine ran the external tool (I guess snap-generate-profiles in the above) itself if the profile didn’t exist for the digest.
I maintain the most important issue that the mknod/2.25 issues underscores is that the wrong profiles are being used with the wrong snapd/snap-confine and that we need to make sure that we always use the correct profiles. This versionized profiles/digest approach cleanly handles that for both daemon and non-daemon commands, which the ‘systemd unit start after snapd’ does not. We can choose when to rev values in the inputs however we see fit from just syntax changes all the way to every new snapd version. The most determinism would be if revved every time, but we could start with simply at incompatible syntax versions and security-related or otherwise important profile changes.
With the above, the system and policy should always be in a predictable, robust state, but then we also need to make it easy for developers/admins/auditors to find the policy that is in use. One idea would be to have a current symlink but we’d need to be careful to make sure it is up to date, perhaps by having the snap-generate-profiles command manage it. Another idea is to have a tool to introspect the system and report which digest would be used.
This should not add any complexity to interface developers or to users/admins. Why would it?
For people trying to audit or debug the system, yes, they need to be aware that profiles need to change over time, but I’d argue that it’s actually better for auditing purposes that the alternative versions (older or newer) of the profile aren’t just going away.
Again I don’t see how that’d be the case. It doesn’t seem to add any steps in that sense that aren’t already there. For example, today if the profiles aren’t in place when something starts, that something won’t start.
Yes, this is the simplistic view of the problem. The more general view was described here:
The general statement of the problem is that software changes and the format of data they use also changes in incompatible ways. That incompatibility may sometimes be observed going forward, when version N+1 doesn’t understand the data of version N, but it’s more frequently observed when going backwards, when version N doesn’t understand the data of version N+1, because it’s easy to do a migration forwards, but harder to it backwards or introduce completely new ideas while preserving a limited understanding for the old.
That’s pretty much what is being proposed. One detail here is that “every time” is not just about snpad… it needs to take into account every single system dependency that plays a role: snapd, kernel, udev, etc.
Unfortunately the real world is not so kind. The robust and predictable state can easily get completely unusable after a simple boot onto a different kernel, which may happen completely behind the back of snapd.
@mvo The conversation above reminds me: we didn’t cover the problems that will arise from rolling back into out-of-date profiles that were generated before system configuration (interface connections, etc) changed. Perhaps we’re doing a lot of work without actually solving the problem appropriately, which is a hint of time waste. We may need to rediscuss this.
Interface developers typically will modify the policy on the system in /var/lib/snapd/*/profiles directly when developing the policy. The digest approach complicates that, but not in an insurmountable way. It does mean we need to document the new way so people can see where the profiles are. It only affects admins and users who care about the policy on the system, but again, not in an insurmountable way. The point of my comment on this is to capture what needs documenting.
There is the “We need to make snap-confine wait for some amount of time and warning about it if its required profile is not yet in place” idea-- this puts snapd in the critical path. I put forth one way to get out of the critical path in addition to @mvo’s proposal, snapd-confine could itself strategically call snap-generate-profiles or some other appropriate tool that doesn’t rely on snapd running when the profile isn’t there, so you don’t have a timeout and services can start without snapd.
My point is what is missing in all other views of the problem. The quoted ‘general view’ only talks about ‘format data’ but it isn’t just about parseable policy; it is also about the content of the policy. Consider a rule is added in a new version of snapd that fixes a security issue in the policy where that rule is parseable in previous revisions. We want to rev the digest input there so that snaps don’t race and run with the old policy.
Yes, I was summarizing why I consider the proposed change a good one.
I don’t quite understand this comment. @mvo is certainly aiming for this in the proposal, and I agree with the proposal (with a few additional details), and we can certain expand on the digest input as we see fit in the future… What’s the problem?
Perhaps you are confused because we were responding at the same time and it seemed like I was countering you? Regardless, I think this approach does add complexity but, again, in a way that IMO is acceptable.
They would be able to do that in /var/lib/snapd/profiles/current/*. Should be just a different path.
Once more, no, it does not put snapd in the critical path. Let me simplify so the point is more clear: assume the system boots and we need profiles that are not in place to handle the kernel that was chosen out of band. Something somewhere somewhen needs to generate those profiles for these applications to work at all. We need to solve that.
Sorry, I’m probably still missing your point then. Today we already generate new profiles when snapd changes. Isn’t that sorted already? The problem we’re addressing now is the fact these profiles may be completely incompatible on a revert, and everything in the system would suddenly stop working before snapd has a chance to fix them.
Your earlier point commented above the content you quoted was that if we generated profiles every time everything would be beautiful and stable. My point is that no, it won’t be beautiful and stable because changes may require the system to suddenly use a different set of profiles which are not available.
There’s not much of “me” in this conversation. We’re just exploring the problem and trying to come up with good solutions. I don’t think we’re there yet.
Historically that has been snapd. The proposal from @mvo tries to address that and does except for when the profiles don’t yet exist and there was talk of “waiting”. It wasn’t clear what we were waiting on-- since snapd historically generates the profiles, it sounded like waiting on snapd. If that is not what you meant by waiting, then fine, it isn’t in the critical path. If it is waiting on snapd there, I gave an idea on how not to wait and just call the tool directly.
Yes, snapd regenerates the policy, but in a racy way since there is nothing today that enforces that snapd has regenerated the profiles before a snap daemon or non-daemon starts. This means that if in 2.x there was a policy flaw and 2.y has a policy fix for the flaw (that doesn’t constitute a syntax change), then because of the current race a reboot might allow some things to start before snapd gets around to regenerating the policy. This is wrong and my point all along has simply been that fixing that happens to fix the other issues. I was trying to express that this proposal can nicely address this race condition for both ‘daemon’ and ‘non-daemon’.
My point was meant to include (the ‘above’ intended to mean @mvo’s proposal) that things are beautiful and stable because we would wait for (or have snap-confine call out to generate) any unavailable profiles. Sorry if that wasn’t clear.
I fully agree that we haven’t quite fleshed this whole thing out and that in particular we need to consider the current directories and reverts better as well as digest reaping. I was expressing my thoughts in support of the general idea of using a digest, revving input into the digest in intelligent ways, and loading/using the digests to make sure we are using the right policy.
I can say that, I think that if we revert the mknod changes in 2.25 everywhere and if the digest is in 26, we will be ok if we continue to let /var/lib/snapd/{apparmor,seccom}/profiles be used by the old snapd/snap-confine (<=25) and new snapd/snap-confines (>= 26) always uses /var/lib/snapd/profiles/<digest>/{seccomp,apparmor}/. Eg (assume 25 doesn’t have digest support and 26 does):
25 -> 26: snapd/snap-confine uses digest dirs and makes sure the policy is in place
25 -> 26 -> 25: snapd/snap-confine uses old directories
25 -> 26 -> 25 -> 27: snapd/snap-confine uses digest dirs and makes sure the policy is in place
27 -> N>25: snapd/snap-confine uses digest dirs and makes sure the policy is in place
At some point, <=25 will rotate out and will not be able to revert to them since they will be garbage collected after enough refreshes. At that point the policy in the old directories should be reaped. If for some reason an older revision is locally (and assertedly) installed, it should all still work-- it will use the old directories for policy until enough new refreshes happen and it is reaped.
I feel like we’re going in circles somewhat, so I’ll try to be brief: nothing we’re doing now puts something new into the critical path. If profiles are not in place, they must be generated, and that’s the status quo. The lack of that generation means things simply don’t work, and this needs to be fixed. Whether we call something to generate those profiles or that something runs by itself, we need to wait for them to be generated. Renaming snapd to not-snapd-but-generate-profiles doesn’t really change that. We’ll continue to generate profiles ahead of time whenever we can, and the plan outlined above is incomplete because we need to nail down the question about out-of-date profiles. We’ll be exploring ideas further.
I was saying when we try to fix this that I agree we should not put snapd, the service, in the critical path. The language around ‘waiting’ and my own previous thoughts on adjusting app snap systemd units to specify starting after snapd led me to think we might be adding snapd to the critical path.
I think it is entirely possible to generate the profiles ahead of time. I also think it is possible to generate missing profiles just in time (ie, snap-confine reaches out to some tool), which is an idea I was trying to capture that @mvo and I discussed in a hangout.
I agree we have been talking in circles somewhat and it’s clear to me now we are in alignment.
This is not being a very productive conversation, Jamie. No, we’re clearly not aligned, and as I explained multiple times these points misrepresent the issue at hand. We won’t be able to evolve the conversation further here, though.
As a hint of that, I explained above that if a kernel is updated out of band, snapd may easily be completely unable to update its profiles in time to cover the different kernel. Either it generates, or the applications will need to wait for the generation, or the applications will crash. So no, snapd won’t be able to generate profiles ahead of time in all situations, and that’s part of the problem.
Man we aren’t connecting today! Of course, a call is fine.
What I thought you meant was that a kernel being updated out of band was a future consideration where additional keys would be added to the digest input. Therefore, that can be handled by future iterations on the implementation of this proposal. Put more simply: anything we find interesting as part of the digest can be expressed there (how and what TBD) and the missing profiles for a new digest will need to be generated. Not by snapd (cause don’t want it in critical path) but instead by something else (which my snap-confine calling out to snap-generate-profiles idea could do, but there are other ways to do it too).
We made snapd a “Type=notify” service recently. This means that snapd will now communicate with systemd when it is fully initialized. And snapd is only sending the “READY=1” message to systemd after the security profiles are re-generated.
With that above change in place, I added a simple PR (https://github.com/snapcore/snapd/pull/3442) that uses this new feature. Now snap run will wait until snapd.service is in no longer in the ActivateState=activating state. Snapd will re-generate the security profiles on startup to ensure they match current snapds understanding, kernel etc. If snapd is not running at all (failed state/inactive state) snap run will just continue (we could make it fail in this case but that would mean if snapd ever has a bug and fails to start all applications will no longer work). The downside of this approach is that it will wait for all profiles.
The alternative to this wait-for-all profiles approach would be (as suggested by Gustavo) a helper snap-generate-profiles that snap-confine would call/wait-for for the specific snap that is about to be run if it detects that the profile is out-of-date. This approach is more elegant but it raises some questions:
how do we detect if the profile is outdated?
we need to read the state.json in order to generate the profiles. Zyga suggested it will be ok to just open it read-only and because we write it in an atomic way we will always have a valid snapshot
well we have also the problem that snapd and the helper might try to generate profiles at the same time based on a different state. Unless the plan is to always just do this from snap run but then I don’t know if there are issues around trying to write profiles for different apps of the same snap for bits of the profiles that are one per-snap and not per-app.
As mentioned above, the advantage of having such an external tool is still nebulous, without a clear reason why doing that would be better than simply waiting for snapd. So far it still sounds like just one more moving piece increasing the complexity of the boot process, when snapd is doing that either way.
That alone is dangerous, as we’re putting snapd in the pipeline of every single boot for every single service packed as a snap. The more ubiquitous snaps become, the more dangerous this is. It’s also a bit unfortunate in the sense that it will slow down the boot process.
Instead, I suggest introducing a mechanism through which we detect whether we need to wait or not. We can discuss the details, but the idea is to have snap run or snap-confine quickly and independently figuring whether profiles are up-to-date, and only going into the waiting mode if that’s not the case. Since this will be a rather rare event, another advantage of doing this is that we can increase the amount of time we’re willing to wait for profiles to be sorted, since it’s more likely that proceeding without these profiles would mean breakage.
This makes a lot of sense, thanks for these ideas. In order to detect profile changes we could go back to the original idea about a profile digest. When we write profiles we would also write something like /var/lib/snapd/profiles.digest that contains the digest of the current profiles. We would then introduce a new helper interfaces.ProfileDigest() which snap run calls. If that profile digest and the one in /var/lib/snapd/profile.digest are different, it knows it needs to wait, otherwise it can just continue. The helper would be very much like what was done in https://github.com/snapcore/snapd/compare/master...mvo5:profile-digest?diff=unified&expand=1&name=profile-digest - if that sounds sensible, I will expand #3442 accordingly.
Something along those lines sounds sensible, but “profiles.digest” sounds a bit misleading. We’re not really talking about the digest of the profiles themselves, but rather about what environment produced them. Perhaps something like “system-key”, which is a digest produced from an aggregated view of the system. This would be built based on:
snapd build hash (same we use to decide to regenerate profiles)
core snap version revision (for REEXECing… what’s the relationship with the above today?)
kernel version
… what else?
Then, we might store it inside /var/lib/snapd/system-key whenever snapd finishes acknowledging the given system changes, whatever that means (doesn’t need to be constrained to profiles).
It’s worth noting that interfaces being connected and disconnected do not modify the system key, and that sounds desirable and another reason why “system key” might be appropriate. We don’t want to get “snap run” into the business of introspecting interface data to figure whether to wait or not. If the profile is out of date because a connection didn’t finish properly yet but the system key is the same, it should just go ahead and use the profiles that are already built. If the system key is the same, the profiles are compatible, and this is no different from performing a connection after the snap is installed.
At the risk of prematurely optimizing, at some point (not necessarily today, but maybe today) we might prefer to consider interrogating the kernel via sysfs/proc at runtime for capabilities instead of simply the kernel version when generating “system-key”. On Ubuntu Touch we tried hard only to regenerate the AppArmor profiles only when we had to (ie, when the kernel capabilities for AppArmor changed or the profiles themselves changed) because on constrained armhf devices, depending on the profile, it could take up to 1 second to recompile the profile. With a system with 60 profiles (ie, snap app commands), that takes a while.
Put more simply, most kernel refreshes won’t require a profile regeneration and we can simply use the cache files and it would be nice if we didn’t needlessly regenerate if we can pull it off.
With the seccomp bpf precompilation it shouldn’t be nearly as expensive to regenerate (the profile syntax and what seccomp does is incredibly simple compared to apparmor), but with the upcoming changes to the kernel regarding seccomp complain mode we’ll be in the business of generating different seccomp policy based on kernel capabilities. Perhaps it makes sense to do that at runtime instead of kernel version as well?
I think apparmor_parser --version is needed to handle new policy syntax only available in newer apparmor where a new core snap has new apparmor and policy syntax and we revert to an older core snap that doesn’t.
You’re right that there’s no reason to look at the kernel version. We should really be looking at real factors that will cause profiles to be generated differently. Even the apparmor_parser version would be irrelevant unless we’re actually using that as a factor to do something else at profile generation time. That’s not the case at least today, but we’ll probably need to do that once we support different and incompatible syntax. For that we also need to find a way to discover the version of apparmor_parser that doesn’t involve running it every time we “snap run” something.
