overlord/ifacestate: setup security backends phased by backends first #6128

mvo5 · 2018-11-12T13:42:42Z

This is a tweak of #6127 which will setup all security systems based on backend first. This needs some more tests but I'm keen to see spread tests working for this one.

My feeling is that we should rework this a bit more and setup all slot security first and then all plug security (which we don't do right now). But before doing that this may unblock the problematic system(s).

This patch adds a helper method to obtain a given security backend by name. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Fixes: https://bugs.launchpad.net/tillamook/+bug/1802581 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

To fix LP: #1802581 a new helper setupPhasedSecurity() was added. This commit simplifies this helper - we already order the backends and the systemd backend is already first so we can skip the extra first step and instead setup security for all affected snaps at the same time (ordered by backends).

zyga

Some comments, more review in progress.

zyga · 2018-11-13T08:15:23Z

tests/regression/lp-1802581/task.yaml

+    # shellcheck source=tests/lib/systemd.sh
+    . "$TESTSLIB/systemd.sh"
+    wait_for_service mock-gpio
+    mount --bind "$GPIO_MOCK_DIR" /sys/class/gpio


This is the monkey patching of the Linux kernel :D

zyga · 2018-11-13T08:15:53Z

tests/regression/lp-1802581/task.yaml

@@ -0,0 +1,38 @@
+summary: Regression test for  1802581


Stray space

Could you please cherry pick the documentation of the issue from the snap attached to the bug report?

Yep, the bugreport seems to be a private one.

zyga · 2018-11-13T08:17:28Z

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

+
+
+if __name__ == "__main__":
+    os.chdir(sys.argv[1])


Could you please move this to main. Doing it like this makes sel, efd and ufd globals that can silently be picked up from typos elsewhere in the code.

stolowski · 2018-11-13T09:07:45Z

tests/regression/lp-1802581/task.yaml

+
+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


"pint" 🍺

stolowski · 2018-11-13T09:30:55Z

tests/regression/lp-1802581/task.yaml

@@ -0,0 +1,38 @@
+summary: Regression test for  1802581


Yep, the bugreport seems to be a private one.

stolowski

I couldn't find the associated bug report (seems to be a private one), so this may be a silly question, but could we recommend using interface hooks instead of evaluating a symlink (readlink?)? Basically, the prepare-slot-* hook could pass whatever needs to be known to the plug side. Or am I missing something?

zyga · 2018-11-13T12:03:22Z

We could perhaps use something like interface hooks but as it stands now the GPIO interface doesn’t work if you disable and enable a snap using it. We need to fix that aspect and this is why the shuffling happened.

We could change the interface to use something like hooks internally but that is much more work than necessary at this time.

stolowski

LGTM, first pass comments.

stolowski · 2018-11-13T13:52:55Z

overlord/ifacestate/helpers.go

+	st := task.State()
+
+	// Setup all affected snaps, start with the most important security
+	// backend and run it for all snaps. See LP: 1802581


Might be worth clarifying the comment and say the ordering is established at init time (and maintained in the repo).

indeed, this comment probably needs some expanding, important doesn't convey the fact that some backend behavior depends on the work of some other backend

stolowski · 2018-11-13T13:57:13Z

overlord/ifacestate/helpers.go

+	// backend and run it for all snaps. See LP: 1802581
+	for _, backend := range m.repo.Backends() {
+		for i, snapInfo := range snaps {
+			st.Unlock()


Can we just st.Unlock() and defer st.Lock() before the loop (I know this is how the old setupSnapSecurity does that, just curious.

the reason is done like that is that the systemd backend can take a long time

stolowski · 2018-11-13T13:58:31Z

tests/regression/lp-1802581/task.yaml

+debug: |
+    journalctl -u mock-gpio.service
+
+execute: |


pedronis

looks good, nothing blocking that can't be done in a follow up, except it would be good if the comment was improved already here

pedronis · 2018-11-14T16:12:05Z

overlord/ifacestate/handlers.go

+		}
+	}
+	sort.Strings(affectedNames)
+	affectedNames = append([]string{snapName}, affectedNames...)


we could do this before the for loop instead of after

not because Sort but then maybe the comment needs moving

pedronis · 2018-11-14T16:13:38Z

overlord/ifacestate/helpers.go

+	st := task.State()
+
+	// Setup all affected snaps, start with the most important security
+	// backend and run it for all snaps. See LP: 1802581


indeed, this comment probably needs some expanding, important doesn't convey the fact that some backend behavior depends on the work of some other backend

pedronis · 2018-11-14T16:16:20Z

overlord/ifacestate/helpers.go

+	for _, backend := range m.repo.Backends() {
+		for i, snapInfo := range snaps {
+			st.Unlock()
+			err := backend.Setup(snapInfo, opts[i], m.repo)


@mvo @zyga one interesting thing with this change is that backends could grow a backend.SetupMany() that could optimize a bit in some case

Interesting observation. I think we need to sit down and design the backend setup process to optimise the apparmor wasted cycles we spend. In my mind I was thinking about backend.StartTransaction() ... and CommitTransaction() like API where we don't commit unless something needs to observe the intermediate state of some interfaces being connected and setup and some not yet when we know there are more setup calls just around the corner.

pedronis · 2018-11-14T16:16:52Z

overlord/ifacestate/helpers.go

+	// backend and run it for all snaps. See LP: 1802581
+	for _, backend := range m.repo.Backends() {
+		for i, snapInfo := range snaps {
+			st.Unlock()


the reason is done like that is that the systemd backend can take a long time

pedronis · 2018-11-14T16:17:33Z

overlord/ifacestate/helpers.go

+
+	// 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() {


can we add some kind of unit tests that detects somehow the change of order?

Yes, I will work on that now.

zyga and others added 5 commits November 10, 2018 11:06

interfaces: add Repository.Backend

61727a4

This patch adds a helper method to obtain a given security backend by name. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

overlord/ifacestate: add setupPhasedSecurity

b59f5a4

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

overlord/ifacestate: use setupPhasedSecurity

6127cb0

Fixes: https://bugs.launchpad.net/tillamook/+bug/1802581 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

tests: add regression test for lp-1802581

9588c9f

mvo5 force-pushed the fix/lp-1802581-2 branch from dac2c8b to 9588c9f Compare November 12, 2018 17:15

tests: improve mock-gpio.py

bd190c2

zyga reviewed Nov 13, 2018

View reviewed changes

mock-gpio.py: make pylint/pep8/mypy (mostly) clean

071616a

stolowski reviewed Nov 13, 2018

View reviewed changes

mvo5 mentioned this pull request Nov 13, 2018

interfaces: export gpio pin in AppArmorConnectedPlug #6141

Closed

Merge remote-tracking branch 'upstream/master' into fix/lp-1802581-2

b6b1626

mvo5 mentioned this pull request Nov 14, 2018

tests: use mock-gpio.py in enable-disable-units-gpio test #6153

Merged

ifacestate: rename setupPhasedSecurity -> setupSecurityByBackend

71b219f

pedronis approved these changes Nov 14, 2018

View reviewed changes

mvo5 merged commit 6e89da5 into snapcore:master Nov 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord/ifacestate: setup security backends phased by backends first #6128

overlord/ifacestate: setup security backends phased by backends first #6128

mvo5 commented Nov 12, 2018

zyga left a comment

zyga Nov 13, 2018

zyga Nov 13, 2018

zyga Nov 13, 2018

stolowski Nov 13, 2018

zyga Nov 13, 2018

stolowski Nov 13, 2018

stolowski Nov 13, 2018

stolowski left a comment

zyga commented Nov 13, 2018

stolowski left a comment

stolowski Nov 13, 2018

pedronis Nov 14, 2018

stolowski Nov 13, 2018

pedronis Nov 14, 2018

stolowski Nov 13, 2018

pedronis left a comment

pedronis Nov 14, 2018 •

edited

pedronis Nov 14, 2018

pedronis Nov 14, 2018

pedronis Nov 14, 2018

zyga Nov 14, 2018

pedronis Nov 14, 2018

pedronis Nov 14, 2018

zyga Nov 14, 2018

overlord/ifacestate: setup security backends phased by backends first #6128

overlord/ifacestate: setup security backends phased by backends first #6128

Conversation

mvo5 commented Nov 12, 2018

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolowski left a comment

Choose a reason for hiding this comment

zyga commented Nov 13, 2018

stolowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis Nov 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Nov 14, 2018 •

edited