apparmor,release: add better apparmor detection/mocking code

[zyga]

This branch adds improved apparmor detection and mocking code. For
compatibility the existing code in the release package is left as a
pass-through to the new functionality.

The main improvement, apart from not being in the release module where it makes
little sense anymore (as it is no longer based on static inspection of the
distribution name) is that we can have varying levels of support.

Distributions that don't use apparmor, distributions that use upstream apparmor
and distributions that use ubuntu-patched apparmor are all detected separately.
Having this distinction will allow us to enable apparmor whenever it is
available and use graceful fallback when a specific feature is not available.

Building on this work apparmor will no longer have to be disabled on Debian and
in openSUSE. In addition as those distributions update to more recent kernels,
full confinement will kick-in automatically.

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

[jdstrand]

Elsewhere we use dirs/dirs.go for this sort of thing.

[jdstrand]

While not currently used in our policy, we should maybe list rlimit here for completeness. rlimit is an old feature that was added in apparmor 2.3 (the same time the /sys/kernel/security/apparmor/features was added). I confirmed that Ubuntu 12.04 3.2 kernels have it and unpatched 4.9 Debian has it.

[jdstrand]

It is probably sufficient to say if the features directory isn't present, then to return None, but aa_is_enabled (http://bazaar.launchpad.net/~apparmor-dev/apparmor/master/view/head:/libraries/libapparmor/src/kernel.c#L106) is a little more robust and might be useful for logging why we are returning None. Note that aa-enabled is a small C program wrapper around aa_is_enabled, so calling it directly from the core snap might be best.

[jdstrand]

It might be nice to return the 'why' here for future logging. Eg:

    return None, fmt.Errof("...")
    return Partial, fmt.Errof("missing x, y, z")
    return Full, nil

[jdstrand]

The direction of this commit is fine. Couple of small comments.

[zyga]

Ooops, what did I do here :)

[codecov-io]

Codecov Report

Merging #3808 into master will increase coverage by 0.03%.
The diff coverage is 87.69%.

@@            Coverage Diff             @@
##           master    #3808      +/-   ##
==========================================
+ Coverage   75.82%   75.85%   +0.03%     
==========================================
  Files         402      403       +1     
  Lines       34793    34835      +42     
==========================================
+ Hits        26381    26425      +44     
+ Misses       6539     6537       -2     
  Partials     1873     1873

Impacted Files	Coverage Δ
release/release.go	100% <100%> (+10.81%)	⬆️
apparmor/probe.go	86.44% <86.44%> (ø)
interfaces/sorting.go	98.71% <0%> (-1.29%)	⬇️
wrappers/binaries.go	79.54% <0%> (+6.81%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0586e14...6fc7559. Read the comment docs.

[jdstrand]

+1 but there are two questions inline.

I'll also note that since this code will support the possibility of booting in a Full kernel, rebooting into a Partial kernel, rebooting into a None kernel, then back into a Full kernel (and any combination), the apparmor_parser should handle this for us with how the apparmor unit calls it, since it will invalidate the cache if the version in the cache is different from the calculated version on the running system (the version is calculated in part based on the features of the kernel).

[jdstrand]

Can you comment (not necessarily in the code, but in the PR) why you weren't able to use dirs/dirs.go?

[zyga]

Because of recursive imports: apparmor->dirs->release->apparmor

[jdstrand]

I'd like to see a prominent (ie,non-DEBUG) log message stating that snapd is going to force all snaps to be in devmode, with the reason why. Because you discard the error here, we can't do that.

Are you planning on doing this in a follow-up PR?

[zyga]

I have this already in the branch that uses this. I think that once we get to the warnings framework it will be a proper user-visible message. EDIT: by user-visible I mean that this will be in the inbox until actually read/dismissed and a proper set of "you have an important message" notifications will be displayed.

[mvo5]

Thanks for the branch. Some ideas/suggestions inline.

[mvo5]

This could be written in a single line: if _, err := ...; err != nil {

[zyga]

Will do

[zyga]

Done

[mvo5]

This feels strange from an API perspective. One could argue that there is no error here, the fact that there is no apparmor dir is not an error, its what is expected when there is no apparmor support. The api is also strange in that the FeatureLevel is actually valid even when an error is returned (which is not common at all in go code).

Maybe FeatureLevel can become a richer type? Something like:

type FeatureLevel struct {
   Summary string
}
type Full struct {
   FeatureLevel
}
type Partial struct {
    Featurelevel
}
type None struct {
   FeatureLevel
}
return Partial{Summary: fmt.Sprintf("apparmor features missing: %s", strings.Join(missing, ", "))

or similar? And only returning an error if there is an actual error condition (or not returning one at all as it seems we have no real errors right now in the code)?

[zyga]

I agree about the error comment and I'll see what I can do to make this nicer. I really like the simplicity of the enum approach so far and would prefer not to cross YAGNI until I actually need it.

[zyga]

I'm working on something nice, I'll push it shortly.

[zyga]

Done! I re-factored the code a little and I think you will be happy with the result.

[mvo5]

Why not assign featureSysPath = filepath.Join(..) here already? The real featuresSysPath is stored so that should be fine, no?

[zyga]

No reason, it's just lifted from the older Mock function this originated from. I'll improve this.

[zyga]

Done

[mvo5]

I don't think you don't need these "break"s, go has no automatic fallthrough.

[zyga]

Right, this was just my C muscle memory. I'll clean this up.

[zyga]

Done

[mvo5]

Some comments/suggestions inline. I would prefer waiting for two +1 for non-trivial PRs. I know we sometimes bend the rules, but usually it is for test-only PRs or trivial stuff (what is considered trivial is of course a matter of debatte :)

[mvo5]

Why not just return the struct instead of a pointer to avoid the non-nil checks below?

[zyga]

Just the feeling that structs containing arrays/slices should be passed by pointer, not by value.

[mvo5]

Under what circumstances would ks be nil? And why do we care here and not in any other place in the snapd code? I guess you want to be able to do something like https://play.golang.org/p/Z3OzKNPkm7 but it is not clear to me why we need it. Also, we could simply make it a non-pointer receiver to force a non-nil value here.

[zyga]

I return nil when enabled == false, this is just a concept I learned from @chipaca recently, that you can make calls on nil receivers as long as the method handles that. If it is too magic I can change that to be more obvious / typical (no nil pointers )

[zyga]

I return nil when enabled == false, this is just a concept I learned from @chipaca recently, that you can make calls on nil receivers as long as the method handles that. If it is too magic I can change that to be more obvious / typical (no nil pointers )

[mvo5]

Same question here as above.

[mvo5]

If this is the API we use externally, we could consider to limit the API usage we expose in apparmor to exactly this. I.e. do not export probeKernel, do not return an error (that we ignore anyway) but instead just have "apparmor.Evaluate()" (or similar). YAGNI etc :)

[zyga]

Ah, I plan to store the result of ProbeKernel (probably in the backend) and then use it in interface methods. This is just the first step towards that. Evaluate will be used alongside other tests, that check if a specific feature is on or off.