Skip to content

interfaces/builtin/network-setup-{control,observe}: allow using netplan directly #9784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mvo5 merged 14 commits into from
Mar 9, 2021
Merged

interfaces/builtin/network-setup-{control,observe}: allow using netplan directly #9784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
cd18731
interfaces/builtin/network-setup-control: allow using netplan apply d…
anonymouse64 Dec 10, 2020
307066c
interfaces/builtin/network-setup-observe: allow using netplan binary too
anonymouse64 Dec 10, 2020
f0c16b3
tests/main/fake-netplan-apply: update snap to use shell script callin…
anonymouse64 Dec 10, 2020
f76a102
tests/core/netplan/task.yaml: update to use netplan from shell script…
anonymouse64 Dec 10, 2020
4f206b4
interfaces/builtin/network-setup-{control,observe}: adjust comments
anonymouse64 Dec 10, 2020
d241bd6
Merge branch 'master' into feature/network-setup-control-allows-netpl…
anonymouse64 Feb 3, 2021
ebe115c
Merge branch 'master' into feature/network-setup-control-allows-netpl…
anonymouse64 Mar 3, 2021
f701f46
tests/core/netplan: expand test to test all bases on all core releases
anonymouse64 Mar 3, 2021
a1f64aa
tests/lib/prepare.sh: use /etc/group and friends from the core20 snap
anonymouse64 Mar 3, 2021
e063228
tests/core/netplan: enable for UC20 with permissions issue resolved
anonymouse64 Mar 3, 2021
88e8811
tests/lib/snaps/netplan-snap/bin/netplan.sh: add newline
anonymouse64 Mar 4, 2021
8baa27a
tests/main/fake-netplan-apply: update comment to explain support matrix
anonymouse64 Mar 4, 2021
55d20a2
Merge branch 'master' into feature/network-setup-control-allows-netpl…
anonymouse64 Mar 5, 2021
47077e4
Merge remote-tracking branch 'upstream/master' into feature/network-s…
mvo5 Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions interfaces/builtin/network_setup_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,19 @@ const networkSetupControlBaseDeclarationSlots = `
const networkSetupControlConnectedPlugAppArmor = `
# Description: Can read/write netplan configuration files

# Allow use of the netplan binary from the base snap. With this interface, this
# is expected to be able to apply and generate new network configuration, as
# well as get information about the current network configuration.
/usr/sbin/netplan ixr,
Copy link
Copy Markdown
Contributor

@pedronis pedronis Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit unclear what to do about that but it's not clear we can mandate for all bases to have neptlan inside them

Copy link
Copy Markdown
Contributor Author

@anonymouse64 anonymouse64 Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I would expect this only to be used on Ubuntu Core systems, so really this is only a requirement of Ubuntu Core boot bases. We could make the snippet conditional and only applied on core systems if that makes you more comfortable?

Copy link
Copy Markdown
Contributor Author

@anonymouse64 anonymouse64 Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern that the policy allows using the binary from the base snap, even if the base snap does not provide it, but I don't think this should be reason enough to block this pr because:

  • there are no boot base snaps out there today that currently don't ship netplan
  • the only base snaps that don't ship netplan are not boot base snaps (i.e. the freedesktop and the bare base snaps that I know of)
  • if we consider what the effect of allowing a snap to be provided permission to run netplan is when there may eventually be a boot base snap that does not have netplan, the impact is very small, specifically if you built an app today (using this pr) on a base snap that has netplan, then you decide to switch base snaps, then your snap will no longer work, but I don't view this as a problem because we don't automatically move snaps from one base snap to another, and we don't allow base snaps to break the contract of providing the file.
  • it's very unclear why someone would expect to be able to use network-setup-control on a non-boot base snap, i.e. what does it mean to be able to run netplan from the base snap when you have a freedesktop base snap?

Really the concern here is about the interface contract providing access to something that might not be there, which is fine IMHO because there are plenty of other cases where this is true in interfaces such as when there is a resource in /dev/, we provide access via apparmor to the /dev/ file even if it doesn't exist.

At the end of the day allowing this access will not break snaps and will not permit folks to build a snap that works then someday becomes broken. It only allows folks to knowingly build broken snaps, which they can already do today. Additionally this access addresses a product offering gap which is that in order to actually use netplan you have to essentially maintain your own netplan inside your snap which is a really unfortunate user story.

Copy link
Copy Markdown
Contributor

@pedronis pedronis Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's say we consider netplan optional, the other question is do we expect newer and older versions of netplan client to interoperate with newer/older version of actual system netplan?

I suppose we are ok because in the end it can only use the dbus interface that is the contract here? but actually we let it read the files as well under netplan? is the latter a problem?

Copy link
Copy Markdown
Contributor Author

@anonymouse64 anonymouse64 Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well there are 2 cases:

  1. with an older boot base and a newer app base snap
  2. with a newer boot base and an older app base snap

For 1), the only way that the app snap could be broken is if the new netplan binary decides to send a different D-Bus API, which IMHO would be netplan breaking a compatibility guarantee, but I don't know if there exists such a version compatibility guarantee

For 2), the only way that the app snap could be broken is if the new netplan service implementation stops understanding old API, which is 100% a broken compatibility promise, and is the same kind of breakage that would happen without this PR if the client bundles and ships netplan in their app snap anyways (like they must be doing today anyways).

Thinking about this again, I think that if the netplan team says they do not guarantee forward compatibility such that a new netplan binary could some-day send incompatible data to an older netplan service, then we should not do this. I asked the team about this and will report back what I hear.

Regarding reading the files, I don't view this as a problem since really those are just python files underneath /usr/share/netplan/... that the netplan binary needs to read/load in order to work, as netplan doesn't install it's python files inside the typical site-packages directory which is allowed by the python apparmor abstraction AFAICT.

# core18+ has /usr/sbin/netplan as a symlink to this script
/usr/share/netplan/netplan.script ixr,
# netplan related files
/usr/share/netplan/ r,
/usr/share/netplan/** r,

# Netplan uses busctl internally, so allow using that as well
/usr/bin/busctl ixr,
Copy link
Copy Markdown

@jdstrand jdstrand Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed when checking this that busctl triggers a net_admin denial, which is unfortunate, and not part of these interfaces but the command still succeeds:

bash-4.3# busctl call io.netplan.Netplan /io/netplan/Netplan io.netplan.Netplan Info
a(sv) 1 "Features" as 8 "generate-just-in-time" "generated-supplicant" "auth-phase2" "dhcp-use-domains" "ipv6-mtu" "modems" "sriov" "openvswitch"

Since we are using 'ixr', we get dbus mediation which is why no other dbus rules are needed.


/etc/netplan/{,**} rw,
/etc/network/{,**} rw,
/etc/systemd/network/{,**} rw,
Expand Down
14 changes: 14 additions & 0 deletions interfaces/builtin/network_setup_observe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,20 @@ const networkSetupObserveBaseDeclarationSlots = `
const networkSetupObserveConnectedPlugAppArmor = `
# Description: Can read netplan configuration files

# Allow use of the netplan binary from the base snap. With this interface, this
# is expected to be able to only get information about the current network
# configuration and not generate or apply it like is allowed with
# network-setup-control.
Comment thread
anonymouse64 marked this conversation as resolved.
/usr/sbin/netplan ixr,
# core18+ has /usr/sbin/netplan as a symlink to this script
/usr/share/netplan/netplan.script ixr,
# netplan related files
/usr/share/netplan/ r,
/usr/share/netplan/** r,

# Netplan uses busctl internally, so allow using that as well
/usr/bin/busctl ixr,

/etc/netplan/{,**} r,
/etc/network/{,**} r,
/etc/systemd/network/{,**} r,
Expand Down
135 changes: 74 additions & 61 deletions tests/core/netplan/task.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,75 +6,88 @@ details: |
environment:
NETPLAN: io.netplan.Netplan

# TODO: re-enable this on ubuntu-core-16-* when network-setup-control is updated
# to allow running netplan directly from the base snap and we can update this
# test to just using a simple shell script snap instead of building netplan from
# source, as currently that is broken on xenial with test-snapd-netplan-apply
# for some reason, see https://github.com/anonymouse64/test-snapd-netplan-apply/issues/1
systems: [ubuntu-core-18-*, ubuntu-core-2*]

prepare: |
snap install test-snapd-netplan-apply --edge
# build the netplan snap for this system
#shellcheck source=tests/lib/snaps.sh
. "$TESTSLIB"/snaps.sh

# use no base setting to use effectively "base: core"
sed "$TESTSLIB/snaps/netplan-snap/meta/snap.yaml.in" -e "s/base: BASESNAP/# no base snap for core base/" > "$TESTSLIB/snaps/netplan-snap/meta/snap.yaml"
snap pack "$TESTSLIB/snaps/netplan-snap" --filename=netplan-snap-16.snap

# use base: core18
sed "$TESTSLIB/snaps/netplan-snap/meta/snap.yaml.in" -e "s/base: BASESNAP/base: core18/" > "$TESTSLIB/snaps/netplan-snap/meta/snap.yaml"
snap pack "$TESTSLIB/snaps/netplan-snap" --filename=netplan-snap-18.snap

# use base: core20
sed "$TESTSLIB/snaps/netplan-snap/meta/snap.yaml.in" -e "s/base: BASESNAP/base: core20/" > "$TESTSLIB/snaps/netplan-snap/meta/snap.yaml"
snap pack "$TESTSLIB/snaps/netplan-snap" --filename=netplan-snap-20.snap

execute: |
echo "The interface is disconnected by default"
snap connections test-snapd-netplan-apply | MATCH 'network-setup-control +test-snapd-netplan-apply:network-setup-control +- +-'

echo "Running netplan apply without network-setup-control fails"
if test-snapd-netplan-apply.netplan apply; then
echo "Expected access denied error for netplan apply"
exit 1
fi

echo "Count how many network service restarts happened before calling netplan apply"
stopped_before="$("$TESTSTOOLS"/journal-state get-log -u systemd-networkd | grep -c 'Stopped Network Service.' || true)"
started_before="$("$TESTSTOOLS"/journal-state get-log -u systemd-networkd | grep -c 'Started Network Service.' || true)"

echo "When the interface is connected"
snap connect test-snapd-netplan-apply:network-setup-control

echo "Running netplan apply now works"
if ! test-snapd-netplan-apply.netplan apply; then
echo "Unexpected error running netplan apply"
exit 1
fi

echo "Ensure that network config was stopped and restarted from netplan"
for _ in $(seq 60); do
stopped_after="$("$TESTSTOOLS"/journal-state get-log -u systemd-networkd | grep -c 'Stopped Network Service.' || true)"
started_after="$("$TESTSTOOLS"/journal-state get-log -u systemd-networkd | grep -c 'Started Network Service.' || true)"
if [ "$stopped_after" -gt "$stopped_before" ] && \
[ "$started_after" -gt "$started_before" ] ; then
break
# test all base versions of netplan on all combos of core releases
for rel in 16 18 20; do
snap install --dangerous "netplan-snap-$rel.snap"
echo "The interface is disconnected by default"
snap connections netplan-snap | MATCH 'network-setup-control +netplan-snap:network-setup-control +- +-'

echo "Running netplan apply without network-setup-control fails"
if netplan-snap.netplan apply; then
echo "Expected access denied error for netplan apply"
exit 1
fi
sleep 1
done

echo "Ensure that the number of network restarts is greater after netplan apply was run"
[ "$stopped_after" -gt "$stopped_before" ] && [ "$started_after" -gt "$started_before" ]
echo "Count how many network service restarts happened before calling netplan apply"
stopped_before="$("$TESTSTOOLS"/journal-state get-log -u systemd-networkd | grep -c 'Stopped Network Service.' || true)"
started_before="$("$TESTSTOOLS"/journal-state get-log -u systemd-networkd | grep -c 'Started Network Service.' || true)"

if os.query is-core16; then
echo "Skipping Ubuntu Core 16 which does not have Info D-Bus method"
exit 0
fi
echo "When the interface is connected"
snap connect netplan-snap:network-setup-control

echo "Disconnecting network-setup-control to test network-setup-observe"
snap disconnect test-snapd-netplan-apply:network-setup-control
echo "Running netplan apply now works"
if ! netplan-snap.netplan apply; then
echo "Unexpected error running netplan apply"
exit 1
fi

echo "The network-setup-observe interface is disconnected by default"
snap connections test-snapd-netplan-apply | MATCH 'network-setup-observe +test-snapd-netplan-apply:network-setup-observe +- +-'
echo "Ensure that network config was stopped and restarted from netplan"
for _ in $(seq 60); do
stopped_after="$("$TESTSTOOLS"/journal-state get-log -u systemd-networkd | grep -c 'Stopped Network Service.' || true)"
started_after="$("$TESTSTOOLS"/journal-state get-log -u systemd-networkd | grep -c 'Started Network Service.' || true)"
if [ "$stopped_after" -gt "$stopped_before" ] && \
[ "$started_after" -gt "$started_before" ] ; then
break
fi
sleep 1
done

echo "Ensure that the number of network restarts is greater after netplan apply was run"
[ "$stopped_after" -gt "$stopped_before" ] && [ "$started_after" -gt "$started_before" ]

if os.query is-core16; then
echo "Skipping Ubuntu Core 16 which does not have Info D-Bus method"
exit 0
fi

echo "Running netplan info via D-Bus without network-setup-observe fails"
if test-snapd-netplan-apply.info; then
echo "Expected access denied error for netplan info via D-Bus"
exit 1
fi
echo "Disconnecting network-setup-control to test network-setup-observe"
snap disconnect netplan-snap:network-setup-control

echo "When the interface is connected"
snap connect test-snapd-netplan-apply:network-setup-observe
echo "The network-setup-observe interface is disconnected by default"
snap connections netplan-snap | MATCH 'network-setup-observe +netplan-snap:network-setup-observe +- +-'

echo "Running netplan info via D-Bus now works"
if ! test-snapd-netplan-apply.info; then
echo "Unexpected error running netplan info via D-Bus"
exit 1
fi
echo "Running netplan info via D-Bus without network-setup-observe fails"
if netplan-snap.netplan-info; then
echo "Expected access denied error for netplan info via D-Bus"
exit 1
fi

echo "When the interface is connected"
snap connect netplan-snap:network-setup-observe

echo "Running netplan info via D-Bus now works"
if ! netplan-snap.netplan-info; then
echo "Unexpected error running netplan info via D-Bus"
exit 1
fi

snap remove netplan-snap
done
0 ...s/main/fake-netplan-apply/netplan-info.sh → ...ib/snaps/netplan-snap/bin/netplan-info.sh
View file
Open in desktop
File renamed without changes.
6 changes: 6 additions & 0 deletions tests/lib/snaps/netplan-snap/bin/netplan.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/bin/sh

set -e

# netplan should be on $PATH from the base snap at /usr/sbin/netplan
netplan "$@"
22 changes: 22 additions & 0 deletions tests/lib/snaps/netplan-snap/meta/snap.yaml.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
name: netplan-snap
version: 0.1

# this gets replaced for different bases for the test
base: BASESNAP

plugs:
network: {}
network-bind: {}
network-setup-observe: {}
network-setup-control: {}

apps:
netplan:
command: bin/netplan.sh
# use a separate command for info since this one doesn't use netplan binary,
# instead it uses busctl to call the D-Bus endpoint
# we can't use netplan info, because netplan info will return things without
# using D-Bus, but for this test we want to test the D-Bus endpoint
# specifically
netplan-info:
command: bin/netplan-info.sh
74 changes: 0 additions & 74 deletions tests/main/fake-netplan-apply/snapcraft.yaml
View file
Open in desktop

This file was deleted.

Loading