i/apparmor: add snippets with priorities #14061

sergio-costas · 2024-06-10T16:47:54Z

AppArmor rules that forbid access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface.

An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected.

To allow this specific case, a temporary patch (
#13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit

This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet.

This generic mechanism allows to give an interface priority over others if needed, like in the previous case.

Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?

zyga

Quick review to share my feelings.

I think the premise is right, some details on usability should be explored to make sure we're not making this readable for ourselves.

interfaces/apparmor/spec.go

interfaces/apparmor/spec_test.go

interfaces/apparmor/spec.go

sergio-costas · 2024-06-11T10:41:15Z

Rebased to take advantage of the changes at #14032

zyga

A quick pass. I like almost everything. Small notes on the use of types - no need to have key.GetFoo that returns the string. Use the real type all the way.

Please remove uid from any comments and reword to mention keys.

Ping for a final pass.

+0.9

interfaces/apparmor/spec.go

interfaces/connection.go

interfaces/apparmor/spec.go

sergio-costas · 2024-06-11T11:44:12Z

I kept the SnippetKey type as a struct to ensure that a string can't be used instead in the functions (unless there's a way to forbid using a string instead of type SnippetKey string... this is: to avoid allowing spec.RegisterSnippetKey("akeyvalue") if SnippetKey type is derived from string).

sergio-costas · 2024-06-11T11:47:40Z

(unless you think it's a good idea to allow it, of course...)

zyga

Have a look.

I'm almost inclined to approve but please tweak the tests and let's get a +1/-1 on the panic behaviour from @pedronis

interfaces/apparmor/spec.go

interfaces/connection.go

interfaces/apparmor/spec.go

interfaces/apparmor/spec_test.go

zyga

LGTM! Thanks :-)

pedronis

I'm not sure how we plan to use SnippetKey registration, where would we do the registration from, as it doesn't allow the re-register the same key

interfaces/apparmor/spec.go

pedronis

I'm don't find Get... very useful, having a string inside the key is still useful for debugging though. A way to get all the keys would be useful though for tests

interfaces/apparmor/spec.go

sergio-costas · 2024-07-02T11:34:45Z

@pedronis Rebased the code and re-implemented the priority between unity7/desktop-legacy and desktop-launch using the new mechanism.

pedronis

suggestion

interfaces/builtin/desktop_launch.go

pedronis

thank you, couple of small comments

interfaces/builtin/desktop_launch.go

last commits need a review

zyga

LGTM with a few nitpicks on non-naming things and some on naming (apologies).

interfaces/apparmor/spec.go

interfaces/apparmor/spec_test.go

AppArmor rules that forbid access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface. An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected. To allow this specific case, a temporary patch ( canonical#13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet. This generic mechanism allows to give an interface priority over others if needed, like in the previous case.

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

This creates a centralized key registry inside apparmor module, so keys can be registered using top variables, and any duplicated key will produce a panic when snapd is launched, thus just panicking in any test too.

A previous PR was merged with a Quick&Dirty(tm) solution to the priority problem between unity7 and desktop-legacy interfaces against desktop-launch interface. Now that it has been merged, that code must be updated to the new mechanism implemented in this PR. This is exactly what this commit does.

codecov-commenter · 2024-07-08T14:24:56Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 93.68421% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.75%. Comparing base (ea13a33) to head (e51e95b).
Report is 37 commits behind head on master.

Files	Patch %	Lines
interfaces/apparmor/spec.go	94.73%	2 Missing and 1 partial ⚠️
interfaces/builtin/desktop_launch.go	86.36%	3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14061      +/-   ##
==========================================
+ Coverage   78.73%   78.75%   +0.02%     
==========================================
  Files        1055     1061       +6     
  Lines      138275   139262     +987     
==========================================
+ Hits       108866   109671     +805     
- Misses      22588    22739     +151     
- Partials     6821     6852      +31

Flag	Coverage Δ
unittests	`78.75% <93.68%> (+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.

sergio-costas force-pushed the UDENG-3108-create-proposal-and-patch-for-app-armor-rule-priorization branch from 07ec35f to c2a031e Compare June 10, 2024 16:48

sergio-costas changed the title ~~Add snippets with priorities~~ snippets: Add snippets with priorities Jun 10, 2024

sergio-costas force-pushed the UDENG-3108-create-proposal-and-patch-for-app-armor-rule-priorization branch from c2a031e to 1fd1a38 Compare June 10, 2024 17:01

zyga reviewed Jun 10, 2024

View reviewed changes

interfaces/apparmor/spec.go Outdated Show resolved Hide resolved

interfaces/apparmor/spec_test.go Outdated Show resolved Hide resolved

interfaces/apparmor/spec.go Outdated Show resolved Hide resolved

sergio-costas force-pushed the UDENG-3108-create-proposal-and-patch-for-app-armor-rule-priorization branch from 8c93503 to 96ebe29 Compare June 11, 2024 10:40

sergio-costas requested a review from zyga June 11, 2024 10:53

zyga reviewed Jun 11, 2024

View reviewed changes

sergio-costas requested a review from zyga June 11, 2024 11:40

zyga reviewed Jun 18, 2024

View reviewed changes

pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Jun 18, 2024

pedronis self-requested a review June 18, 2024 08:47

zyga previously approved these changes Jun 19, 2024

View reviewed changes

pedronis changed the title ~~snippets: Add snippets with priorities~~ i/apparmor: add snippets with priorities Jun 19, 2024

pedronis reviewed Jun 19, 2024

View reviewed changes

interfaces/apparmor/spec.go Outdated Show resolved Hide resolved

pedronis reviewed Jun 20, 2024

View reviewed changes

interfaces/apparmor/spec.go Outdated Show resolved Hide resolved

sergio-costas force-pushed the UDENG-3108-create-proposal-and-patch-for-app-armor-rule-priorization branch from 0e61400 to 49d8788 Compare July 2, 2024 11:33

pedronis self-requested a review July 4, 2024 08:27

pedronis reviewed Jul 4, 2024

View reviewed changes

interfaces/builtin/desktop_launch.go Outdated Show resolved Hide resolved

pedronis self-requested a review July 5, 2024 13:00

pedronis added this to the 2.64 milestone Jul 5, 2024

pedronis approved these changes Jul 8, 2024

View reviewed changes

interfaces/builtin/desktop_launch.go Outdated Show resolved Hide resolved

interfaces/builtin/desktop_launch.go Outdated Show resolved Hide resolved

zyga approved these changes Jul 8, 2024

View reviewed changes

sergio-costas and others added 3 commits July 8, 2024 16:04

Remove slices.Contains, since seems to be not supported

f151926

Update interfaces/apparmor/spec.go

d32e202

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

sergio-costas and others added 21 commits July 8, 2024 16:04

Use testutils.Contains

ed6d3f0

Replace "uid" with "key" for clarity and sanity

958dd6e

Add specific type for priority keys and force registering them

f18d575

Remove unneeded return

7bb87f1

Use SnippetKey as type

1edea76

Don't use "slice" since MacOS seems to not support it

6cb8fe9

Update interfaces/apparmor/spec.go

30f5be8

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

Update interfaces/apparmor/spec.go

0d3ab0a

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

Use String instead of GetValue

57eef42

Use SnippetKey as key instead of the inner string

67e5ba6

Update interfaces/connection.go

65dcd20

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

Several changes requested

fb017b9

Create the SnippetKeys inside Spec

c83658d

Move key registration outside Spec

8642102

This creates a centralized key registry inside apparmor module, so keys can be registered using top variables, and any duplicated key will produce a panic when snapd is launched, thus just panicking in any test too.

Added extra ways of working with SnippetKeys

090a9d5

Add extra check

d147110

Replace GetSnippetKey with GetSnippetKeys

093985e

Add explanation and constants for prioritized snippets

f99daba

Fix prioritized snippet key and add test in all_test

07470e9

Several changes requested by Zygmunt Vazyli

e51e95b

sergio-costas force-pushed the UDENG-3108-create-proposal-and-patch-for-app-armor-rule-priorization branch from b321132 to e51e95b Compare July 8, 2024 14:04

ernestl merged commit c59a5f6 into canonical:master Jul 8, 2024
50 of 53 checks passed

sergio-costas mentioned this pull request Jul 11, 2024

Add desktop-launch in the plug list canonical/snapd-desktop-integration#79

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i/apparmor: add snippets with priorities #14061

i/apparmor: add snippets with priorities #14061

sergio-costas commented Jun 10, 2024

zyga left a comment

sergio-costas commented Jun 11, 2024

zyga left a comment

sergio-costas commented Jun 11, 2024 •

edited

Loading

sergio-costas commented Jun 11, 2024

zyga left a comment

zyga left a comment

pedronis left a comment

pedronis left a comment

sergio-costas commented Jul 2, 2024

pedronis left a comment

pedronis left a comment

zyga left a comment

codecov-commenter commented Jul 8, 2024

i/apparmor: add snippets with priorities #14061

i/apparmor: add snippets with priorities #14061

Conversation

sergio-costas commented Jun 10, 2024

zyga left a comment

Choose a reason for hiding this comment

sergio-costas commented Jun 11, 2024

zyga left a comment

Choose a reason for hiding this comment

sergio-costas commented Jun 11, 2024 • edited Loading

sergio-costas commented Jun 11, 2024

zyga left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

sergio-costas commented Jul 2, 2024

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 8, 2024

Codecov Report

sergio-costas commented Jun 11, 2024 •

edited

Loading