Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s/apparmor: expand apparmor prompting support checks #14448

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ba92682
s/apparmor: expand apparmor prompting support checks
olivercalder Aug 30, 2024
d42676e
s/apparmor: add missing feature to mocked features list in prompting …
olivercalder Aug 31, 2024
df40331
features: remove superfluous test of apparmor function which has in-s…
olivercalder Aug 31, 2024
0a31aed
Revert "tests/main/interfaces-snap-interfaces-requests-control: disab…
olivercalder Aug 31, 2024
e6dfc59
Revert "tests/main/apparmor-prompting-snapd-startup: account for vend…
olivercalder Sep 5, 2024
20eb498
Revert "tests: account for prompting supported on >= 22.04 with inter…
olivercalder Aug 31, 2024
ead3eba
s/apparmor,tests: improve comments around apparmor prompting
olivercalder Aug 31, 2024
9cda3e0
tests: add debug checks for prompting support
olivercalder Sep 5, 2024
8416753
tests: make prompting-related tests work on any supported ubuntu system
olivercalder Sep 6, 2024
4d45b15
s/apparmor: add test case when policy/notify/user file exists but doe…
olivercalder Sep 6, 2024
b3e525c
s/apparmor: adjust prompting unsupported reason error messages
olivercalder Sep 6, 2024
a7e43d8
s/apparmor: prompting support checks for permstable32_version >= 2
olivercalder Sep 9, 2024
3babde3
s/apparmor: add TODO about using dirs instead of rootPath
olivercalder Sep 9, 2024
0c4166a
o/c/configcore,s/apparmor: mock aa root path in prompting tests
olivercalder Sep 10, 2024
22927fa
s/apparmor: trim whitespace from permstable32_version before parsing
olivercalder Sep 10, 2024
93c6a23
s/apparmor: adjust prompting unsupported reason error messages to ref…
olivercalder Sep 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 0 additions & 71 deletions features/features_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package features_test

import (
"fmt"
"os"
"path/filepath"
"testing"
Expand All @@ -30,7 +29,6 @@ import (
"github.com/snapcore/snapd/features"
"github.com/snapcore/snapd/overlord/configstate/config"
"github.com/snapcore/snapd/overlord/state"
"github.com/snapcore/snapd/sandbox/apparmor"
"github.com/snapcore/snapd/systemd"
)

Expand Down Expand Up @@ -147,75 +145,6 @@ func (*featureSuite) TestUserDaemonsSupportedCallback(c *C) {
c.Check(reason, Equals, "")
}

func (*featureSuite) TestAppArmorPromptingSupportedCallback(c *C) {
callback, ok := features.FeaturesSupportedCallbacks[features.AppArmorPrompting]
c.Assert(ok, Equals, true)

for _, t := range []struct {
kernelFeatures []string
kernelError error
parserFeatures []string
parserError error
expectedReason string
}{
{
// Both unsupported
kernelFeatures: []string{"foo", "bar"},
kernelError: nil,
parserFeatures: []string{"baz", "qux"},
parserError: nil,
expectedReason: "apparmor kernel features do not support prompting",
},
{
// Kernel unsupported, parser supported
kernelFeatures: []string{"foo", "bar"},
kernelError: nil,
parserFeatures: []string{"baz", "qux", "prompt"},
parserError: nil,
expectedReason: "apparmor kernel features do not support prompting",
},
{
// Kernel supported, parser unsupported
kernelFeatures: []string{"foo", "bar", "policy:permstable32:prompt"},
kernelError: nil,
parserFeatures: []string{"baz", "qux"},
parserError: nil,
expectedReason: "apparmor parser does not support the prompt qualifier",
},
{
// Kernel error
kernelFeatures: []string{"foo", "bar", "policy:permstable32:prompt"},
kernelError: fmt.Errorf("bad kernel"),
parserFeatures: []string{"baz", "qux", "prompt"},
parserError: nil,
expectedReason: "cannot check apparmor kernel features: bad kernel",
},
{
// Parser error
kernelFeatures: []string{"foo", "bar", "policy:permstable32:prompt"},
kernelError: nil,
parserFeatures: []string{"baz", "qux", "prompt"},
parserError: fmt.Errorf("bad parser"),
expectedReason: "cannot check apparmor parser features: bad parser",
},
} {
restore := apparmor.MockFeatures(t.kernelFeatures, t.kernelError, t.parserFeatures, t.parserError)
supported, reason := callback()
c.Check(supported, Equals, false)
c.Check(reason, Equals, t.expectedReason)
restore()
}

// Both supported
kernelFeatures := []string{"foo", "bar", "policy:permstable32:prompt"}
parserFeatures := []string{"baz", "qux", "prompt"}
restore := apparmor.MockFeatures(kernelFeatures, nil, parserFeatures, nil)
defer restore()
supported, reason := callback()
c.Check(supported, Equals, true)
c.Check(reason, Equals, "")
}

func (*featureSuite) TestIsSupported(c *C) {
fakeFeature := features.SnapdFeature(len(features.KnownFeatures()))

Expand Down
10 changes: 10 additions & 0 deletions overlord/configstate/configcore/prompting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,17 @@ package configcore_test
import (
"errors"
"fmt"
"os"
"os/user"
"path/filepath"
"time"

. "gopkg.in/check.v1"
"gopkg.in/tomb.v2"

"github.com/snapcore/snapd/boot"
"github.com/snapcore/snapd/client"
"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/features"
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/builtin"
Expand All @@ -45,6 +48,7 @@ import (
"github.com/snapcore/snapd/overlord/state"
"github.com/snapcore/snapd/release"
"github.com/snapcore/snapd/sandbox/apparmor"
"github.com/snapcore/snapd/sandbox/apparmor/notify"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/snap/snaptest"
)
Expand All @@ -65,6 +69,12 @@ func (s *promptingSuite) SetUpTest(c *C) {
[]string{"policy:permstable32:prompt"}, nil,
[]string{"prompt"}, nil,
))
// mock the presence of the notification socket
os.MkdirAll(notify.SysPath, 0o755)
// mock the presence of permstable32_version with supported version
s.AddCleanup(apparmor.MockFsRootPath(dirs.GlobalRootDir))
os.MkdirAll(filepath.Join(dirs.GlobalRootDir, "sys/kernel/security/apparmor/features/policy"), 0o755)
os.WriteFile(filepath.Join(dirs.GlobalRootDir, "sys/kernel/security/apparmor/features/policy/permstable32_version"), []byte("0x000002"), 0o644)

s.overlord = overlord.MockWithState(nil)
// override state set up by configcoreSuite
Expand Down
104 changes: 85 additions & 19 deletions sandbox/apparmor/apparmor.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@ import (
"os/exec"
"path/filepath"
"sort"
"strconv"
"strings"
"sync"

"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/logger"
"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/sandbox/apparmor/notify"
"github.com/snapcore/snapd/snapdtool"
"github.com/snapcore/snapd/strutil"
)
Expand Down Expand Up @@ -428,7 +430,8 @@ func PromptingSupported() (bool, string) {
}

// PromptingSupportedByFeatures returns whether prompting is supported by the
// given AppArmor kernel and parser features.
// given AppArmor kernel and parser features, and by the presence of the kernel
// notification socket.
func PromptingSupportedByFeatures(apparmorFeatures *FeaturesSupported) (bool, string) {
if apparmorFeatures == nil {
return false, "no apparmor features provided"
Expand All @@ -439,6 +442,38 @@ func PromptingSupportedByFeatures(apparmorFeatures *FeaturesSupported) (bool, st
if !strutil.ListContains(apparmorFeatures.ParserFeatures, "prompt") {
return false, "apparmor parser does not support the prompt qualifier"
}
// In the future, the complain redirect will be added to apparmor in the
// kernel, and it will share the same notify framework as the listener, so
// the presence of the notification socket will no longer be sufficient to
// indicate that the system supports prompting (assuming the kernel and
// parser features checks above pass). When this lands, an additional
// directory will be added at featuresSysPath/policy/notify. At this point,
// there will be a file at featuresSysPath/policy/notify/user which contains
// a list of mediation classes for which prompting is supported. Thus:
// - If featuresSysPath/policy/notify does not exist, and the above checks
// pass, then prompting is supported.
// - If featuresSysPath/policy/notify exists, but does not contain a file
// called user, then prompting is not supported.
// - If featuresSysPath/policy/notify exists and includes a file called
// user, then prompting is supported.
// The user file should contain a list of mediation classes which are
// supported, and this should at the bare minimum include "file", so check
// for its presence among the kernel features.
if strutil.ListContains(apparmorFeatures.KernelFeatures, "policy:notify") {
if !strutil.ListContains(apparmorFeatures.KernelFeatures, "policy:notify:user:file") {
return false, "apparmor kernel features do not support prompting for file access"
}
}
if !notify.SupportAvailable() {
return false, "apparmor kernel notification socket required by prompting listener is not present"
}
version, err := probeKernelFeaturesPermstable32Version()
if err != nil {
return false, "apparmor kernel permissions table version must be at least 2 for prompting to be supported, but version could not be read"
}
if version < 2 {
return false, fmt.Sprintf("apparmor kernel permissions table version must be at least 2 for prompting to be supported, but version is %d", version)
}
return true, ""
}

Expand Down Expand Up @@ -481,6 +516,7 @@ var (

// Filesystem root defined locally to avoid dependency on the
// 'dirs' package
// TODO: replace rootPath with dirs.GlobalRootDir, since dirs is used elsewhere
rootPath = "/"

// hostAbi30File is the path to the apparmor "3.0" ABI file and is typically
Expand Down Expand Up @@ -617,38 +653,60 @@ func (aap *appArmorProbe) ParserFeatures() ([]string, error) {
}

func probeKernelFeatures() ([]string, error) {
// note that os.ReadDir() is already sorted
dentries, err := os.ReadDir(filepath.Join(rootPath, featuresSysPath))
features, err := probeKernelFeaturesInDirRecursively(filepath.Join(rootPath, featuresSysPath), "")
if err != nil {
return []string{}, err
}
if data, err := os.ReadFile(filepath.Join(rootPath, featuresSysPath, "policy", "permstable32")); err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given we are revisiting this, what's the relation between permstable32 and permstable32_version, do we need to worry about the latter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JJ says the minimum version is 2. Technically 1 can work, but it has problems. I think we can add a check for version 2+, but the question is whether to include it in the apparmor kernel features probing, or to do a one-off check here (like we do for the socket presence).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per sync with Samuele, do this in the prompting support checks, rather than in the general apparmor kernel feature probing

permstableFeatures := strings.Fields(string(data))
for _, feat := range permstableFeatures {
features = append(features, "policy:permstable32:"+feat)
}
}
if data, err := os.ReadFile(filepath.Join(rootPath, featuresSysPath, "policy", "notify", "user")); err != nil {
// XXX: there's no feature added for policy:notify:user, since user is
// a file rather than a directory.
notifyUserFeatures := strings.Fields(string(data))
for _, feat := range notifyUserFeatures {
features = append(features, "policy:notify:user:"+feat)
}
}
// Features must be sorted
sort.Strings(features)
return features, nil
}

func probeKernelFeaturesInDirRecursively(dir string, prefix string) ([]string, error) {
dentries, err := os.ReadDir(dir)
if err != nil {
return []string{}, err
}
features := make([]string, 0, len(dentries))
for _, fi := range dentries {
if fi.IsDir() {
features = append(features, fi.Name())
// also read any sub-features
subdenties, err := os.ReadDir(filepath.Join(rootPath, featuresSysPath, fi.Name()))
featureName := fi.Name()
if prefix != "" {
featureName = prefix + ":" + fi.Name()
}
features = append(features, featureName)
subFeatures, err := probeKernelFeaturesInDirRecursively(filepath.Join(dir, fi.Name()), featureName)
if err != nil {
return []string{}, err
}
for _, subfi := range subdenties {
if subfi.IsDir() {
features = append(features, fi.Name()+":"+subfi.Name())
}
}
}
}
if data, err := os.ReadFile(filepath.Join(rootPath, featuresSysPath, "policy", "permstable32")); err == nil {
permstableFeatures := strings.Fields(string(data))
for _, feat := range permstableFeatures {
features = append(features, fmt.Sprintf("policy:permstable32:%s", feat))
features = append(features, subFeatures...)
}
}
// Features must be sorted
sort.Strings(features)
return features, nil
}

func probeKernelFeaturesPermstable32Version() (int64, error) {
data, err := os.ReadFile(filepath.Join(rootPath, featuresSysPath, "policy", "permstable32_version"))
if err != nil {
return 0, err
}
return strconv.ParseInt(strings.TrimSpace(string(data)), 0, 64)
}

func probeParserFeatures() ([]string, error) {
var featureProbes = []struct {
feature string
Expand Down Expand Up @@ -991,3 +1049,11 @@ func MockParserSearchPath(new string) (restore func()) {
parserSearchPath = oldAppArmorParserSearchPath
}
}

func MockFsRootPath(path string) (restorer func()) {
old := rootPath
rootPath = path
return func() {
rootPath = old
}
}
Loading
Loading