From 318896ad6bced49bcf347e61086f93ffa105f1c5 Mon Sep 17 00:00:00 2001 From: Maciej Borzecki Date: Thu, 20 Dec 2018 14:48:04 +0100 Subject: [PATCH] release: use sync.Once around lazy intialized state (#6306) Some state data in the release package, namely AppArmor level/summary & kernel/parser features as well as Seccomp feature set, is only initialized when needed. Since there is a chance, this information could be accessed in concurrently, we need to add some locking around the code that mutates the data. This also shrinks the number of globals used to keep the state and uses pointers to structs instead. This makes the Mock* methods for the state a little bit clearer. --- release/apparmor.go | 253 +++++++++++++++++++++------------------ release/apparmor_test.go | 69 ++++++----- release/export_test.go | 24 ++-- release/seccomp.go | 64 ++++++---- release/seccomp_test.go | 19 +++ 5 files changed, 245 insertions(+), 184 deletions(-) diff --git a/release/apparmor.go b/release/apparmor.go index 11f713f4546..baf58d1934d 100644 --- a/release/apparmor.go +++ b/release/apparmor.go @@ -28,6 +28,7 @@ import ( "path/filepath" "sort" "strings" + "sync" "github.com/snapcore/snapd/strutil" ) @@ -66,61 +67,35 @@ func (level AppArmorLevelType) String() string { return fmt.Sprintf("AppArmorLevelType:%d", level) } -var ( - // appArmorLevel contains the assessment of the "level" of apparmor support. - appArmorLevel = UnknownAppArmor - // appArmorSummary contains a human readable description of the assessment. - appArmorSummary string - // appArmorKernelFeatures contains a list of kernel features that are supported. - // If the value is nil then the features were not probed yet. - appArmorKernelFeatures []string - // appArmorKernelError contains an error, if any, encountered when - // discovering available kernel features. - appArmorKernelError error - // appArmorParserFeatures contains a list of parser features that are supported. - // If the value is nil then the features were not probed yet. - appArmorParserFeatures []string - // appArmorParserError contains an error, if any, encountered when - // discovering available parser features. - appArmorParserError error -) +// appArmorAssessment represents what is supported AppArmor-wise by the system. +var appArmorAssessment = &appArmorAssess{appArmorProber: &appArmorProbe{}} // AppArmorLevel quantifies how well apparmor is supported on the current // kernel. The computation is costly to perform. The result is cached internally. func AppArmorLevel() AppArmorLevelType { - if appArmorLevel == UnknownAppArmor { - assessAppArmor() - } - return appArmorLevel + appArmorAssessment.assess() + return appArmorAssessment.level } // AppArmorSummary describes how well apparmor is supported on the current // kernel. The computation is costly to perform. The result is cached // internally. func AppArmorSummary() string { - if appArmorLevel == UnknownAppArmor { - assessAppArmor() - } - return appArmorSummary + appArmorAssessment.assess() + return appArmorAssessment.summary } // AppArmorKernelFeatures returns a sorted list of apparmor features like // []string{"dbus", "network"}. The result is cached internally. func AppArmorKernelFeatures() ([]string, error) { - if appArmorKernelFeatures == nil { - appArmorKernelFeatures, appArmorKernelError = probeAppArmorKernelFeatures() - } - return appArmorKernelFeatures, appArmorKernelError + return appArmorAssessment.KernelFeatures() } // AppArmorParserFeatures returns a sorted list of apparmor parser features // like []string{"unsafe", ...}. The computation is costly to perform. The // result is cached internally. func AppArmorParserFeatures() ([]string, error) { - if appArmorParserFeatures == nil { - appArmorParserFeatures, appArmorParserError = probeAppArmorParserFeatures() - } - return appArmorParserFeatures, appArmorParserError + return appArmorAssessment.ParserFeatures() } // AppArmorParserMtime returns the mtime of the parser, else 0. @@ -136,64 +111,6 @@ func AppArmorParserMtime() int64 { return mtime } -// MockAppArmorLevel makes the system believe it has certain level of apparmor -// support. -// -// AppArmor kernel and parser features are set to unrealistic values that do -// not match the requested level. Use this function to observe behavior that -// relies solely on the apparmor level value. -func MockAppArmorLevel(level AppArmorLevelType) (restore func()) { - oldAppArmorLevel := appArmorLevel - oldAppArmorSummary := appArmorSummary - oldAppArmorKernelFeatures := appArmorKernelFeatures - oldAppArmorKernelError := appArmorKernelError - oldAppArmorParserFeatures := appArmorParserFeatures - oldAppArmorParserError := appArmorParserError - appArmorLevel = level - appArmorSummary = fmt.Sprintf("mocked apparmor level: %s", level) - appArmorKernelFeatures = []string{"mocked-kernel-feature"} - appArmorKernelError = nil - appArmorParserFeatures = []string{"mocked-parser-feature"} - appArmorParserError = nil - return func() { - appArmorLevel = oldAppArmorLevel - appArmorSummary = oldAppArmorSummary - appArmorKernelFeatures = oldAppArmorKernelFeatures - appArmorKernelError = oldAppArmorKernelError - appArmorParserFeatures = oldAppArmorParserFeatures - appArmorParserError = oldAppArmorParserError - } -} - -// MockAppArmorFeatures makes the system believe it has certain kernel and -// parser features. -// -// AppArmor level and summary are automatically re-assessed on both the change -// and the restore process. Use this function to observe real assessment of -// arbitrary features. -func MockAppArmorFeatures(kernelFeatures []string, kernelError error, parserFeatures []string, parserError error) (restore func()) { - oldAppArmorKernelFeatures := appArmorKernelFeatures - oldAppArmorKernelError := appArmorKernelError - oldAppArmorParserFeatures := appArmorParserFeatures - oldAppArmorParserError := appArmorParserError - appArmorKernelFeatures = kernelFeatures - appArmorKernelError = kernelError - appArmorParserFeatures = parserFeatures - appArmorParserError = parserError - if appArmorKernelFeatures != nil && appArmorParserFeatures != nil { - assessAppArmor() - } - return func() { - appArmorKernelFeatures = oldAppArmorKernelFeatures - appArmorKernelError = oldAppArmorKernelError - appArmorParserFeatures = oldAppArmorParserFeatures - appArmorParserError = oldAppArmorParserError - if appArmorKernelFeatures != nil && appArmorParserFeatures != nil { - assessAppArmor() - } - } -} - // probe related code var ( @@ -234,21 +151,38 @@ var ( appArmorFeaturesSysPath = "/sys/kernel/security/apparmor/features" ) -func assessAppArmor() { +type appArmorProber interface { + KernelFeatures() ([]string, error) + ParserFeatures() ([]string, error) +} + +type appArmorAssess struct { + appArmorProber + // level contains the assessment of the "level" of apparmor support. + level AppArmorLevelType + // summary contains a human readable description of the assessment. + summary string + + once sync.Once +} + +func (aaa *appArmorAssess) assess() { + aaa.once.Do(func() { + aaa.level, aaa.summary = aaa.doAssess() + }) +} + +func (aaa *appArmorAssess) doAssess() (level AppArmorLevelType, summary string) { // First, quickly check if apparmor is available in the kernel at all. - kernelFeatures, err := AppArmorKernelFeatures() + kernelFeatures, err := aaa.KernelFeatures() if os.IsNotExist(err) { - appArmorLevel = NoAppArmor - appArmorSummary = "apparmor not enabled" - return + return NoAppArmor, "apparmor not enabled" } // Then check that the parser supports the required parser features. // If we have any missing required features then apparmor is unusable. - parserFeatures, err := AppArmorParserFeatures() + parserFeatures, err := aaa.ParserFeatures() if os.IsNotExist(err) { - appArmorLevel = NoAppArmor - appArmorSummary = "apparmor_parser not found" - return + return NoAppArmor, "apparmor_parser not found" } var missingParserFeatures []string for _, feature := range requiredAppArmorParserFeatures { @@ -257,10 +191,9 @@ func assessAppArmor() { } } if len(missingParserFeatures) > 0 { - appArmorLevel = UnusableAppArmor - appArmorSummary = fmt.Sprintf("apparmor_parser is available but required parser features are missing: %s", + summary := fmt.Sprintf("apparmor_parser is available but required parser features are missing: %s", strings.Join(missingParserFeatures, ", ")) - return + return UnusableAppArmor, summary } // Next, check that the kernel supports the required kernel features. @@ -271,10 +204,9 @@ func assessAppArmor() { } } if len(missingKernelFeatures) > 0 { - appArmorLevel = UnusableAppArmor - appArmorSummary = fmt.Sprintf("apparmor is enabled but required kernel features are missing: %s", + summary := fmt.Sprintf("apparmor is enabled but required kernel features are missing: %s", strings.Join(missingKernelFeatures, ", ")) - return + return UnusableAppArmor, summary } // Next check that the parser supports preferred parser features. @@ -285,10 +217,9 @@ func assessAppArmor() { } } if len(missingParserFeatures) > 0 { - appArmorLevel = PartialAppArmor - appArmorSummary = fmt.Sprintf("apparmor_parser is available but some features are missing: %s", + summary := fmt.Sprintf("apparmor_parser is available but some features are missing: %s", strings.Join(missingParserFeatures, ", ")) - return + return PartialAppArmor, summary } // Lastly check that the kernel supports preferred kernel features. @@ -298,15 +229,43 @@ func assessAppArmor() { } } if len(missingKernelFeatures) > 0 { - appArmorLevel = PartialAppArmor - appArmorSummary = fmt.Sprintf("apparmor is enabled but some kernel features are missing: %s", + summary := fmt.Sprintf("apparmor is enabled but some kernel features are missing: %s", strings.Join(missingKernelFeatures, ", ")) - return + return PartialAppArmor, summary } // If we got here then all features are available and supported. - appArmorLevel = FullAppArmor - appArmorSummary = "apparmor is enabled and all features are available" + return FullAppArmor, "apparmor is enabled and all features are available" +} + +type appArmorProbe struct { + // kernelFeatures contains a list of kernel features that are supported. + kernelFeatures []string + // kernelError contains an error, if any, encountered when + // discovering available kernel features. + kernelError error + // parserFeatures contains a list of parser features that are supported. + parserFeatures []string + // parserError contains an error, if any, encountered when + // discovering available parser features. + parserError error + + probeKernelOnce sync.Once + probeParserOnce sync.Once +} + +func (aap *appArmorProbe) KernelFeatures() ([]string, error) { + aap.probeKernelOnce.Do(func() { + aap.kernelFeatures, aap.kernelError = probeAppArmorKernelFeatures() + }) + return aap.kernelFeatures, aap.kernelError +} + +func (aap *appArmorProbe) ParserFeatures() ([]string, error) { + aap.probeParserOnce.Do(func() { + aap.parserFeatures, aap.parserError = probeAppArmorParserFeatures() + }) + return aap.parserFeatures, aap.parserError } func probeAppArmorKernelFeatures() ([]string, error) { @@ -357,3 +316,67 @@ func tryAppArmorParserFeature(parser, rule string) bool { } return true } + +// mocking + +type mockAppArmorProbe struct { + kernelFeatures []string + kernelError error + parserFeatures []string + parserError error +} + +func (m *mockAppArmorProbe) KernelFeatures() ([]string, error) { + return m.kernelFeatures, m.kernelError +} + +func (m *mockAppArmorProbe) ParserFeatures() ([]string, error) { + return m.parserFeatures, m.parserError +} + +// MockAppArmorLevel makes the system believe it has certain level of apparmor +// support. +// +// AppArmor kernel and parser features are set to unrealistic values that do +// not match the requested level. Use this function to observe behavior that +// relies solely on the apparmor level value. +func MockAppArmorLevel(level AppArmorLevelType) (restore func()) { + oldAppArmorAssessment := appArmorAssessment + mockProbe := &mockAppArmorProbe{ + kernelFeatures: []string{"mocked-kernel-feature"}, + parserFeatures: []string{"mocked-parser-feature"}, + } + appArmorAssessment = &appArmorAssess{ + appArmorProber: mockProbe, + level: level, + summary: fmt.Sprintf("mocked apparmor level: %s", level), + } + appArmorAssessment.once.Do(func() {}) + return func() { + appArmorAssessment = oldAppArmorAssessment + } +} + +// MockAppArmorFeatures makes the system believe it has certain kernel and +// parser features. +// +// AppArmor level and summary are automatically re-assessed as needed +// on both the change and the restore process. Use this function to +// observe real assessment of arbitrary features. +func MockAppArmorFeatures(kernelFeatures []string, kernelError error, parserFeatures []string, parserError error) (restore func()) { + oldAppArmorAssessment := appArmorAssessment + mockProbe := &mockAppArmorProbe{ + kernelFeatures: kernelFeatures, + kernelError: kernelError, + parserFeatures: parserFeatures, + parserError: parserError, + } + appArmorAssessment = &appArmorAssess{ + appArmorProber: mockProbe, + } + appArmorAssessment.assess() + return func() { + appArmorAssessment = oldAppArmorAssessment + } + +} diff --git a/release/apparmor_test.go b/release/apparmor_test.go index 2f454473e85..35ccc407554 100644 --- a/release/apparmor_test.go +++ b/release/apparmor_test.go @@ -44,35 +44,6 @@ func (*apparmorSuite) TestAppArmorLevelTypeStringer(c *C) { c.Check(release.AppArmorLevelType(42).String(), Equals, "AppArmorLevelType:42") } -func (*apparmorSuite) TestAppArmorLevelTriggersAssesment(c *C) { - // Pretend that we know the apparmor kernel and parser features. - restore := release.MockAppArmorFeatures([]string{"feature"}, nil, []string{"feature"}, nil) - defer restore() - // Pretend that we don't know what the state of apparmor is. - release.ResetAppArmorAssesment() - - // Calling AppArmorLevel assesses the kernel and parser features and sets - // the level to not-unknown value, returning it. - c.Check(release.CurrentAppArmorLevel(), Equals, release.UnknownAppArmor) - level := release.AppArmorLevel() - c.Check(level, Not(Equals), release.UnknownAppArmor) - c.Check(level, Equals, release.CurrentAppArmorLevel()) -} - -func (*apparmorSuite) TestAppArmorSummaryTriggersAssesment(c *C) { - // Pretend that we know the apparmor kernel and parser features. - restore := release.MockAppArmorFeatures([]string{"feature"}, nil, []string{"feature"}, nil) - defer restore() - // Pretend that we don't know what the state of apparmor is. - release.ResetAppArmorAssesment() - - // Calling AppArmorSummary assesses the kernel and parser features and sets - // the level to something other than unknown. - c.Check(release.CurrentAppArmorLevel(), Equals, release.UnknownAppArmor) - release.AppArmorSummary() - c.Check(release.CurrentAppArmorLevel(), Not(Equals), release.UnknownAppArmor) -} - func (*apparmorSuite) TestMockAppArmorLevel(c *C) { for _, lvl := range []release.AppArmorLevelType{release.NoAppArmor, release.UnusableAppArmor, release.PartialAppArmor, release.FullAppArmor} { restore := release.MockAppArmorLevel(lvl) @@ -224,6 +195,8 @@ func (s *apparmorSuite) TestProbeAppArmorParserFeatures(c *C) { } func (s *apparmorSuite) TestInterfaceSystemKey(c *C) { + release.FreshAppArmorAssessment() + d := c.MkDir() restore := release.MockAppArmorFeaturesSysPath(d) defer restore() @@ -235,7 +208,7 @@ func (s *apparmorSuite) TestInterfaceSystemKey(c *C) { restore = release.MockAppArmorParserSearchPath(mockParserCmd.BinDir()) defer restore() - release.AssessAppArmor() + release.AppArmorLevel() features, err := release.AppArmorKernelFeatures() c.Assert(err, IsNil) @@ -262,3 +235,39 @@ func (s *apparmorSuite) TestAppArmorParserMtime(c *C) { mtime = release.AppArmorParserMtime() c.Check(mtime, Equals, int64(0)) } + +func (s *apparmorSuite) TestFeaturesProbedOnce(c *C) { + release.FreshAppArmorAssessment() + + d := c.MkDir() + restore := release.MockAppArmorFeaturesSysPath(d) + defer restore() + c.Assert(os.MkdirAll(filepath.Join(d, "policy"), 0755), IsNil) + c.Assert(os.MkdirAll(filepath.Join(d, "network"), 0755), IsNil) + + mockParserCmd := testutil.MockCommand(c, "apparmor_parser", "") + defer mockParserCmd.Restore() + restore = release.MockAppArmorParserSearchPath(mockParserCmd.BinDir()) + defer restore() + + features, err := release.AppArmorKernelFeatures() + c.Assert(err, IsNil) + c.Check(features, DeepEquals, []string{"network", "policy"}) + features, err = release.AppArmorParserFeatures() + c.Assert(err, IsNil) + c.Check(features, DeepEquals, []string{"unsafe"}) + + // this makes probing fails but is not done again + err = os.RemoveAll(d) + c.Assert(err, IsNil) + + _, err = release.AppArmorKernelFeatures() + c.Assert(err, IsNil) + + // this makes probing fails but is not done again + err = os.RemoveAll(mockParserCmd.BinDir()) + c.Assert(err, IsNil) + + _, err = release.AppArmorParserFeatures() + c.Assert(err, IsNil) +} diff --git a/release/export_test.go b/release/export_test.go index ce836ed5d36..45394d8e999 100644 --- a/release/export_test.go +++ b/release/export_test.go @@ -58,26 +58,10 @@ func MockIoutilReadfile(newReadfile func(string) ([]byte, error)) (restorer func } } -// CurrentAppArmorLevel returns the internal cached apparmor level. -func CurrentAppArmorLevel() AppArmorLevelType { - return appArmorLevel -} - -// ResetAppArmorAssesment resets the internal apparmor level and summary. -// -// Both appArmorLevel and appArmorSummary are assigned with zero values -// that trigger probing and assessment on the next access via the public APIs. -func ResetAppArmorAssesment() { - appArmorLevel = UnknownAppArmor - appArmorSummary = "" -} - var ( ProbeAppArmorKernelFeatures = probeAppArmorKernelFeatures ProbeAppArmorParserFeatures = probeAppArmorParserFeatures - AssessAppArmor = assessAppArmor - RequiredAppArmorKernelFeatures = requiredAppArmorKernelFeatures RequiredAppArmorParserFeatures = requiredAppArmorParserFeatures PreferredAppArmorKernelFeatures = preferredAppArmorKernelFeatures @@ -85,3 +69,11 @@ var ( IsWSL = isWSL ) + +func FreshAppArmorAssessment() { + appArmorAssessment = &appArmorAssess{appArmorProber: &appArmorProbe{}} +} + +func FreshSecCompProbe() { + secCompProber = &secCompProbe{} +} diff --git a/release/seccomp.go b/release/seccomp.go index 6222885fb3d..059e0107a84 100644 --- a/release/seccomp.go +++ b/release/seccomp.go @@ -20,37 +20,17 @@ package release import ( - "io/ioutil" "sort" "strings" + "sync" ) -var ( - secCompAvailableActionsPath = "/proc/sys/kernel/seccomp/actions_avail" -) - -var secCompActions []string - -func MockSecCompActions(actions []string) (restore func()) { - old := secCompActions - secCompActions = actions - return func() { secCompActions = old } -} +var secCompProber = &secCompProbe{} // SecCompActions returns a sorted list of seccomp actions like // []string{"allow", "errno", "kill", "log", "trace", "trap"}. func SecCompActions() []string { - if secCompActions == nil { - var actions []string - contents, err := ioutil.ReadFile(secCompAvailableActionsPath) - if err != nil { - return actions - } - actions = strings.Split(strings.TrimRight(string(contents), "\n"), " ") - sort.Strings(actions) - secCompActions = actions - } - return secCompActions + return secCompProber.actions() } func SecCompSupportsAction(action string) bool { @@ -61,3 +41,41 @@ func SecCompSupportsAction(action string) bool { } return false } + +// probing + +type secCompProbe struct { + probedActions []string + + once sync.Once +} + +func (scp *secCompProbe) actions() []string { + scp.once.Do(func() { + scp.probedActions = probeSecCompActions() + }) + return scp.probedActions +} + +func probeSecCompActions() []string { + contents, err := ioutilReadFile("/proc/sys/kernel/seccomp/actions_avail") + if err != nil { + return []string{} + } + actions := strings.Split(strings.TrimRight(string(contents), "\n"), " ") + sort.Strings(actions) + return actions +} + +// mocking + +func MockSecCompActions(actions []string) (restore func()) { + old := secCompProber + secCompProber = &secCompProbe{ + probedActions: actions, + } + secCompProber.once.Do(func() {}) + return func() { + secCompProber = old + } +} diff --git a/release/seccomp_test.go b/release/seccomp_test.go index c05145e952a..6f4a9d3fcd5 100644 --- a/release/seccomp_test.go +++ b/release/seccomp_test.go @@ -20,6 +20,8 @@ package release_test import ( + "io" + . "gopkg.in/check.v1" "github.com/snapcore/snapd/release" @@ -48,3 +50,20 @@ func (s *seccompSuite) TestSecCompSupportsAction(c *C) { defer reset() c.Check(release.SecCompSupportsAction("log"), Equals, true) } + +func (s *seccompSuite) TestProbe(c *C) { + release.FreshSecCompProbe() + r1 := release.MockIoutilReadfile(func(string) ([]byte, error) { + return []byte("a b\n"), nil + }) + defer r1() + + c.Check(release.SecCompActions(), DeepEquals, []string{"a", "b"}) + + r2 := release.MockIoutilReadfile(func(string) ([]byte, error) { + return nil, io.ErrUnexpectedEOF + }) + defer r2() + + c.Check(release.SecCompActions(), DeepEquals, []string{"a", "b"}) +}