interfaces: add microstack-support interface

[dshcherb]

Add an interface to enable MicroStack to work in a confined environment.

[zyga]

I didn't review the policy yet. Some quick comments:

• there needs to be a new plug/slot added to a spread test that checks each kind of interface - the test failures usually indicate which
• many tests need changes to the tests in the interfaces/policy package

[jdstrand]

Thanks for submitting this. Lots of things to change, but much is just removing things that exist in other interfaces. Please do see various questions inline.

[dshcherb]

Commented where things needed changing or clarifying and submitted a follow-up commit.

I also added most of the rules needed for LVM and loop device interaction but I need to test a few more things to complete that.

Other basic functionality worked the same way with the updated interface (booting a VM and connecting to it via ssh over the networking layout created for/via OVN).

[dshcherb]

Rebased on top of recent snapd changes and added rules for the LVM support.

One more thing besides the LVM support that's needed for MicroStack to have an LVM-backed volume backend is running tgtd. This has to do with the way Cinder exposes access to LVM volumes to VMs - it is done via iSCSI. There are multiple reasons why that's the case:

• a VM might be live migrated from a host with the LVM volume but the volume will stay at the old node;
• LVM volumes may be created on a storage node so they need to be accessed over a network.

In the absence of a better volume backend, LVM provides a way to test volume creation which is what is needed to improve MicroStack gate tests since certain tempest tests require volume-backed instances to be created.

I am going to check which rules will be needed for running tgtd and if the interface will need to be modified further to make that work.

[dshcherb]

Added the iSCSI bits in 4db6c1a.

Unfortunately, upstream OpenStack does not currently allow one to leverage the QEMU's built-in iscsi support, therefore, MicroStack has to include iscsid in addition to tgtd.

Using iscsid is certainly more complicated as it uses the kernel data plane via the iscsi_tcp kernel module which means that snap removal becomes a more complex exercise involving the iSCSI session cleanup and that the microstack_support interfaces needs to be much more privileged in terms of sysfs access.

[dshcherb]

For reference, when tested with snapd built with this PR applied and a test-time workaround for LP #1892895 applied, I managed to get 0 failures of Refstack tests.

https://review.opendev.org/#/c/738242/9
https://zuul.opendev.org/t/openstack/build/19f8f05e192b471cbdb24c8d48467c47/log/job-output.txt#31891

2020-08-26 12:05:37.874765 \| ubuntu-bionic \| Ran: 104 tests in 629.866 sec.
2020-08-26 12:05:37.874779 \| ubuntu-bionic \|  - Success: 102
2020-08-26 12:05:37.874818 | ubuntu-bionic |  - Failures: 0

This is to show the applicability of the interface code to the target workload.

[mardy]

I've written some comments inline.

[mardy]

Hi again :-) I've been thinking a bit about this interface, and it seems to me that, no matter how carefully we write it, we are always leaving the door open for a process running inside the snap to break out of the confinement, if we allow libvirt to create apparmor profiles and run new processes with the newly created profiles. That's because the process running libvirt could be compromised, or there might be bugs that make it so that the generated apparmor profiles are way more permissive than what we'd like them to be.

This would not be an issue if we could rely on the NoNewPrivileges flag, because then we would be sure that only profile transitions reducing the permission set would be allowed; unfortunately, though, for reasons that I've yet to fully grasp, we are not setting that flag in snap-confine.

So, I wonder if it wouldn't be better, until we have the NoNewPrivileges flag set, to disable apparmor in libvirt and have all processes inside this snap running under the same AppArmor profile. It's a suboptimal solution, but at least it would guarantee us that no process from inside this snap can escape the confinement (as long as we don't have cap_sys_admin enabled).

[jdstrand]

Hi again :-) I've been thinking a bit about this interface, and it seems to me that, no matter how carefully we write it, we are always leaving the door open for a process running inside the snap to break out of the confinement, if we allow libvirt to create apparmor profiles and run new processes with the newly created profiles. That's because the process running libvirt could be compromised, or there might be bugs that make it so that the generated apparmor profiles are way more permissive than what we'd like them to be.

This would not be an issue if we could rely on the NoNewPrivileges flag, because then we would be sure that only profile transitions reducing the permission set would be allowed; unfortunately, though, for reasons that I've yet to fully grasp, we are not setting that flag in snap-confine.

So, I wonder if it wouldn't be better, until we have the NoNewPrivileges flag set, to disable apparmor in libvirt and have all processes inside this snap running under the same AppArmor profile. It's a suboptimal solution, but at least it would guarantee us that no process from inside this snap can escape the confinement (as long as we don't have cap_sys_admin enabled).

NoNewPrivileges interferes with profile transitions by snap-confine which is why we do not set it there. There has been some work in this area upstream but it is not yet available everywhere. As for setting it after snap-confine has transitioned, because of the interactions between historic apparmor and NNP, this can alter the behavior of the applications that try to use NNP themselves, etc. We've felt that the combination of the existing sandbox technologies (apparmor, seccomp, etc) achieved many of the same goals as NNP but with added flexibility for (privileged) interfaces that need it.

As for the observation about libvirt, you are correct. If an application can load apparmor policy and transition into it, it can trivially break out of confinement (note, libvirt (and docker, containerd, lxd, etc, etc) have many more ways to break out (consider mounting a raw disk that would allow writes that changed policy, apparmor_parser/snapd itself, etc)), so how we've decided to handle that is to called these 'superprivileged' interfaces, where the interfaces allow the application to do its job but we disallow installation/connection/etc through snapd interface policy in the snap declarations. In this manner, we might grant microk8s-support to Canonical since it is the upstream for microk8s, but disallow it to everyone else. We then document that the interface is advisory and grants ownership to the system.

In general, IMHO approaching interfaces that require lots of privilege can be done in phases. The first phase is like I describe above (lenient policy that let's them get the job done, with some guardrails (eg, let them only transition to policy with a certain naming convention, etc)). As technologies mature, later phases could further restrict (where the interface could have attributes that allow choosing the 'flavor' of the interface policy, and these attributes are mediatable in the snap declaration) and take advantage of improvements to sandboxing technologies. It's also possible to make adjustments to the internal components of the snap to understand snap sandboxing (eg, in the case of libvirt, perhaps there is a snap helper that libvirtd could call out to do policy writes/transitions or libvirt is modified to have reduced functionality when running in a snap sandbox (ie, it can't or can only attach a subset of devices, etc)).

CC @alexmurray for awareness

[pedronis]

this is a high-level remark but it seems some of the accesses here like LVM or iscsi (maybe) and maybe libvirt (not sure, Jamie made that remark a while ago) could be extracted into separate -control or -support interfaces. This makes sense only if the sets are cohesive and not too specific to what microstack does vs what a generic consumer of those (kernel) interfaces would do.

[pedronis]

... could be extracted into separate -control or -support interfaces

there is something going in that direction here #9585

[dshcherb]

Sorry about the long delay in response. Answered some in-line comments in the review.

[dshcherb]

I just did another functional test run (Tempest, refstack subset) with locally built snapd that had microstack-support included.

All volume tests failed due to https://bugs.launchpad.net/snapd/+bug/1892895 which is not something I can fix in this interface (see the details in the bug, including comment #4).

When I manually allowed the necessary devices via wildcard rules to avoid the race condition like this

sudo bash -c 'echo b 253:* rwm > /sys/fs/cgroup/devices/snap.microstack.<service-name>/devices.allow'

I was able to make to get a full pass: https://paste.ubuntu.com/p/TB6VtxThSz/

In summary: I do not see any new rules that need to be added to this interface in order for us to get our functional tests to pass (except for volume tests for experimental/optional volume functionality).

[mardy]

Looks fine to me, just minor comments. Given that the interface is relatively trivial in logic (it's complex as far as AppArmor rules are concerned, but it's a very typical interface as far as snapd is concerned) I wouldn't insist on a spread test, but let's see what other people think.

[mardy]

Excellent! Let's wait, though, for the security team review (as well for some more expert team-mates of mine). :-)

[alexmurray]

Overall this looks pretty solid - the only glaring bit which stood out was the iptables related change to the network-control interface - this doesn't seem right to me, but otherwise this seems fine as is (whilst I like the idea of a libvirt-support or similar interface which could encapsulate some of this, I don't think it is fair to block this PR on waiting for that, especially given we don't seem to have other users for such an interface at the moment).

[dshcherb]

I did another functional test run after ebc2f4f (without volume testing since those rules were not changed). Results: https://paste.ubuntu.com/p/rPtZFv6DVg/.

Unit tests are passing and most of the spread tests too (the failures are not related to this change).

[anonymouse64]

lgtm, one question about non-systemd layouts and a test suggestion

[codecov-commenter]

Codecov Report

Merging #8926 (dfdb5c1) into master (8170654) will decrease coverage by 2.38%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8926      +/-   ##
==========================================
- Coverage   80.71%   78.32%   -2.39%     
==========================================
  Files         727      883     +156     
  Lines       58158    99322   +41164     
==========================================
+ Hits        46940    77797   +30857     
- Misses       7544    16634    +9090     
- Partials     3674     4891    +1217     

Flag	Coverage Δ
unittests	78.32% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
daemon/api_users.go
interfaces/apparmor/apparmor.go
cmd/snap/color.go
overlord/ifacestate/udevmonitor/udevmon.go
cmd/snap/cmd_sign_build.go
interfaces/ifacetest/spec.go
errtracker/errtracker.go
cmd/snap/cmd_find.go
overlord/healthstate/healthstate.go
interfaces/builtin/gpio_control.go
... and 1516 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc6a45f...dfdb5c1. Read the comment docs.

[dshcherb]

Hey @jdstrand, any chance you could approve this or dismiss the change request? I think what was requested originally has been addressed but the change request is still active.

Thanks in advance!

pr has changed significantly

[pedronis]

looks alright