add spread test for connecting all interfaces (excepting gadget slots) #4157

Merged
merged 13 commits into from Nov 10, 2017

Conversation

Projects
None yet
6 participants
Contributor

jdstrand commented Nov 6, 2017

Test each interface for app and os snaps and verify a simple command can be
run when connected. This provides high-level testing for things like AppArmor
policy syntax errors, seccomp policy parsing, udev querying bugs, etc.

This provides two provider snaps: test-policy-app-provider-core and
test-policy-app-provider-classic and chooses which to install based on if the
system is a core system or not.

It also provides the test-policy-app-consumer snap that is used to connect to
the provider snap for app-provided slots and to the core snap for implicit
slots.

The provider and consumer snaps in tests/lib/snaps should be updated when new
interfaces are added.

jdstrand added some commits Nov 2, 2017

add spread test for connecting all interfaces (excepting gadget slots)
Test each interface for app and os snaps and verify a simple can be run when
connected. This provides high-level testing for things like AppArmor policy
syntax errors, seccomp policy parsing, udev querying bugs, etc.

This provides two provider snaps: test-policy-app-provider-core and
test-policy-app-provider-classic and chooses which to install based on if the
system is a core system or not.

It also provides the test-policy-app-consumer snap that is used to connect to
the provider snap for app-provided slots and to the core snap for implicit
slots.

The provider and consumer snaps in tests/lib/snaps should be updated when new
interfaces are added.
add spread test for connecting all interfaces (excepting gadget slots)
Test each interface for app and os snaps and verify a simple can be run when
connected. This provides high-level testing for things like AppArmor policy
syntax errors, seccomp policy parsing, udev querying bugs, etc.

This provides two provider snaps: test-policy-app-provider-core and
test-policy-app-provider-classic and chooses which to install based on if the
system is a core system or not.

It also provides the test-policy-app-consumer snap that is used to connect to
the provider snap for app-provided slots and to the core snap for implicit
slots.

The provider and consumer snaps in tests/lib/snaps should be updated when new
interfaces are added.
Merge branch 'add-test-policy-app-spread-tests' of github.com:jdstran…
…d/snapd into add-test-policy-app-spread-tests

jdstrand added a commit to jdstrand/snapd that referenced this pull request Nov 7, 2017

interfaces/kmod: treat failure to load module as non-fatal
Since different kernels may not have the requested module, treat any error from
modprobe as non-fatal and attempt any subsequent module loads. Without this,
failure to load a module means failure to connect the interface and the other
security backends.

This issue was found as part of snapcore#4157
where the broadcom-asic-control and firewall-control interfaces could not be
connected because some of the modules were not available in the test
environments. Importantly, this change is not just for the test environment,
different kernels will have different kernel configs and we need to make sure
that the interface is connectable for the other backends.
Contributor

jdstrand commented Nov 7, 2017

Note, when #4162 is committed, we can remove the workarounds for broadcom-asic-control, firewall-control, etc.

codecov-io commented Nov 7, 2017

Codecov Report

Merging #4157 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4157      +/-   ##
==========================================
+ Coverage   75.56%   75.56%   +<.01%     
==========================================
  Files         436      436              
  Lines       37814    37814              
==========================================
+ Hits        28573    28574       +1     
+ Misses       7246     7245       -1     
  Partials     1995     1995
Impacted Files Coverage Δ
cmd/snap/cmd_aliases.go 95% <0%> (+1.66%) ⬆️

Continue to review full report at Codecov.

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

Contributor

jdstrand commented Nov 7, 2017

@mvo5 - on systems with apparmor available the execution takes 200+ seconds per system due to all the interface connections which all combined causes a travis timeout. The test should only be connecting interfaces that are already disconnected (respectively). I think this test is really valuable-- the technique used found 3 different bugs already.

Would it be good enough to only run this on Ubuntu Core 16, ubuntu 16.04 and systems where backends are disabled (eg, fedora)? This may be sufficient for detecting the errors I'm thinking about (ie, those in the description), but ideally we'd test on all platforms. We could also up the timeout value in travis (if it is possible). Please advise.

jdstrand added some commits Nov 7, 2017

Contributor

zyga commented Nov 8, 2017

@jdstrand some test errors:

grep: tests/lib/snaps/test-policy-app-consumer/meta/snap.yaml: No such file or directory
Missing high-level test for interface 'account-control'. Please add to:

  • tests/lib/snaps/test-policy-app-consumer/meta/snap.yaml
  • tests/lib/snaps/test-policy-app-provider-core/meta/snap.yaml (if needed)
  • tests/lib/snaps/test-policy-app-provider-classic/meta/snap.yaml (if needed)

jdstrand added some commits Nov 8, 2017

Contributor

sergiocazzolato commented Nov 8, 2017

@jdstrand thanks for the test!
I see that we have many of these interfaces already covered by spread tests, perhaps we should cover here the interfaces that are not tested yet to save test execution time.

Contributor

jdstrand commented Nov 8, 2017

@sergiocazzolato - while I think this is good idea in principle, in practice I think that will introduce testing gaps. For example, right now I added something to run-checks to make sure that the interface is added to spread, but that would otherwise be impossible if split out. These tests also verify implicit core and classic interfaces whereas broken out tests may not, depending on the test. These tests also verify slot policy on core and classic whereas broken out tests may not, depending on the test. These tests also verify interface connections on non-Ubuntu systems whereas broken out tests may not, depending on the test.

What this test is meant to achieve is a guaranteed no-gaps high-level test on representative systems (eg, those with full, partial or no security confinement) to show all interfaces are connectable (the techniques used in this test found 4 different bugs that were not exposed in our existing tests).

Contributor

jdstrand commented Nov 8, 2017

@sergiocazzolato - I mentioned core vs classic and slot vs implicit because in all of these cases security policy may be different, depending on the interface. I think if the existing tests could be verified to test all of these, then we could remove the interface from this test, and add something to the case statement in run-checks to say these are fully tested elsewhere.

run-checks
+ snap_yaml="tests/lib/snaps/test-snapd-policy-app-consumer/meta/snap.yaml"
+ core_snap_yaml="tests/lib/snaps/test-snapd-policy-app-provider-core/meta/snap.yaml"
+ classic_snap_yaml="tests/lib/snaps/test-snapd-policy-app-provider-classic/meta/snap.yaml"
+ for iface in `ls -1 ./interfaces/builtin/ | grep -Ev '_test\.go' | sed -e 's/\.go$//' -e 's/_/-/g'` ; do
@bboozzoo

bboozzoo Nov 9, 2017

Contributor

Just a thought bu maybe it would be better to use a tiny Go program like this one instead of transforming file names:

import "fmt"
import "github.com/snapcore/snapd/interfaces/builtin"

func main() {
	for _, iface := range builtin.Interfaces() {
		fmt.Printf("%s\n", iface.Name())
	}
}

You can put it under tests/lib/listbuiltin.go and run with go run tests/lib/listbuiltin.go

Even better would be to verify spread yamls directly in Go.

@jdstrand

jdstrand Nov 9, 2017

Contributor

Done (with list-interfaces.go)

mvo5 approved these changes Nov 10, 2017

Very nice, thank you!

@mvo5 mvo5 merged commit c683249 into snapcore:master Nov 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jdstrand jdstrand deleted the jdstrand:add-test-policy-app-spread-tests branch Nov 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment