s/apparmor: expand apparmor prompting support checks #14448

olivercalder · 2024-08-30T23:12:11Z

This PR is based on #14294, only the final few commits are relevant to this PR. Should rebase this PR on master once that PR lands.

Prompting cannot be supported if the kernel notification socket does not exist. Thus, add a check for its presence to PromptingSupportedByFeatures.

However, this alone is not sufficient. The notification socket may be used for other purposes as well. When this becomes the case, a directory will be added at /sys/kernel/security/apparmor/features/policy/notify. If that directory exists, and it contains a file called user, which contains the list of prompting mediation classes, then prompting is supported on the system. If no file called user exists in that directory, but the directory itself exists, then we know prompting is not supported.

This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-31642

codecov · 2024-08-30T23:30:31Z

Codecov Report

Attention: Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.75%. Comparing base (3492595) to head (eaa08a2).
Report is 52 commits behind head on master.

Files with missing lines	Patch %	Lines
sandbox/apparmor/apparmor.go	91.42%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14448      +/-   ##
==========================================
- Coverage   78.77%   78.75%   -0.02%     
==========================================
  Files        1073     1074       +1     
  Lines      143693   144511     +818     
==========================================
+ Hits       113191   113807     +616     
- Misses      23401    23561     +160     
- Partials     7101     7143      +42

Flag	Coverage Δ
unittests	`78.75% <91.42%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

olivercalder · 2024-09-03T18:49:43Z

This now covers the debian sid TODO from #14458

bboozzoo

LGTM

bboozzoo · 2024-09-04T11:35:06Z

tests/main/apparmor-prompting-flag-restart/task.yaml

@@ -106,9 +106,8 @@ execute: |
    fi

    echo "Enable prompting via snap client where possible"
-    if os.query is-core || os.query is-ubuntu-lt 22.04; then
+    if os.query is-core || os.query is-ubuntu-lt 24.04; then


seems like 22.04 GCP kernel has the right bits already and this is failing?

Well that sure is "interesting"..... and there's no debug log with kernel versions :( guess I'll have to add some debugging and see what's going on.

In general, we don't expect Jammy to have prompting. One could install a new Ubuntu kernel(/apparmor?) on Jammy (or potentially older) and get prompting to work, but I don't love the idea of testing this.

I think a more robust solution than os.query is-ubuntu-lt 24.04 would be to check is-ubuntu and then check the kernel version itself. It's possible prompting could be backported to older kernels, but I hope not.

hm debug output should contain this:

snapd/spread.yaml

Lines 587 to 588 in e9b6640

echo '# Kernel information'

uname -a

FWIW, I think it's ok to count 22.04 as supported, since there's a hwe kernel which may have the right patches, and given that 22.04 isn't the most recent distro around I would expect the hwe kernel to be quite popular.

hm debug output should contain this:

snapd/spread.yaml

Lines 587 to 588 in e9b6640

echo '# Kernel information'

uname -a

Yeah, for some reason the debug output is not showing at all on failure?? I'm trying some things.

FWIW, I think it's ok to count 22.04 as supported, since there's a hwe kernel which may have the right patches, and given that 22.04 isn't the most recent distro around I would expect the hwe kernel to be quite popular.

I agree, I think ideally we should change the spread checks to actually check kernel version/features for support. So instead of just asserting things about Ubuntu versions, we check whether it is Ubuntu and the kernel is [some constraints]. This might look a bit like the actual checks within snapd though, which I don't love. Ideally there's a very clear versioning scheme where prompting is only backported to kernels >= 6.x, but I'll have to check with JJ.

zyga

This all looks good to me. Thank you for making easy-to-review patches!

olivercalder · 2024-09-05T16:04:10Z

I missed reverting the 22.04 changes to tests/main/apparmor-prompting-snapd-startup/task.yaml as well. Reverting it makes that test fail too. But I don't expect any of these tests to fail on 22.04, as prompting shouldn't be supported. So I'll add better debugging and see if I can figure out why it things prompting is supported on 22.04, and see what happens when running integration tests on it.

olivercalder · 2024-09-05T16:53:59Z

sandbox/apparmor/apparmor.go

+	if strutil.ListContains(apparmorFeatures.KernelFeatures, "policy:notify") {
+		if !strutil.ListContains(apparmorFeatures.KernelFeatures, "policy:notify:user:file") {
+			return false, "the kernel does not support prompting for file access"
+			// XXX: should this error message be "apparmor kernel features do not support prompting" as well?


Any thoughts on this? The error message if the other apparmor kernel features lack prompting support is "apparmor kernel features do not support prompting".

I think it is useful to distinguish each separate case to aid in debugging but perhaps each could share a common prefix to make it easy to know this relates to prompting?

olivercalder · 2024-09-05T19:22:18Z

For some reason the spread debug hooks don't seem to be running when the tests fail? Afaik these should run even if spread is not run with -debug (I thought that was just to drop into a debug shell afterwards), and that enabling debug logging in GitHub just set SNAPD_DEBUG in the environment.

olivercalder · 2024-09-05T21:35:50Z

I ran prompting client integration tests on jammy and they did pass: #14387 (comment)

So it looks like we need a more robust check for kernel version on pre-noble releases. Prompting should still never work on non-ubuntu systems, at least until the prompting patches are upstreamed.

alexmurray

LGTM but I think an additional test case is needed on the kernel features.

alexmurray · 2024-09-06T01:26:51Z

sandbox/apparmor/apparmor.go

+	if strutil.ListContains(apparmorFeatures.KernelFeatures, "policy:notify") {
+		if !strutil.ListContains(apparmorFeatures.KernelFeatures, "policy:notify:user:file") {
+			return false, "the kernel does not support prompting for file access"
+			// XXX: should this error message be "apparmor kernel features do not support prompting" as well?


I think it is useful to distinguish each separate case to aid in debugging but perhaps each could share a common prefix to make it easy to know this relates to prompting?

sandbox/apparmor/apparmor_test.go

alexmurray

LGTM!

pedronis

did a pass, some comments/questions

pedronis · 2024-09-06T07:15:26Z

sandbox/apparmor/apparmor.go

+		}
+	}
+	if !notify.SupportAvailable() {
+		return false, "kernel notification socket required by listener is not present"


this message doesn't make clear that this issue related to prompting, in general I agree with Alex that some kind of common starting prefix/prefix would be good

How does this sound?

--- a/sandbox/apparmor/apparmor.go +++ b/sandbox/apparmor/apparmor.go @@ -460,12 +460,11 @@ func PromptingSupportedByFeatures(apparmorFeatures *FeaturesSupported) (bool, st // 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, "the kernel does not support prompting for file access" - // XXX: should this error message be "apparmor kernel features do not support prompting" as well? + return false, "apparmor kernel features do not support prompting for file access" } } if !notify.SupportAvailable() { - return false, "kernel notification socket required by listener is not present" + return false, "apparmor kernel notification socket required by prompting listener is not present" } return true, "" }

Since these are "unsupported reason" strings returned by the apparmor.PromptingSupported function, I don't think a full-on prefix like apparmor prompting: ... is necessary, but I do think having the common apparmor kernel ... and including the word prompting later could at least make things a bit clearer?

pedronis · 2024-09-06T07:16:24Z

sandbox/apparmor/apparmor.go

@@ -481,6 +509,7 @@ var (

 	// Filesystem root defined locally to avoid dependency on the
 	// 'dirs' package
+	// XXX: is this still useful/relevant? The 'dirs' package is used here already.


until the code stops using it is relevant

I meant whether "avoid dependency on the 'dirs' package" is still relevant. Could we move to just use dirs.GlobalRootDir? I worked around it already so it's not that important for the time being, but it means tests have to be careful not to dig around the host filesystem.

TODO: fix this eventually in follow-up PR

pedronis · 2024-09-06T07:20:20Z

sandbox/apparmor/apparmor.go

+	if err != nil {
+		return []string{}, err
+	}
+	if data, err := os.ReadFile(filepath.Join(rootPath, featuresSysPath, "policy", "permstable32")); err == nil {


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

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).

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

pedronis

comments about the new error messages

pedronis · 2024-09-12T09:43:15Z

sandbox/apparmor/apparmor.go

@@ -466,6 +467,13 @@ func PromptingSupportedByFeatures(apparmorFeatures *FeaturesSupported) (bool, st
 	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, but version could not be read"


this message doesn't mention prompting

How does this sound?

Suggested change

return false, "apparmor kernel permissions table version must be at least 2, but version could not be read"

return false, "apparmor kernel permissions table version must be at least 2 to support prompting, but version could not be read"

pedronis · 2024-09-12T09:45:03Z

sandbox/apparmor/apparmor.go

+		return false, "apparmor kernel permissions table version must be at least 2, but version could not be read"
+	}
+	if version < 2 {
+		return false, fmt.Sprintf("apparmor kernel permissions table version must be at least 2, but version is %d", version)


Alternatively,

Suggested change

return false, fmt.Sprintf("apparmor kernel permissions table version must be at least 2, but version is %d", version)

return false, fmt.Sprintf("apparmor kernel permissions table version must be at least 2 for prompting to be supported, but version is %d", version)

Prompting cannot be supported if the kernel notification socket does not exist. Thus, add a check for its presence to PromptingSupportedByFeatures. However, this alone is not sufficient. The notification socket may be used for other purposes as well. When this becomes the case, a directory will be added at `/sys/kernel/security/apparmor/features/policy/notify`. If that directory exists, and it contains a file called `user`, which contains the list of prompting mediation classes, then prompting is supported on the system. If no file called `user` exists in that directory, but the directory itself exists, then we know prompting is not supported. Signed-off-by: Oliver Calder <[email protected]>

…support test Signed-off-by: Oliver Calder <[email protected]>

…itu test Signed-off-by: Oliver Calder <[email protected]>

…le on debian-sid" This reverts commit 66eb862. Debian Sid reports prompting support in its AppArmor kernel and parser features, the former because the upstream kernel has extended permissions merged (but non-functional), and the latter because we use vendored AppArmor with snapd. But the kernel does not actually support AppArmor prompting, as this requires patches from the Ubuntu kernel which have not yet been upstreamed. Now that we check for the presence of the kernel notification socket as well as the AppArmor kernel and parser features, we can correctly identify that prompting is not supported on Debian Sid. Signed-off-by: Oliver Calder <[email protected]>

…ored apparmor on 22.04" This reverts commit 1ec4132. Signed-off-by: Oliver Calder <[email protected]>

…nal apparmor" This reverts commit a4e46bb. On Ubuntu LTS releases < 24.04, prompting is still not supported by the standard (non-hwe) kernels. This is reflected in tests, now that AppArmor prompting support checks test for the presence of the kernel notification socket. Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

Ubuntu systems older than 24.04 are capable of supporting AppArmor prompting if they are running a new enough Ubuntu kernel. In particular, the Ubuntu 22.04 image on GCP runs a 6.5 kernel which supports prompting. Thus, if run on GCP, Jammy supports prompting, but if run on lxd, it does not. Therefore, checks for Ubuntu version are insufficient for spread to identify whether prompting should be supported. Instead, check for the presence of the `prompt` directive in the permissions listed in `/sys/kernel/security/apparmor/features/policy/permstable32`. Any Ubuntu system which has `prompt` in that file should support prompting. This can't be the entirety of the check, because those permissions have been upstreamed to the Linux kernel, but other pieces which are required for prompting to function have not yet been upstreamed. Thus, Debian Sid reports supporting the prompt directive, even though prompting is not supported. Therefore, we must check that the system is Ubuntu, that it's not core, and that the `prompt` directive is supported. Signed-off-by: Oliver Calder <[email protected]>

…s't include file Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

…erence prompting Signed-off-by: Oliver Calder <[email protected]>

olivercalder · 2024-09-12T16:16:41Z

Adjusted error messages as suggested, and rebased to pull in test fixes from master.

pedronis

thank you

* s/apparmor: expand apparmor prompting support checks Prompting cannot be supported if the kernel notification socket does not exist. Thus, add a check for its presence to PromptingSupportedByFeatures. However, this alone is not sufficient. The notification socket may be used for other purposes as well. When this becomes the case, a directory will be added at `/sys/kernel/security/apparmor/features/policy/notify`. If that directory exists, and it contains a file called `user`, which contains the list of prompting mediation classes, then prompting is supported on the system. If no file called `user` exists in that directory, but the directory itself exists, then we know prompting is not supported. Signed-off-by: Oliver Calder <[email protected]> * s/apparmor: add missing feature to mocked features list in prompting support test Signed-off-by: Oliver Calder <[email protected]> * features: remove superfluous test of apparmor function which has in-situ test Signed-off-by: Oliver Calder <[email protected]> * Revert "tests/main/interfaces-snap-interfaces-requests-control: disable on debian-sid" This reverts commit 66eb862. Debian Sid reports prompting support in its AppArmor kernel and parser features, the former because the upstream kernel has extended permissions merged (but non-functional), and the latter because we use vendored AppArmor with snapd. But the kernel does not actually support AppArmor prompting, as this requires patches from the Ubuntu kernel which have not yet been upstreamed. Now that we check for the presence of the kernel notification socket as well as the AppArmor kernel and parser features, we can correctly identify that prompting is not supported on Debian Sid. Signed-off-by: Oliver Calder <[email protected]> * Revert "tests/main/apparmor-prompting-snapd-startup: account for vendored apparmor on 22.04" This reverts commit 1ec4132. Signed-off-by: Oliver Calder <[email protected]> * Revert "tests: account for prompting supported on >= 22.04 with internal apparmor" This reverts commit a4e46bb. On Ubuntu LTS releases < 24.04, prompting is still not supported by the standard (non-hwe) kernels. This is reflected in tests, now that AppArmor prompting support checks test for the presence of the kernel notification socket. Signed-off-by: Oliver Calder <[email protected]> * s/apparmor,tests: improve comments around apparmor prompting Signed-off-by: Oliver Calder <[email protected]> * tests: add debug checks for prompting support Signed-off-by: Oliver Calder <[email protected]> * tests: make prompting-related tests work on any supported ubuntu system Ubuntu systems older than 24.04 are capable of supporting AppArmor prompting if they are running a new enough Ubuntu kernel. In particular, the Ubuntu 22.04 image on GCP runs a 6.5 kernel which supports prompting. Thus, if run on GCP, Jammy supports prompting, but if run on lxd, it does not. Therefore, checks for Ubuntu version are insufficient for spread to identify whether prompting should be supported. Instead, check for the presence of the `prompt` directive in the permissions listed in `/sys/kernel/security/apparmor/features/policy/permstable32`. Any Ubuntu system which has `prompt` in that file should support prompting. This can't be the entirety of the check, because those permissions have been upstreamed to the Linux kernel, but other pieces which are required for prompting to function have not yet been upstreamed. Thus, Debian Sid reports supporting the prompt directive, even though prompting is not supported. Therefore, we must check that the system is Ubuntu, that it's not core, and that the `prompt` directive is supported. Signed-off-by: Oliver Calder <[email protected]> * s/apparmor: add test case when policy/notify/user file exists but does't include file Signed-off-by: Oliver Calder <[email protected]> * s/apparmor: adjust prompting unsupported reason error messages Signed-off-by: Oliver Calder <[email protected]> * s/apparmor: prompting support checks for permstable32_version >= 2 Signed-off-by: Oliver Calder <[email protected]> * s/apparmor: add TODO about using dirs instead of rootPath Signed-off-by: Oliver Calder <[email protected]> * o/c/configcore,s/apparmor: mock aa root path in prompting tests Signed-off-by: Oliver Calder <[email protected]> * s/apparmor: trim whitespace from permstable32_version before parsing Signed-off-by: Oliver Calder <[email protected]> * s/apparmor: adjust prompting unsupported reason error messages to reference prompting Signed-off-by: Oliver Calder <[email protected]> --------- Signed-off-by: Oliver Calder <[email protected]>

olivercalder added Needs Samuele review Needs a review from Samuele before it can land Needs security review Can only be merged once security gave a :+1: labels Aug 30, 2024

olivercalder requested review from bboozzoo, zyga, pedronis and ernestl August 30, 2024 23:12

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Aug 30, 2024

olivercalder force-pushed the prompting-check-notify-socket-presence branch from eaa08a2 to 279771f Compare September 3, 2024 18:30

bboozzoo approved these changes Sep 4, 2024

View reviewed changes

zyga approved these changes Sep 5, 2024

View reviewed changes

olivercalder force-pushed the prompting-check-notify-socket-presence branch from 279771f to 58e8da3 Compare September 5, 2024 16:04

olivercalder commented Sep 5, 2024

View reviewed changes

olivercalder added this to the 2.66 milestone Sep 5, 2024

olivercalder mentioned this pull request Sep 5, 2024

tests: add prompting client integration tests #14387

Closed

alexmurray requested changes Sep 6, 2024

View reviewed changes

olivercalder requested a review from alexmurray September 6, 2024 05:46

alexmurray approved these changes Sep 6, 2024

View reviewed changes

pedronis reviewed Sep 6, 2024

View reviewed changes

olivercalder requested a review from pedronis September 6, 2024 18:17

olivercalder force-pushed the prompting-check-notify-socket-presence branch from d4aa4ac to 094a2c7 Compare September 10, 2024 01:13

olivercalder requested a review from bboozzoo September 11, 2024 13:11

pedronis reviewed Sep 12, 2024

View reviewed changes

olivercalder added 3 commits September 12, 2024 11:09

s/apparmor: add missing feature to mocked features list in prompting …

d42676e

…support test Signed-off-by: Oliver Calder <[email protected]>

features: remove superfluous test of apparmor function which has in-s…

df40331

…itu test Signed-off-by: Oliver Calder <[email protected]>

olivercalder added 13 commits September 12, 2024 11:09

Revert "tests/main/apparmor-prompting-snapd-startup: account for vend…

e6dfc59

…ored apparmor on 22.04" This reverts commit 1ec4132. Signed-off-by: Oliver Calder <[email protected]>

s/apparmor,tests: improve comments around apparmor prompting

ead3eba

Signed-off-by: Oliver Calder <[email protected]>

tests: add debug checks for prompting support

9cda3e0

Signed-off-by: Oliver Calder <[email protected]>

s/apparmor: add test case when policy/notify/user file exists but doe…

4d45b15

…s't include file Signed-off-by: Oliver Calder <[email protected]>

s/apparmor: adjust prompting unsupported reason error messages

b3e525c

Signed-off-by: Oliver Calder <[email protected]>

s/apparmor: prompting support checks for permstable32_version >= 2

a7e43d8

Signed-off-by: Oliver Calder <[email protected]>

s/apparmor: add TODO about using dirs instead of rootPath

3babde3

Signed-off-by: Oliver Calder <[email protected]>

o/c/configcore,s/apparmor: mock aa root path in prompting tests

0c4166a

Signed-off-by: Oliver Calder <[email protected]>

s/apparmor: trim whitespace from permstable32_version before parsing

22927fa

Signed-off-by: Oliver Calder <[email protected]>

s/apparmor: adjust prompting unsupported reason error messages to ref…

93c6a23

…erence prompting Signed-off-by: Oliver Calder <[email protected]>

olivercalder force-pushed the prompting-check-notify-socket-presence branch from 5e27752 to 93c6a23 Compare September 12, 2024 16:16

olivercalder requested a review from pedronis September 12, 2024 16:17

pedronis approved these changes Sep 12, 2024

View reviewed changes

olivercalder merged commit 8745fb1 into canonical:master Sep 12, 2024
54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

s/apparmor: expand apparmor prompting support checks #14448

s/apparmor: expand apparmor prompting support checks #14448

olivercalder commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

olivercalder commented Sep 3, 2024

bboozzoo left a comment

bboozzoo Sep 4, 2024

olivercalder Sep 5, 2024

bboozzoo Sep 5, 2024

bboozzoo Sep 5, 2024

olivercalder Sep 5, 2024

zyga left a comment

olivercalder commented Sep 5, 2024

olivercalder Sep 5, 2024

alexmurray Sep 6, 2024

olivercalder commented Sep 5, 2024

olivercalder commented Sep 5, 2024

alexmurray left a comment

alexmurray Sep 6, 2024

alexmurray left a comment

pedronis left a comment

pedronis Sep 6, 2024

olivercalder Sep 6, 2024

pedronis Sep 6, 2024

olivercalder Sep 6, 2024

olivercalder Sep 9, 2024

pedronis Sep 6, 2024

olivercalder Sep 6, 2024

olivercalder Sep 9, 2024

pedronis left a comment

pedronis Sep 12, 2024

olivercalder Sep 12, 2024

pedronis Sep 12, 2024

olivercalder Sep 12, 2024

olivercalder commented Sep 12, 2024

pedronis left a comment

	return false, "apparmor kernel permissions table version must be at least 2, but version could not be read"
	return false, "apparmor kernel permissions table version must be at least 2 to support prompting, but version could not be read"

	return false, fmt.Sprintf("apparmor kernel permissions table version must be at least 2, but version is %d", version)
	return false, fmt.Sprintf("apparmor kernel permissions table version must be at least 2 for prompting to be supported, but version is %d", version)

s/apparmor: expand apparmor prompting support checks #14448

s/apparmor: expand apparmor prompting support checks #14448

Conversation

olivercalder commented Aug 30, 2024 • edited Loading

codecov bot commented Aug 30, 2024 • edited Loading

Codecov Report

olivercalder commented Sep 3, 2024

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

olivercalder commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder commented Sep 5, 2024

olivercalder commented Sep 5, 2024

alexmurray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmurray left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder commented Sep 12, 2024

pedronis left a comment

Choose a reason for hiding this comment

olivercalder commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading