interfaces: add classic-support interface by mvo5 · Pull Request #2604 · canonical/snapd

mvo5 · 2017-01-11T09:11:28Z

This avoids the huge amount of logging that is currently generated.
It also allows to install classic in strict mode (with the caveat
that it can not do most privilidged stuff in the chroot).

Will not auto-connect by default, but we should update the package declaratior for the classic snap so that it will auto-connect.

LP: #1654642

This avoids the huge amount of logging that is currently generated. It also allows to install classic in strict mode (with the caveat that it can not do most privilidged stuff in the chroot). LP: #1654642

zyga

Looks good. I wonder if we could scope that so that it only applies to the "classic" snap.

zyga · 2017-01-11T09:14:27Z

@mvo5 you also want to add this to snap/implicit.go

jdstrand

In addition to inline comments, please add classic_dimension_test.go ala network_control_test.go.

jdstrand · 2017-01-11T13:36:09Z

+    allow-installation:
+      slot-snap-type:
+        - core
+    deny-auto-connection: true


If there was ever a definition of a super-privileged interface, it is this one since it allows running /usr/bin/systemd-run unconfined (and thus can run any code) as well as allowing manipulating anything in systemd via unconfined systemctl. As such, please remove this part of the declaration and add to the 'plugs' part of the declaration:

classic-dimension: allow-installation: false deny-auto-connection: true

With this, we'll grant a snap declaration to the classic snap for installation and auto-connection and it'll work like normal, but no other signed snaps will be able to be installed with this interface.

Thanks! I updated the code to do it this way now.

jdstrand · 2017-01-11T13:38:36Z

+		"ppp":               {"core"},
+		"pulseaudio":        {"app", "core"},
+		"serial-port":       {"core", "gadget"},
+		"udisks2":           {"app"},


This is a lot of whitespace changes-- is go fmt happy?

That was all done by go fmt - I just added the classic-dimension: {"core"} change.

jdstrand · 2017-01-11T13:43:11Z

+		"udisks2":           {"app"},
 		// snowflakes
 		"docker": nil,
 		"lxd":    nil,


Please add corresponding tests in basedeclaration_test.go in TestPlugInstallation, TestAutoConnectPlugSlot and additional tests like there are with docker-support and lxd-support.

Thanks! I added more testing like you suggested.

jdstrand · 2017-01-11T13:45:39Z

+)
+
+const classicDimensionPlugAppArmor = `
+# Description: Extra permissions to use the classic dimension on Ubuntu Core


Please adjust to be:

# Description: permissions to use classic dimension. This policy is intentionally # not restricted. This gives device ownership to connected snaps.

jdstrand · 2017-01-11T14:04:25Z

+/etc/sudoers.d/{,*} r,
+/usr/bin/systemd-run Uxr,
+/bin/systemctl Uxr,
+`


You mentioned that you want this to run in strict mode. Here is the policy for that (tested on amd64):

# Description: permissions to use classic dimension. This policy is intentionally # not restricted. This gives device ownership to connected snaps. # for 'create' /{,usr/}bin/unsquashfs ixr, /var/lib/snapd/snaps/core_*.snap r, capability chown, capability fowner, capability mknod, # This allows running anything unconfined /{,usr/}bin/sudo Uxr, capability fsetid, capability dac_override, # Allow copying configuration to the chroot /etc/{,**} r, /var/lib/extrausers/{,*} r, # Allow bind mounting various directories capability sys_admin, /{,usr/}bin/mount ixr, /{,usr/}bin/mountpoint ixr, /run/mount/utab rw, @{PROC}/[0-9]*/mountinfo r, mount options=(rw bind) /home/ -> /var/snap/@{SNAP_NAME}/**/, mount options=(rw bind) /run/ -> /var/snap/@{SNAP_NAME}/**/, mount options=(rw bind) /proc/ -> /var/snap/@{SNAP_NAME}/**/, mount options=(rw bind) /sys/ -> /var/snap/@{SNAP_NAME}/**/, mount options=(rw bind) /dev/ -> /var/snap/@{SNAP_NAME}/**/, mount options=(rw bind) / -> /var/snap/@{SNAP_NAME}/**/, mount fstype=devpts options=(rw) devpts -> /dev/pts/, mount options=(rw rprivate) -> /var/snap/@{SNAP_NAME}/**/, # reset /{,usr/}bin/umount ixr, umount /var/snap/@{SNAP_NAME}/**/, # These rules allow running anything unconfined as well as managing systemd /usr/bin/systemd-run Uxr, /bin/systemctl Uxr,

This was tested on amd64. Note, the policy adjustments are only needed for the setup/reset to get to systemd-run/systemctl since once you hit those commands, the policy goes unconfined.

jdstrand · 2017-01-11T14:06:27Z

+func NewClassicDimensionInterface() interfaces.Interface {
+	return &commonInterface{
+		name: "classic-dimension",
+		connectedPlugAppArmor: classicDimensionPlugAppArmor,


For strict mode you also need:

connectedPlugSecComp: classicDimensionPlugSecComp,

and to define this:

const classicDimensionPlugSecComp = ` # Description: permissions to use classic dimension. This policy is intentionally # not restricted. This gives device ownership to connected snaps. # create chown chown32 lchown lchown32 fchown fchown32 fchownat mknod chroot # sudo bind sendmsg sendmmsg sendto recvfrom recvmsg setgroups setgroups32 # classic mount getsockopt # reset umount umount2

Thanks, added those now too!

mvo5

Thanks for the excellent review!I addressed the raised points.

mvo5 · 2017-01-11T15:28:26Z

+    allow-installation:
+      slot-snap-type:
+        - core
+    deny-auto-connection: true


Thanks! I updated the code to do it this way now.

mvo5 · 2017-01-11T15:29:14Z

+		"ppp":               {"core"},
+		"pulseaudio":        {"app", "core"},
+		"serial-port":       {"core", "gadget"},
+		"udisks2":           {"app"},


That was all done by go fmt - I just added the classic-dimension: {"core"} change.

mvo5 · 2017-01-11T15:40:30Z

+		"udisks2":           {"app"},
 		// snowflakes
 		"docker": nil,
 		"lxd":    nil,


Thanks! I added more testing like you suggested.

mvo5 · 2017-01-11T15:43:03Z

+func NewClassicDimensionInterface() interfaces.Interface {
+	return &commonInterface{
+		name: "classic-dimension",
+		connectedPlugAppArmor: classicDimensionPlugAppArmor,


Thanks, added those now too!

mvo5 · 2017-01-11T15:43:43Z

+)
+
+const classicDimensionPlugAppArmor = `
+# Description: Extra permissions to use the classic dimension on Ubuntu Core


jdstrand · 2017-01-11T19:28:58Z

+    deny-auto-connection: true
  docker-support:
    allow-installation: false
    deny-auto-connection: true


Whoops! We do still need the slot side to say this is ok when provided by the core snap. Sorry about not remembering this in the initial review. Please, in addition to the above, add:

slots: ... classic-dimension: allow-installation: slot-snap-type: - core deny-auto-connection: true

Thanks! I added that now too.

jdstrand · 2017-01-11T22:17:52Z

    deny-auto-connection: true
+  classic-dimension:
+    allow-installation: false
+    deny-auto-connection: true


This isn't right for the slots side (see my previous comment). It should be:

classic-dimension: allow-installation: slot-snap-type: - core deny-auto-connection: true

Ups, sorry :/ And thanks for spotting this. I really should not push code late at night. Fixed now.

jdstrand · 2017-01-12T15:42:15Z

The policy and base declaration LGTM, so I am going to 'Approve changes' conditional on adding a classic_dimension_test.go that tests that the policy is applied when connected. See network-control for an example.

(I thought I asked for this before but I don't see it....)

…imension-interface

niemeyer

I don't understand the background here. It feels like redoing classic snaps as an interface?

jdstrand · 2017-01-17T18:30:01Z

@niemeyer - this is for the classic snap ('sudo snap install classic --devmode -edge') not classic confinement ('confinement: classic'). The classic snap currently uses devmode and as such triggers a lot of ALLOWED messages in the logs since devmode logs all policy violations, but allows the action. Michael's changes creates a 'classic' interface that adds the necessary policy for the classic snap to not only run in devmode with no logged policy violations, but to run in strict mode so one may simply use 'sudo snap install classic').

I outlined as an option using 'confinement: classic' for the classic snap instead of implementing the classic interface, but Michael said that due to the restriction that snaps using 'confinement: classic' are not installable on all-snaps images, this was a better option. You might want to review the discussion in the bug for more context:
https://bugs.launchpad.net/snappy/+bug/1654642/comments/1
https://bugs.launchpad.net/snappy/+bug/1654642/comments/3

niemeyer · 2017-01-19T11:09:04Z

Sounds good.

@mvo5 Let's please catch up next week about this one.

Also, this should probably be "classic-support" rather than dimension (note "docker-support", "lxd-support", etc).

…imension-interface

jdstrand · 2017-01-20T14:51:08Z

+1 for 'classic-support'

niemeyer

LGTM assuming the questions on the base-declaration below are cleared out.

niemeyer · 2017-02-13T12:41:15Z

+# connected snaps.
+
+# Description: permissions to use classic dimension. This policy is intentionally
+# not restricted. This gives device ownership to connected snaps.


Seems duplicated (see right above).

niemeyer · 2017-02-13T12:42:18Z

+    allow-installation:
+      slot-snap-type:
+        - core
+    deny-auto-connection: true


deny-connection: true?

We don't want anyone but "classic" using this, whether auto or manual, right?

deny-connection and deny-auto-connection are independent, so we would want both

We could have 'deny-connection: true' but we've used that only for non-implicit interfaces historically. In this case, the interface is implicit and the base declaration says it may only be supplied by a core snap, so what is currently implemented was deemed sufficient.

I may definitely be missing something, but I'd appreciate some insight into what it is. If this is allowed into the core slot, then arbitrary snaps will be able to connect to it, won't they? Because the search logic is "plug, slot, base plug, base slot". We don't want any snaps other than "classic" (the classic one, not other classic-confined ones) connecting to this slot.

niemeyer · 2017-02-13T12:44:05Z

 plugs:
+  classic-support:
+    allow-installation: false
+    deny-auto-connection: true


@jdstrand Do we actually need this? Won't the slot already prevent both the installation and the auto-connection unless it's the core snap?

Same question for docker-support below.

This could be written different ways but the way we have done it here and with docker-support is that the 'slots' part looks at it from the slot implementation's pov and is therefore focused on the core snap and its implicit interface. Only core snaps are allowed to be installed and provide the implicit slot and because of that, we only disallow auto-connection here. With this, there is no snap declaration required for a core snap (the store already prompts for human review for 'type: os').

The 'plugs' part looks at it from the plugging snap's pov, so a plugging snap can't be installed if it plugs this and we are explicitly saying it can't auto-connect here for clarity (it could techinically be dropped since the constraint is covered on the slots stanza). With this, the classic snap must have a declaration to be installed at all and may have a declaration to auto-connect.

…imension-interface

jdstrand · 2017-02-13T18:00:08Z

Note that the current implementation is following the agreed to 'base declaration patterns' as documented in comments at the top of basedeclaration.go. Please see that for future reference. If we want to change what we are doing for the classic interface, we should re-review the pattern this is following and update the other interfaces that use this pattern (I don't think this is necessary, just saying if we are going to think about things differently, we should be consistent).

…imension-interface

niemeyer · 2017-02-14T12:47:54Z

@jdstrand @mvo5 @pedronis Let's please try to organize a call about this today or tomorrow. We need to be in sync about the meaning of the snap declaration, and there is clearly misunderstandings here.

niemeyer · 2017-02-15T15:42:14Z

We didn't find a time slot soon enough, so I'm merging this. Discussing online with @pedronis, the only thing we need in the base declaration is really the "allow-installation" bits, both in the slot and in the plug. Nothing else.

That works because it prevents the installation of snaps holding both slots and plugs of that kind, unless it's the core snap. So for anything else to be able to be installed, it will necessarily have to hold a more specific declaration which will define how the given case is supposed to be handled.

We also need to clean up the base-declarations. Many of them suffer from similar issues, which makes reading them quite misleading. The key thing to keep in mind is this: there's no merging of rules. In other words, if installation is denied, game over. The connection can't possibly be established because the snap isn't even installed! For the snap to be installed, a more specific declaration will need to allow it, and once we have a more specific declaration, it will also define when connections and auto-connections are performed. If the respective statements are missing, their defaults will be used.

As a reminder, the order of lookup when installing is:

plug → base plug
slot → base slot

and when establishing a connection (there are two ends, so we need to look in both):

plug → slot → base plug → base slot

In each of these cases, the rule set is used completely! In other words, if we're establishing a connection and there is a rule set for the plug of the interface at stake, this will define whether to establish the connection or not. Alone.

niemeyer · 2017-02-15T16:08:04Z

Fixed and improved comment above after more discussions.

add classic-dimension interface

26b66ec

This avoids the huge amount of logging that is currently generated. It also allows to install classic in strict mode (with the caveat that it can not do most privilidged stuff in the chroot). LP: #1654642

zyga approved these changes Jan 11, 2017

View reviewed changes

zyga requested a review from jdstrand January 11, 2017 09:14

add to implicit.go and fix tests (thanks zyga!)

ba90824

jdstrand suggested changes Jan 11, 2017

View reviewed changes

address review comments (thanks to Jamie!)

3221893

mvo5 commented Jan 11, 2017

View reviewed changes

jdstrand suggested changes Jan 11, 2017

View reviewed changes

add classic-dimension to slots as well (thanks to Jamie!)

46478b3

jdstrand reviewed Jan 11, 2017

View reviewed changes

fix slot side base declaration (thanks Jamie!)

6de0a5b

jdstrand approved these changes Jan 12, 2017

View reviewed changes

mvo5 added 2 commits January 13, 2017 09:26

add unit test for the classic-dimension interface

fd7d913

Merge remote-tracking branch 'upstream/master' into feature/classic-d…

7169dfc

…imension-interface

niemeyer requested changes Jan 17, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/classic-d…

4c3c532

…imension-interface

rename classic-dimension to classic-support

1f60d54

mvo5 changed the title ~~interfaces: add classic-dimension interface~~ Jan 25, 2017

niemeyer approved these changes Feb 13, 2017

View reviewed changes

mvo5 force-pushed the feature/classic-dimension-interface branch 3 times, most recently from a68d822 to 1db3568 Compare February 13, 2017 16:04

Merge remote-tracking branch 'upstream/master' into feature/classic-d…

8148f90

…imension-interface

mvo5 force-pushed the feature/classic-dimension-interface branch from 1db3568 to 8148f90 Compare February 13, 2017 16:05

Merge remote-tracking branch 'upstream/master' into feature/classic-d…

4c3752b

…imension-interface

mvo5 added this to the 2.23 milestone Feb 15, 2017

niemeyer merged commit 9eafd17 into canonical:master Feb 15, 2017

Uh oh!

Conversation

mvo5 commented Jan 11, 2017

zyga left a comment

Choose a reason for hiding this comment

zyga commented Jan 11, 2017

jdstrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

jdstrand commented Jan 17, 2017

niemeyer commented Jan 19, 2017

jdstrand commented Jan 20, 2017

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Feb 13, 2017

niemeyer commented Feb 14, 2017

niemeyer commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

niemeyer commented Feb 15, 2017

Labels

5 participants

jdstrand Jan 11, 2017 •

edited

Loading

jdstrand commented Jan 12, 2017 •

edited

Loading

niemeyer commented Feb 15, 2017 •

edited

Loading