interfaces/builtin,docs,snap: add the pulseaudio interface #1133

Merged
merged 9 commits into from May 23, 2016

Conversation

Projects
None yet
5 participants
Contributor

zyga commented May 5, 2016

This patch adds Classic-only interface for pulseaudio. This interface
allows applications with connected slots to talk to the pulseaudio
socket and to access the shared memory files created by pulse.

The new interface is implicitly added to the OS snap, it is also
automatically connected to clients.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

interfaces/builtin,docs,snap: add the pulseaudio interface
This patch adds Classic-only interface for pulseaudio. This interface
allows applications with connected slots to talk to the pulseaudio
socket and to access the shared memory files created by pulse.

The new interface is implicitly added to the OS snap, it is also
automatically connected to clients.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin/pulseaudio.go
+const pulseaudioConnectedPlugAppArmor = `
+/etc/pulse r,
+/etc/pulse/* r,
+/dev/shm/pulse-shm-* mrwlkix,
@jdstrand

jdstrand May 5, 2016

Contributor

Please use 'rk' instead. Touch policy doesn't require 'w' and the 'mlix' accesses are wrong.

@jdstrand

jdstrand May 5, 2016

Contributor

Also, use: '/{run,dev}/' instead of '/dev/'. Some Personal devices will use /run/ instead.

@zyga

zyga May 5, 2016

Contributor

Done

interfaces/builtin/pulseaudio.go
+)
+
+const pulseaudioConnectedPlugAppArmor = `
+/etc/pulse r,
@jdstrand

jdstrand May 5, 2016

Contributor

Please use (trailing '/' is important):
/etc/pulse/ r,

@zyga

zyga May 5, 2016

Contributor

Done

interfaces/builtin: tweak apparmor policy for pulseaudio
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin/pulseaudio.go
+/etc/pulse r,
+/etc/pulse/* r,
+/dev/shm/pulse-shm-* mrwlkix,
+owner /run/user/*/pulse/native rw,
@jdstrand

jdstrand May 5, 2016

Contributor

This should be:
owner /{,var/}run/user/*/pulse/ r,
owner /{,var/}run/user/*/pulse/native rwk,

On Touch we also had:
owner /{,var/}run/user/*/pulse/ w, # shouldn't be needed, but rmdir fail otherwise
but let's hold off on that write access for the directory until we know we need it.

@jdstrand

jdstrand May 5, 2016

Contributor

Honestly, I'm surprised you don't also need:
owner @{HOME}/.pulse/ r,
owner @{HOME}/.pulse/* rk,

@zyga

zyga May 5, 2016

Contributor

$HOME is the snap's HOME directory

zyga added some commits May 5, 2016

interfaces/builtin: tweak apparmor policy for pulseaudio
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: tweak apparmor policy for pulseaudio
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: auto-connect pulseaudio
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
docs/interfaces.md
+media application.
+
+Usage: common
+Auto-Connect: yes
@jdstrand

jdstrand May 5, 2016

Contributor

Note that this is actually pretty dangerous because it means that any app can be uploaded, be installed, be autoconnected, run in the background and record audio without the user having any idea it is happening. On Touch pulseaudio was adjusted to integrate with trust-store and prompt the user when an application tried to record audio (playback was always allowed) and this feature was borne out of both security team and customer requirements. This works well on Touch because there is a guaranteed UI that can be used, the user is in full control via System Settings and there are no background services on Touch.

Snappy changes all of that-- the pulseaudio on classic doesn't have the trust-store integration (though it could) and snappy allows background services and there may be no UI for IoT systems. IMO for this to be auto-connectable it should be limited to playback only, which will require an SRU for pulseaudio on classic to disallow recording if the security label of the connecting process starts with "snap.". The patch to Touch's stable-phone-overlay should be adaptable enough for this.

That buys us time regarding how to design safe recording (perhaps it is a different interface or an attribute of the pulseaudio interface, which could be implemented as on interface connect/disconnect for recording, snapd updates pulseaudio's trust-store, then much of the existing code could be reused. Other implementations are possible).

@zyga

zyga May 5, 2016

Contributor

I think this is reasonable. We should perhaps consider SRU-ing the trust store as well so that applications like firefox can use this for media recording (hangouts, certain type of games, etc).

@niemeyer

niemeyer May 5, 2016

Contributor

Just for pondering, if we wanted to split the recording from the playback interface, how could we do that?

@jdstrand

jdstrand May 5, 2016

Contributor

@niemeyer (cc @vosst ) - this is what I was getting at in my last paragraph: "That buys us time regarding how to design safe recording (perhaps it is a different interface or an attribute of the pulseaudio interface, which could be implemented as on interface connect/disconnect for recording, snapd updates pulseaudio's trust-store, then much of the existing code could be reused. Other implementations are possible)."

To put more directly, we need pulseaudio to make a choice on whether to allow playback and/or recording. The patch for Touch does this by (in essence):

  1. allowing anyone to playback
  2. if recording, using libapparmor to get the security label
  3. cross-checking that security label with trust-store (a dynamically linked library)
  4. checking trust-store to see if the security-label exists in the db. If not, prompt the user to see if this app should be allowed to record and save the answer, otherwise go to the next step
  5. trust-store checking the user's response and allows/disallows recording based on that

(the user can later adjust the microphone settings via System Settings)

Considering the above, to support splitting recording from playback, pulseaudio needs to either be told what is allowed to record or query the system on what is allowed to record.

For the first and with the current pulseaudio trust-store patchset, it should be relatively straightforward for, upon record interface connect, snapd to add an entry to pulseaudio's trust-store db (an sqlite db iirc) to say the app may record, and upon disconnect to say the app may not record. We would probably upon install of the snap also want to populate pulseaudio's trust-store with 'may not record' for this snap so we would avoid prompting (since we won't autoconnect).

Alternatively we can turn this around and have pulseaudio still use trust-store, but adjust trust-store to talk to snapd to ask if the client process' snap is connected or not. Or we replace trust-store with a small library that talks to snapd.

Both approaches have their merits. The first approach is probably faster, doesn't require a new snapd API and won't require giving pulseaudio access to the sensitive snapd-control interface. The 2nd approach feels cleaner but would require careful design so that snaps providing slots like this don't have access to the entire rest API.

I think it is important to understand that the path chosen here will have a direct impact on Ubuntu Personal, since many 'trusted helpers' on Touch are like pulseaudio and use trust-store in some manner. It should also be understood that there are customer requirements for Touch/Personal that state there must be a UI prompt for first time access (in the manner implemented) for microphone, video recording, and location, so while right this second we wouldn't have to implement a UI prompt, the design should support this going forward. I highly recommend talking to tvoss since he is the technical architect for Touch/Personal, understands the requirements for it and designed trust-store and its prompting.

@niemeyer

niemeyer May 18, 2016

Contributor

Thanks for these insights, @jdstrand. I'm tempted to actually move on with a simpler pulseaudio interface that allows both playback and recording, and leave the recording constraint for later. But we should at least understand how we want that to work in the future so we're not putting ourselves in a corner. I will talk to tvoss before moving forward.

@jdstrand

jdstrand May 18, 2016

Contributor

@niemeyer - @morphis created a PoC pulseaudio plugin that examines the apparmor label of the connecting process and if it starts with 'snap.', disallow recording. Doing this in the immediate term would satisfy the security concerns until the proper mediation for recording is available.

zyga added some commits May 5, 2016

interfaces/builtin: adjust tests for pulseaudio
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/ifacestate: adjust tests for pulseaudio
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap: adjust tests for pulseaudio
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
+const pulseaudioConnectedPlugAppArmor = `
+/etc/pulse/ r,
+/etc/pulse/* r,
+/{run,dev}/shm/pulse-shm-* rk,
@niemeyer

niemeyer May 5, 2016

Contributor

Let's please figure what we want to do about those before moving this interface in.

@niemeyer

niemeyer May 18, 2016

Contributor

This has been discussed and sorted at the sprint. The agreement is to move on with this as-is, using the native path format, and also to open up tag-prefixed read/writes within the process itself so that applications have a way to be unblocked for their own needs.

In other words, cross-application will now require interfaces, which matches with our practices, but internal usage can be properly confined without talking to anyone by using the proper path.

@jdstrand

jdstrand May 18, 2016

Contributor

IIUC you are referring to the shm accesses and I believe what you outlined here is allow this shm access for pulseaudio and to also allow what is in ubuntu-core#1135. If not, please respond in whichever PR you feel is the correct place to respond. :)

Contributor

zyga commented May 18, 2016

Simon has now patched pulseaudio to disable recording. We are closer to being able to land this for everyone: https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/1583057

@chipaca chipaca added the Blocked label May 18, 2016

Contributor

jdstrand commented May 19, 2016

@zyga and @morphis - great news on disabling recording. @niemeyer and I discussed this in IRC today and feel that pulseaudio interface should not be blocked on the pulseaudio SRU. Instead, commit it then land pulseaudio SRU soon. This way everyone gets playback now, recroding gets blocked soon and then we can design a pulseaudio-record interface or 'record' attribute in the fullness of time.

@niemeyer niemeyer added Reviewed and removed Blocked labels May 19, 2016

Contributor

morphis commented May 20, 2016

@jdstrand Sounds good to me. I also have a first shot on getting the slot side of this ready. Will push a PR on top of this in a bit.

Merge remote-tracking branch 'upstream/master' into iface-pulseaudio
I also took the opportunity to change ifacestate_test.go to have a less
precise test so that each new interface doesn't have to update that file
or resolve useless conflicts.

@zyga zyga merged commit c6a8f93 into snapcore:master May 23, 2016

4 checks passed

Integration tests Success
Details
autopkgtest Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 79.24%
Details

@zyga zyga deleted the zyga:iface-pulseaudio branch May 23, 2016

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