Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

actions: read actions from /etc/, /run/ and /usr/local/share/ too #499

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

bluca
Copy link
Member

@bluca bluca commented Sep 13, 2024

In order to allow adding services running from other images than the rootfs, read actions from /etc/polkit-1/actions too. This can happen with systemd services using RootImage= or so, which are not installed as packages and so their action files are not installed in /usr/, which might be read-only.

Fixes #180

This is an alternative to https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/148 that handles it in a different way. I do not mind which implementation is chosen, just providing an alternative.

@bluca bluca changed the title actions: read from /etc/polkit/actions too actions: read from /etc/polkit-1/actions too Sep 13, 2024
@bluca
Copy link
Member Author

bluca commented Sep 13, 2024

This is an alternative to https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/148 that handles it in a different way. I do not mind which implementation is chosen, just providing an alternative.

@jrybar-rh let me know if you prefer the approach from that older PR and if so I can forward-port it, I do not have an opinion on which one is better, so up to you, I can work with either

@jrybar-rh
Copy link
Member

This is an alternative to https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/148 that handles it in a different way. I do not mind which implementation is chosen, just providing an alternative.

@jrybar-rh let me know if you prefer the approach from that older PR and if so I can forward-port it, I do not have an opinion on which one is better, so up to you, I can work with either

I like the simplicity of your solution, i.e. duplicating all the operations for both directories. However, we're currently implementing reading .rules files from few more directories (counting 4 now). In case we need to read also action files from the same directories, incl. /run and /usr/local, it would make sense to use a cycle instead. Though I don't register any request to read those additional directories, but that's for now. Do you?

@bluca
Copy link
Member Author

bluca commented Sep 26, 2024

This is an alternative to https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/148 that handles it in a different way. I do not mind which implementation is chosen, just providing an alternative.

@jrybar-rh let me know if you prefer the approach from that older PR and if so I can forward-port it, I do not have an opinion on which one is better, so up to you, I can work with either

I like the simplicity of your solution, i.e. duplicating all the operations for both directories. However, we're currently implementing reading .rules files from few more directories (counting 4 now). In case we need to read also action files from the same directories, incl. /run and /usr/local, it would make sense to use a cycle instead. Though I don't register any request to read those additional directories, but that's for now. Do you?

I haven't come across those use cases so far, so maybe we can leave it for when it actually comes up?

@smcv
Copy link
Contributor

smcv commented Oct 1, 2024

https://bugs.debian.org/1010228 was a request for reading both rules and actions from /usr/local, so apparently use cases for that one do exist (the bug submitter's use-case seems to have been ./configure && make && sudo make install, or similar, for locally-installed software that integrates with polkit).

And if the search paths for rules and actions are already 75% similar (/etc + /usr/local + /usr), then I think it might make sense to make them completely consistent with each other even though there's no known use-case for /run/polkit-1/actions, just to make documentation clearer (and possibly be able to share more code, although I haven't looked at the implementation).

@bluca
Copy link
Member Author

bluca commented Oct 1, 2024

It seems likely that use case in that bug would be covered by /etc/ ? IE some local rule, not shipped by the package

@smcv
Copy link
Contributor

smcv commented Oct 1, 2024

It seems likely that use case in that bug would be covered by /etc/ ? IE some local rule, not shipped by the package

I don't think so? gnome-control-center installs e.g. ${prefix}/share/polkit-1/actions/org.gnome.controlcenter.datetime.policy and ${prefix}/share/polkit-1/rules.d/gnome-control-center.rules, and normally, you install gnome-control-center_*.deb (or other OSs' equivalents) into /usr, which puts those actions and rules into places where polkit will read them.

My interpretation of the Debian bug is that the bug submitter expected that if instead of installing the .deb, they do a make install (or equivalent) into /usr/local, both of those files would still be taken into account, without them needing to specify non-default paths.

@jrybar-rh
Copy link
Member

Well, to be fair, currently the default meson configuration sets prefix to /usr, so the built files shall be installed there, not /usr/local. But I like the idea of unifying the documentation and making it all simple (when we're doing it for .rules files) and by the standards (those set by systemd, sysctl etc.), i.e. we're loading whatever configuration files from this standard set of directories. @bluca, are you interested in reimplementing this to be more robust and "futureproof" and read also from the other directories?

@bluca
Copy link
Member Author

bluca commented Oct 2, 2024

ok, then I've cherry-picked the change from https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/148 and rebased it instead, and added /run/ and /usr/local/share/ together with /etc/

@bluca bluca changed the title actions: read from /etc/polkit-1/actions too actions: read actions from /etc/, /run/ and /usr/local/share/ too Oct 2, 2024
src/polkitbackend/polkitbackendactionpool.c Outdated Show resolved Hide resolved
&error);
if (monitor == NULL)
{
g_warning ("Error monitoring actions directory: %s", error->message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can divert a little from the original MR and use polkit_backend_authority_log() instead, so the failed monitors or loaded files (below) get into journal. g_warning is muted when --no-debug is used in .service file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But therer's no reference to the authority here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, that's true. I'll probably revise polkitbackend to see if authority is really needed in the logging function.

src/polkitbackend/polkitbackendactionpool.c Outdated Show resolved Hide resolved
As with the rules.d change, also read actions from /etc/,
/run/ and /usr/local/share/ before /usr/share/, in this order

Co-authored-by: Luca Boccassi <bluca@debian.org>
@jrybar-rh jrybar-rh merged commit 9958c25 into polkit-org:main Oct 9, 2024
33 checks passed
@bluca bluca deleted the actions_etc branch October 9, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add possibility to load actions from multiple directories
5 participants