Re: [PATCH 05/10] Allow colord_t to read the color profile stored in ~/.local/share/icc/

[Laurent Bigonville <bigon@debian.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3526 bytes --]

Le 12/10/19 à 18:09, Dominick Grift a écrit :
> On Sat, Oct 12, 2019 at 11:51:43AM -0400, Chris PeBenito wrote:
>> On 10/12/19 3:53 AM, Dominick Grift wrote:
>>> On Fri, Oct 11, 2019 at 02:54:23PM +0200, Dominick Grift wrote:
>>>> On Fri, Oct 11, 2019 at 02:24:11PM +0200, Laurent Bigonville wrote:
>>>>> From: Laurent Bigonville <bigon@bigon.be>
>>>>>
>>>>> colord reads the color profiles files that are stored in
>>>>> ~/.local/share/icc/, The file descriptor to that file is passed over
>>>>> D-Bus so it needs to be inherited
>>>> This patch is cutting corners a little. It only takes unconfined_t into account and not the confined users (an alternative would be to call "userdom_use_all_users_fds(colord_t)" instead. Which is arguable too broad as well but closest you can get to "common users" without surgery.
>>>> Secondly xdg_read_data_files() is a little broad.
>>>> Also if this patch implies that whatever maintains XDG_DATA_DIR/icc is able to maintain generic xdg data files, which is arguable broad as well.
>>>>
>>>> The second and third argument are subject to how far you want to take things, and so I won't object if that is not addressed.
>>>> The fd use issue, in my view, should be addressed for all login (common) users with colord access.
>>> Actually, I take this review back. I am not sure how to best deal with this fd.
>> It seems that going to a colord_role() would be the way to go.  There
>> already is a colord_dbus_chat($1_t) in userdomain.if, so you could put those
>> dbus rules plus the rules to address the fds together.
>>
>> I agree the xdg_read_data_files() is somewhat broad, but it seems like
>> xdg_data_t files aren't sensitive.  Maybe that's just how it is on system?
>> I don't feel strongly on this.
> Yes it depends i guess. The thing is that like /usr theres really all kinds of things below  ~/.local, like bin, lib, doc etc (pip for example installs to ~/.local/{bin,lib}).
> So I would surely at least consider that beforehand
>
> ls -aZ ~/.local/
> wheel.id:wheel.role:users.generic_home_data.home_data_file:s0 .            wheel.id:wheel.role:users.home_libraries.home_file:s0 lib
>                     wheel.id:wheel.role:users.home_dir.file:s0 ..   wheel.id:wheel.role:users.generic_home_data.home_data_file:s0 share
>           wheel.id:wheel.role:users.home_commands.home_file:s0 bin
>
> There's also other gotchas, take for example your personal libvirt pool in ~/.local, this content may potentially also be need to be accessible by the qemu user.
>
> I guess what i am saying is that not everything below /usr is always just "data"
>
> I dont have enough experience with colord to give advice, looking at my policy there's also a colord --user instance, it seems also heavily integrated with gnome-settings-daemon.
>
> I think this patch is probably alright as is for now (maybe its best to just ignore confined users in this stage) as for further partitioning ~/.local, i suppose we can alway's revisit these changes later as this only applies to ~/.local/share/icc anyway.
>
> If this change is one of the few controversial changes that are needed to make gnome work on debian with unconfined, then i think it might be worth it to just accept this and make a note about it to address this properly when someone wants to work on the confined support for this aspect.

This patch is not "the last patch to make GNOME work", there are still 
other stuff to fix, so we might want to take some time to do that properly.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]