interfaces: clean system apparmor cache on core device #4060
Conversation
The system apparmor cache relies on the mtime of the input files and will only check for the newest mtime. However this is problematic on rollbacks when we rollback to a core the mtime of the apparmor files in the rollback core will be older so that apparmor cache does not get updated. This means that on rollback of core we run e.g. snap-confine with the appamor profile of the core we just reverted from. Ideally this would be fixed in apparmor itself, however as a short term fix we can simply clean the cache (it usually contains just sbin.dhclient and snap-confine anyway) and let apparmor rebuild the cache dynamically. See also https://forum.snapcraft.io/t/core-snap-revert-issues-on-core-devices
Codecov Report
@@ Coverage Diff @@
## master #4060 +/- ##
==========================================
- Coverage 75.77% 75.76% -0.01%
==========================================
Files 433 433
Lines 37311 37318 +7
==========================================
+ Hits 28272 28275 +3
- Misses 7065 7068 +3
- Partials 1974 1975 +1
Continue to review full report at Codecov.
|
interfaces/apparmor/backend.go
Outdated
| // See LP:#1460152 and | ||
| // https://forum.snapcraft.io/t/core-snap-revert-issues-on-core-devices/ | ||
| if snapName == "core" && !release.OnClassic { | ||
| if l, err := filepath.Glob(dirs.SystemApparmorCacheDir + "/*"); err == nil { |
There was a problem hiding this comment.
filepath.Glob(filepath.Join(dirs.SystemApparmorCacheDir, "*")) perhaps?
There was a problem hiding this comment.
Nitpick: use 'l' as a variable can be hard to disambiguate from '1' or 'I'. I suggest using 'fn'.
| // for this snap-confine on core | ||
| s.InstallSnap(c, interfaces.ConfinementOptions{}, coreYaml, 111) | ||
|
|
||
| l, err := filepath.Glob(filepath.Join(dirs.SystemApparmorCacheDir, "*")) |
| // confused too easy, especially at rollbacks, so we delete the cache. | ||
| // See LP:#1460152 and | ||
| // https://forum.snapcraft.io/t/core-snap-revert-issues-on-core-devices/ | ||
| if snapName == "core" && !release.OnClassic { |
There was a problem hiding this comment.
I wonder if it would make sense to check if the snap type is "os" instead of checking the snapName (I can see why you picked snapName-- the conditional immediately before does the same thing). I realize we are much farther out for non-Ubuntu Core devices with alternate os snap than alternate base snaps, so I won't block on that, but mentioning it for your consideration.
interfaces/apparmor/backend.go
Outdated
| // See LP:#1460152 and | ||
| // https://forum.snapcraft.io/t/core-snap-revert-issues-on-core-devices/ | ||
| if snapName == "core" && !release.OnClassic { | ||
| if l, err := filepath.Glob(dirs.SystemApparmorCacheDir + "/*"); err == nil { |
There was a problem hiding this comment.
Nitpick: use 'l' as a variable can be hard to disambiguate from '1' or 'I'. I suggest using 'fn'.
interfaces/apparmor/backend.go
Outdated
| // https://forum.snapcraft.io/t/core-snap-revert-issues-on-core-devices/ | ||
| if snapName == "core" && !release.OnClassic { | ||
| if l, err := filepath.Glob(dirs.SystemApparmorCacheDir + "/*"); err == nil { | ||
| for _, p := range l { |
There was a problem hiding this comment.
Nitpick: while there shouldn't be any non-files in /etc/apparmor.d/cache, I wondered if it would make sense to verify if IsRegular. You are checking the return value of os.Remove(), so this isn't strictly necessary, and looking at the implementation, it just calls unlink followed by rmdir, so there isn't a security risk with what you are doing. Not a blocker; just mentioning for your consideration.
interfaces/apparmor/backend_test.go
Outdated
| l, err := filepath.Glob(filepath.Join(dirs.SystemApparmorCacheDir, "*")) | ||
| c.Assert(err, IsNil) | ||
| c.Check(l, HasLen, 0) | ||
| } |
There was a problem hiding this comment.
Perhaps it is worth adding unexpected data in dirs.SystemApparmorCacheDir? Eg, a symlink, a directory, a pipe, ...
| err = os.MkdirAll(dirsAreKept, 0755) | ||
| c.Assert(err, IsNil) | ||
| symlinksAreKept := filepath.Join(dirs.SystemApparmorCacheDir, "symlink") | ||
| err = os.Symlink("some-sylink-target", symlinksAreKept) |
The system apparmor cache relies on the mtime of the input files
and will only check for the newest mtime. However this is problematic
on rollbacks when we rollback to a core the mtime of the apparmor
files in the rollback core will be older so that apparmor cache
does not get updated. This means that on rollback of core we run
e.g. snap-confine with the appamor profile of the core we just
reverted from.
Ideally this would be fixed in apparmor itself, however as a short
term fix we can simply clean the cache (it usually contains just
sbin.dhclient and snap-confine anyway) and let apparmor rebuild
the cache dynamically.
See also
https://forum.snapcraft.io/t/core-snap-revert-issues-on-core-devices
A full spread this for will be in #4059 - however we need to run this via spread-cron as it needs some extra inputs and also does not need run with each PR.