/snapd

apparmor,release: add better apparmor detection/mocking code #3808

Merged
merged 12 commits into from Aug 26, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+79 −24
Split
Viewing a subset of changes. View all
Prev

apparmor,release: refactor apparmor probing / evaluation code 

This patch is inspired by an idea from Michael Vogt. The probing code
now returns an apparmor.KernelSupport object which can be queried for
distinct facts: is apparmor enabled, is specific feature available.

The object can also be asked to evaluate overal support as required by
snapd.  This last operation matches the previous model where a tri-state
answer is provided (None, Partial, Full) as well as a textual summary
with more human-readable information ("this-and-that feature is
missing").

This will also allow specific interfaces to behave appropriately in
light of presence or absence of specific features.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
  • Loading branch information...
commit 6fc7559580e89f60f8055f00371153d64d67d111 @zyga zyga committed Aug 26, 2017
View
66 apparmor/probe.go
@@ -24,6 +24,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
+ "sort"
"strings"
)
@@ -57,24 +58,59 @@ var (
}
)
-// Probe checks which apparmor features are available.
-//
-// The error is returned whenever less-than-full support is detected.
-func Probe() (FeatureLevel, error) {
- if _, err := os.Stat(featuresSysPath); err != nil {
- return None, fmt.Errorf("apparmor feature directory not found: %s", err)
+// KernelSupport describes apparmor features supported by the kernel.
+type KernelSupport struct {
+ enabled bool
+ features map[string]bool
+}
+
+// ProbeKernel checks which apparmor features are available.
+func ProbeKernel() *KernelSupport {
@mvo5

mvo5 Aug 28, 2017

Collaborator

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

@zyga

zyga Aug 28, 2017

Contributor

Just the feeling that structs containing arrays/slices should be passed by pointer, not by value.
+ entries, err := ioutil.ReadDir(featuresSysPath)
+ if err != nil {
@mvo5

mvo5 Aug 25, 2017

Collaborator

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

zyga Aug 25, 2017

Contributor

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

zyga Aug 25, 2017

Contributor

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

@zyga

zyga Aug 26, 2017

Contributor

Done! I re-factored the code a little and I think you will be happy with the result.
+ return nil
+ }
+ ks := &KernelSupport{
+ enabled: err == nil,
+ features: make(map[string]bool, len(entries)),
+ }
+ for _, entry := range entries {
+ // Each sub-directory represents a speicfic feature. Some have more
+ // details as additional sub-directories or files therein but we are
+ // not inspecting that at the moment.
+ if entry.IsDir() {
+ ks.features[entry.Name()] = true
+ }
+ }
+ return ks
+}
+
+// IsEnabled returns true if apparmor is enabled.
+func (ks *KernelSupport) IsEnabled() bool {
+ return ks != nil && ks.enabled
@mvo5

mvo5 Aug 28, 2017

Collaborator

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

zyga Aug 28, 2017

Contributor

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

zyga Aug 28, 2017

Contributor

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 )
+}
+
+// SupportsFeature returns true if a given apparmor feature is supported.
+func (ks *KernelSupport) SupportsFeature(feature string) bool {
+ return ks != nil && ks.features[feature]
@mvo5

mvo5 Aug 28, 2017

Collaborator

Same question here as above.
+}
+
+// Evaluate checks if the apparmor module is enabled and if all the required features are available.
+func (ks *KernelSupport) Evaluate() (level FeatureLevel, summary string) {
+ if !ks.IsEnabled() {
+ return None, fmt.Sprintf("apparmor is not enabled")
}
var missing []string
for _, feature := range requiredFeatures {
- p := filepath.Join(featuresSysPath, feature)
- if _, err := os.Stat(p); err != nil {
+ if !ks.SupportsFeature(feature) {
missing = append(missing, feature)
}
}
if len(missing) > 0 {
- return Partial, fmt.Errorf("apparmor features missing: %s", strings.Join(missing, ", "))
+ sort.Strings(missing)
+ return Partial, fmt.Sprintf("apparmor is enabled but some features are missing: %s", strings.Join(missing, ", "))
}
- return Full, nil
+ return Full, "apparmor is enabled and all features are available"
}
// MockFeatureLevel fakes the desired apparmor feature level.
@@ -91,12 +127,14 @@ func MockFeatureLevel(level FeatureLevel) (restore func()) {
case None:
// create no directory at all (apparmor not available).
case Partial:
- // create just the empty directory with no features.
- if err := os.MkdirAll(featuresSysPath, 0755); err != nil {
- panic(err)
+ // create several feature directories, matching vanilla 4.12 kernel.
+ for _, feature := range []string{"caps", "domain", "file", "network", "policy", "rlimit"} {
+ if err := os.MkdirAll(filepath.Join(featuresSysPath, feature), 0755); err != nil {
+ panic(err)
+ }
}
case Full:
- // create all the feature directories.
+ // create all the feature directories, matching Ubuntu kernels.
for _, feature := range requiredFeatures {
if err := os.MkdirAll(filepath.Join(featuresSysPath, feature), 0755); err != nil {
panic(err)
View
35 apparmor/probe_test.go
@@ -37,24 +37,41 @@ var _ = Suite(&probeSuite{})
func (s *probeSuite) TestMockProbeNone(c *C) {
restore := apparmor.MockFeatureLevel(apparmor.None)
defer restore()
- probed, err := apparmor.Probe()
- c.Assert(probed, Equals, apparmor.None)
- c.Assert(err, ErrorMatches, `apparmor feature directory not found: stat .*/features: no such file or directory`)
+
+ ks := apparmor.ProbeKernel()
+ c.Assert(ks.IsEnabled(), Equals, false)
+ c.Assert(ks.SupportsFeature("dbus"), Equals, false)
+ c.Assert(ks.SupportsFeature("file"), Equals, false)
+
+ level, summary := ks.Evaluate()
+ c.Assert(level, Equals, apparmor.None)
+ c.Assert(summary, Equals, "apparmor is not enabled")
}
func (s *probeSuite) TestMockProbePartial(c *C) {
restore := apparmor.MockFeatureLevel(apparmor.Partial)
defer restore()
- probed, err := apparmor.Probe()
- c.Assert(probed, Equals, apparmor.Partial)
- c.Assert(err, ErrorMatches, `apparmor features missing: caps, dbus, domain, file, mount, namespaces, network, ptrace, rlimit, signal`)
+ ks := apparmor.ProbeKernel()
+ c.Assert(ks.IsEnabled(), Equals, true)
+ c.Assert(ks.SupportsFeature("dbus"), Equals, false)
+ c.Assert(ks.SupportsFeature("file"), Equals, true)
+
+ level, summary := ks.Evaluate()
+ c.Assert(level, Equals, apparmor.Partial)
+ c.Assert(summary, Equals, "apparmor is enabled but some features are missing: dbus, mount, namespaces, ptrace, signal")
}
func (s *probeSuite) TestMockProbeFull(c *C) {
restore := apparmor.MockFeatureLevel(apparmor.Full)
defer restore()
- probed, err := apparmor.Probe()
- c.Assert(probed, Equals, apparmor.Full)
- c.Assert(err, IsNil)
+
+ ks := apparmor.ProbeKernel()
+ c.Assert(ks.IsEnabled(), Equals, true)
+ c.Assert(ks.SupportsFeature("dbus"), Equals, true)
+ c.Assert(ks.SupportsFeature("file"), Equals, true)
+
+ level, summary := ks.Evaluate()
+ c.Assert(level, Equals, apparmor.Full)
+ c.Assert(summary, Equals, "apparmor is enabled and all features are available")
}
View
2 release/release.go
@@ -55,7 +55,7 @@ var (
// ForceDevMode returns true if the distribution doesn't implement required
// security features for confinement and devmode is forced.
func (o *OS) ForceDevMode() bool {
- level, _ := apparmor.Probe()
+ level, _ := apparmor.ProbeKernel().Evaluate()
@mvo5

mvo5 Aug 28, 2017

Collaborator

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

zyga Aug 28, 2017

Contributor

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.
return level != apparmor.Full
}