many: add interfaces.SystemKey() helper #3456
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3456 +/- ##
==========================================
+ Coverage 77.26% 77.27% +0.01%
==========================================
Files 373 374 +1
Lines 25687 25707 +20
==========================================
+ Hits 19846 19866 +20
+ Misses 4097 4096 -1
- Partials 1744 1745 +1
Continue to review full report at Codecov.
|
| if err != nil { | ||
| buildID = "unknown" | ||
| } | ||
| mySystemKey.BuildID = buildID |
There was a problem hiding this comment.
A current property of the system is that on boot the security profiles are regenerated by snapd, but snapd is smart in that it checks to see if the profile changed and only recompiles if the profile changed. Is that property maintained with this new series of PRs even when the buildID is changing across refreshes? (I believe it is, I just want to make sure)
This property should be safe, because the apparmor_parser will handle changes to kernel capabilities across reboots and recompile and update the cache accordingly. I verified that the apparmor systemd unit has 'Before=sysinit.target' and snapd unit has 'Requires=snapd.socket' and snapd.socket has 'WantedBy=sockets.target' and man systemd.special indicates that sockets are setup as part of sysinit. The apparmor unit is of 'Type=oneshot', therefore it is guaranteed to have run and have finished before snapd is started.
What this means is that for this PR, if we maintain the current property of only recompiling only when the policy changes, then by the time snapd comes around to regenerate the profiles, it will not throwaway the recompilation work in the apparmor unit and instead only regenerate the ones that need recompilation, which is precisely what we need. Now, that does put a question mark on if snapd itself should be interrogating the kernel's features for apparmor at this time, but I think the answer is still 'yes' because it means we can at any point in the future decide to do things conditionally on apparmor kernel capabilities and we won't have to worry about fiddling with the system key file.
interfaces/system_key.go
Outdated
| // if the profiles need to be re-generated to match the new system. | ||
| type systemKey struct { | ||
| BuildID string `yaml:"build-id"` | ||
| ApparmorFeatures []string `yaml:"apparmor-features"` |
There was a problem hiding this comment.
Can we use 'AppArmor' instead of 'Apparmor' here and elsewhere. We aren't consistent across the code base, but by far most of the time we use 'AppArmor'.
interfaces/system_key.go
Outdated
| for i, f := range dentries { | ||
| mySystemKey.ApparmorFeatures[i] = f.Name() | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks good (if we don't read the dir then 'apparmor-features' is empty, otherwise it is populated, which is what we want).
interfaces/system_key_test.go
Outdated
| func (ts *systemKeySuite) TestInterfaceDigest(c *C) { | ||
| systemKey := interfaces.SystemKey() | ||
| c.Check(systemKey, Matches, "(?sm)^build-id: (unknown|[a-z0-9]+)$") | ||
| c.Check(systemKey, Matches, "(?sm).*apparmor-features:") |
There was a problem hiding this comment.
It might be nice to mock up the features dir and see if you get back what you expect when it exists and doesn't exist (would increase code coverage too).
Also, shouldn't this be named TestInterfacesSystemKey?
|
Closing for now until I have time to work on this properly. |
This PR implements a new interfaces.SystemKey() helper as discussed in https://forum.snapcraft.io/t/versionized-profiles/827/18?u=mvo
The SystemKey describes the system environment for which security profiles are valid. Right now the inputs are the build-id of snapd itself and the apparmor capabilities from the kernel. Adding extra inputs (like seccomp capabilities once we have those) is trivial and will result in a different SystemKey which is desirable.
Note that the SystemKey() string is currently the yaml that describes the system. We discussed hashing it but it seems like its actually nicer to keep it a string to make debugging easier and we can always change it, the signature of SystemKey() is a string (but happy to change to a digest again if that is preferred).
This is just the first step (I split it up for easier reviewing), the next steps are:
snap runwait for security profile generation if the system-key changes (snap: makesnap runlook at the system-key for security profiles #3471)