/snapd

interfaces/builtin: improve pulseaudio interface #1432

Merged
merged 23 commits into from Aug 5, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Reviewed
Projects
None yet
Milestone
No milestone
6 participants
@jhodapp @zyga @morphis @niemeyer @snappy-m-o @jdstrand
@jhodapp

jhodapp commented Jun 28, 2016 •
Edited 1 time

Add more complex PulseAudio interface, also add PulseAudio interface unit tests.

Simon Fels and others added some commits May 24, 2016

interfaces: extend pulseaudio interface with slot side

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage decreased (-0.3%) to 79.045%
Details
9cb1488
Add PulseAudio dbus interface unit tests
f7340fc
Merged with interface changes branch from morphis
fabc582
Update for the new interface changes from morphis
1f4feea
@zyga
Contributor

zyga commented Jun 28, 2016

whitelist this please
interfaces/builtin/pulseaudio.go
+}
+
+func (iface *PulseAudioInterface) AutoConnect() bool {
+ return false
@zyga

zyga Jun 28, 2016

Contributor

Why did this switch to false?

@jhodapp

jhodapp Jun 29, 2016 •
Edited 1 time

Not sure why it got changed (Simon did the interface changes) but it's fixed now. I'll double check that he didn't change this for a specific reason.

interfaces/builtin/pulseaudio.go
+setpriority
+setsockopt
+getsockname
+bind
@zyga

zyga Jun 28, 2016 •
Edited 1 time

Contributor

Not 100% sure but don't we just want to make pulseaudio to use the network and network-bind slots?

@jdstrand

jdstrand Jun 29, 2016

Contributor

All of these I imagine are for the UNIX socket, not INET, so, no, let's leave this as is.

@morphis

morphis Jun 29, 2016

Contributor

They are for UNIX sockets.

@morphis

morphis Jun 29, 2016

Contributor

@jhodapp Can you add a note that we're using them for UNIX sockets?

@jhodapp

jhodapp Jun 29, 2016

Added

interfaces/builtin/pulseaudio.go
+
+# For udev
+network netlink raw,
+/sys/devices/virtual/dmi/id/sys_vendor r,
@zyga

zyga Jun 28, 2016

Contributor

While something that pulse probably needs for valid reasons, we could move this to a dmi interface. Not sure.

@morphis

morphis Jun 29, 2016

Contributor

Unsure too, I would say we leave this for now and create a DMI interface if needed.

interfaces/builtin/pulseaudio.go
+capability sys_resource,
+
+@{PROC}/self/exe r,
+/etc/machine-id r,
@zyga

zyga Jun 28, 2016

Contributor

Can we move this to the apparmor template instead? @jdstrand what do you think?

@jdstrand

jdstrand Jun 29, 2016 •
Edited 1 time

Contributor

That's fine.

UPDATE: (for machine-id)

@jdstrand

jdstrand Jul 8, 2016

Contributor

Please remove /etc/machine-id here too-- it is part of the default policy now.

interfaces/builtin/pulseaudio.go
- name: "pulseaudio",
- connectedPlugAppArmor: pulseaudioConnectedPlugAppArmor,
- connectedPlugSecComp: pulseaudioConnectedPlugSecComp,
- reservedForOS: true,
@zyga

zyga Jun 28, 2016 •
Edited 1 time

Contributor

Removing this reservedForOS will allow anyone to upload a snap with the pulseaudio slot. Can we please make sure that is flagging the snap for manual review in the store?

@jdstrand

jdstrand Jun 29, 2016

Contributor

Specifying slots in the yaml by default triggers a manual review, so this should be fine.

interfaces/builtin/pulseaudio.go
+`)
+
+var pulseaudioConnectedPlugAppArmorDesktop = []byte(`
+# Only on desktop we need access to /etc/pulse for any PulseAudio client
@zyga

zyga Jun 28, 2016

Contributor

This requires more work but you can use release.OnClassic and construct the rules dynamically so that we don't hand out too many permissions.

@jhodapp

jhodapp Jun 29, 2016

I've got what I think is the right thing for this. Take a look and let me know if it's what you had in mind. I'm completely new to snaps, snap interfaces, etc - just an FYI.

@zyga
Contributor

zyga commented Jun 28, 2016

Some good stuff there but I would encourage you to split classic and all-snap systems harder. We also want to ensure that pulseaudio automatically connects on classic (on all-snaps this is more of a topic to discuss but I don't see why not) so that we don't break existing systems.

I'd like to see some more testing on this, perhaps we can snap some tools (paplay?) and write a spread test that installs it and ensures it is connected. We could even try to play some sound, ensuring that it doesn't crash. If you need help with any of that please let me know.

What is our all-snap testing strategy? I think we could also use spread with an all-snap system and install pulseaudio along with a test snap and ensure that same dummy paplay thing works.

@zyga zyga added the Reviewed label Jun 28, 2016

@jhodapp

jhodapp commented Jun 28, 2016

Thanks for the feedback @zyga. Simon wrote the interface part, so I'm going to defer to him to comment on those parts.
interfaces/builtin/pulseaudio.go
-/etc/pulse/ r,
-/etc/pulse/* r,
+var pulseaudioConnectedPlugAppArmor = []byte(`
+capability dac_override,
@jdstrand

jdstrand Jun 29, 2016

Contributor

What is this needed? This grants more access than we want to grant and I'd like to better understand why you added it.

@morphis

morphis Jun 29, 2016

Contributor

Pulseaudio tries to switch to a different user which is configured at compile time (root, root in our case). I wasn't sure if we want this left in or better disable the setuid/setgid call in the code.

@morphis

morphis Jun 29, 2016

Contributor

But to be honest, I can't remember right now why this was required for the plug side as setuid/setgid happens on the slot side.

interfaces/builtin/pulseaudio.go
+var pulseaudioConnectedPlugAppArmor = []byte(`
+capability dac_override,
+
+/etc/machine-id r,
@jdstrand

jdstrand Jun 29, 2016

Contributor

This is currently allowed in 'abstractions/dbus-session-strict'. On touch we specifically disabled the DBus socket and only allowed apps to connect via the native socket. Is this changing or is pulseaudio using /etc/machine-id for something unrelated?

@morphis

morphis Jun 29, 2016

Contributor

It uses machine-id in various places to name its sockets. On the client side its using this to build the correct socket name which it will then access.

interfaces/builtin/pulseaudio.go
-owner /{,var/}run/user/*/pulse/ r,
+
+# FIXME Desktop-only
+owner /{,var/}run/user/*/pulse/ rwk,
owner /{,var/}run/user/*/pulse/native rwk,
@jdstrand

jdstrand Jun 29, 2016

Contributor

This is easy enough to do programmatically-- use release.OnClassic strategically to add these.

@morphis

morphis Jun 29, 2016

Contributor

@jhodapp Can you split those two lines out and combine with the snippet depending on if release.OnClassic is set?

@jhodapp

jhodapp Jun 29, 2016

Done

interfaces/builtin/pulseaudio.go
-const pulseaudioConnectedPlugSecComp = `
+# Running as system instance
+owner /{,var/}run/pulse rwk,
@jdstrand

jdstrand Jun 29, 2016

Contributor

This is a weird access (why does the client create the pulse directory?) and you are also missing the trailing '/'. I think you want this instead:
owner /{,var/}run/pulse/ r,

@morphis

morphis Jun 29, 2016

Contributor

It doesn't. My bad, lets switch this to just r

@jhodapp

jhodapp Jun 29, 2016

Done

@morphis
Contributor

morphis commented Jun 29, 2016

@zyga We have a pulseaudio snap which includes paplay/pactl at https://code.launchpad.net/~morphis/snappy-hwe-snaps/+git/pulseaudio/+ref/add-initial-packaging which can be used for testing. Adding a spread test would be a good thing but would require more tíme than we currently have for this. @jhodapp can you have a quick look what it would mean to come up with a spread test for this?

@zyga Does spread require the snap being published in the store?
@zyga
Contributor

zyga commented Jun 30, 2016

@morphis As for spread tests, I don't think this is a strict requirement, we have some demo snaps that we build during the testing process. Still I think it is fine to have an array of snaps in the store that are only there for testing. Use your best judgment to decide.

As an alternative to a spread test can we please have a manual test case so that we can check this once in a while? @fgimenez and @elopio will advise on how to express that.

Thanks for the big update, I'll re-review it soon.
@jhodapp

jhodapp commented Jun 30, 2016

I don't understand why the Travis CI build keeps failing now. I don't see a specific error from the code but I do see something that looks like an infrastructure timeout or related.
@niemeyer
Contributor

niemeyer commented Jul 4, 2016

@jhodapp There was a bug in master introduced by a change in spread (we moved from sh to bash).

Can you please merge master and re-push?

@niemeyer niemeyer changed the title from Pulse audio tests to interfaces/builtin: improve pulseaudio interface Jul 4, 2016

@snappy-m-o
Contributor

snappy-m-o commented Jul 6, 2016

Can one of the admins verify this patch?
@snappy-m-o
Contributor

snappy-m-o commented Jul 6, 2016

Can one of the admins verify this patch?
@snappy-m-o
Contributor

snappy-m-o commented Jul 7, 2016

Can one of the admins verify this patch?
@snappy-m-o
Contributor

snappy-m-o commented Jul 7, 2016

Can one of the admins verify this patch?
@snappy-m-o
Contributor

snappy-m-o commented Jul 7, 2016

Can one of the admins verify this patch?
interfaces/builtin/pulseaudio.go
+var pulseaudioConnectedPlugAppArmor = []byte(`
+capability dac_override,
+
+/etc/machine-id r,
@jdstrand

jdstrand Jul 8, 2016

Contributor

Please remove, this is now in the default template.

interfaces/builtin/pulseaudio.go
)
-const pulseaudioConnectedPlugAppArmor = `
+var pulseaudioConnectedPlugAppArmor = []byte(`
+capability dac_override,
@jdstrand

jdstrand Jul 8, 2016

Contributor

Please remove this. From @morphis "But to be honest, I can't remember right now why this was required for the plug side as setuid/setgid happens on the slot side." The connected plug should not need elevated privileges to work with pulseaudio, which this capability rule grants. If after removing it you still need it, please investigate way and adjust the slot snap to not require the plug snap to need this.

interfaces/builtin/pulseaudio.go
+var pulseaudioConnectedPlugAppArmorDesktop = []byte(`
+# Only on desktop do we need access to /etc/pulse for any PulseAudio client
+# to read available client side configuration settings. On an Ubuntu Core
+# device those things will be stored inside the snap directory.
/etc/pulse/ r,
/etc/pulse/* r,
@jdstrand

jdstrand Jul 8, 2016

Contributor

In addition to these desktop-only accesses, shouldn't we also add these for the pulse cookie (from Touch policy):
owner @{HOME}/.pulse-cookie rk,
owner @{HOME}/.config/pulse/cookie rk,

and when are these needed (if ever):
owner @{HOME}/.pulse/ r,
owner @{HOME}/.pulse/* rk,

interfaces/builtin/pulseaudio.go
+
+/{run,dev}/shm/pulse-shm-* rwk,
+
+# Running as system instance
@jdstrand

jdstrand Jul 8, 2016

Contributor

I think this comment is not quite right as it applies to both when running as a snap and when running on the system, no?

interfaces/builtin/pulseaudio.go
+capability sys_nice,
+capability sys_resource,
+
+@{PROC}/self/exe r,
@jdstrand

jdstrand Jul 8, 2016

Contributor

This rule is likely not correct since self is a symlink. Can you try removing this rule? If you get a denial after removing it, use this instead: owner @{PROC}/@{pid}/exe r,

interfaces/builtin/pulseaudio.go
+network netlink raw,
+/sys/devices/virtual/dmi/id/sys_vendor r,
+/sys/devices/virtual/dmi/id/bios_vendor r,
+/run/udev/data/** r,
@jdstrand

jdstrand Jul 8, 2016 •
Edited 1 time

Contributor

I'll broken record this one-- we should use udev queries to obtain a specific list of accesses in /run/udev/data/**. For now, can you add a #FIXME: use udev queries to make this more specific?

interfaces/builtin/pulseaudio.go
+owner /{,var/}run/pulse/.config/pulse/ rwk,
+
+# Shared memory based communication with clients
+/{run,dev}/shm/pulse-shm-* rwkcix,
@jdstrand

jdstrand Jul 8, 2016

Contributor

'c' shouldn't be used and is covered by 'w'. You also shouldn't granted e'x'ec here either. Please use: /{run,dev}/shm/pulse-shm-* rwk, instead.

interfaces/builtin/pulseaudio.go
+owner /{,var/}run/pulse/native rwk,
+owner /{,var/}run/pulse/pid rwk,
+owner /{,var/}run/pulse/.config/ rwk,
+owner /{,var/}run/pulse/.config/pulse/ rwk,
@jdstrand

jdstrand Jul 8, 2016

Contributor

I think you should simplify this to simply be:

owner /{,var/}run/pulse/ rw,
owner /{,var/}run/pulse/** rwk,
interfaces/builtin/pulseaudio.go
+
+# Shared memory based communication with clients
+/{run,dev}/shm/pulse-shm-* rwkcix,
+owner /{,var/}run/pulse/.config/pulse/cookie rwk,
@jdstrand

jdstrand Jul 8, 2016

Contributor

This can be removed if you take my simplified rules suggestion, above.

@jdstrand
Contributor

jdstrand commented Jul 8, 2016

Please adjust the policy as I suggested just now. All other policy LGTM. Thanks!
interfaces/builtin/pulseaudio.go
)
-const pulseaudioConnectedPlugAppArmor = `
+var pulseaudioConnectedPlugAppArmor = []byte(`
+/{run,dev}/shm/pulse-shm-* rwk,
@jdstrand

jdstrand Jul 20, 2016

Contributor

On the plugs side, shouldn't we use this instead: owner /{run,dev}/shm/pulse-shm-* rwk, ?

interfaces/builtin/pulseaudio.go
+ b := bytes.NewBuffer(pulseaudioConnectedPlugAppArmorDesktop)
+ // Add these bytes to the connected plug apparmor rules
+ b.Write([]byte("owner /{,var/}run/user/*/pulse/ rwk,\n"))
+ b.Write([]byte("owner /{,var/}run/user/*/pulse/native rwk,\n"))
@jdstrand

jdstrand Jul 20, 2016

Contributor

Wouldn't this be better so concatenate pulseaudioConnectedPlugAppArmor and pulseaudioConnectedPlugAppArmorDesktop when OnClassic and continue to use only pulseaudioConnectedPlugAppArmor when not?

@morphis

morphis Jul 21, 2016

Contributor

@jdstrand Why not, that would match what we do for other interfaces.

@jdstrand

jdstrand Jul 21, 2016

Contributor

The difference with other interfaces is that they are different rules (if not, then that was a mistake). This is very clearly "always use these few rules and if on classic, also add these others", so the code should reflect that by putting the "always use these few rules" in pulseaudioConnectedPlugAppArmor and then the desktop only ones in pulseaudioConnectedPlugAppArmorDesktop. As it stands there is a small maintenance and auditibility issue since the /{,var/}run/user/*/pulse/ rules are duplicated in two quite different places.

interfaces/builtin/pulseaudio.go
+ b1.Write(b0.Bytes())
+ // Add these bytes to the connected plug apparmor rules
+ b1.Write([]byte("owner /{,var/}run/user/*/pulse/ rwk,\n"))
+ b1.Write([]byte("owner /{,var/}run/user/*/pulse/native rwk,\n"))
@jdstrand

jdstrand Jul 27, 2016

Contributor

Please move these two apparmor rules to the definition of pulseaudioConnectedPlugAppArmorDesktop that starts at line 36 to complete what I suggested (sorry if that wasn't clear before).

Once done, LGTM and +1.

@jdstrand
Contributor

jdstrand commented Jul 27, 2016

The security policy looks fine now. Thanks!

Assuming that the checks pass and Jim is happy with his testing, +1
@jhodapp

jhodapp commented Jul 28, 2016

This is ready for final review by @zyga.
@zyga
Contributor

zyga commented Jul 28, 2016

Thank you for contributing this improvement. I'm sorry I took forever to review it. LGTM
@zyga
Contributor

zyga commented Jul 28, 2016

Hmm, the spread test failure looks genuine. Let me have a look
@jhodapp

jhodapp commented Jul 29, 2016

@zyga any update on why the spread test failure is happening? I did not get any test failures when running the local check.
interfaces/builtin/pulseaudio.go
+
+func (iface *PulseAudioInterface) PermanentPlugSnippet(plug *interfaces.Plug, securitySystem interfaces.SecuritySystem) ([]byte, error) {
+ switch securitySystem {
+ case interfaces.SecurityDBus, interfaces.SecurityAppArmor, interfaces.SecuritySecComp, interfaces.SecurityUDev:
@morphis

morphis Aug 1, 2016

Contributor

Need to support interfaces.MountSecurity too now.

interfaces/builtin/pulseaudio.go
+ }
+ case interfaces.SecuritySecComp:
+ return pulseaudioConnectedPlugSecComp, nil
+ case interfaces.SecurityDBus, interfaces.SecurityUDev:
@morphis

morphis Aug 1, 2016

Contributor

Need to support interfaces.MountSecurity too now.

interfaces/builtin/pulseaudio.go
+ return pulseaudioPermanentSlotAppArmor, nil
+ case interfaces.SecuritySecComp:
+ return pulseaudioPermanentSlotSecComp, nil
+ case interfaces.SecurityDBus, interfaces.SecurityUDev:
@morphis

morphis Aug 1, 2016

Contributor

Need to support interfaces.MountSecurity too now.

interfaces/builtin/pulseaudio.go
+
+func (iface *PulseAudioInterface) ConnectedSlotSnippet(plug *interfaces.Plug, slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) {
+ switch securitySystem {
+ case interfaces.SecurityDBus, interfaces.SecurityAppArmor, interfaces.SecuritySecComp, interfaces.SecurityUDev:
@morphis

morphis Aug 1, 2016

Contributor

Need to support interfaces.MountSecurity too now.

interfaces/builtin/pulseaudio_test.go
+ snippet, err = s.iface.PermanentSlotSnippet(s.slot, interfaces.SecurityUDev)
+ c.Assert(err, IsNil)
+ c.Assert(snippet, IsNil)
+ snippet, err = s.iface.ConnectedPlugSnippet(s.plug, s.slot, interfaces.SecurityDBus)
@morphis

morphis Aug 1, 2016

Contributor

Please check for interfaces.MountSecurity as well.

interfaces/builtin/pulseaudio_test.go
+ c.Assert(err, Equals, interfaces.ErrUnknownSecurity)
+ c.Assert(snippet, IsNil)
+ snippet, err = s.iface.ConnectedSlotSnippet(s.plug, s.slot, "foo")
+ c.Assert(err, Equals, interfaces.ErrUnknownSecurity)
@morphis

morphis Aug 1, 2016

Contributor

Please check for interfaces.MountSecurity as well.

@morphis
Contributor

morphis commented Aug 1, 2016 •
Edited 1 time

@jhodapp The reason why the CI is failing is because the interface does not support the recently added MountSecurity subsystem. Its pretty clear in what it says:

error: cannot perform the following tasks:

  • Setup snap "ubuntu-core" (122) security profiles (cannot setup mount for snap "ubuntu-core": cannot obtain mount security snippets for snap "ubuntu-core": unknown security system)
  • Setup snap "ubuntu-core" (122) security profiles (cannot obtain mount security snippets for snap "ubuntu-core": unknown security system)

I've added a comment at the different places where you need to add interfaces.MountSecurity.

@jhodapp Also are you sure you executed the spread tests locally? If you just run run-checks it will not execute them by default unless you specify --spread
@jhodapp

jhodapp commented Aug 1, 2016

@morphis Are you sure that's what is required, or perhaps named something else? I see this both locally and while running with Travis here: "interfaces/builtin/pulseaudio.go:123: undefined: interfaces.MountSecurity"
@morphis
Contributor

morphis commented Aug 2, 2016

@zyga Can you have a final look and merge this?
@jhodapp

jhodapp commented Aug 3, 2016

@niemeyer just a reminder to take a look at this PR today if you can, approve if happy and merge.

@niemeyer niemeyer merged commit d20013f into snapcore:master Aug 5, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment