Merge lp:~michihenning/apparmor-easyprof-ubuntu/new-thumbnailer-methods into lp:apparmor-easyprof-ubuntu
| 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 | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jamie Strandboge | 2016-01-06 | Needs Information on 2016-01-19 | |
| Pat McGowan | 2016-01-15 | Pending | |
| James Henstridge | 2016-01-06 | Pending | |
|
Review via email:
|
|||
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-
Related MR: https:/
| Michi Henning (michihenning) wrote : | # |
| Michi Henning (michihenning) wrote : | # |
OK, we need both after all.
| Sebastien Bacher (seb128) wrote : | # |
could you change it to "work in progress" meanwhile so it's out of the sponsoring queue?
| Jamie Strandboge (jdstrand) wrote : | # |
Can max-backlog by abused by a malicious client? What if I set it to -1, 0, 1000000, etc?
| 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-
| 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.
| Jamie Strandboge (jdstrand) wrote : | # |
ACK, ok, so the read only bit is settled.
As for the stable-
| 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.
| 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).
| 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.
| 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.
| 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.
| 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.
| Jamie Strandboge (jdstrand) wrote : | # |
Pat, what are your thought on this for OTA10?
| 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?
| 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.)
| Marcus Tomlinson (marcustomlinson) wrote : | # |
Just for reference purposes, here is the bug about slow boot performance due to AppArmor policy recompilation: https:/
| 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?
| Jamie Strandboge (jdstrand) wrote : | # |
AIUI the other apparmor-
| Michi Henning (michihenning) wrote : | # |
How do I find a member of the touch release team?
| 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
Please hold this review for the moment. We'll definitely need TraceClient, but we may not need MaxBacklog.