Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces: add apparmor support for hooks. #1373
Conversation
zyga
reviewed
Jun 21, 2016
| @@ -0,0 +1,2 @@ | ||
| +name: basic-hooks | ||
| +version: 1.0 |
kyrofa
Jun 21, 2016
•
Edited 1 time
-
kyrofa
Jun 21, 2016
Member
-
Jun 21, 2016
Only if they require plugs. Otherwise they're implicit just by being included in the snap.
kyrofa
referenced this pull request
Jun 21, 2016
Merged
interfaces: add seccomp support for hooks. #1374
niemeyer
reviewed
Jun 21, 2016
| - fmt.Fprintf(&buf, "@{APP_NAME}=\"%s\"\n", appInfo.Name) | ||
| - fmt.Fprintf(&buf, "@{SNAP_NAME}=\"%s\"\n", appInfo.Snap.Name()) | ||
| - fmt.Fprintf(&buf, "@{SNAP_REVISION}=\"%s\"\n", appInfo.Snap.Revision) | ||
| + fmt.Fprintf(&buf, "@{APP_NAME}=\"%s\"\n", appName) |
niemeyer
Jun 21, 2016
Contributor
A bit worried here as we're mixing app name and hook name on the same namespace with the same variable name. How is this variable being used today, and what is the potential for security issues?
kyrofa
Jun 21, 2016
Member
It's not used at all as far as I can see (so I'd prefer to remove it all together). @jdstrand or @zyga might have more information, as @zyga recommended I re-use the APP_NAME variable here.
niemeyer
Jun 21, 2016
Contributor
Reusing sounds quite dangerous, even more if we have no use case to understand why it'd be okay to reuse it without it being a security issue. If we're not using it today, let's please drop it.
kyrofa
Jun 21, 2016
Member
From @jdstrand:
I can say we used APP_NAME extensively on touch and may with personal. I'd prefer not to drop it until we know we don't need it, and I can't say we won't for personal.
So maybe it comes down to whether we want to re-use APP_NAME, or introduce HOOK_NAME?
niemeyer
reviewed
Jun 21, 2016
| + done | ||
| +execute: | | ||
| + echo "AppArmor profiles are generated and loaded for apps" | ||
| + sudo snap install ./basic-binaries_1.0_all.snap |
niemeyer
Jun 21, 2016
Contributor
Spread runs everything as root by default, so sudo may be dropped.
I'd also suggest dropping all the "./" prefixes.
niemeyer
reviewed
Jun 21, 2016
| +prepare: | | ||
| + for snap in basic-binaries basic-hooks | ||
| + do | ||
| + snapbuild ./../fixtures/snaps/$snap . |
niemeyer
Jun 21, 2016
Contributor
I forgot to talk to @fgimenez about this. Can we please move these snaps to under tests/lib/snaps, so we have a single place for all test aiding content?
The "./" prefix may also be dropped here and on the rm below.
fgimenez
Jun 22, 2016
Contributor
I forgot to talk to @fgimenez about this. Can we please move these snaps to under tests/lib/snaps, so we have a single place for all test aiding content?
niemeyer
reviewed
Jun 21, 2016
| + for snap in basic-binaries basic-hooks | ||
| + do | ||
| + sudo snap remove $snap | ||
| + rm ./${snap}_1.0_all.snap |
niemeyer
Jun 21, 2016
Contributor
Removing the built file makes sense, but no need to remove the snap. The suite restore-each script is resetting the whole snapd state for every test.
|
A few trivials, and one point worth talking about interactively. |
kyrofa
referenced this pull request
Jun 21, 2016
Merged
interfaces: add dbus support for hooks. #1376
kyrofa
added some commits
Jun 21, 2016
kyrofa
Kyle Fazzari
|
LGTM |
kyrofa
referenced this pull request
Jun 21, 2016
Merged
interfaces: add udev support for hooks. #1377
zyga
reviewed
Jun 21, 2016
| var buf bytes.Buffer | ||
| - fmt.Fprintf(&buf, "@{APP_NAME}=\"%s\"\n", appInfo.Name) |
mvo5
reviewed
Jun 22, 2016
| + profile := filepath.Join(dirs.SnapAppArmorDir, "snap.foo.hook.test-hook") | ||
| + | ||
| + // Verify that profile "snap.foo.hook.test-hook" was created | ||
| + _, err := os.Stat(profile) |
mvo5
Jun 22, 2016
Collaborator
You could use osutil.FileExists(profile) here (but stat is fine of course).
kyrofa
Jun 22, 2016
Member
Yeah using os.Stat() here keeps the tests consistent with the rest of the suite.
|
Branch looks good! It seems fine to drop |
Thanks for the review @mvo5! Agreed about the documentation: It's not mentioned in snapd anywhere and I'm not seeing anything on google. That combined with the fact that it wasn't being used for anything makes me feel fairly comfortable removing it. |
|
LGTM when tests pass |
kyrofa commentedJun 21, 2016
More progress on LP: #1586465. Also port AppArmor integration tests to Spread and include tests for hooks. Note that this isn't a complete port (it requires a reboot), so the old integration tests are left alone.