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/builtin: rework for avahi interface #3624
Conversation
kubiko
added some commits
Jun 14, 2017
All checks have failed
|
Hey. Is there any forum discussion on the background of this change? |
zyga
requested review from
jdstrand and
zyga
Jul 26, 2017
|
@zyga not really, something I hit when snapping various projects. Also avahi snap as it exists now is quite limited compare to what avahi delivers on classic system. So this is just evening behaviour between UC and Classic on observer part of the interface. And control is just to go all the way on avahi interface. @jdstrand suggested to split it into observe and control parts which sounds reasonable. Additionally avahi is often used when building bridges to Apple ecosystem, so common thing in IOT. homebridge project is good example. Happy to create forum post though |
zyga
requested changes
Jul 26, 2017
Some early feedback. Let's iterate on this together.
One of the helpers I mentioned are not merged yet, I'll get them in ASAP.
| +// -*- Mode: Go; indent-tabs-mode: t -*- | ||
| + | ||
| +/* | ||
| + * Copyright (C) 2016 Canonical Ltd |
| + on-classic: false | ||
| +` | ||
| + | ||
| +const avahiControlSummary = `allows control over local domains, hostnames and services` |
zyga
Jul 26, 2017
Contributor
The summary doesn't really say this is about mDNS and service discovery.
| +func (iface *avahiControlInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error { | ||
| + old := "###SLOT_SECURITY_TAGS###" | ||
| + var new string | ||
| + if release.OnClassic { |
| + return nil | ||
| +} | ||
| + | ||
| +func (iface *avahiControlInterface) SanitizePlug(plug *interfaces.Plug) error { |
| +} | ||
| + | ||
| +func (iface *avahiControlInterface) SanitizeSlot(slot *interfaces.Slot) error { | ||
| + return nil |
| +} | ||
| + | ||
| +// The label glob when all apps are bound to the avahi slot | ||
| +func (s *AvahiControlInterfaceSuite) TestConnectedPlugSnippetUsesSlotLabelAll(c *C) { |
zyga
Jul 26, 2017
Contributor
Can you please use snaptest.MockInfo and extract those from the resulting snap.Info instead?
| + Apps: map[string]*snap.AppInfo{"app1": app1, "app2": app2}, | ||
| + }, | ||
| + } | ||
| + release.OnClassic = false |
| + | ||
| +// The label glob when all apps are bound to the avahi slot | ||
| +func (s *AvahiControlInterfaceSuite) TestConnectedPlugSnippetUsesSlotLabelSome(c *C) { | ||
| + app1 := &snap.AppInfo{Name: "app1"} |
| + Apps: map[string]*snap.AppInfo{"app1": app1, "app2": app2}, | ||
| + }, | ||
| + } | ||
| + release.OnClassic = false |
| + | ||
| +// The label glob when all apps are bound to the avahi slot | ||
| +func (s *AvahiControlInterfaceSuite) TestConnectedPlugSnippetUsesSlotLabelOne(c *C) { | ||
| + app := &snap.AppInfo{Name: "app"} |
| + Apps: map[string]*snap.AppInfo{"app": app}, | ||
| + }, | ||
| + } | ||
| + release.OnClassic = false |
| + } | ||
| + release.OnClassic = false | ||
| + | ||
| + apparmorSpec := &apparmor.Specification{} |
zyga
Jul 26, 2017
Contributor
Just say spec here, it's shorter. If you need multiple split those to individual tests. This will also clean up the problem of mocking the right data and you can move that up to the top of the file / into the suite type.
| + | ||
| +func (s *AvahiControlInterfaceSuite) TestConnectedPlugSnippedUsesUnconfinedLabelOnClassic(c *C) { | ||
| + slot := &interfaces.Slot{} | ||
| + release.OnClassic = true |
| +} | ||
| + | ||
| +func (iface *avahiObserveInterface) SanitizePlug(plug *interfaces.Plug) error { | ||
| + return nil |
| +} | ||
| + | ||
| +func (iface *avahiObserveInterface) SanitizeSlot(slot *interfaces.Slot) error { | ||
| + return nil |
| version: 1.0 | ||
| apps: | ||
| - app: | ||
| + app2: |
| + Apps: map[string]*snap.AppInfo{"app1": app1, "app2": app2}, | ||
| + }, | ||
| + } | ||
| + release.OnClassic = false |
| + Apps: map[string]*snap.AppInfo{"app": app}, | ||
| + }, | ||
| + } | ||
| + release.OnClassic = false |
| + | ||
| +func (s *AvahiObserveInterfaceSuite) TestConnectedPlugSnippedUsesUnconfinedLabelOnClassic(c *C) { | ||
| + slot := &interfaces.Slot{} | ||
| + release.OnClassic = true |
| @@ -292,6 +292,7 @@ plugs: | ||
| func (s *DbusInterfaceSuite) TestPermanentSlotAppArmorSession(c *C) { | ||
| apparmorSpec := &apparmor.Specification{} | ||
| + release.OnClassic = true |
| @@ -341,6 +342,7 @@ func (s *DbusInterfaceSuite) TestPermanentSlotAppArmorSessionClassic(c *C) { | ||
| func (s *DbusInterfaceSuite) TestPermanentSlotAppArmorSystem(c *C) { | ||
| apparmorSpec := &apparmor.Specification{} | ||
| + release.OnClassic = true |
|
I'll push some cleanups into this PR, please don't close it |
All checks have failed
|
I merged master and resolved API breaking conflicts. Let's please iterate on this branch to address remaining points and one @jdstrand is happy, land it. |
All checks have failed
pedronis
changed the title from
rework for avahi interface
to
interfaces/builtin: rework for avahi interface
Aug 3, 2017
zyga
added some commits
Aug 4, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
Some checks were not successful
zyga
requested changes
Aug 4, 2017
Actually, I just noticed one more thing to fix. Give me a moment.
zyga
added some commits
Aug 4, 2017
codecov-io
commented
Aug 4, 2017
•
Edited 1 time
-
codecov-io
Aug 4, 2017
-
Aug 4, 2017
Codecov Report
@@ Coverage Diff @@
## master #3624 +/- ##
==========================================
+ Coverage 75.16% 75.22% +0.06%
==========================================
Files 388 389 +1
Lines 33655 33725 +70
==========================================
+ Hits 25297 25371 +74
+ Misses 6539 6536 -3
+ Partials 1819 1818 -1
Continue to review full report at Codecov.
|
|
I think this is ready for review. @jdstrand please note that the two interfaces, avahi-{observe,control} share some snippets. I would perhaps rather have them explicitly shared (e.g. using a common 3rd snipped) or just copy-paste the relevant parts to the primary snippets. Please advise on how you'd like to see this. @kubiko I updated the PR and re-worked tests (have a look). I cut tests that were just checking the slot/plug app pattern generator and instead focused on testing where and what kind of snippets do we generate. I also tweaked how tests are prepared, how variables and snap apps are named, all for readability and (hopefully) some more consistency with other interfaces. This is constantly evolving but I'm trying my best to cut the amount of useless code and signal/noise ratio. |
All checks have passed
| @@ -124,25 +124,29 @@ func (iface *avahiControlInterface) AppArmorConnectedPlug(spec *apparmor.Specifi | ||
| } else { | ||
| new = slotAppLabelExpr(slot) | ||
| } | ||
| - snippet := strings.Replace(avahiObserveConnectedPlugAppArmor+avahiControlConnectedPlugAppArmor, old, new, -1) | ||
| + snippet := strings.Replace(avahiControlConnectedPlugAppArmor+avahiControlConnectedPlugAppArmor, old, new, -1) |
kubiko
Aug 4, 2017
Contributor
are you sure about this? Idea here is that control also includes observe part.
avahiControlConnectedPlugAppArmor + avahiControlConnectedPlugAppArmor seems odd to me
zyga
Aug 4, 2017
Contributor
Well, this is avahi_control and otherwise the control snippet was just unused. Perhaps the rules need tweaking but this was a clear mistake IMO.
| spec.AddSnippet(avahiObservePermanentSlotAppArmor) | ||
| return nil | ||
| } | ||
| func (iface *avahiControlInterface) AppArmorConnectedSlot(spec *apparmor.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error { | ||
| old := "###PLUG_SECURITY_TAGS###" | ||
| new := plugAppLabelExpr(plug) | ||
| - snippet := strings.Replace(avahiObserveConnectedSlotAppArmor+avahiControlConnectedSlotAppArmor, old, new, -1) | ||
| + snippet := strings.Replace(avahiControlConnectedSlotAppArmor+avahiControlConnectedSlotAppArmor, old, new, -1) |
Some checks were not successful
jdstrand
requested changes
Aug 4, 2017
This looks really good, thanks! :)
Just a few small things and I think it will be ready to merge.
| + | ||
| +const avahiControlSummary = `allows control over service discovery on a local network via the mDNS/DNS-SD protocol suite` | ||
| + | ||
| +const avahiControlConnectedSlotAppArmor = ` |
jdstrand
Aug 4, 2017
Contributor
It would be nice to have a comment here. Eg:
# Description: allows configuration of service discovery via mDNS/DNS-SD
| + } else { | ||
| + new = slotAppLabelExpr(slot) | ||
| + } | ||
| + snippet := strings.Replace(avahiObserveConnectedPlugAppArmor, old, new, -1) |
jdstrand
Aug 4, 2017
Contributor
I think it is fine to re-use this variable, but can you make a comment here. I suggest:
# avahi-control implies avahi-observe, so add snippets for both here
| + if !release.OnClassic { | ||
| + old := "###PLUG_SECURITY_TAGS###" | ||
| + new := plugAppLabelExpr(plug) | ||
| + snippet := strings.Replace(avahiObserveConnectedSlotAppArmor, old, new, -1) |
jdstrand
Aug 4, 2017
Contributor
Same here:
# avahi-control implies avahi-observe, so add snippets for both here
| + "github.com/snapcore/snapd/interfaces/dbus" | ||
| + "github.com/snapcore/snapd/release" | ||
| +) | ||
| + | ||
| const avahiObserveSummary = `allows discovering local domains, hostnames and services` |
jdstrand
Aug 4, 2017
Contributor
Let's update the summary to be in line with the control summary. I suggest:
const avahiObserveSummary = `allows discovery on a local network via the mDNS/DNS-SD protocol suite`
| - core | ||
| deny-auto-connection: true | ||
| + deny-connection: | ||
| + on-classic: false |
| + peer=(label=unconfined), | ||
| +` | ||
| + | ||
| +const avahiObserveConnectedSlotAppArmor = ` |
jdstrand
Aug 4, 2017
Contributor
Also, since avahiObserveConnectedSlotAppArmor is used in avahi-control, let's mention that above avahiObserveConnectedSlotAppArmor:
# Note: avahiObserveConnectedSlotAppArmor is also used by avahi-control in AppArmorConnectedSlot
| + peer=(name=org.freedesktop.Avahi, label=###PLUG_SECURITY_TAGS###), | ||
| +` | ||
| + | ||
| +const avahiObservePermanentSlotDBus = ` |
jdstrand
Aug 4, 2017
Contributor
And here:
# Note: avahiObservePermanentSlotDBus is used by avahi-control in DBusPermanentSlot
| +<policy group="netdev"> | ||
| + <allow send_destination="org.freedesktop.Avahi"/> | ||
| + <allow receive_sender="org.freedesktop.Avahi"/> | ||
| +</policy> |
jdstrand
Aug 4, 2017
Contributor
I realize you (essentially) copied upstream's bus policy, but I wonder how applicable the 'netdev' policy is here on Ubuntu Core. I suggest either removing it or commenting it out with a comment as to why.
kubiko
Aug 7, 2017
•
Edited 1 time
-
kubiko
Aug 7, 2017
Contributor
-
Aug 7, 2017
Good find, indeed makes no sense here.
I removed the policy and added comment about it there.
| +<policy user="root"> | ||
| + <allow send_destination="org.freedesktop.Avahi"/> | ||
| + <allow receive_sender="org.freedesktop.Avahi"/> | ||
| +</policy> | ||
| ` | ||
| const avahiObserveConnectedPlugAppArmor = ` |
jdstrand
Aug 4, 2017
Contributor
Can you adjust this comment:
# Description: allows domain browsing, service browsing and service resolving
to be:
# Description: allows domain, record, service, and service type browsing
# as well as address, host and service resolving
| +<policy user="root"> | ||
| + <allow send_destination="org.freedesktop.Avahi"/> | ||
| + <allow receive_sender="org.freedesktop.Avahi"/> | ||
| +</policy> | ||
| ` | ||
| const avahiObserveConnectedPlugAppArmor = ` |
jdstrand
Aug 4, 2017
Contributor
Same here for avahiObserveConnectedPlugAppArmor:
# Note: avahiObserveConnectedPlugAppArmor is also used by avahi-control in AppArmorConnectedPlug
| dbus (send) | ||
| bus=system | ||
| path=/ | ||
| interface=org.freedesktop.Avahi.Server | ||
| - member=Get* | ||
| - peer=(name=org.freedesktop.Avahi,label=unconfined), | ||
| + member={Get*,Resolve*,IsNSSSupportAvailable} |
jdstrand
Aug 4, 2017
Contributor
Can you comment on the addition of Resolve* and IsNSSSupportAvailable? It seems like Resolve* should be listed below....
kubiko
Aug 7, 2017
Contributor
when I was testing it with third party apps, some are using instead of {Host/Address/Domain}Resolver direct resolve functions (ResolveHostName, ResolveAddress, ResolveService, ….). Then I was split to either use separate rules and add it under each respective resolving section, or to add it to existing Get* rule all at once. Similar for IsNSSSupportAvailable which did not fit anywhere. I did not have strong opinion here, smaller amounts of rules won eventually.
But happy to split them and add each to respective sections, if that improves readability.
jdstrand
Aug 7, 2017
Contributor
I think it is fine to leave them combined, but perhaps add this comment:
# Allow accessing DBus properties and resolving
| + member={Get*,Resolve*,IsNSSSupportAvailable} | ||
| + peer=(name=org.freedesktop.Avahi,label=###SLOT_SECURITY_TAGS###), | ||
| + | ||
| +dbus (receive) |
jdstrand
Aug 4, 2017
Contributor
Can you add a comment above this rule:
# Allow receiving anything from the slot server
| + c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.appSlot, nil), IsNil) | ||
| + c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"}) | ||
| + c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "name=org.freedesktop.Avahi") | ||
| + c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `peer=(label="snap.producer.app"),`) |
jdstrand
Aug 4, 2017
Contributor
It might be nice to verify a specific rule from avahi-observe and from avahi-control for AddConnectedPlug and AddConnectedSlot since avahi-control pulls in both.
kubiko
Aug 7, 2017
•
Edited 1 time
-
kubiko
Aug 7, 2017
Contributor
-
Aug 7, 2017
Tests are now improved for both observe and control to check if observe does not include control and if control includes observe
|
@jdstrand I now pushed changes in, let me know if you want me to split |
|
@kubiko I suggest that you install some plugin for your editor to |
Some checks were not successful
|
formating now fixed as well |
jdstrand
approved these changes
Aug 7, 2017
Thanks for all the updates! Approving, though please consider adding a comment for the 'Get*/Resolve*' as mentioned earlier today.
All checks have passed
|
@jdstrand thanks you |
zyga
merged commit af4cc96
into
snapcore:master
Aug 8, 2017
7 checks passed
|
Great collaboration in this PR. Thanks to all involved. |
kubiko commentedJul 26, 2017
rework for avahi-observe
Adding new avahi-control for control level of capabilities