[pulseaudio-discuss] Access control

[Ahmed S. Darwish]

Hi!

On Fri, Jan 27, 2017 at 05:15:29PM +0100, Wim Taymans wrote:
> Hi All,
> 
> I took another look at the access control patches.
>

Thanks a lot for resuming the original work on this; didn't have
the time here :-)

> There was a desire from Arun and Tanu to have the access control
> checks more integrated
> into the core objects instead of just some checks in the native protocol.
> 
> I was a bit reluctant to start this because it would involve passing
> around a lot of pa_client
> objects to many functions in order to do access control. This week, I
> put my brain to 0 and
> started this anyway, You can find the result in this branch:
> 
> (1)  https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=access-hooks-arg
> 
> I then spent some time thinking about using TLS to pass the client
> around, more like a context
> to evaluate the functions in than an argument of the function. Tanu
> then suggested to put the
> current_client in the pa_core object where it can be picked up from
> anywhere. The trick is then
> to set and clear the current_client in the various protocols. The
> result can be seen here:
> 
> (2) https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=access-hooks-core
>
> You can still find the old way here:
> 
> (3) https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=access-hooks
> 
> I think (1) is quite ugly and requires you to modify many functions
> with a new parameter that
> is not at all related to what the function does. It does however give
> you good control over what
> you can check. One example is pa_sink_update_rate() which might
> suspend and unsuspend
> the source to implement the rate change. It a new client does not have
> permission to suspend a
> sink, it will not be able to change the rate.
> 
> (2) looks quite ok. Setting and clearing the current_client is
> straightforward and easily verifiable.
> You can also be certain that when you add new checks, you can just get
> the client from there
> instead of having to change a method. The downside is that if ever a
> non-main thread tries to
> do some access control, you get random behaviour. using TLS will, of
> course fix this.
> 
> (3) is probably still the most simple solution but only deals with the
> native-protocol. (1) and (2)
> now also automatically work for the cli, DBus, esound, http, simple
> and native protocol.
> 
> What do you think? Which option do you like best?

IMHO option (2) sounds like the most generic and forward-looking.
Pretty nice to access the client object from everywhere -- similar
to the kernel's code pattern where any context can access the
current (per-CPU) process context through 'current'.

Since PA is mostly eventloop-driven, except in the RT I/O threads,
maybe the threading issue is not a big blocker for that choice?

After some thinking, I had some worries about any code using
pa_threaded_mainloop. Turns out it's mostly used to maintain a
synchronous feeling for clients; and in module-zeroconf-public to
deal with libavahi-client synchronous calls away from the main
eventloop.

Unless I'm misunderstanding something, we can also have both, a
minimal TLS boolean flag and pa_core_get_current_client()? [*]
The current-client accessor would just complain loudly if it
discovers that it's called away from the main thread (TLS
boolean flag is false).

regards,

[*] https://cgit.freedesktop.org/~wtay/pulseaudio/commit/?h=access-hooks-core&id=35fd11d9c85042ceb26c73f72c39b09e9bf47a2e

-- 
Darwish
http://darwish.chasingpointers.com