Merge lp:~michihenning/apparmor-easyprof-ubuntu/new-thumbnailer-methods into lp:apparmor-easyprof-ubuntu

Proposed by Michi Henning
Status: Merged
Merged at revision: 57
Proposed branch: lp:~michihenning/apparmor-easyprof-ubuntu/new-thumbnailer-methods
Merge into: lp:apparmor-easyprof-ubuntu
Diff against target: 66 lines (+11/-4)
5 files modified
data/policygroups/ubuntu/1.0/audio (+1/-1)
data/templates/ubuntu/1.0/ubuntu-sdk (+1/-1)
data/templates/ubuntu/1.1/ubuntu-sdk (+1/-1)
data/templates/ubuntu/1.3/ubuntu-sdk (+1/-1)
debian/changelog (+7/-0)
To merge this branch: bzr merge lp:~michihenning/apparmor-easyprof-ubuntu/new-thumbnailer-methods
Reviewer Review Type Date Requested Status
Jamie Strandboge (community) Needs Information
Pat McGowan Pending
James Henstridge Pending
Review via email: mp+281712@code.launchpad.net

Commit message

Added TraceClient and MaxBacklog dbus methods to the list of allowed methods for applications using the thumbnailer.

Description of the change

Added ClientConfig dbus method to the list of allowed methods for applications using the thumbnailer. The method returns configuration settings that are required by our client API, namely, whether to write thumbnailer-related trace messages into the application log, and how many outstanding dbus requests will be sent by the client before they go into a queue.

Because confined apps cannot access gsettings, and because trying to control this via environment variables is really awkward, the easiest option was to add a dbus method to provide these values to the client. The method returns gsettings values we need on the client side (see man thumbnailer-settings (5)).

Related MR: https://code.launchpad.net/~michihenning/thumbnailer/config-access/+merge/281580

To post a comment you must log in.
Revision history for this message
Michi Henning (michihenning) wrote :

Please hold this review for the moment. We'll definitely need TraceClient, but we may not need MaxBacklog.

Revision history for this message
Michi Henning (michihenning) wrote :

OK, we need both after all.

Revision history for this message
Sebastien Bacher (seb128) wrote :

could you change it to "work in progress" meanwhile so it's out of the sponsoring queue?

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Can max-backlog by abused by a malicious client? What if I set it to -1, 0, 1000000, etc?

review: Needs Information
Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Oh, I think that MaxBacklog is simply a get method, correct? If the client can't set it, there is no problem of course.

In general, I have no problem with adding these accesses, however because of the files they touch, it means every SDK app (ubuntu-sdk change) and webapp (due to audio policy group change) will need a policy recompile, and thus slow down the first reboot after upgrade. If after you confirm MaxBacklog is simply a get method, I will apply this to 16.04.

Pat can you comment on if you are willing to take this change in stable-phone-overlay?

review: Needs Information
Revision history for this message
Michi Henning (michihenning) wrote :

Yes, MaxBacklog is read-only (as is TraceClient). It can't be negative or zero either. (The config parsing we do prevents that.)

I cannot think of any potential for abuse here. No sensitive information is leaked. The parameter simply tells the client-side API how many async dbus requests it should send before queuing additional requests on the client side, and whether to turn on debug trace for the client-side API.

We need this change in stable phone overlay too. Without it, we can't turn on client-side tracing for debugging.

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

ACK, ok, so the read only bit is settled.

As for the stable-phone-overlay, apps/scopes typically use the 'debug' policy for debugging. This policy group does not currently allow these proposed dbus methods. Are both MaxBacklog and TraceClient only used for debugging? Would it make sense to add them to the debug policy group instead? (This would be fine for a stable-phone-overlay because apps aren't supposed to ship with the debug policy group so there is little (if not nothing) that would need to be recompiled.

review: Needs Information
Revision history for this message
Michi Henning (michihenning) wrote :

MaxBacklog is a tunable parameter that affects the internals of the client API. It's not necessarily used only for debugging.

TraceClient is sort of like debug. When turned on, we write log messages about API activity into the application log (well, stderr, really).

I don't really know anything about a debug policy group. (This is the first time I've heard of it.)

Basically, where this MR is coming from is that we have settings in gsettings, but the client API can't get at the settings because apparmor denies access. So, the easiest thing was to have the server read the settings values and feed them to the client, so the client-side API can configure itself appropriately.

I think it might be easiest to use the MR as is? I added the methods to all the groups that currently need access to the thumbnailer. It seemed easiest to go that way because the new methods are on the same interface, and anything that uses the thumbnailer needs to call them.

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Ok. Sure, I understand why you put the accesses where you did. The problem is the effect it will have on first reboot after upgrade since your changes will require virtually all security policy on the device to be recompiled which can take a long time with the number of profiles that are on the device. We try to avoid that whenever possible and this sort of thing must be approved by the Touch release team (hence the request for Pat to comment).

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Actually, this would require policy regeneration for all SDK apps and webapps, but only scopes that use the 'audio' policy group.

Revision history for this message
Michi Henning (michihenning) wrote :

Pat, could you approve this for OTA-10? It would really help us to be able to turn on client-side trace without having to recompile and push new packages to the phone.

Revision history for this message
Michi Henning (michihenning) wrote :

Putting this on hold for the moment. I didn't know that updating the security policy is such a big deal. So, rather than having one method per config param, I'll make it one method for several config params. That way, the change to the policy is needed only once, instead of each time we come with a new config param that we need to feed to the client-side lib.

Revision history for this message
Michi Henning (michihenning) wrote :

I've updated the change to require access to only a single method, which we use to get at whatever client-side configuration we need to provide.

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Pat, what are your thought on this for OTA10?

Revision history for this message
Pat McGowan (pat-mcgowan) wrote :

Why do the clients need to specify these values rather than having the server simply do this?
How are the values configured? Do they ever change?

Revision history for this message
Michi Henning (michihenning) wrote :

The clients don’t specify the values. The values live in gsettings. Some of the values are relevant to the client-side library, such as whether to write additional trace. The problem is that confined apps cannot read gsettings. So, the only solution is to have the server provide those config values that are relevant to the client side over DBus. The client only has read-only access to the values, and can read only those values that the server provides.

The values need to change when we have to debug things and for performance tuning. Without this, we can’t just tell someone “change this gsetting, and send us the log”, for example. It also makes it much easier for us to diagnose problems caused by misbehaving applications because, with trace on the client side, we can correlate the interchange of messages between the application and the thumbnailer. (And we can't leave the trace on permanently because that would hopelessly spam the application log.)

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

Just for reference purposes, here is the bug about slow boot performance due to AppArmor policy recompilation: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1350598

Revision history for this message
Michi Henning (michihenning) wrote :

Is there a chance to get this bundled in with the next OTA? I seem to remember some other apparmor updates making it into that?

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

AIUI the other apparmor-easyprof-ubuntu changes landed before the OTA-13 cutoff. If you can get a member of the Touch release team to approve this for OTA-13 I'd be happy to land it in the branches, and then someone could take those through the silo process.

Revision history for this message
Michi Henning (michihenning) wrote :

How do I find a member of the touch release team?

Revision history for this message
kevin gunn (kgunn72) wrote :

So Pat is out, in his stead I will weigh in and say I think it would be a good idea to fold this into OTA13 since we know there are other AA recompiles happening and we may as well do add it in for this. So I support trying to land this in OTA13

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/policygroups/ubuntu/1.0/audio'
2--- data/policygroups/ubuntu/1.0/audio 2015-02-03 22:08:27 +0000
3+++ data/policygroups/ubuntu/1.0/audio 2016-02-24 03:35:17 +0000
4@@ -38,7 +38,7 @@
5 dbus (send)
6 bus=session
7 path="/com/canonical/Thumbnailer"
8- member={GetAlbumArt,GetArtistArt}
9+ member={GetAlbumArt,GetArtistArt,ClientConfig}
10 peer=(label=unconfined),
11
12 # Allow communications with mediascanner2
13
14=== modified file 'data/templates/ubuntu/1.0/ubuntu-sdk'
15--- data/templates/ubuntu/1.0/ubuntu-sdk 2015-07-08 14:08:47 +0000
16+++ data/templates/ubuntu/1.0/ubuntu-sdk 2016-02-24 03:35:17 +0000
17@@ -361,7 +361,7 @@
18 bus=session
19 path="/com/canonical/Thumbnailer"
20 interface="com.canonical.Thumbnailer"
21- member="GetThumbnail"
22+ member={GetThumbnail,ClientConfig}
23 peer=(label=unconfined),
24
25 #
26
27=== modified file 'data/templates/ubuntu/1.1/ubuntu-sdk'
28--- data/templates/ubuntu/1.1/ubuntu-sdk 2015-07-08 14:08:47 +0000
29+++ data/templates/ubuntu/1.1/ubuntu-sdk 2016-02-24 03:35:17 +0000
30@@ -360,7 +360,7 @@
31 bus=session
32 path="/com/canonical/Thumbnailer"
33 interface="com.canonical.Thumbnailer"
34- member="GetThumbnail"
35+ member={GetThumbnail,ClientConfig}
36 peer=(label=unconfined),
37
38 #
39
40=== modified file 'data/templates/ubuntu/1.3/ubuntu-sdk'
41--- data/templates/ubuntu/1.3/ubuntu-sdk 2015-11-19 21:20:24 +0000
42+++ data/templates/ubuntu/1.3/ubuntu-sdk 2016-02-24 03:35:17 +0000
43@@ -360,7 +360,7 @@
44 bus=session
45 path="/com/canonical/Thumbnailer"
46 interface="com.canonical.Thumbnailer"
47- member="GetThumbnail"
48+ member={GetThumbnail,ClientConfig}
49 peer=(label=unconfined),
50
51 #
52
53=== modified file 'debian/changelog'
54--- debian/changelog 2016-02-23 22:58:39 +0000
55+++ debian/changelog 2016-02-24 03:35:17 +0000
56@@ -1,3 +1,10 @@
57+apparmor-easyprof-ubuntu (16.04.5) UNRELEASED; urgency=medium
58+
59+ [ Michi Henning ]
60+ * Added ClientConfig to permitted dbus calls for apps using the thumbnailer.
61+
62+ -- Michi Henning <michi.henning@canonical.com> Wed, 24 Feb 2016 13:32:48 +1000
63+
64 apparmor-easyprof-ubuntu (16.04.4) xenial; urgency=medium
65
66 [ Jamie Strandboge ]

Subscribers

People subscribed via source and target branches