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: expose bluez interface on classic OS #3812
Conversation
codecov-io
commented
Aug 26, 2017
•
Edited 1 time
-
codecov-io
Aug 26, 2017
-
Aug 26, 2017
Codecov Report
@@ Coverage Diff @@
## master #3812 +/- ##
==========================================
+ Coverage 75.7% 75.71% +<.01%
==========================================
Files 409 409
Lines 35227 35241 +14
==========================================
+ Hits 26670 26684 +14
Misses 6672 6672
Partials 1885 1885
Continue to review full report at Codecov.
|
pedronis
changed the title from
Expose bluez interface on classic OS
to
interfaces: expose bluez interface on classic OS
Aug 29, 2017
zyga
requested review from
jdstrand and
zyga
and removed request for
jdstrand
Aug 30, 2017
zyga
approved these changes
Aug 30, 2017
Looks good to me, please resolve the conflicts and get a 2nd +1 from @jdstrand
zyga
requested a review
from
jdstrand
Aug 30, 2017
|
Shouldn't this only expose bluez if the system has bluez running? |
jdstrand
requested changes
Aug 30, 2017
Thanks for the updates! This is well on its way, just a few more small changes and we should be able to commit this.
| deny-auto-connection: true | ||
| + deny-connection: | ||
| + on-classic: false | ||
| ` |
| - spec.AddSnippet(bluezPermanentSlotAppArmor) | ||
| + if !release.OnClassic { | ||
| + spec.AddSnippet(bluezPermanentSlotAppArmor) | ||
| + } | ||
| return nil | ||
| } |
| } | ||
| func (s *BluezInterfaceSuite) TestDBusSpec(c *C) { | ||
| + restore := release.MockOnClassic(false) |
jdstrand
Aug 30, 2017
Contributor
Please add this comment before this line:
// on a core system with bluez slot coming from a regular app snap.
| c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.producer.app"}) | ||
| c.Assert(spec.SnippetForTag("snap.producer.app"), testutil.Contains, `<allow own="org.bluez"/>`) | ||
| + | ||
| + spec = &dbus.Specification{} |
jdstrand
Aug 30, 2017
Contributor
Before this line you should:
// on a classic system with bluez slot coming from the core snap.
restore = release.MockOnClassic(true)
defer restore()
| } | ||
| func (s *BluezInterfaceSuite) TestSecCompSpec(c *C) { | ||
| spec := &seccomp.Specification{} | ||
| - c.Assert(spec.AddPermanentSlot(s.iface, s.slot), IsNil) | ||
| + c.Assert(spec.AddPermanentSlot(s.iface, s.appSlot), IsNil) | ||
| c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.producer.app"}) | ||
| c.Assert(spec.SnippetForTag("snap.producer.app"), testutil.Contains, "listen\n") |
jdstrand
Aug 30, 2017
Contributor
Like in TestAppArmorSpec, you need to test for when on classic and when not on classic (to test the (requested) changes to SecCompPermanentSlot(), above)
| c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.producer.app"}) | ||
| c.Assert(spec.SnippetForTag("snap.producer.app"), testutil.Contains, "listen\n") | ||
| } | ||
| func (s *BluezInterfaceSuite) TestUDevSpec(c *C) { | ||
| spec := &udev.Specification{} | ||
| - c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil) | ||
| + c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.appSlot, nil), IsNil) | ||
| c.Assert(spec.Snippets(), HasLen, 1) | ||
| c.Assert(spec.Snippets()[0], testutil.Contains, `KERNEL=="rfkill", TAG+="snap_consumer_app"`) |
jdstrand
Aug 30, 2017
Contributor
Here you should test for both when on classic and when not on classic. In this case, the resulting testutil.Contains will be the same, since you didn't (and we don't need to) change UDevConnectedPlug().
jdstrand
Aug 30, 2017
Contributor
Also, while I didn't explicitly request it, please add the appropriate comments // on a classic|core system with bluez slot coming from the core|app snap. when updating the testsuite for udev and seccomp.
|
"Shouldn't this only expose bluez if the system has bluez running?" @chipaca - no. In terms of security policy, it doesn't matter if the slot side (in this case, bluez on classic) is running. In terms of dynamic slots (something planned for the future), this is interesting to think about. For example, the plan is that on boot the core snap would be able to enumerate the serial ports, i2c devices, etc and then dynamically expose them as slots. It is theoretically possible to extend this to classic services-- eg, if bluez or network-manager isn't running, don't expose those slots. This is a bit tricky because the services not running could simply be a temporary situation (admin stopped them, they come up after snapd, etc). @niemeyer - perhaps you want to have in the back of your mind dynamic slots for (classic) services when considering hotplugging (obviously not for this PR :). |
| + defer restore() | ||
| + | ||
| + spec = &udev.Specification{} | ||
| + c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.appSlot, nil), IsNil) |
All checks have passed
jdstrand
approved these changes
Aug 30, 2017
Thanks for all your care on this PR! Assuming the tests pass, LGTM.
|
Thanks a lot @willdeberry - also thanks for signing the contributor agreement! |
willdeberry commentedAug 26, 2017
No description provided.