[pulseaudio-discuss] [PATCH 0/5] Add access control to protocol-native

[David Henningsson]

On 2016-07-01 03:30, Ahmed S. Darwish wrote:
> Hi David,
>
> [ Thanks a lot for chiming back in! ]

Anytime :-)

>
> On Fri, Jun 24, 2016 at 05:21:53PM +0200, David Henningsson wrote:
>>
>> On 2016-06-24 02:30, Ahmed S. Darwish wrote:
>>> Hi,
>>>
>>> Apologies for resurrecting this year-old thread. [*] Hopefully we
>>> can finish this, with Flatpak integration as an access-control
>>> module, before v10.0 ;-) [1] [2] [3]
>>>
>>> Some questions below..
>>>
>>> On 2015-07-17 13:02, David Henningsson wrote:
>>>>
>>>> On 2015-04-07 17:13, Wim Taymans wrote:
>>>>> This is a new patch series that preplaces the previous patches regarding
>>>>> access control.
>>>>>
>>> ...
>>>>>
>>>>> There is an example access control module that shows how one could implement
>>>>> client specific access control checks.
>>>>>
>>>>
>>> ...
>>>>
>>>> A few design thoughts that I think we need to resolve first before
>>>> reviewing details:
>>>>
>>>> 1. Coupling between core and native-protocol. Right now the hooks are in
>>>> core, and mapped 1-to-1 (or? are there exceptions?) to PA_COMMAND_*.
>>>>
>>>> Other options would be:
>>>>     - Just have a "pa_hook access[PA_COMMAND_MAX]" in pa_core instead?
>>>> Then we could skip the long list of PA_ACCESS_HOOK_ constants. However,
>>>> the increased dependency between the native protocol and the pa_core
>>>> object might be undesirable.
>>>>     - On the other side, we could have the "pa_hook
>>>> access[PA_COMMAND_MAX]" in pa_native_protocol instead. However, native
>>>> protocol instances could - at least in theory - come and go, so they
>>>> probably need stored somewhere more reachable anyhow...
>>>>
>>>> To sum up, I feel that since the hooks are specific to the native
>>>> protocol, they should be put closer to that protocol, but I can't point
>>>> to a better place to put it.
>>>>
>>>
>>> After looking at this, isn't protocol-native a right place for
>>> the hooks? We're only protecting the protocol-native commands
>>> anyway.
>>>
>>> There's also a precedent in having hooks in pa_protocol_native.
>>> Namely at protocol-native.c:
>>>
>>>      struct pa_native_protocol {
>>>          PA_REFCNT_DECLARE;
>>>          pa_core *core;
>>>          ...
>>>          pa_hook hooks[PA_NATIVE_HOOK_MAX];  // <-- here
>>>          ...
>>>      };
>>>
>>>      pa_hook *pa_native_protocol_hooks(pa_native_protocol *p) {
>>>          ...
>>>          return p->hooks;
>>>      }
>>>
>>> And modules can get access to the hooks directly using the
>>> pa_native_protocol_hooks() accessor above. This is how it's done
>>> in module  device-manager, module device-restore, and module
>>> stream-restore, etc.
>>>
>>> Anything blocking us from doing the same for the access control
>>> hooks? This would indeed save us from the 1-to-1 mapping table.
>>>
>>> _ at diwic_: You mention that native protocol instances could at
>>> least in theory come and go. But if that happened, what would be
>>> the dangers of freeing the hooks along with protocol-native and
>>> be done with it? In a sense, the access-control hooks themselves
>>> won't make any sense without protocol-native anyway?
>>
>> Suppose the following chain of events:
>>
>>   1. Protocol-native loads
>>   2. Access control module loads, puts hooks into existing protocol-native
>> instance
>>   3. Protocol-native unloads
>>   4. Another Protocol-native loads
>>
>> At this point the new protocol-native is unprotected, which is probably not
>> what you want?
>>
>> Also we have to make sure we don't get use-after-free errors, no matter
>> which order the modules are unloaded. But that's certainly solvable, just an
>> additional thing to remember.
>>
>
> Can we solve the above two points by by introducing a pulse module
> dependencies feature, just like what the linux kernel does? [1]
>
> If so, we can do the following:
>
> (a) Introduce module dependencies
>
> (b) Put the access-control hooks inside protocol-native itself,
>      and thus remove the 1-to-1 mappings [2] and avoid making core
>      dependent on protocol-native (even implicitly)
>
> (c) Make all policy modules, which contains all the hooks
>      implementations, hard-dependent on protocol-native
>
> In the first point raised, to unload protocol-native, user will
> have to unload the dependent protocol-native-policy-X first. So if
> the user loads protocol-native again, he/she will know that the
> policy module will need to be reloaded as well ..
>
> In the second point raised, hopefully module dependencies will
> simplify the relationship: use-after-free errors can be eliminated
> given proper cleanup by the policy modules ..
>
> Any thoughts on this?
>

Just looking at core.h there seems to be both PA_CORE_HOOK_MODULE_NEW 
and PA_CORE_HOOK_MODULE_UNLINK hooks, perhaps it could be as simple as 
just requiring any access control module to listen to these and take the 
appropriate action?

As for whether or not a generic "module dependencies" feature would be 
good or bad: I would find it more user friendly myself if the user would 
not have to manually reload access control modules, but since I'm not 
that involved anymore, I don't think I should have decision power on 
that matter. :-)

> thanks,
>
> [1] http://man7.org/linux/man-pages/man5/modules.dep.5.html
>
> [2] For details on the 1-to-1 mapping, check the pa_access_hook
>      enumerations + these hooks placement inside struct pa_core,
>      at patch #3 and the map_table[] at patch #4
>