overlord/ifacestate: setup security backends phased by backends first

overlord/ifacestate/handlers.go

@@ -105,6 +105,8 @@ func (m *InterfaceManager) doSetupProfiles(task *state.Task, tomb *tomb.Tomb) er
 }
 
 func (m *InterfaceManager) setupProfilesForSnap(task *state.Task, _ *tomb.Tomb, snapInfo *snap.Info, opts interfaces.ConfinementOptions) error {
+	st := task.State()
+
 	if err := addImplicitSlots(task.State(), snapInfo); err != nil {
 		return err
 	}
@@ -137,28 +139,61 @@ func (m *InterfaceManager) setupProfilesForSnap(task *state.Task, _ *tomb.Tomb,
 		task.Logf("%s", snap.BadInterfacesSummary(snapInfo))
 	}
 
+	// Reload the connections and compute the set of affected snaps. The set
+	// affectedSet set contains name of all the affected snap instances.  The
+	// arrays affectedNames and affectedSnaps contain, arrays of snap names and
+	// snapInfo's, respectively. The arrays are sorted by name with the special
+	// exception that the snap being setup is always first. The affectedSnaps
+	// array may be shorter than the set of affected snaps in case any of the
+	// snaps cannot be found in the state.
 	reconnectedSnaps, err := m.reloadConnections(snapName)
 	if err != nil {
 		return err
 	}
-	if err := m.setupSnapSecurity(task, snapInfo, opts); err != nil {
-		return err
-	}
 	affectedSet := make(map[string]bool)
 	for _, name := range disconnectedSnaps {
 		affectedSet[name] = true
 	}
 	for _, name := range reconnectedSnaps {
 		affectedSet[name] = true
 	}
-	// The principal snap was already handled above.
-	delete(affectedSet, snapInfo.InstanceName())
-	affectedSnaps := make([]string, 0, len(affectedSet))
+
+	// Sort the set of affected names, ensuring that the snap being setup
+	// is first regardless of the name it has.
+	affectedNames := make([]string, 0, len(affectedSet))
 	for name := range affectedSet {
-		affectedSnaps = append(affectedSnaps, name)
+		if name != snapName {
+			affectedNames = append(affectedNames, name)
+		}
+	}
+	sort.Strings(affectedNames)
+	affectedNames = append([]string{snapName}, affectedNames...)
+
+	// Obtain snap.Info for each affected snap, skipping those that cannot be
+	// found and compute the confinement options that apply to it.
+	affectedSnaps := make([]*snap.Info, 0, len(affectedSet))
+	confinementOpts := make([]interfaces.ConfinementOptions, 0, len(affectedSet))
+	// For the snap being setup we know exactly what was requested.
+	affectedSnaps = append(affectedSnaps, snapInfo)
+	confinementOpts = append(confinementOpts, opts)
+	// For remaining snaps we need to interrogate the state.
+	for _, name := range affectedNames[1:] {
+		var snapst snapstate.SnapState
+		if err := snapstate.Get(st, name, &snapst); err != nil {
+			task.Errorf("cannot obtain state of snap %s: %s", name, err)
+			continue
+		}
+		snapInfo, err := snapst.CurrentInfo()
+		if err != nil {
+			return err
+		}
+		if err := addImplicitSlots(st, snapInfo); err != nil {
+			return err
+		}
+		affectedSnaps = append(affectedSnaps, snapInfo)
+		confinementOpts = append(confinementOpts, confinementOptions(snapst.Flags))
 	}
-	sort.Strings(affectedSnaps)
-	return m.setupAffectedSnaps(task, snapName, affectedSnaps)
+	return m.setupSecurityByBackend(task, affectedSnaps, confinementOpts)
 }
 
 func (m *InterfaceManager) doRemoveProfiles(task *state.Task, tomb *tomb.Tomb) error {

overlord/ifacestate/helpers.go

@@ -310,6 +310,26 @@ func (m *InterfaceManager) reloadConnections(snapName string) ([]string, error)
 	return result, nil
 }
 
+func (m *InterfaceManager) setupSecurityByBackend(task *state.Task, snaps []*snap.Info, opts []interfaces.ConfinementOptions) error {
+	st := task.State()
+
+	// Setup all affected snaps, start with the most important security
+	// backend and run it for all snaps. See LP: 1802581
+	for _, backend := range m.repo.Backends() {
+		for i, snapInfo := range snaps {
+			st.Unlock()
+			err := backend.Setup(snapInfo, opts[i], m.repo)
+			st.Lock()
+			if err != nil {
+				task.Errorf("cannot setup %s for snap %q: %s", backend.Name(), snapInfo.InstanceName(), err)
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
 func (m *InterfaceManager) setupSnapSecurity(task *state.Task, snapInfo *snap.Info, opts interfaces.ConfinementOptions) error {
 	st := task.State()
 	instanceName := snapInfo.InstanceName()

tests/regression/lp-1802581/mock-gpio.py

@@ -0,0 +1,96 @@
+#!/usr/bin/python3
+"""Mock the /sys/class/gpio subsystem"""
+
+import atexit
+import logging
+import os
+import selectors
+import shutil
+import subprocess
+import sys
+import tempfile
+
+from typing import Type
+
+
+def read_gpio_pin(read_fd: int) -> str:
+    """
+    read_gpio_pin reads from the given fd and return a string with the
+    numeric pin or "" if an invalid pin was specified.
+    """
+    data = os.read(read_fd, 128)
+    pin = data.decode().strip()
+    if not pin.isnumeric():
+        logging.warning("invalid gpio pin %s", pin)
+        return ""
+    return "gpio{}".format(pin)
+
+
+def export_ready(read_fd: int, _) -> bool:
+    """export_ready is run when the "export" file is ready for reading"""
+    pin = read_gpio_pin(read_fd)
+    # allow quit
+    if pin == 'quit':
+        return False
+    if pin:
+        with open(pin, "w"):
+            pass
+    return True
+
+
+def unexport_ready(read_fd: int, _) -> bool:
+    """export_ready is run when the "unexport" file is ready for reading"""
+    pin = read_gpio_pin(read_fd)
+    try:
+        os.remove(pin)
+    except OSError as err:
+        logging.warning("got exception %s", err)
+    return True
+
+
+def dispatch(sel):
+    """
+    dispatch dispatches the events from the "sel" source
+    """
+    for key, mask in sel.select():
+        callback = key.data
+        if not callback(key.fileobj, mask):
+            return False
+    return True
+
+
+def main():
+    """the main method"""
+    if os.getuid() != 0:
+        print("must run as root")
+        sys.exit(1)
+
+    # setup mock env
+    mock_gpio_dir = tempfile.mkdtemp("mock-gpio")
+    atexit.register(shutil.rmtree, mock_gpio_dir)
+    os.chdir(mock_gpio_dir)
+    subprocess.check_call(
+        ["mount", "--bind", mock_gpio_dir, "/sys/class/gpio"])
+    atexit.register(lambda: subprocess.call(["umount", "/sys/class/gpio"]))
+
+    # fake gpio export/unexport files
+    os.mkfifo("export")
+    os.mkfifo("unexport")
+
+    # react to the export/unexport calls
+    sel = selectors.DefaultSelector()
+    efd = os.open("export", os.O_RDWR | os.O_NONBLOCK)
+    ufd = os.open("unexport", os.O_RDWR | os.O_NONBLOCK)
+    sel.register(efd, selectors.EVENT_READ, export_ready)
+    sel.register(ufd, selectors.EVENT_READ, unexport_ready)
+    while True:
+        if not dispatch(sel):
+            break
+
+    # cleanup when we get a quit call
+    os.close(efd)
+    os.close(ufd)
+
+
+if __name__ == "__main__":
+    main()

tests/regression/lp-1802581/task.yaml

@@ -0,0 +1,56 @@
+summary: Regression test for LP 1802581
+
+description: |
+    When using the snapd GPIO interface the slot side needs to export the GPIO
+    pint so that a file appears under /sys/class/gpio/gpioNNN. That file is a
+    symbolic link to a special, platform specific device path. The symbolic
+    link needs to be evaluated so that apparmor rules for the plug side can be
+    constructed.
+
+    A customer has reported that when a snap is disabled and then re-enabled
+    the GPIO pin is not exported and apparmor backend cannot setup security
+    of the snap being enabled.
+
+    A similar issue has occurred once in the past where the order in which
+    security backends operated was not deterministic. We fixed the issue by
+    applying a fixed order so that the systemd backend, responsible for
+    exporting the pin, would always run before the apparmor backend, which
+    could now rely on the pin being exposed to userspace.
+
+systems: [ubuntu-core-16-64]
+
+prepare: |
+    echo "Run mock gpio daemon"
+    systemd-run --unit mock-gpio -- "$(pwd)/mock-gpio.py"
+    # shellcheck source=tests/lib/systemd.sh
+    . "$TESTSLIB/systemd.sh"
+    wait_for_service mock-gpio
+
+restore: |
+    systemctl stop mock-gpio || true
+    # for good measure, mock-gpio.py does this umount already on exit
+    umount /sys/class/gpio || true
+
+debug: |
+    journalctl -u mock-gpio.service
+
+execute: |
+    echo "Install a snap that uses the gpio consumer"
+    #shellcheck source=tests/lib/snaps.sh
+    . "$TESTSLIB"/snaps.sh
+    install_local gpio-consumer
+
+    echo "And connect the gpio pin"
+    snap connect gpio-consumer:gpio :gpio-pin
+    snap interfaces | MATCH ":gpio-pin.*gpio-consumer:gpio"
+
+    # LP-1802581
+    echo "Now disable and enable the snap to ensure lp: #1802581 is fixed"
+    snap disable gpio-consumer
+    snap enable gpio-consumer
+
+    echo "Check that the connection is still here after enable"
+    snap interfaces | MATCH ":gpio-pin.*gpio-consumer:gpio"
+
+    echo "Ensure that our mock service is full functional"
+    systemctl status mock-gpio.service | MATCH "Active: active"