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

Merged
merged 12 commits into from Aug 26, 2017
View
@@ -0,0 +1,151 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2017 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package apparmor
+
+import (
+ "fmt"
+ "io/ioutil"
+ "os"
+ "path/filepath"
+ "sort"
+ "strings"
+)
+
+// FeatureLevel encodes the kind of support for apparmor found on this system.
+type FeatureLevel int
+
+const (
+ // None indicates that apparmor is not enabled.
+ None FeatureLevel = iota
+ // Partial indicates that apparmor is enabled but some features are missing.
+ Partial
+ // Full indicates that all features are supported.
+ Full
+)
+
+var (
+ // featureSysPath points to the sysfs directory where apparmor features are listed.
+ featuresSysPath = "/sys/kernel/security/apparmor/features"
@jdstrand

jdstrand Aug 25, 2017

Contributor

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

@zyga

zyga Aug 25, 2017

Contributor

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

+ // requiredFeatures are the apparmor features needed for strict confinement.
+ requiredFeatures = []string{
+ "caps",
+ "dbus",
+ "domain",
+ "file",
+ "mount",
+ "namespaces",
+ "network",
+ "ptrace",
+ "rlimit",
+ "signal",
+ }
+)
+
+// 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 {
+ 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 {
+ if !ks.SupportsFeature(feature) {
+ missing = append(missing, feature)
+ }
+ }
+ if len(missing) > 0 {
+ sort.Strings(missing)
+ return Partial, fmt.Sprintf("apparmor is enabled but some features are missing: %s", strings.Join(missing, ", "))
+ }
+ return Full, "apparmor is enabled and all features are available"
+}
+
+// MockFeatureLevel fakes the desired apparmor feature level.
+func MockFeatureLevel(level FeatureLevel) (restore func()) {
+ oldFeaturesSysPath := featuresSysPath
+
+ temp, err := ioutil.TempDir("", "mock-apparmor-feature-level")
+ if err != nil {
+ panic(err)
+ }
+ featuresSysPath = filepath.Join(temp, "features")
+
+ switch level {
+ case None:
+ // create no directory at all (apparmor not available).
+ case Partial:
+ // 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, matching Ubuntu kernels.
+ for _, feature := range requiredFeatures {
+ if err := os.MkdirAll(filepath.Join(featuresSysPath, feature), 0755); err != nil {
+ panic(err)
+ }
+ }
+ }
+
+ return func() {
+ if err := os.RemoveAll(temp); err != nil {
+ panic(err)
+ }
+ featuresSysPath = oldFeaturesSysPath
+ }
+}
View
@@ -0,0 +1,77 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2017 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package apparmor_test
+
+import (
+ . "gopkg.in/check.v1"
+ "testing"
+
+ "github.com/snapcore/snapd/apparmor"
+)
+
+func Test(t *testing.T) {
+ TestingT(t)
+}
+
+type probeSuite struct{}
+
+var _ = Suite(&probeSuite{})
+
+func (s *probeSuite) TestMockProbeNone(c *C) {
+ restore := apparmor.MockFeatureLevel(apparmor.None)
+ defer restore()
+
+ 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()
+
+ 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()
+
+ 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
@@ -21,11 +21,11 @@ package release
import (
"bufio"
- "io/ioutil"
"os"
- "path/filepath"
"strings"
"unicode"
+
+ "github.com/snapcore/snapd/apparmor"
)
// Series holds the Ubuntu Core series for snapd to use.
@@ -55,16 +55,8 @@ var (
// ForceDevMode returns true if the distribution doesn't implement required
// security features for confinement and devmode is forced.
func (o *OS) ForceDevMode() bool {
- for _, req := range requiredApparmorFeatures {
- // Also ensure appamor is enabled (cannot use
- // osutil.FileExists() here because of cyclic imports)
- p := filepath.Join(apparmorFeaturesSysPath, req)
- if _, err := os.Stat(p); err != nil {
- return true
- }
- }
-
- return false
+ 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
}
var (
@@ -151,26 +143,9 @@ func MockReleaseInfo(osRelease *OS) (restore func()) {
// MockForcedDevmode fake the system to believe its in a distro
// that is in ForcedDevmode
func MockForcedDevmode(isDevmode bool) (restore func()) {
- oldApparmorFeaturesSysPath := apparmorFeaturesSysPath
-
- temp, err := ioutil.TempDir("", "mock-forced-devmode")
- if err != nil {
- panic(err)
- }
- fakeApparmorFeaturesSysPath := filepath.Join(temp, "apparmor")
- if !isDevmode {
- for _, req := range requiredApparmorFeatures {
- if err := os.MkdirAll(filepath.Join(fakeApparmorFeaturesSysPath, req), 0755); err != nil {
- panic(err)
- }
- }
- }
- apparmorFeaturesSysPath = fakeApparmorFeaturesSysPath
-
- return func() {
- if err := os.RemoveAll(temp); err != nil {
- panic(err)
- }
- apparmorFeaturesSysPath = oldApparmorFeaturesSysPath
+ level := apparmor.Full
+ if isDevmode {
+ level = apparmor.None
}
+ return apparmor.MockFeatureLevel(level)
}