cmd/snap-confine: switch to dynamic SNAP_MOUNT_DIR #14074
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zyga
changed the title
pedronis
added
the
Needs security review
zyga
force-pushed
the
feature/probe-and-use-snap-mount-dir-SNAPDENG-22335
branch
2 times, most recently
from
ce41f3d
to
bc56b12
Compare
|
There are some denials that look like missing bits in the security profile. I will investigate tomorrow. |
alexmurray
requested changes
Jun 19, 2024
zyga
force-pushed
the
feature/probe-and-use-snap-mount-dir-SNAPDENG-22335
branch
from
bc56b12
to
7b3089a
Compare
alexmurray
removed
the
Needs security review
zyga
force-pushed
the
feature/probe-and-use-snap-mount-dir-SNAPDENG-22335
branch
3 times, most recently
from
fe2de96
to
87bca11
Compare
bboozzoo
approved these changes
Jun 25, 2024
| @@ -32,6 +32,10 @@ | |||
|
|
|||
| static const char *_snap_mount_dir = NULL; | |||
|
|
|||
| // Function is exported only for tests. | |||
| void sc_set_snap_mount_dir(const char *dir); | |||
There was a problem hiding this comment.
this forward declaration doesn't do anything useful
Suggested change
| void sc_set_snap_mount_dir(const char *dir); |
There was a problem hiding this comment.
turns out it's needed after all, we use -Wmissing-prototypes
Signed-off-by: Zygmunt Krynicki <[email protected]>
Signed-off-by: Zygmunt Krynicki <[email protected]>
Signed-off-by: Zygmunt Krynicki <[email protected]>
Signed-off-by: Zygmunt Krynicki <[email protected]>
Call the probe early in snap-confine's startup logic and display the discovered value. This triggers all the extra LSM permissions necessary to read through /proc/1/root. Signed-off-by: Zygmunt Krynicki <[email protected]>
Instead of a pre-processed apparmor profile, with a statically compiled /snap or /var/lib/snapd/snap, use an apparmor parser variable that expands to both locations. The sed replacement of @SNAP_MOUNT_DIR@ is now pointless. Signed-off-by: Zygmunt Krynicki <[email protected]>
Signed-off-by: Zygmunt Krynicki <[email protected]>
Signed-off-by: Zygmunt Krynicki <[email protected]>
Signed-off-by: Zygmunt Krynicki <[email protected]>
Signed-off-by: Zygmunt Krynicki <[email protected]>
The profile is full of "slave" as in MS_SLAVE, which is a kernel interface. Signed-off-by: Zygmunt Krynicki <[email protected]>
zyga
force-pushed
the
feature/probe-and-use-snap-mount-dir-SNAPDENG-22335
branch
from
87bca11
to
39919ba
Compare
The symlink /snap -> /var/lib/snapd/snap may be alternatively encoded as /snap -> var/lib/snapd/snap in order to stay compatible with symlink packaging rules. Adjusts tests to handle both variatns and tweak naming to match the non-test code. Signed-off-by: Zygmunt Krynicki <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is based on #14068 which contains the first patch in this series (snap-dir.[ch] and snap-dir-test.c). Please disregard that when reviewing.
Bulk of the changes are related to testing sc-invocation. I can break that out if it helps. The rest is fairly straigtforward replacement of
SNAP_MOUNT_DIRwithsc_snap_mount_dir(NULL)except when it was used as a compile time string concatenation (only in tests) where a sligthly more verbose construct is in use.The apparmor profile has two changes: one to allow it to probe (stat,readlink)
/proc/1/root/snap. This change is contained in the patch "cmd/snap-confine: probe SNAP_MOUNT_DIR on startup". The second change is to remove@SNAP_MOUNT_DIR@- sed text replacement - from snap-confie.apparmor.in and replace it with@{SNAP_MOUNT_DIR_LIST}- an apparmor parser variable - so that the same loaded profile allows probing and interacting with either of the locations (canonical and alternate).This works for me locally. I'm opening a draft PR to run tests more widely.
Autoconf still needs
AC_SUBSTonSNAP_MOUNT_DIRas we have a number of places that still hard-code the location at compile time - most notably shell scripts (snap-mgmt.sh, snap-mgmt-selinux.sh) and the snad-generator. This will be left as-is until another pass.Jira: SNAPDENG-22336
Spec: SD-179