interfaces: add classic-support interface

[mvo5]

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

[zyga]

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

[zyga]

@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]

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.

[mvo5]

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

[jdstrand]

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

[mvo5]

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

[jdstrand]

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

[mvo5]

Thanks! I added more testing like you suggested.

[jdstrand]

Please adjust to be:

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

[mvo5]

Updated.

[jdstrand]

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]

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

[mvo5]

Thanks, added those now too!

[mvo5]

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

[jdstrand]

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.

[mvo5]

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

[jdstrand]

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

[mvo5]

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

[jdstrand]

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

[mvo5]

Thanks! I added more testing like you suggested.

[jdstrand]

Please adjust to be:

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

[mvo5]

Updated.

[jdstrand]

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

[mvo5]

Thanks, added those now too!

[jdstrand]

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

[mvo5]

Thanks! I added that now too.

[jdstrand]

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

[mvo5]

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

[jdstrand]

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....)

[niemeyer]

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

[jdstrand]

@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]

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).

[jdstrand]

+1 for 'classic-support'

[niemeyer]

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

[niemeyer]

@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.

[jdstrand]

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.

[niemeyer]

deny-connection: true?

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

[pedronis]

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

[jdstrand]

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.

[niemeyer]

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]

Seems duplicated (see right above).

[jdstrand]

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).

[niemeyer]

@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]

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]

Fixed and improved comment above after more discussions.