Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Enable snap-confine namespace sharing #145
Conversation
jdstrand
reviewed
Sep 14, 2016
| + /run/snapd/ns/ rw, | ||
| + /run/snapd/ns/*.lock rwk, | ||
| + /run/snapd/ns/*.mnt rw, | ||
| + ptrace, |
jdstrand
Sep 14, 2016
Contributor
This seems to work ok:
ptrace (tracedby) peer=/usr/lib/snapd/snap-confine//mount-namespace-capture-helper,
jdstrand
reviewed
Sep 14, 2016
| + /run/snapd/ r, | ||
| + /run/snapd/ns/ r, | ||
| + /run/snapd/ns/*.mnt rw, | ||
| + # mount options=(rw bind) /proc/*/ns/mnt/ -> /run/snapd/ns/*.mnt/, |
zyga
Sep 14, 2016
Collaborator
Ah, I'll check if it works with the version in the comment and then remove the one that's not required.
jdstrand
reviewed
Sep 14, 2016
| + /run/snapd/ns/ r, | ||
| + /run/snapd/ns/*.mnt rw, | ||
| + # mount options=(rw bind) /proc/*/ns/mnt/ -> /run/snapd/ns/*.mnt/, | ||
| + mount options=(rw bind) -> /run/snapd/ns/*.mnt, |
jdstrand
Sep 14, 2016
Contributor
What are the denials for this-- is it possible to set the source mount?
zyga
Sep 14, 2016
Collaborator
This is paired with the commented-out version above. I'll check if the source can be used (last time I checked it couldn't be but then I've added attach_disconnected above.
jdstrand
Sep 14, 2016
Contributor
This seems to work with just:
mount options=(rw bind) / -> /run/snapd/ns/*.mnt,
jdstrand
reviewed
Sep 14, 2016
| + # Those two rules are exactly the same but we don't know if the parent process is still alive | ||
| + # and hence has the appropriate label or is already dead and hence has no label. | ||
| + signal (send) set=(exists) peer=@LIBEXECDIR@/snap-confine, | ||
| + signal (send) set=(exists) peer=unconfined, |
jdstrand
reviewed
Sep 14, 2016
| + signal (receive) peer=unconfined, | ||
| + # This allows snap-confine to wait for us | ||
| + ptrace (tracedby) peer=@LIBEXECDIR@/snap-confine, | ||
| + ptrace, |
jdstrand
Sep 14, 2016
Contributor
Can we fine-tune this ptrace rule? What are the denials you see? Also, the second rule is a super-set of the first.
zyga
Sep 14, 2016
Collaborator
Indeed, this was a quick-n-dirty approach. I'll get rid of both of the broad ptraces
jdstrand
Sep 14, 2016
Contributor
Change these two rules:
ptrace (tracedby) peer=@LIBEXECDIR@/snap-confine,
ptrace,
to just this:
ptrace (trace, tracedby) peer=/usr/lib/snapd/snap-confine,
and it seems to work.
|
I've so far only focused on blackbox testing and found:
Here is a snap that can be used for testing: https://git.launchpad.net/~jdstrand/+git/snap-netns-test Run: A properly working namespace sharing implementation will allow the above commands to work within the same shell process group (this worked at one point but doesn't in the current implementation in 16.04), as well as running 'ip netns add testnet' in one process group and 'ip netns exec testnet bash' and 'ip netns del testnet' in another (the impetus for namespace sharing; see https://bugs.launchpad.net/snappy/+bug/1611444/comments/8 for details). The testing was performed on an all-snaps system: |
|
I also discovered that the lock files are created with the gid of the calling user: |
|
FYI, I though snap-discard-ns wasn't working since the files still exist in /run/snapd/ns, but those files are no longer magic, so it is working. I'm not sure if this is intended to be shipped anywhere, but if so, removing the non-magical files would probably avoid confusion. |
zyga
added some commits
Sep 15, 2016
|
@jdstrand |
| + sc_preserve_populated_ns_group(group); | ||
| + } | ||
| + sc_unlock_ns_mutex(group); | ||
| + sc_close_ns_group(group); |
jdstrand
Sep 15, 2016
Contributor
This logic seems fine but seems like a lot to have in sc-main.c. Before we had two simple calls to unshare and to populate, but now we have tricky logic and ordering. I think this is fine to land as is, but perhaps we could abstract this out to a wrapper function in ns-support.c and then sc-main.c can just call that. This would also provide an opportunity to document in an easier to understand fashion by documenting the high-level wrapper function and what it does, and then leave documenting the nitty-gritty details of what it uses to above the respective functions.
|
After further discussion on IRC, the 'ip netns' failures are not introduced by this PR and they should not block the landing. However, @zyga did say that he will add spread tests demonstrating the failures and document that 'ip netns' is currently broken while solutions are investigated. |
|
@zyga asked me to check the cgroup handling with this branch. It all works fine. If I assign a device to a snap command it shows up for that command only. If I try to access that device from another command from within the same snap, it is correctly not added to that snap's device cgroup. |
|
I'm merging this because the bug found by jdstrand above (reported as https://bugs.launchpad.net/snap-confine/+bug/1624300) seems to be a genuine error in apparmor and we found a workaround that makes "ip netns" work. I have a failure test case but I don't know if it makes sense to add to snap-confine at this time, I will propose it separately next. |
zyga
merged commit c5612d7
into
master
Sep 16, 2016
1 check passed
zyga
deleted the
really-enable-ns-sharing
branch
Sep 16, 2016
|
FYI, changed the bug number to: https://bugs.launchpad.net/apparmor/+bug/1624497 I've verified that after connecting the 'network-control' interface to a devmode snap (ie, workaround the apparmor bug), with this PR I can:
|
zyga commentedSep 14, 2016
This branch changes mount-support.[ch] a little so that there's no
explicit unshare API anymore (this is handled by ns-support.h) and so
that mount-suppor.h is really just about populating the namespace that
is already provided.
In addition, sc-main.c now uses SNAP_NAME to join or create a namespace
group and if populates it if necessary. The apparmor profile is updated
to let snap-confine perform the additional tasks.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com