Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Implement lxd-client interface exposing the lxd snap (LP: #1634880) #2225
Conversation
| + allow-installation: | ||
| + slot-snap-type: | ||
| + - core | ||
| + deny-auto-connection: false |
stgraber
Oct 31, 2016
Contributor
Not sure what that flag means, but if it means that other snaps will be allowed to auto-connect without special casing in the store, then this is a terrible idea.
Accessing the LXD socket is equivalent to having root access on the system. It lets you access everything on the filesystem and effectively bypass all seccomp and apparmor policies.
It's certainly fine for us to hardcode a few exceptions for well behaved, trusted snaps, but we should never allow a random snap to the store to connect to LXD without an explicit "snap connect" from the user.
| +} | ||
| + | ||
| +func (iface *LxdClientInterface) AutoConnect(*interfaces.Plug, *interfaces.Slot) bool { | ||
| + return true |
stgraber
Oct 31, 2016
Contributor
Right, so again, if my understanding of how snapd works nowadays is correct, this should absolutely not be True.
|
Left a few in-line comments. tl&dr is that we should NEVER allow an untrusted snap to connect to the LXD daemon. Only snaps that we'd trust with full unconfined root privileges on the machine should be allowed to talk to connect to the lxd interface without requiring user interaction. Access to the lxd socket allows you to alter the host network setup, access any character or block devices in /dev, bind-mount any path on the filesystem, load any apparmor policy and bypass any existing apparmor or seccomp policies. I'd say that this interface should be changed to never auto-connect for now. Then the downstream snaps that want to use it can start using it by asking users to manually connect, then once those snaps are deemed stable enough and have been reviewed for safety, an exception can be added so that the store version of those snaps will auto-connect to the lxd interface. |
| +const lxdClientConnectedPlugSecComp = ` | ||
| +# Description: Can manage socket to use the 'lxd' API. | ||
| +setsockopt | ||
| +bind |
stgraber
Oct 31, 2016
Contributor
Hmm, so those don't cover what you actually need to connect to a unix socket, which would be "socket()" and "connect()", rather than "setsockopt()" and "bind()".
Those two are the usual suspects for Go clients which will always call them when establishing a network connection. That's certainly fine, but also not really relevant for this interface.
Especially when you consider that all the needed bits are already in the auto-connectable "network" interface.
| + lxd-client: | ||
| + allow-installation: | ||
| + slot-snap-type: | ||
| + - core |
jdstrand
Nov 1, 2016
•
Edited 1 time
-
jdstrand
Nov 1, 2016
Contributor
-
Nov 1, 2016
After changing to a slot implementation, change this to:
lxd:
allow-installation: false
deny-connection: true
deny-auto-connection: true
This will require the slot side implementation to need a snap declaration to even be installed and requires a plugging snap to also need a snap declaration to connect to lxd. I fully agree with @stgraber's comments regarding how privileged this interface is, and adjusting the base declaration to do this addresses those concerns. You'll of course need to adjust the testsuite changes accordingly.
| +type LxdClientInterface struct{} | ||
| + | ||
| +func (iface *LxdClientInterface) Name() string { | ||
| + return "lxd-client" |
jdstrand
Nov 1, 2016
•
Edited 1 time
-
jdstrand
Nov 1, 2016
Contributor
-
Nov 1, 2016
This should be renamed as 'lxd' with the lxd snap adding 'slots: [ lxd ]' and clients using 'plugs: [ lxd ]'. See interfaces/builtin/docker.go for an example of what this might look like. We do this because as implemented, the 'lxd-client' interface is assuming there is something to connect to at /var/snap/lxd/common/lxd/unix.socket, but that is only there when the lxd snap is installed. Doing it as I suggest will allow the lxd snap to be auto-installed when installing a client that needs it (when that feature is implemented).
stgraber
Nov 1, 2016
Contributor
Sounds good, though I think you meant "lxd" and "lxd-client" above rather than "lxc" and "lxc-client" :)
| + return nil, nil | ||
| + } | ||
| + return nil, nil | ||
| +} |
jdstrand
Nov 1, 2016
Contributor
I think you do want whatever seccomp rules are needed to talk to and use the socket without any other 'plugs'.
stgraber
Nov 1, 2016
Contributor
From a basic netcat, we see:
socket(AF_LOCAL, SOCK_STREAM, 0) = 3
fcntl(3, F_SETFD, FD_CLOEXEC) = 0
connect(3, {sa_family=AF_LOCAL, sun_path="/var/lib/lxd/unix.socket"}, 26) = 0
poll([{fd=3, events=POLLIN}, {fd=0, events=POLLIN}], 2, -1
) = 1 ([{fd=0, revents=POLLIN}])
read(0, "\n", 2048) = 1
write(3, "\n", 1) = 1
poll([{fd=3, events=POLLIN}, {fd=0, events=POLLIN}], 2, -1) = 1 ([{fd=3, revents=POLLIN|POLLHUP}])
read(3, "HTTP/1.1 400 Bad Request\r\nConten"..., 2048) = 88
write(1, "HTTP/1.1 400 Bad Request\r\nConten"..., 88HTTP/1.1 400 Bad Request
Content-Type: text/plain
Connection: close
400 Bad Request) = 88
poll([{fd=3, events=POLLIN}, {fd=0, events=POLLIN}], 2, -1) = 1 ([{fd=3, revents=POLLIN|POLLHUP}])
read(3, "", 2048) = 0
shutdown(3, SHUT_RD) = 0
close(3) = 0
So that's:
- socket (included in base)
- fcntl (included in base)
- connect (included in base)
- poll (included in base)
- read (included in base)
- write (included in base)
- shutdown
- close (included in base)
So looks like all that would be allowed for a basic unix socket connection would be "shutdown". Everything else being allowed already by default.
That obviously won't actually let you interact with the socket from something that's written in Go and does extra fancy socket setup, but that's specific to the client rather than the interface.
| + | ||
| +func (iface *LxdClientInterface) AutoConnect(*interfaces.Plug, *interfaces.Slot) bool { | ||
| + return false | ||
| +} |
jdstrand
Nov 1, 2016
Contributor
This should be:
// allow what declarations allowed
return true
which simply says that the interface won't override whatever the base and snap declarations allow, and with the suggested changes for the base declaration, this is the correct approach.
| @@ -36,6 +36,7 @@ var implicitSlots = []string{ | ||
| "hardware-observe", | ||
| "locale-control", | ||
| "log-observe", | ||
| + "lxd-client", |
|
So the current state is, by changing the rules locally I can install an lxd snap with slots: [lxd], and a test snap using the Go API via the socket works. From what I understand it's expected that local testing of the correct rules as in the current code is impossible. |
|
Looks like your branch needs rebasing. You don't usually want Merge commits in a pull request. |
kalikiana
added some commits
Oct 20, 2016
jdstrand
requested changes
Nov 8, 2016
This is close; thanks for your changes! Just a few more minor requests.
| @@ -179,6 +179,7 @@ func (s *baseDeclSuite) TestAutoConnectPlugSlot(c *C) { | ||
| snowflakes := map[string]bool{ | ||
| "content": true, | ||
| "home": true, | ||
| + "lxd": true, |
jdstrand
Nov 8, 2016
Contributor
I'm not sure why lxd is a snowflake in TestAutoConnectPlugSlot. Can you comment?
| @@ -466,6 +468,7 @@ func (s *baseDeclSuite) TestConnection(c *C) { | ||
| "fwupd": true, | ||
| "location-control": true, | ||
| "location-observe": true, | ||
| + "lxd": true, | ||
| "mir": true, | ||
| "modem-manager": true, | ||
| "udisks2": true, |
jdstrand
Nov 8, 2016
Contributor
Please add another test to TestSlotInstallation() for lxd, underneath the docker one.
| +) | ||
| + | ||
| +const lxdConnectedPlugAppArmor = ` | ||
| +# Description: Can access commands and socket from the 'lxd' snap. |
jdstrand
Nov 8, 2016
Contributor
Can you update this description to be like the one you have in lxdConnectedPlugSecComp?
| +const lxdConnectedPlugSecComp = ` | ||
| +# Description: allow access to the LXD daemon socket. This gives privileged | ||
| +# access to the system via LXD's socket API. | ||
| +# Usage: reserved |
| + // connected plugs have a non-nil security snippet for apparmor | ||
| + snippet, err := s.iface.ConnectedPlugSnippet(s.plug, s.slot, interfaces.SecurityAppArmor) | ||
| + c.Assert(err, IsNil) | ||
| + c.Assert(snippet, Not(IsNil)) |
| + c.Assert(snippet, Not(IsNil)) | ||
| +} | ||
| + | ||
| +func (s *LxdInterfaceSuite) TestPermanentSlotPolicyAppArmor(c *C) { |
jdstrand
Nov 8, 2016
Contributor
This function is named incorrectly. You are testing ConnectedPlugSnippet.
| + snippet, err := s.iface.ConnectedPlugSnippet(s.plug, s.slot, interfaces.SecurityAppArmor) | ||
| + c.Assert(err, IsNil) | ||
| + c.Check(string(snippet), testutil.Contains, "/var/snap/lxd/common/lxd/unix.socket rw,\n") | ||
| +} |
jdstrand
Nov 8, 2016
Contributor
Can you add another test for TestConnectedPlugPolicySecComp and look for shutdown in the same manner that you are searching for an apparmor rule here?
| + c.Check(s.iface.LegacyAutoConnect(), Equals, false) | ||
| +} | ||
| + | ||
| +func (s *LxdInterfaceSuite) TestAutoConnect(c *C) { |
jdstrand
Nov 8, 2016
Contributor
Please add the following comment: // allow what declarations allowed
|
Thanks for the updates! |
|
LGTM as well, if @jdstrand is happy please merge. |
kalikiana
changed the title from
Implement lxd-client interface exposing the lxd snap
to
Implement lxd-client interface exposing the lxd snap (LP: #1634880)
Nov 15, 2016
jdstrand
merged commit 935a9cd
into
snapcore:master
Nov 15, 2016
|
@jdstrand any example how may one downstream snap use this interface? |
|
@vicamo - The interface only gives access to the lxd socket via |
|
@jdstrand, yes, I read that notice from your comments, and it's what we need. Thank you. |
kalikiana commentedOct 27, 2016
•
Edited 1 time
-
kalikiana
Nov 14, 2016
https://bugs.launchpad.net/snappy/+bug/1634880