From 7be93caec08866b55af3face1806e309f941a3e5 Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Tue, 11 Aug 2020 16:10:54 +0800 Subject: [PATCH 01/14] o/hookstate/ctlcmd: add a --pid option to is-connected --- overlord/hookstate/ctlcmd/export_test.go | 8 +++++ overlord/hookstate/ctlcmd/is_connected.go | 23 +++++++++++-- .../hookstate/ctlcmd/is_connected_test.go | 33 +++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/overlord/hookstate/ctlcmd/export_test.go b/overlord/hookstate/ctlcmd/export_test.go index 1433754109e..75ced5b2f10 100644 --- a/overlord/hookstate/ctlcmd/export_test.go +++ b/overlord/hookstate/ctlcmd/export_test.go @@ -88,3 +88,11 @@ func (c *MockCommand) Execute(args []string) error { return nil } + +func MockCgroupSnapNameFromPid(f func(int) (string, error)) (restore func()) { + old := cgroupSnapNameFromPid + cgroupSnapNameFromPid = f + return func() { + cgroupSnapNameFromPid = old + } +} diff --git a/overlord/hookstate/ctlcmd/is_connected.go b/overlord/hookstate/ctlcmd/is_connected.go index 555b4ab5c5f..7cd30d04ea6 100644 --- a/overlord/hookstate/ctlcmd/is_connected.go +++ b/overlord/hookstate/ctlcmd/is_connected.go @@ -26,14 +26,18 @@ import ( "github.com/snapcore/snapd/interfaces" "github.com/snapcore/snapd/overlord/ifacestate" "github.com/snapcore/snapd/overlord/snapstate" + "github.com/snapcore/snapd/sandbox/cgroup" ) +var cgroupSnapNameFromPid = cgroup.SnapNameFromPid + type isConnectedCommand struct { baseCommand Positional struct { PlugOrSlotSpec string `positional-args:"true" positional-arg-name:""` } `positional-args:"true" required:"true"` + Pid int `long:"pid" description:"Process ID for a connected process"` } var shortIsConnectedHelp = i18n.G(`Return success if the given plug or slot is connected, and failure otherwise`) @@ -87,6 +91,15 @@ func (c *isConnectedCommand) Execute(args []string) error { return fmt.Errorf("internal error: cannot get connections: %s", err) } + var otherSnapName string + if c.Pid != 0 { + otherSnapName, err = cgroupSnapNameFromPid(c.Pid) + // FIXME: should we treat non-snap processes as connected? + if err != nil { + return fmt.Errorf("internal error: cannot get snap name for pid %d: %s", c.Pid, err) + } + } + // snapName is the name of the snap executing snapctl command, it's // obtained from the context (ephemeral if run by apps, or full if run by // hooks). plug and slot names are unique within a snap, so there is no @@ -102,8 +115,14 @@ func (c *isConnectedCommand) Execute(args []string) error { matchingPlug := connRef.PlugRef.Snap == snapName && connRef.PlugRef.Name == plugOrSlot matchingSlot := connRef.SlotRef.Snap == snapName && connRef.SlotRef.Name == plugOrSlot - if matchingPlug || matchingSlot { - return nil + if otherSnapName != "" { + if matchingPlug && connRef.SlotRef.Snap == otherSnapName || matchingSlot && connRef.PlugRef.Snap == otherSnapName { + return nil + } + } else { + if matchingPlug || matchingSlot { + return nil + } } } diff --git a/overlord/hookstate/ctlcmd/is_connected_test.go b/overlord/hookstate/ctlcmd/is_connected_test.go index 55b9c66c7ea..fc6f2b4b1f1 100644 --- a/overlord/hookstate/ctlcmd/is_connected_test.go +++ b/overlord/hookstate/ctlcmd/is_connected_test.go @@ -20,6 +20,8 @@ package ctlcmd_test import ( + "fmt" + "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/overlord/hookstate" "github.com/snapcore/snapd/overlord/hookstate/ctlcmd" @@ -74,6 +76,28 @@ var isConnectedTests = []struct { }, { args: []string{"is-connected", "foo"}, err: `snap "snap1" has no plug or slot named "foo"`, +}, { + // snap1:plug1 is connected to snap2 + args: []string{"is-connected", "--pid", "1002", "plug1"}, +}, { + // snap1:plug1 is not connected to snap3 + args: []string{"is-connected", "--pid", "1003", "plug1"}, + exitCode: 1, +}, { + // snap1:plug1 is not connected to a non-snap pid + args: []string{"is-connected", "--pid", "42", "plug1"}, + err: "internal error: cannot get snap name for pid 42: Not a snap", +}, { + // snap1:slot1 is connected to snap3 + args: []string{"is-connected", "--pid", "1003", "slot1"}, +}, { + // snap1:slot1 is not connected to snap2 + args: []string{"is-connected", "--pid", "1002", "slot1"}, + exitCode: 1, +}, { + // snap1:slot1 is not connected to a non-snap pid + args: []string{"is-connected", "--pid", "42", "slot1"}, + err: "internal error: cannot get snap name for pid 42: Not a snap", }} func mockInstalledSnap(c *C, st *state.State, snapYaml string) { @@ -103,6 +127,15 @@ plugs: slots: slot1: interface: x11`) + restore := ctlcmd.MockCgroupSnapNameFromPid(func(pid int) (string, error) { + switch { + case 1000 < pid && pid < 1100: + return fmt.Sprintf("snap%d", pid-1000), nil + default: + return "", fmt.Errorf("Not a snap") + } + }) + defer restore() s.st.Set("conns", map[string]interface{}{ "snap1:plug1 snap2:slot2": map[string]interface{}{}, From 27c3ee64c22ea12f81aee0ae286820c747976e6a Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Tue, 11 Aug 2020 17:06:55 +0800 Subject: [PATCH 02/14] tests: add a test for snapctl is-connected --pid --- tests/main/snapctl-is-connected-pid/task.yaml | 37 +++++++++++++++++++ .../test-snap1/bin/service.sh | 4 ++ .../test-snap1/meta/snap.yaml | 21 +++++++++++ .../test-snap2/bin/run-snapctl.sh | 2 + .../test-snap2/meta/snap.yaml | 20 ++++++++++ 5 files changed, 84 insertions(+) create mode 100644 tests/main/snapctl-is-connected-pid/task.yaml create mode 100755 tests/main/snapctl-is-connected-pid/test-snap1/bin/service.sh create mode 100644 tests/main/snapctl-is-connected-pid/test-snap1/meta/snap.yaml create mode 100755 tests/main/snapctl-is-connected-pid/test-snap2/bin/run-snapctl.sh create mode 100644 tests/main/snapctl-is-connected-pid/test-snap2/meta/snap.yaml diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml new file mode 100644 index 00000000000..dbd931a5a22 --- /dev/null +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -0,0 +1,37 @@ +summary: Ensure that "snapctl is-connected --pid" works. + +prepare: | + #shellcheck source=tests/lib/snaps.sh + . "$TESTSLIB"/snaps.sh + install_local test-snap1 + install_local test-snap2 + +execute: | + echo "The test-snap1 service is running" + systemctl is-active snap.test-snap1.svc.service + + svc_pid=$(systemctl show --property=MainPID snap.test-snap1.svc.service | cut -d = -f 2) + + echo "Plugs and slots are initially disconnected" + not test-snap2.snapctl is-connected bar-plug + not test-snap2.snapctl is-connected foo-slot + + echo "Disconnected interfaces are not connected to a snap process" + not test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug + not test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot + + echo "Disconnected interfaces are not connected to non-snap process" + not test-snap2.snapctl is-connected --pid 1 bar-plug + not test-snap2.snapctl is-connected --pid 1 foo-slot + + echo "Connect interfaces" + snap connect test-snap1:foo-plug test-snap2:foo-slot + snap connect test-snap2:bar-plug test-snap1:bar-slot + + echo "Connected interfaces report as connected to snap process" + test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug + test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot + + echo "Interfaces still not connected to non-snap process" + not test-snap2.snapctl is-connected --pid 1 bar-plug + not test-snap2.snapctl is-connected --pid 1 foo-slot diff --git a/tests/main/snapctl-is-connected-pid/test-snap1/bin/service.sh b/tests/main/snapctl-is-connected-pid/test-snap1/bin/service.sh new file mode 100755 index 00000000000..9bf35c47814 --- /dev/null +++ b/tests/main/snapctl-is-connected-pid/test-snap1/bin/service.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +echo "service running" +exec sleep infinity diff --git a/tests/main/snapctl-is-connected-pid/test-snap1/meta/snap.yaml b/tests/main/snapctl-is-connected-pid/test-snap1/meta/snap.yaml new file mode 100644 index 00000000000..afb66781d12 --- /dev/null +++ b/tests/main/snapctl-is-connected-pid/test-snap1/meta/snap.yaml @@ -0,0 +1,21 @@ +name: test-snap1 +version: 1 +summary: First test snap + +plugs: + foo-plug: + interface: content + content: foo + target: $SNAP_COMMON/foo + +slots: + bar-slot: + interface: content + content: bar + read: + - $SNAP + +apps: + svc: + command: bin/service.sh + daemon: simple diff --git a/tests/main/snapctl-is-connected-pid/test-snap2/bin/run-snapctl.sh b/tests/main/snapctl-is-connected-pid/test-snap2/bin/run-snapctl.sh new file mode 100755 index 00000000000..342b0ca89e8 --- /dev/null +++ b/tests/main/snapctl-is-connected-pid/test-snap2/bin/run-snapctl.sh @@ -0,0 +1,2 @@ +#!/bin/sh +exec snapctl "$@" diff --git a/tests/main/snapctl-is-connected-pid/test-snap2/meta/snap.yaml b/tests/main/snapctl-is-connected-pid/test-snap2/meta/snap.yaml new file mode 100644 index 00000000000..84d4d1344e7 --- /dev/null +++ b/tests/main/snapctl-is-connected-pid/test-snap2/meta/snap.yaml @@ -0,0 +1,20 @@ +name: test-snap2 +version: 1 +summary: Second test snap + +plugs: + bar-plug: + interface: content + content: bar + target: $SNAP_COMMON/bar + +slots: + foo-slot: + interface: content + content: foo + read: + - $SNAP + +apps: + snapctl: + command: bin/run-snapctl.sh From 260dff996f94d8377a578108a7cd913e34bb076c Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Thu, 27 Aug 2020 13:09:18 +0800 Subject: [PATCH 03/14] o/hookstate/ctlcmd: add distinct exit codes for cases when pid is not a snap, or pid is a classic snap. --- overlord/hookstate/ctlcmd/is_connected.go | 29 +++++++++++++---- .../hookstate/ctlcmd/is_connected_test.go | 32 ++++++++++++++++--- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/overlord/hookstate/ctlcmd/is_connected.go b/overlord/hookstate/ctlcmd/is_connected.go index 7cd30d04ea6..3bb8f8cdf0a 100644 --- a/overlord/hookstate/ctlcmd/is_connected.go +++ b/overlord/hookstate/ctlcmd/is_connected.go @@ -27,6 +27,7 @@ import ( "github.com/snapcore/snapd/overlord/ifacestate" "github.com/snapcore/snapd/overlord/snapstate" "github.com/snapcore/snapd/sandbox/cgroup" + "github.com/snapcore/snapd/snap" ) var cgroupSnapNameFromPid = cgroup.SnapNameFromPid @@ -37,7 +38,7 @@ type isConnectedCommand struct { Positional struct { PlugOrSlotSpec string `positional-args:"true" positional-arg-name:""` } `positional-args:"true" required:"true"` - Pid int `long:"pid" description:"Process ID for a connected process"` + Pid int `long:"pid" description:"Process ID for a plausibly connected process"` } var shortIsConnectedHelp = i18n.G(`Return success if the given plug or slot is connected, and failure otherwise`) @@ -51,6 +52,12 @@ $ echo $? Snaps can only query their own plugs and slots - snap name is implicit and implied by the snapctl execution context. + +The --pid option can be used to determine whether a plug or slot is +connected to the snap identified by the given process ID. In this +mode, additional failure exit codes may be returned: 10 if the other +process is not snap confined, or 11 if the other snap is not connected +but uses classic confinement. `) func init() { @@ -91,12 +98,16 @@ func (c *isConnectedCommand) Execute(args []string) error { return fmt.Errorf("internal error: cannot get connections: %s", err) } - var otherSnapName string + var otherSnap *snap.Info if c.Pid != 0 { - otherSnapName, err = cgroupSnapNameFromPid(c.Pid) - // FIXME: should we treat non-snap processes as connected? + name, err := cgroupSnapNameFromPid(c.Pid) + if err != nil { + // Indicate that this pid is not a snap + return &UnsuccessfulError{ExitCode: 10} + } + otherSnap, err = snapstate.CurrentInfo(st, name) if err != nil { - return fmt.Errorf("internal error: cannot get snap name for pid %d: %s", c.Pid, err) + return fmt.Errorf("internal error: cannot get snap info for pid %d: %s", c.Pid, err) } } @@ -115,8 +126,8 @@ func (c *isConnectedCommand) Execute(args []string) error { matchingPlug := connRef.PlugRef.Snap == snapName && connRef.PlugRef.Name == plugOrSlot matchingSlot := connRef.SlotRef.Snap == snapName && connRef.SlotRef.Name == plugOrSlot - if otherSnapName != "" { - if matchingPlug && connRef.SlotRef.Snap == otherSnapName || matchingSlot && connRef.PlugRef.Snap == otherSnapName { + if otherSnap != nil { + if matchingPlug && connRef.SlotRef.Snap == otherSnap.InstanceName() || matchingSlot && connRef.PlugRef.Snap == otherSnap.InstanceName() { return nil } } else { @@ -126,5 +137,9 @@ func (c *isConnectedCommand) Execute(args []string) error { } } + if otherSnap != nil && otherSnap.Confinement == snap.ClassicConfinement { + return &UnsuccessfulError{ExitCode: 11} + } + return &UnsuccessfulError{ExitCode: 1} } diff --git a/overlord/hookstate/ctlcmd/is_connected_test.go b/overlord/hookstate/ctlcmd/is_connected_test.go index fc6f2b4b1f1..3cef152ca80 100644 --- a/overlord/hookstate/ctlcmd/is_connected_test.go +++ b/overlord/hookstate/ctlcmd/is_connected_test.go @@ -85,8 +85,12 @@ var isConnectedTests = []struct { exitCode: 1, }, { // snap1:plug1 is not connected to a non-snap pid - args: []string{"is-connected", "--pid", "42", "plug1"}, - err: "internal error: cannot get snap name for pid 42: Not a snap", + args: []string{"is-connected", "--pid", "42", "plug1"}, + exitCode: 10, +}, { + // snap1:plug1 is not connected to snap4, but snap4 is classic + args: []string{"is-connected", "--pid", "1004", "plug1"}, + exitCode: 11, }, { // snap1:slot1 is connected to snap3 args: []string{"is-connected", "--pid", "1003", "slot1"}, @@ -96,8 +100,12 @@ var isConnectedTests = []struct { exitCode: 1, }, { // snap1:slot1 is not connected to a non-snap pid - args: []string{"is-connected", "--pid", "42", "slot1"}, - err: "internal error: cannot get snap name for pid 42: Not a snap", + args: []string{"is-connected", "--pid", "42", "slot1"}, + exitCode: 10, +}, { + // snap1:slot1 is not connected to snap4, but snap4 is classic + args: []string{"is-connected", "--pid", "1004", "slot1"}, + exitCode: 11, }} func mockInstalledSnap(c *C, st *state.State, snapYaml string) { @@ -126,6 +134,22 @@ plugs: interface: x11 slots: slot1: + interface: x11`) + mockInstalledSnap(c, s.st, `name: snap2 +slots: + slot2: + interface: x11`) + mockInstalledSnap(c, s.st, `name: snap3 +plugs: + plug4: + interface: x11 +slots: + slot3: + interface: x11`) + mockInstalledSnap(c, s.st, `name: snap4 +confinement: classic +slots: + slot4: interface: x11`) restore := ctlcmd.MockCgroupSnapNameFromPid(func(pid int) (string, error) { switch { From f02503d186674c1f10bceb92015d3417fba35c32 Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Fri, 28 Aug 2020 09:46:27 +0800 Subject: [PATCH 04/14] tests: test special exit codes for "snapctl is-connected --pid" --- tests/main/snapctl-is-connected-pid/task.yaml | 45 ++++++++++++++++--- .../test-snap-classic/bin/service.sh | 4 ++ .../test-snap-classic/meta/snap.yaml | 15 +++++++ 3 files changed, 57 insertions(+), 7 deletions(-) create mode 100755 tests/main/snapctl-is-connected-pid/test-snap-classic/bin/service.sh create mode 100644 tests/main/snapctl-is-connected-pid/test-snap-classic/meta/snap.yaml diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml index dbd931a5a22..945172a5125 100644 --- a/tests/main/snapctl-is-connected-pid/task.yaml +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -9,20 +9,31 @@ prepare: | execute: | echo "The test-snap1 service is running" systemctl is-active snap.test-snap1.svc.service - svc_pid=$(systemctl show --property=MainPID snap.test-snap1.svc.service | cut -d = -f 2) + fail() { + expected="$1" + shift + if "$@"; then + echo "Expected command to fail" + exit 1 + else + err="$?" + test "$err" -eq "$expected" + fi + } + echo "Plugs and slots are initially disconnected" not test-snap2.snapctl is-connected bar-plug not test-snap2.snapctl is-connected foo-slot echo "Disconnected interfaces are not connected to a snap process" - not test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug - not test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot + fail 1 test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug + fail 1 test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot echo "Disconnected interfaces are not connected to non-snap process" - not test-snap2.snapctl is-connected --pid 1 bar-plug - not test-snap2.snapctl is-connected --pid 1 foo-slot + fail 10 test-snap2.snapctl is-connected --pid 1 bar-plug + fail 10 test-snap2.snapctl is-connected --pid 1 foo-slot echo "Connect interfaces" snap connect test-snap1:foo-plug test-snap2:foo-slot @@ -33,5 +44,25 @@ execute: | test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot echo "Interfaces still not connected to non-snap process" - not test-snap2.snapctl is-connected --pid 1 bar-plug - not test-snap2.snapctl is-connected --pid 1 foo-slot + fail 10 test-snap2.snapctl is-connected --pid 1 bar-plug + fail 10 test-snap2.snapctl is-connected --pid 1 foo-slot + + # The remaining tests rely on classic confinement, so skip Ubuntu Core + if [[ "$SPREAD_SYSTEM" = ubuntu-core-* ]]; then + exit 0 + fi + + #shellcheck source=tests/lib/snaps.sh + . "$TESTSLIB"/snaps.sh + install_local_classic test-snap-classic + + echo "The test-snap-classic service is running" + systemctl is-active snap.test-snap-classic.svc.service + classic_pid=$(systemctl show --property=MainPID snap.test-snap-classic.svc.service | cut -d = -f 2) + + echo "Unconnected classic snaps report a special exit code" + fail 11 test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot + + echo "But still reports success when connected" + snap connect test-snap-classic:foo-plug test-snap2:foo-slot + test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot diff --git a/tests/main/snapctl-is-connected-pid/test-snap-classic/bin/service.sh b/tests/main/snapctl-is-connected-pid/test-snap-classic/bin/service.sh new file mode 100755 index 00000000000..9bf35c47814 --- /dev/null +++ b/tests/main/snapctl-is-connected-pid/test-snap-classic/bin/service.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +echo "service running" +exec sleep infinity diff --git a/tests/main/snapctl-is-connected-pid/test-snap-classic/meta/snap.yaml b/tests/main/snapctl-is-connected-pid/test-snap-classic/meta/snap.yaml new file mode 100644 index 00000000000..ae24d6ba448 --- /dev/null +++ b/tests/main/snapctl-is-connected-pid/test-snap-classic/meta/snap.yaml @@ -0,0 +1,15 @@ +name: test-snap-classic +confinement: classic +version: 1 +summary: Classic test snap + +plugs: + foo-plug: + interface: content + content: foo + target: $SNAP_COMMON/foo + +apps: + svc: + command: bin/service.sh + daemon: simple From 352e7deaf3fa676ee075357d9bcd036cf9981905 Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Fri, 28 Aug 2020 11:39:41 +0800 Subject: [PATCH 05/14] tests: fix up test for fedora/arch/centos --- tests/main/snapctl-is-connected-pid/task.yaml | 144 ++++++++++-------- 1 file changed, 81 insertions(+), 63 deletions(-) diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml index 945172a5125..ce4b7d11834 100644 --- a/tests/main/snapctl-is-connected-pid/task.yaml +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -1,68 +1,86 @@ summary: Ensure that "snapctl is-connected --pid" works. prepare: | - #shellcheck source=tests/lib/snaps.sh - . "$TESTSLIB"/snaps.sh - install_local test-snap1 - install_local test-snap2 + #shellcheck source=tests/lib/snaps.sh + . "$TESTSLIB"/snaps.sh + install_local test-snap1 + install_local test-snap2 + + #shellcheck source=tests/lib/dirs.sh + . "$TESTSLIB"/dirs.sh + case "$SPREAD_SYSTEM" in + fedora-*|arch-*|centos-*) + # although classic snaps do not work out of the box on fedora, + # we still want to verify if the basics do work if the user + # symlinks /snap to $SNAP_MOUNT_DIR themselves + ln -sf $SNAP_MOUNT_DIR /snap + ;; + esac + +restore: | + case "$SPREAD_SYSTEM" in + fedora-*|arch-*|centos-*) + rm -f /snap + ;; + esac execute: | - echo "The test-snap1 service is running" - systemctl is-active snap.test-snap1.svc.service - svc_pid=$(systemctl show --property=MainPID snap.test-snap1.svc.service | cut -d = -f 2) - - fail() { - expected="$1" - shift - if "$@"; then - echo "Expected command to fail" - exit 1 - else - err="$?" - test "$err" -eq "$expected" - fi - } - - echo "Plugs and slots are initially disconnected" - not test-snap2.snapctl is-connected bar-plug - not test-snap2.snapctl is-connected foo-slot - - echo "Disconnected interfaces are not connected to a snap process" - fail 1 test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug - fail 1 test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot - - echo "Disconnected interfaces are not connected to non-snap process" - fail 10 test-snap2.snapctl is-connected --pid 1 bar-plug - fail 10 test-snap2.snapctl is-connected --pid 1 foo-slot - - echo "Connect interfaces" - snap connect test-snap1:foo-plug test-snap2:foo-slot - snap connect test-snap2:bar-plug test-snap1:bar-slot - - echo "Connected interfaces report as connected to snap process" - test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug - test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot - - echo "Interfaces still not connected to non-snap process" - fail 10 test-snap2.snapctl is-connected --pid 1 bar-plug - fail 10 test-snap2.snapctl is-connected --pid 1 foo-slot - - # The remaining tests rely on classic confinement, so skip Ubuntu Core - if [[ "$SPREAD_SYSTEM" = ubuntu-core-* ]]; then - exit 0 - fi - - #shellcheck source=tests/lib/snaps.sh - . "$TESTSLIB"/snaps.sh - install_local_classic test-snap-classic - - echo "The test-snap-classic service is running" - systemctl is-active snap.test-snap-classic.svc.service - classic_pid=$(systemctl show --property=MainPID snap.test-snap-classic.svc.service | cut -d = -f 2) - - echo "Unconnected classic snaps report a special exit code" - fail 11 test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot - - echo "But still reports success when connected" - snap connect test-snap-classic:foo-plug test-snap2:foo-slot - test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot + echo "The test-snap1 service is running" + systemctl is-active snap.test-snap1.svc.service + svc_pid=$(systemctl show --property=MainPID snap.test-snap1.svc.service | cut -d = -f 2) + + fail() { + expected="$1" + shift + if "$@"; then + echo "Expected command to fail" + exit 1 + else + err="$?" + test "$err" -eq "$expected" + fi + } + + echo "Plugs and slots are initially disconnected" + not test-snap2.snapctl is-connected bar-plug + not test-snap2.snapctl is-connected foo-slot + + echo "Disconnected interfaces are not connected to a snap process" + fail 1 test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug + fail 1 test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot + + echo "Disconnected interfaces are not connected to non-snap process" + fail 10 test-snap2.snapctl is-connected --pid 1 bar-plug + fail 10 test-snap2.snapctl is-connected --pid 1 foo-slot + + echo "Connect interfaces" + snap connect test-snap1:foo-plug test-snap2:foo-slot + snap connect test-snap2:bar-plug test-snap1:bar-slot + + echo "Connected interfaces report as connected to snap process" + test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug + test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot + + echo "Interfaces still not connected to non-snap process" + fail 10 test-snap2.snapctl is-connected --pid 1 bar-plug + fail 10 test-snap2.snapctl is-connected --pid 1 foo-slot + + # The remaining tests rely on classic confinement, so skip Ubuntu Core + if [[ "$SPREAD_SYSTEM" = ubuntu-core-* ]]; then + exit 0 + fi + + #shellcheck source=tests/lib/snaps.sh + . "$TESTSLIB"/snaps.sh + install_local_classic test-snap-classic + + echo "The test-snap-classic service is running" + systemctl is-active snap.test-snap-classic.svc.service + classic_pid=$(systemctl show --property=MainPID snap.test-snap-classic.svc.service | cut -d = -f 2) + + echo "Unconnected classic snaps report a special exit code" + fail 11 test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot + + echo "But still reports success when connected" + snap connect test-snap-classic:foo-plug test-snap2:foo-slot + test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot From f10bd08ba29b079e17fbc56171a734a68d2e94a2 Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Fri, 28 Aug 2020 14:23:04 +0800 Subject: [PATCH 06/14] tests: spread test cleanups suggested by @zyga --- tests/main/snapctl-is-connected-pid/task.yaml | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml index ce4b7d11834..4e6d2695e24 100644 --- a/tests/main/snapctl-is-connected-pid/task.yaml +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -1,6 +1,7 @@ summary: Ensure that "snapctl is-connected --pid" works. prepare: | + tests.cleanup prepare #shellcheck source=tests/lib/snaps.sh . "$TESTSLIB"/snaps.sh install_local test-snap1 @@ -14,31 +15,24 @@ prepare: | # we still want to verify if the basics do work if the user # symlinks /snap to $SNAP_MOUNT_DIR themselves ln -sf $SNAP_MOUNT_DIR /snap + tests.cleanup defer rm -f /snap ;; esac restore: | - case "$SPREAD_SYSTEM" in - fedora-*|arch-*|centos-*) - rm -f /snap - ;; - esac + tests.cleanup restore execute: | echo "The test-snap1 service is running" systemctl is-active snap.test-snap1.svc.service svc_pid=$(systemctl show --property=MainPID snap.test-snap1.svc.service | cut -d = -f 2) - fail() { + expect_status() { expected="$1" shift - if "$@"; then - echo "Expected command to fail" - exit 1 - else - err="$?" - test "$err" -eq "$expected" - fi + # Temporarily turn off "set -e" so we can check the exit status + set +e; "$@"; local ret=$?; set -e + test "$ret" -eq "$expected" } echo "Plugs and slots are initially disconnected" @@ -46,12 +40,12 @@ execute: | not test-snap2.snapctl is-connected foo-slot echo "Disconnected interfaces are not connected to a snap process" - fail 1 test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug - fail 1 test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot + expect_status 1 test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug + expect_status 1 test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot echo "Disconnected interfaces are not connected to non-snap process" - fail 10 test-snap2.snapctl is-connected --pid 1 bar-plug - fail 10 test-snap2.snapctl is-connected --pid 1 foo-slot + expect_status 10 test-snap2.snapctl is-connected --pid 1 bar-plug + expect_status 10 test-snap2.snapctl is-connected --pid 1 foo-slot echo "Connect interfaces" snap connect test-snap1:foo-plug test-snap2:foo-slot @@ -62,8 +56,8 @@ execute: | test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot echo "Interfaces still not connected to non-snap process" - fail 10 test-snap2.snapctl is-connected --pid 1 bar-plug - fail 10 test-snap2.snapctl is-connected --pid 1 foo-slot + expect_status 10 test-snap2.snapctl is-connected --pid 1 bar-plug + expect_status 10 test-snap2.snapctl is-connected --pid 1 foo-slot # The remaining tests rely on classic confinement, so skip Ubuntu Core if [[ "$SPREAD_SYSTEM" = ubuntu-core-* ]]; then @@ -79,7 +73,7 @@ execute: | classic_pid=$(systemctl show --property=MainPID snap.test-snap-classic.svc.service | cut -d = -f 2) echo "Unconnected classic snaps report a special exit code" - fail 11 test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot + expect_status 11 test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot echo "But still reports success when connected" snap connect test-snap-classic:foo-plug test-snap2:foo-slot From 0712f41b6b60726b70eb68ce6a65ca89ad5e112f Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Tue, 15 Sep 2020 18:52:03 +0800 Subject: [PATCH 07/14] o/hookstate/ctlcmd: add a simple whitelist of allowed interfaces for --pid feature --- overlord/hookstate/ctlcmd/is_connected.go | 14 +++++ .../hookstate/ctlcmd/is_connected_test.go | 56 +++++++++++-------- tests/main/snapctl-is-connected-pid/task.yaml | 8 +-- .../test-snap-classic/meta/snap.yaml | 4 +- .../test-snap1/meta/snap.yaml | 11 +--- .../test-snap2/meta/snap.yaml | 11 +--- 6 files changed, 51 insertions(+), 53 deletions(-) diff --git a/overlord/hookstate/ctlcmd/is_connected.go b/overlord/hookstate/ctlcmd/is_connected.go index 3bb8f8cdf0a..e57da838a8d 100644 --- a/overlord/hookstate/ctlcmd/is_connected.go +++ b/overlord/hookstate/ctlcmd/is_connected.go @@ -66,6 +66,17 @@ func init() { }) } +func isConnectedPidCheckAllowed(info *snap.Info, plugOrSlot string) bool { + slot := info.Slots[plugOrSlot] + if slot != nil { + switch slot.Interface { + case "pulseaudio", "audio-record", "cups-control": + return true + } + } + return false +} + func (c *isConnectedCommand) Execute(args []string) error { plugOrSlot := c.Positional.PlugOrSlotSpec @@ -100,6 +111,9 @@ func (c *isConnectedCommand) Execute(args []string) error { var otherSnap *snap.Info if c.Pid != 0 { + if !isConnectedPidCheckAllowed(info, plugOrSlot) { + return fmt.Errorf("cannot use --pid check with %s:%s", snapName, plugOrSlot) + } name, err := cgroupSnapNameFromPid(c.Pid) if err != nil { // Indicate that this pid is not a snap diff --git a/overlord/hookstate/ctlcmd/is_connected_test.go b/overlord/hookstate/ctlcmd/is_connected_test.go index 3cef152ca80..244d2dd207b 100644 --- a/overlord/hookstate/ctlcmd/is_connected_test.go +++ b/overlord/hookstate/ctlcmd/is_connected_test.go @@ -77,34 +77,32 @@ var isConnectedTests = []struct { args: []string{"is-connected", "foo"}, err: `snap "snap1" has no plug or slot named "foo"`, }, { - // snap1:plug1 is connected to snap2 + // snap1:plug1 does not use a whitelisted interface args: []string{"is-connected", "--pid", "1002", "plug1"}, + err: `cannot use --pid check with snap1:plug1`, }, { - // snap1:plug1 is not connected to snap3 - args: []string{"is-connected", "--pid", "1003", "plug1"}, - exitCode: 1, -}, { - // snap1:plug1 is not connected to a non-snap pid - args: []string{"is-connected", "--pid", "42", "plug1"}, - exitCode: 10, + // snap1:slot1 does not use a whitelisted interface + args: []string{"is-connected", "--pid", "1002", "slot1"}, + err: `cannot use --pid check with snap1:slot1`, }, { - // snap1:plug1 is not connected to snap4, but snap4 is classic - args: []string{"is-connected", "--pid", "1004", "plug1"}, - exitCode: 11, -}, { - // snap1:slot1 is connected to snap3 - args: []string{"is-connected", "--pid", "1003", "slot1"}, -}, { - // snap1:slot1 is not connected to snap2 - args: []string{"is-connected", "--pid", "1002", "slot1"}, + // snap1:cc slot is not connected to snap2 + args: []string{"is-connected", "--pid", "1002", "cc"}, exitCode: 1, }, { - // snap1:slot1 is not connected to a non-snap pid - args: []string{"is-connected", "--pid", "42", "slot1"}, + // snap1:cc slot is connected to snap3 + args: []string{"is-connected", "--pid", "1003", "cc"}, + exitCode: 0, +}, { + // snap1:cc slot is not connected to a non-snap pid + args: []string{"is-connected", "--pid", "42", "cc"}, exitCode: 10, }, { - // snap1:slot1 is not connected to snap4, but snap4 is classic - args: []string{"is-connected", "--pid", "1004", "slot1"}, + // snap1:cc slot is connected to a classic snap5 + args: []string{"is-connected", "--pid", "1005", "cc"}, + exitCode: 0, +}, { + // snap1:audio-record slot is not connected to classic snap5 + args: []string{"is-connected", "--pid", "1005", "audio-record"}, exitCode: 11, }} @@ -134,7 +132,11 @@ plugs: interface: x11 slots: slot1: - interface: x11`) + interface: x11 + cc: + interface: cups-control + audio-record: + interface: audio-record`) mockInstalledSnap(c, s.st, `name: snap2 slots: slot2: @@ -143,14 +145,20 @@ slots: plugs: plug4: interface: x11 + cc: + interface: cups-control slots: slot3: interface: x11`) mockInstalledSnap(c, s.st, `name: snap4 -confinement: classic slots: slot4: interface: x11`) + mockInstalledSnap(c, s.st, `name: snap5 +confinement: classic +plugs: + cc: + interface: cups-control`) restore := ctlcmd.MockCgroupSnapNameFromPid(func(pid int) (string, error) { switch { case 1000 < pid && pid < 1100: @@ -166,6 +174,8 @@ slots: "snap1:plug2 snap3:slot3": map[string]interface{}{"undesired": true}, "snap1:plug3 snap4:slot4": map[string]interface{}{"hotplug-gone": true}, "snap3:plug4 snap1:slot1": map[string]interface{}{}, + "snap3:cc snap1:cc": map[string]interface{}{}, + "snap5:cc snap1:cc": map[string]interface{}{}, }) s.st.Unlock() diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml index 4e6d2695e24..b08bde6a547 100644 --- a/tests/main/snapctl-is-connected-pid/task.yaml +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -36,27 +36,21 @@ execute: | } echo "Plugs and slots are initially disconnected" - not test-snap2.snapctl is-connected bar-plug not test-snap2.snapctl is-connected foo-slot echo "Disconnected interfaces are not connected to a snap process" - expect_status 1 test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug expect_status 1 test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot echo "Disconnected interfaces are not connected to non-snap process" - expect_status 10 test-snap2.snapctl is-connected --pid 1 bar-plug expect_status 10 test-snap2.snapctl is-connected --pid 1 foo-slot - echo "Connect interfaces" + echo "Connect interface" snap connect test-snap1:foo-plug test-snap2:foo-slot - snap connect test-snap2:bar-plug test-snap1:bar-slot echo "Connected interfaces report as connected to snap process" - test-snap2.snapctl is-connected --pid "$svc_pid" bar-plug test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot echo "Interfaces still not connected to non-snap process" - expect_status 10 test-snap2.snapctl is-connected --pid 1 bar-plug expect_status 10 test-snap2.snapctl is-connected --pid 1 foo-slot # The remaining tests rely on classic confinement, so skip Ubuntu Core diff --git a/tests/main/snapctl-is-connected-pid/test-snap-classic/meta/snap.yaml b/tests/main/snapctl-is-connected-pid/test-snap-classic/meta/snap.yaml index ae24d6ba448..7957781b469 100644 --- a/tests/main/snapctl-is-connected-pid/test-snap-classic/meta/snap.yaml +++ b/tests/main/snapctl-is-connected-pid/test-snap-classic/meta/snap.yaml @@ -5,9 +5,7 @@ summary: Classic test snap plugs: foo-plug: - interface: content - content: foo - target: $SNAP_COMMON/foo + interface: cups-control apps: svc: diff --git a/tests/main/snapctl-is-connected-pid/test-snap1/meta/snap.yaml b/tests/main/snapctl-is-connected-pid/test-snap1/meta/snap.yaml index afb66781d12..71053fdf04f 100644 --- a/tests/main/snapctl-is-connected-pid/test-snap1/meta/snap.yaml +++ b/tests/main/snapctl-is-connected-pid/test-snap1/meta/snap.yaml @@ -4,16 +4,7 @@ summary: First test snap plugs: foo-plug: - interface: content - content: foo - target: $SNAP_COMMON/foo - -slots: - bar-slot: - interface: content - content: bar - read: - - $SNAP + interface: cups-control apps: svc: diff --git a/tests/main/snapctl-is-connected-pid/test-snap2/meta/snap.yaml b/tests/main/snapctl-is-connected-pid/test-snap2/meta/snap.yaml index 84d4d1344e7..6d381571625 100644 --- a/tests/main/snapctl-is-connected-pid/test-snap2/meta/snap.yaml +++ b/tests/main/snapctl-is-connected-pid/test-snap2/meta/snap.yaml @@ -2,18 +2,9 @@ name: test-snap2 version: 1 summary: Second test snap -plugs: - bar-plug: - interface: content - content: bar - target: $SNAP_COMMON/bar - slots: foo-slot: - interface: content - content: foo - read: - - $SNAP + interface: cups-control apps: snapctl: From 6114ea46cbc6ee4bc708d4a1fc66ce8a5d66a861 Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Tue, 6 Oct 2020 17:14:20 +0800 Subject: [PATCH 08/14] o/hookstate/ctlcmd: extend is-connected to handle a --apparmor-label argument --- overlord/hookstate/ctlcmd/is_connected.go | 28 ++++++++++++++----- .../hookstate/ctlcmd/is_connected_test.go | 28 +++++++++++++++++++ sandbox/apparmor/export_test.go | 2 -- sandbox/apparmor/process.go | 4 +-- 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/overlord/hookstate/ctlcmd/is_connected.go b/overlord/hookstate/ctlcmd/is_connected.go index e57da838a8d..6ef25d62526 100644 --- a/overlord/hookstate/ctlcmd/is_connected.go +++ b/overlord/hookstate/ctlcmd/is_connected.go @@ -26,6 +26,7 @@ import ( "github.com/snapcore/snapd/interfaces" "github.com/snapcore/snapd/overlord/ifacestate" "github.com/snapcore/snapd/overlord/snapstate" + "github.com/snapcore/snapd/sandbox/apparmor" "github.com/snapcore/snapd/sandbox/cgroup" "github.com/snapcore/snapd/snap" ) @@ -38,7 +39,8 @@ type isConnectedCommand struct { Positional struct { PlugOrSlotSpec string `positional-args:"true" positional-arg-name:""` } `positional-args:"true" required:"true"` - Pid int `long:"pid" description:"Process ID for a plausibly connected process"` + Pid int `long:"pid" description:"Process ID for a plausibly connected process"` + AppArmorLabel string `long:"apparmor-label" description:"AppArmor label for a plausibly connected process"` } var shortIsConnectedHelp = i18n.G(`Return success if the given plug or slot is connected, and failure otherwise`) @@ -53,11 +55,11 @@ $ echo $? Snaps can only query their own plugs and slots - snap name is implicit and implied by the snapctl execution context. -The --pid option can be used to determine whether a plug or slot is -connected to the snap identified by the given process ID. In this -mode, additional failure exit codes may be returned: 10 if the other -process is not snap confined, or 11 if the other snap is not connected -but uses classic confinement. +The --pid and --aparmor-label options can be used to determine whether +a plug or slot is connected to the snap identified by the given +process ID or AppArmor label. In this mode, additional failure exit +codes may be returned: 10 if the other process is not snap confined, +or 11 if the other snap is not connected but uses classic confinement. `) func init() { @@ -110,7 +112,19 @@ func (c *isConnectedCommand) Execute(args []string) error { } var otherSnap *snap.Info - if c.Pid != 0 { + if c.AppArmorLabel != "" { + if !isConnectedPidCheckAllowed(info, plugOrSlot) { + return fmt.Errorf("cannot use --apparmor-label check with %s:%s", snapName, plugOrSlot) + } + name, _, _, err := apparmor.DecodeLabel(c.AppArmorLabel) + if err != nil { + return &UnsuccessfulError{ExitCode: 10} + } + otherSnap, err = snapstate.CurrentInfo(st, name) + if err != nil { + return fmt.Errorf("internal error: cannot get snap info for AppArmor label %q: %s", c.AppArmorLabel, err) + } + } else if c.Pid != 0 { if !isConnectedPidCheckAllowed(info, plugOrSlot) { return fmt.Errorf("cannot use --pid check with %s:%s", snapName, plugOrSlot) } diff --git a/overlord/hookstate/ctlcmd/is_connected_test.go b/overlord/hookstate/ctlcmd/is_connected_test.go index 244d2dd207b..43cc85bdae8 100644 --- a/overlord/hookstate/ctlcmd/is_connected_test.go +++ b/overlord/hookstate/ctlcmd/is_connected_test.go @@ -104,6 +104,34 @@ var isConnectedTests = []struct { // snap1:audio-record slot is not connected to classic snap5 args: []string{"is-connected", "--pid", "1005", "audio-record"}, exitCode: 11, +}, { + // snap1:plug1 does not use a whitelisted interface + args: []string{"is-connected", "--apparmor-label", "snap.snap2.app", "plug1"}, + err: `cannot use --apparmor-label check with snap1:plug1`, +}, { + // snap1:slot1 does not use a whitelisted interface + args: []string{"is-connected", "--apparmor-label", "snap.snap2.app", "slot1"}, + err: `cannot use --apparmor-label check with snap1:slot1`, +}, { + // snap1:cc slot is not connected to snap2 + args: []string{"is-connected", "--apparmor-label", "snap.snap2.app", "cc"}, + exitCode: 1, +}, { + // snap1:cc slot is connected to snap3 + args: []string{"is-connected", "--apparmor-label", "snap.snap3.app", "cc"}, + exitCode: 0, +}, { + // snap1:cc slot is not connected to a non-snap pid + args: []string{"is-connected", "--apparmor-label", "/usr/bin/evince", "cc"}, + exitCode: 10, +}, { + // snap1:cc slot is connected to a classic snap5 + args: []string{"is-connected", "--apparmor-label", "snap.snap5.app", "cc"}, + exitCode: 0, +}, { + // snap1:audio-record slot is not connected to classic snap5 + args: []string{"is-connected", "--apparmor-label", "snap.snap5.app", "audio-record"}, + exitCode: 11, }} func mockInstalledSnap(c *C, st *state.State, snapYaml string) { diff --git a/sandbox/apparmor/export_test.go b/sandbox/apparmor/export_test.go index 31fc1c127d7..ff3ba0b01f1 100644 --- a/sandbox/apparmor/export_test.go +++ b/sandbox/apparmor/export_test.go @@ -43,8 +43,6 @@ var ( RequiredParserFeatures = requiredParserFeatures PreferredKernelFeatures = preferredKernelFeatures PreferredParserFeatures = preferredParserFeatures - - DecodeLabel = decodeLabel ) func FreshAppArmorAssessment() { diff --git a/sandbox/apparmor/process.go b/sandbox/apparmor/process.go index 7d54a4038ed..d1ff8827b0e 100644 --- a/sandbox/apparmor/process.go +++ b/sandbox/apparmor/process.go @@ -53,7 +53,7 @@ func labelFromPid(pid int) (string, error) { return label, nil } -func decodeLabel(label string) (snap, app, hook string, err error) { +func DecodeLabel(label string) (snap, app, hook string, err error) { parts := strings.Split(label, ".") if parts[0] != "snap" { return "", "", "", fmt.Errorf("security label %q does not belong to a snap", label) @@ -72,5 +72,5 @@ func SnapAppFromPid(pid int) (snap, app, hook string, err error) { if err != nil { return "", "", "", err } - return decodeLabel(label) + return DecodeLabel(label) } From 12f6264b5c0cf298ccc066d98f991d3d111e6ab5 Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Tue, 6 Oct 2020 18:01:30 +0800 Subject: [PATCH 09/14] tests: test that --apparmor-label option works --- tests/main/snapctl-is-connected-pid/task.yaml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml index b08bde6a547..da8b6a746a9 100644 --- a/tests/main/snapctl-is-connected-pid/task.yaml +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -1,4 +1,4 @@ -summary: Ensure that "snapctl is-connected --pid" works. +summary: Ensure "snapctl is-connected" --pid and --apparmor-label options work. prepare: | tests.cleanup prepare @@ -53,6 +53,15 @@ execute: | echo "Interfaces still not connected to non-snap process" expect_status 10 test-snap2.snapctl is-connected --pid 1 foo-slot + if [[ "$(snap debug confinement)" = strict ]]; then + svc_label=$(cat "/proc/$svc_pid/attr/current" | sed 's/ (.*)$//') + + echo "We can detect connected interfaces by AppArmor label too" + test-snapd2.snapctl is-connected --apparmor-label "$svc_label" foo-slot + snap connect test-snap1:foo-plug test-snap2:foo-slot + expect_status 1 test-snap2.snapctl is-connected --apparmor-label "$svc_label" foo-slot + fi + # The remaining tests rely on classic confinement, so skip Ubuntu Core if [[ "$SPREAD_SYSTEM" = ubuntu-core-* ]]; then exit 0 From d90122ed06044d8881424d2d551b473ae0c3cc7c Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Tue, 6 Oct 2020 18:55:25 +0800 Subject: [PATCH 10/14] tests: fix shellcheck warning in test --- tests/main/snapctl-is-connected-pid/task.yaml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml index da8b6a746a9..a6e4ed9edae 100644 --- a/tests/main/snapctl-is-connected-pid/task.yaml +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -54,12 +54,15 @@ execute: | expect_status 10 test-snap2.snapctl is-connected --pid 1 foo-slot if [[ "$(snap debug confinement)" = strict ]]; then - svc_label=$(cat "/proc/$svc_pid/attr/current" | sed 's/ (.*)$//') + svc_label=$(sed 's/ (.*)$//' < "/proc/$svc_pid/attr/current") echo "We can detect connected interfaces by AppArmor label too" - test-snapd2.snapctl is-connected --apparmor-label "$svc_label" foo-slot - snap connect test-snap1:foo-plug test-snap2:foo-slot + test-snap2.snapctl is-connected --apparmor-label "$svc_label" foo-slot + snap disconnect test-snap1:foo-plug test-snap2:foo-slot expect_status 1 test-snap2.snapctl is-connected --apparmor-label "$svc_label" foo-slot + + echo "Non-snap AppArmor labels return a special exit code" + expect_status 10 test-snap2.snapctl is-connected --apparmor-label /usr/bin/evince foo-slot fi # The remaining tests rely on classic confinement, so skip Ubuntu Core From 0289f8914b1b35d8da99f7602c500c89841a385c Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Fri, 29 Jan 2021 10:57:54 +0800 Subject: [PATCH 11/14] overlord/hookstate/ctlcmd: add constants for special exit codes, and switch order as suggested by @pedronis --- overlord/hookstate/ctlcmd/export_test.go | 5 +++++ overlord/hookstate/ctlcmd/is_connected.go | 15 ++++++++++----- overlord/hookstate/ctlcmd/is_connected_test.go | 8 ++++---- tests/main/snapctl-is-connected-pid/task.yaml | 8 ++++---- 4 files changed, 23 insertions(+), 13 deletions(-) diff --git a/overlord/hookstate/ctlcmd/export_test.go b/overlord/hookstate/ctlcmd/export_test.go index 5c40a3c97ea..6eb0caafc0d 100644 --- a/overlord/hookstate/ctlcmd/export_test.go +++ b/overlord/hookstate/ctlcmd/export_test.go @@ -28,6 +28,11 @@ import ( "github.com/snapcore/snapd/snap" ) +const ( + NotASnapCode = notASnapCode + ClassicSnapCode = classicSnapCode +) + var AttributesTask = attributesTask func MockServicestateControlFunc(f func(*state.State, []*snap.AppInfo, *servicestate.Instruction, *servicestate.Flags, *hookstate.Context) ([]*state.TaskSet, error)) (restore func()) { diff --git a/overlord/hookstate/ctlcmd/is_connected.go b/overlord/hookstate/ctlcmd/is_connected.go index 6ef25d62526..77571030905 100644 --- a/overlord/hookstate/ctlcmd/is_connected.go +++ b/overlord/hookstate/ctlcmd/is_connected.go @@ -33,6 +33,11 @@ import ( var cgroupSnapNameFromPid = cgroup.SnapNameFromPid +const ( + classicSnapCode = 10 + notASnapCode = 11 +) + type isConnectedCommand struct { baseCommand @@ -58,8 +63,8 @@ implied by the snapctl execution context. The --pid and --aparmor-label options can be used to determine whether a plug or slot is connected to the snap identified by the given process ID or AppArmor label. In this mode, additional failure exit -codes may be returned: 10 if the other process is not snap confined, -or 11 if the other snap is not connected but uses classic confinement. +codes may be returned: 10 if the other snap is not connected but uses +classic confinement, or 11 if the other process is not snap confined. `) func init() { @@ -118,7 +123,7 @@ func (c *isConnectedCommand) Execute(args []string) error { } name, _, _, err := apparmor.DecodeLabel(c.AppArmorLabel) if err != nil { - return &UnsuccessfulError{ExitCode: 10} + return &UnsuccessfulError{ExitCode: notASnapCode} } otherSnap, err = snapstate.CurrentInfo(st, name) if err != nil { @@ -131,7 +136,7 @@ func (c *isConnectedCommand) Execute(args []string) error { name, err := cgroupSnapNameFromPid(c.Pid) if err != nil { // Indicate that this pid is not a snap - return &UnsuccessfulError{ExitCode: 10} + return &UnsuccessfulError{ExitCode: notASnapCode} } otherSnap, err = snapstate.CurrentInfo(st, name) if err != nil { @@ -166,7 +171,7 @@ func (c *isConnectedCommand) Execute(args []string) error { } if otherSnap != nil && otherSnap.Confinement == snap.ClassicConfinement { - return &UnsuccessfulError{ExitCode: 11} + return &UnsuccessfulError{ExitCode: classicSnapCode} } return &UnsuccessfulError{ExitCode: 1} diff --git a/overlord/hookstate/ctlcmd/is_connected_test.go b/overlord/hookstate/ctlcmd/is_connected_test.go index 43cc85bdae8..206083f1eed 100644 --- a/overlord/hookstate/ctlcmd/is_connected_test.go +++ b/overlord/hookstate/ctlcmd/is_connected_test.go @@ -95,7 +95,7 @@ var isConnectedTests = []struct { }, { // snap1:cc slot is not connected to a non-snap pid args: []string{"is-connected", "--pid", "42", "cc"}, - exitCode: 10, + exitCode: ctlcmd.NotASnapCode, }, { // snap1:cc slot is connected to a classic snap5 args: []string{"is-connected", "--pid", "1005", "cc"}, @@ -103,7 +103,7 @@ var isConnectedTests = []struct { }, { // snap1:audio-record slot is not connected to classic snap5 args: []string{"is-connected", "--pid", "1005", "audio-record"}, - exitCode: 11, + exitCode: ctlcmd.ClassicSnapCode, }, { // snap1:plug1 does not use a whitelisted interface args: []string{"is-connected", "--apparmor-label", "snap.snap2.app", "plug1"}, @@ -123,7 +123,7 @@ var isConnectedTests = []struct { }, { // snap1:cc slot is not connected to a non-snap pid args: []string{"is-connected", "--apparmor-label", "/usr/bin/evince", "cc"}, - exitCode: 10, + exitCode: ctlcmd.NotASnapCode, }, { // snap1:cc slot is connected to a classic snap5 args: []string{"is-connected", "--apparmor-label", "snap.snap5.app", "cc"}, @@ -131,7 +131,7 @@ var isConnectedTests = []struct { }, { // snap1:audio-record slot is not connected to classic snap5 args: []string{"is-connected", "--apparmor-label", "snap.snap5.app", "audio-record"}, - exitCode: 11, + exitCode: ctlcmd.ClassicSnapCode, }} func mockInstalledSnap(c *C, st *state.State, snapYaml string) { diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml index a6e4ed9edae..2a1ae33495b 100644 --- a/tests/main/snapctl-is-connected-pid/task.yaml +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -42,7 +42,7 @@ execute: | expect_status 1 test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot echo "Disconnected interfaces are not connected to non-snap process" - expect_status 10 test-snap2.snapctl is-connected --pid 1 foo-slot + expect_status 11 test-snap2.snapctl is-connected --pid 1 foo-slot echo "Connect interface" snap connect test-snap1:foo-plug test-snap2:foo-slot @@ -51,7 +51,7 @@ execute: | test-snap2.snapctl is-connected --pid "$svc_pid" foo-slot echo "Interfaces still not connected to non-snap process" - expect_status 10 test-snap2.snapctl is-connected --pid 1 foo-slot + expect_status 11 test-snap2.snapctl is-connected --pid 1 foo-slot if [[ "$(snap debug confinement)" = strict ]]; then svc_label=$(sed 's/ (.*)$//' < "/proc/$svc_pid/attr/current") @@ -62,7 +62,7 @@ execute: | expect_status 1 test-snap2.snapctl is-connected --apparmor-label "$svc_label" foo-slot echo "Non-snap AppArmor labels return a special exit code" - expect_status 10 test-snap2.snapctl is-connected --apparmor-label /usr/bin/evince foo-slot + expect_status 11 test-snap2.snapctl is-connected --apparmor-label /usr/bin/evince foo-slot fi # The remaining tests rely on classic confinement, so skip Ubuntu Core @@ -79,7 +79,7 @@ execute: | classic_pid=$(systemctl show --property=MainPID snap.test-snap-classic.svc.service | cut -d = -f 2) echo "Unconnected classic snaps report a special exit code" - expect_status 11 test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot + expect_status 10 test-snap2.snapctl is-connected --pid "$classic_pid" foo-slot echo "But still reports success when connected" snap connect test-snap-classic:foo-plug test-snap2:foo-slot From f51b136e4e9359afb2f7c5c1ed3c7ab8684a840e Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Fri, 29 Jan 2021 11:00:25 +0800 Subject: [PATCH 12/14] overlord/hookstate/ctlcmd: improve documentation --- overlord/hookstate/ctlcmd/is_connected.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/overlord/hookstate/ctlcmd/is_connected.go b/overlord/hookstate/ctlcmd/is_connected.go index 77571030905..672c6fc6f6a 100644 --- a/overlord/hookstate/ctlcmd/is_connected.go +++ b/overlord/hookstate/ctlcmd/is_connected.go @@ -65,6 +65,9 @@ a plug or slot is connected to the snap identified by the given process ID or AppArmor label. In this mode, additional failure exit codes may be returned: 10 if the other snap is not connected but uses classic confinement, or 11 if the other process is not snap confined. + +The --pid and --apparmor-label options may only be used with slots of +interface type "pulseaudio", "audio-record", or "cups-control". `) func init() { From b318e515dbce1a0855d459c2555d29bcb11ec744 Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Wed, 3 Feb 2021 10:57:14 +0800 Subject: [PATCH 13/14] o/hookstate/ctlcmd: update comments to use more inclusive language --- overlord/hookstate/ctlcmd/is_connected_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/overlord/hookstate/ctlcmd/is_connected_test.go b/overlord/hookstate/ctlcmd/is_connected_test.go index 206083f1eed..ccef3d99c18 100644 --- a/overlord/hookstate/ctlcmd/is_connected_test.go +++ b/overlord/hookstate/ctlcmd/is_connected_test.go @@ -77,11 +77,11 @@ var isConnectedTests = []struct { args: []string{"is-connected", "foo"}, err: `snap "snap1" has no plug or slot named "foo"`, }, { - // snap1:plug1 does not use a whitelisted interface + // snap1:plug1 does not use an allowed interface args: []string{"is-connected", "--pid", "1002", "plug1"}, err: `cannot use --pid check with snap1:plug1`, }, { - // snap1:slot1 does not use a whitelisted interface + // snap1:slot1 does not use an allowed interface args: []string{"is-connected", "--pid", "1002", "slot1"}, err: `cannot use --pid check with snap1:slot1`, }, { @@ -105,11 +105,11 @@ var isConnectedTests = []struct { args: []string{"is-connected", "--pid", "1005", "audio-record"}, exitCode: ctlcmd.ClassicSnapCode, }, { - // snap1:plug1 does not use a whitelisted interface + // snap1:plug1 does not use an allowed interface args: []string{"is-connected", "--apparmor-label", "snap.snap2.app", "plug1"}, err: `cannot use --apparmor-label check with snap1:plug1`, }, { - // snap1:slot1 does not use a whitelisted interface + // snap1:slot1 does not use an allowed interface args: []string{"is-connected", "--apparmor-label", "snap.snap2.app", "slot1"}, err: `cannot use --apparmor-label check with snap1:slot1`, }, { From f3c91c9608f736ddff8d45185343e3e2406009c2 Mon Sep 17 00:00:00 2001 From: James Henstridge Date: Wed, 3 Feb 2021 13:20:57 +0800 Subject: [PATCH 14/14] tests: disable classic checks of snapctl-is-connected-pid on Ubuntu 14.04 These tests rely on the ability to identify the snap a process ID belongs to, which means either (a) the systemd or unified cgroup name includes the snap name, or (b) the freezer cgroup includes the snap name. On Ubuntu 14.04, processes are not tracked via systemd so (a) fails. And snap-confine does not add classic confined processes to a freezer cgroup, so (b) fails as well. We're still runing the parts of the test that deal with strict confined snaps, since that functions correctly on 14.04. --- tests/main/snapctl-is-connected-pid/task.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/main/snapctl-is-connected-pid/task.yaml b/tests/main/snapctl-is-connected-pid/task.yaml index 2a1ae33495b..c8aa385433e 100644 --- a/tests/main/snapctl-is-connected-pid/task.yaml +++ b/tests/main/snapctl-is-connected-pid/task.yaml @@ -69,6 +69,12 @@ execute: | if [[ "$SPREAD_SYSTEM" = ubuntu-core-* ]]; then exit 0 fi + # We also skip Ubuntu 14.04, since it does not allow us track + # classic confined snap processes (there is no systemd based + # tracking, and they aren't added to a freezer cgroup). + if [[ "$SPREAD_SYSTEM" = ubuntu-14.04-* ]]; then + exit 0 + fi #shellcheck source=tests/lib/snaps.sh . "$TESTSLIB"/snaps.sh