Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/builtin,cmd/snap-confine: add the overmount interface #2524
Conversation
kalikiana
added some commits
Dec 8, 2016
All checks have failed
zyga
requested changes
Dec 21, 2016
Looks good on first try but I bet the security side needs to be inspected deeper (the apparmor profile for snap confine).
I'd like to see a few extra things though:
- documentation entry on the wiki
- a spread test checking this
- change to attributes to give one overmount a set of pairs (src, dst) so that it can be more generic.
| @@ -196,6 +196,12 @@ | ||
| # NOTE: at this stage the /snap directory is stable as we have called | ||
| # pivot_root already. | ||
| + # Support the overmount interface which enables a custom userspace |
zyga
Dec 21, 2016
Contributor
Have a look around the file, I suspect you can kill some rules with this one over-reaching rule.
| + # Support the overmount interface which enables a custom userspace | ||
| + mount options=(ro bind) /snap/*/** -> /*/**, | ||
| + # But no fiddling with snap paths | ||
| + audit deny mount options=(ro bind) /*/** -> /snap/*/**, |
zyga
Dec 21, 2016
Contributor
This will probably deny valid bind mounts allowed by other rules (content interface).
| + } | ||
| + | ||
| + // check that we both a source and a destination | ||
| + source, destination, err := iface.getAttribs(plug.Attrs) |
kalikiana
Jan 4, 2017
Contributor
Currently it looks like this:
plugs:
usrbin:
interface: overmount
source: $SNAP_DATA/bin
destination: /usr/bin
What are you after, in terms of "more generic"? Are you thinking of this?
plugs:
usrbin:
interface: overmount
mount:
- source: $SNAP_DATA/bin
destination: /usr/bin
niemeyer
Feb 13, 2017
Contributor
Yes, that looks good. Let's please just use target instead of destination.
zyga
changed the title from
Overmount
to
many: add the overmount interface
Dec 23, 2016
niemeyer
requested changes
Feb 13, 2017
Thanks for this interface, and sorry for the delay reviewing it. Here is some feedback. If you can go over this quickly, I'm happy to re-review it as soon as you're done.
| + } | ||
| + | ||
| + // check that we both a source and a destination | ||
| + source, destination, err := iface.getAttribs(plug.Attrs) |
kalikiana
Jan 4, 2017
Contributor
Currently it looks like this:
plugs:
usrbin:
interface: overmount
source: $SNAP_DATA/bin
destination: /usr/bin
What are you after, in terms of "more generic"? Are you thinking of this?
plugs:
usrbin:
interface: overmount
mount:
- source: $SNAP_DATA/bin
destination: /usr/bin
niemeyer
Feb 13, 2017
Contributor
Yes, that looks good. Let's please just use target instead of destination.
| + if err != nil { | ||
| + return err | ||
| + } | ||
| + if len(source) == 0 || len(destination) == 0 { |
| + return nil, nil | ||
| +} | ||
| + | ||
| +func (iface *OvermountInterface) overmountMountSnippet(plug *interfaces.Plug, src, dst string) ([]byte, error) { |
niemeyer
Feb 13, 2017
Contributor
The complexity here is unnecessary. This function and the underlying one may be replaced by a single call to fmt.Sprintf while saving code lines, memory, and CPU.
| +} | ||
| + | ||
| +// Obtain yaml-specified source and destination to mount | ||
| +func (iface *OvermountInterface) getAttribs(attribs map[string]interface{}) (string, string, error) { |
niemeyer
Feb 13, 2017
Contributor
s/getAttribs/mounts/; this will need to be tuned so it returns pairs of source/target.
| + return fmt.Sprintf("%s %s none bind%s 0 0", src, dst, mntOpts) | ||
| +} | ||
| + | ||
| +func (iface *OvermountInterface) overmountAppArmorSnippet(plug *interfaces.Plug, src, dst string) ([]byte, error) { |
niemeyer
Feb 13, 2017
Contributor
Same thing. Lots of indirection when a single fmt.Sprintf will do.
| +} | ||
| + | ||
| +func (iface *OvermountInterface) ConnectedPlugSnippet(plug *interfaces.Plug, slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) { | ||
| + source, destination, err := iface.getAttribs(plug.Attrs) |
| + return nil, err | ||
| + } | ||
| + | ||
| + src := resolveSpecialVariable(source, plug.Snap) |
niemeyer
Feb 13, 2017
Contributor
Let's please ensure source is prefixed by $SNAP/ or $SNAP_DATA/ before handing it to resolution, and let's also make sure neither source nor target contain "..".
| + if !strings.HasPrefix(destination, "/") { | ||
| + return nil, fmt.Errorf("destination must be an absolute path: %q", destination) | ||
| + } | ||
| + if strings.HasPrefix(destination, "/snap/") || strings.HasPrefix(destination, "/var/snap/") { |
niemeyer
Feb 13, 2017
Contributor
The checks here are good, but they should be done on sanitize rather than waiting for connect. Or even better, in the common mounts() function.
Let's please also ensure source is prefixed by $SNAP/ or $SNAP_DATA/ and let's also make sure neither source nor target contain "..". Both should be done before resolution, so we're validating the original path used instead of what we already processed.
niemeyer
changed the title from
many: add the overmount interface
to
interfaces/builtin,cmd/snap-confine: add the overmount interface
Feb 13, 2017
|
This has conflicts, broken tests, and open reviews, one since December. We still want something along those lines, but I'm closing this for now as there's nothing we can help you further on before these are done. Please ping us once this is ready for another look again. |
kalikiana commentedDec 21, 2016
No description provided.