Allow D-Bus methods to auth. for more than one polkit action #26365

dtardon · 2023-02-08T19:54:26Z

The current code handling async. polkit authentication allows a D-Bus method to auth. for at most one action. If the same method tries to auth. for another action, it'll end with -ESTALE. That's a problem for verify_shutdown_creds(), which may need to authenticate for both org.freedesktop.login1.hibernate-multiple-sessions and org.freedesktop.login1.hibernate-ignore-inhibit. This PR extends the handling of async. polkit requests to allow auth. for multiple actions. Unfortunately, it increases complexity of already quite complex code (esp. wrt. memory management)...

Fixes #20155

github-actions · 2023-02-08T20:00:38Z

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

src/shared/bus-polkit.c

yuwata · 2023-02-22T13:09:33Z

src/shared/bus-polkit.c

@@ -379,6 +473,20 @@ int bus_verify_polkit_async(
        if (r < 0)
                return r;

+        if (!qs) {


Is this condition necessary?

It is. On the first call of this function (for given call), qs will be NULL.

I mean, here qs is always NULL, no?

No. When a dbus method needs to auth. for a polkit action, it calls bus_verify_polkit_async(). The function creates an AsyncPolkitQueries instance (and stores it into registry), an AsyncPolkitQuery instance, and issues an async call to polkit. The dbus method then returns immediately after returning from bus_verify_polkit_async(). When the async call finishes, async_polkit_callback() gets called, which stores the reply in its associated AsyncPolkitQuery object and reschedules the same bus message for processing. This time, when bus_verify_polkit_async() is called, it finds that a query for this call + action combination is already present in registry, hence it just processes the reply and returns. If the method then needs to auth. for another action, it calls bus_verify_polkit_async() again. This time, no existing query is found in the registry (it's a different action than previously), but the AsyncPolkitQueries is (as we're still processing the same bus message), hence qs is set. The function then proceeds with creating another query (so qs contains two items now, each for a different action) and issues another async call to polkit. The dbus method again returns and is later rescheduled by async_polkit_callback(). And so on...

I hope this is at least a bit understandable...

(Btw, if this sounds complicated, it's because it is :-) But most of the complexity is due to the async. nature of this and has already been there before.)

yuwata · 2023-02-22T13:11:56Z

src/shared/bus-polkit.c


        r = sd_bus_call_async(call->bus, &q->slot, pk, async_polkit_callback, q, 0);
-        if (r < 0) {
-                async_polkit_query_free(q);


Why this is dropped?? q seems to be leaked.

It's not leaked. At this point, q is already managed by qs.

Ah, then, I'd like to move LIST_PREPEND in the above after the bus call.

yuwata · 2023-02-22T13:13:31Z

src/shared/bus-polkit.c

+        if (!qs)
+                return NULL;
+
+        LIST_FOREACH(item, i, qs->items)


How about to store query into a hashmap by the action?

It's not worth it. In most cases there'll be only one item in the list.

src/shared/bus-polkit.c

dtardon · 2023-05-05T08:26:00Z

I've added a detailed (and hopefully understandable) explanation how the polkit auth. works. I hope that makes the change easier to understand.

poettering · 2023-06-14T08:58:45Z

hmm, can you split out the first three commits in a PR of its own? we should be able to merge those as generally useful cleanups independently of the rest, once they pass CI.

poettering · 2023-06-14T09:01:40Z

hmm, what I don't get about this, wouldn't it suffice to add a new strv field to AsyncPolkitQuery that lists all actions that have already been validated in the current method handling? Then, when we can just check against those whenever we are called, and only do requests for the stuff we haven't validated yet.

dtardon · 2023-06-16T07:52:23Z

hmm, can you split out the first three commits in a PR of its own? we should be able to merge those as generally useful cleanups independently of the rest, once they pass CI.

Will do.

dtardon · 2023-06-16T08:24:59Z

hmm, what I don't get about this, wouldn't it suffice to add a new strv field to AsyncPolkitQuery that lists all actions that have already been validated in the current method handling? Then, when we can just check against those whenever we are called, and only do requests for the stuff we haven't validated yet.

Does "I haven't thought of that" count as an answer? .-)

bluca · 2023-07-11T15:50:54Z

@dtardon this is one of the last remaining few RC1 blockers, will you have time to work on this today or tomorrow? If not, it's ok, but we might bump to the next release

There are no user facing stuff, so actually it's fine to get in a bit later for 254 if you have time to get the review comments sorted this week or next week @dtardon

dtardon · 2023-07-13T14:23:26Z

Btw, #28381 is my attempt to create a test for polkit authentication. Unfortunately, it's not ready yet: the script runs just fine locally, but it always fails (at different places) when I run it as a test. And I have no idea why...

This is a preparation for later commits.

... and store just the result.

In systemd#20155, verify_shutdown_creds() needs to authenticate for both org.freedesktop.login1.hibernate-multiple-sessions and org.freedesktop.login1.hibernate-ignore-inhibit . Previously, the second authentication attempt would fail with -ESTALE. Fixes systemd#20155.

poettering · 2023-08-09T08:35:57Z

src/shared/bus-polkit.c

@@ -214,7 +220,11 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) {

        sd_event_source_disable_unref(q->defer_event_source);

-        async_polkit_query_action_free(q->authorized_action);
+        while ((a = q->authorized_actions)) {


…failure As reported by [1] [2], the return code of systemctl command between systemd 253 and 254 has changed when the polkit authentication is refused: /bin/systemctl restart systemd-timesyncd.service The return code changed from 1 to 4. The Polkit test case "TestPolkitSystemd" expected 1 as return code [3]. The service log is not the same either: systemd v253: Failed to restart systemd-timesyncd.service: Interactive authentication required. systemd v254: Failed to restart systemd-timesyncd.service: Access denied git bisect report this commit: systemd/systemd@959301c From the PR (to get more context): systemd/systemd#26365 Note: systemd doesn't recommend using systemctl exit code to check unit states: "The mapping of LSB service states to systemd unit states is imperfect, so it is better to not rely on those return values but to look for specific unit states and substates instead." Since we only want to check if the command failed, update our test to check if systemctl returned a non zero code whatever the reason of the failure. Thanks to Yann E. MORIN for the brainstorming! Fixes: https://gitlab.com/buildroot.org/buildroot/-/jobs/4768561464 (TestPolkitSystemd) [1] http://lists.busybox.net/pipermail/buildroot/2023-August/671900.html [2] https://lists.freedesktop.org/archives/systemd-devel/2023-August/049362.html [3] https://git.buildroot.net/buildroot/tree/support/testing/tests/package/test_polkit.py?h=2023.08-rc1#n45 [4] https://github.com/systemd/systemd/blob/v254/man/systemctl.xml#L2612 Signed-off-by: Romain Naour <romain.naour@smile.fr> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

An error reply from polkit is a valid case and should not be propagated as failure of async_polkit_callback(). It should only be saved here. It'll be returned by bus_verify_polkit_async() later, when it's called for the same method again. Follow-up for systemd#26365.

An error reply from polkit is a valid case and should not be propagated as failure of async_polkit_callback(). It should only be saved here. It'll be returned by bus_verify_polkit_async() later, when it's called for the same method again. Follow-up for #26365.

…failure As reported by [1] [2], the return code of systemctl command between systemd 253 and 254 has changed when the polkit authentication is refused: /bin/systemctl restart systemd-timesyncd.service The return code changed from 1 to 4. The Polkit test case "TestPolkitSystemd" expected 1 as return code [3]. The service log is not the same either: systemd v253: Failed to restart systemd-timesyncd.service: Interactive authentication required. systemd v254: Failed to restart systemd-timesyncd.service: Access denied git bisect report this commit: systemd/systemd@959301c From the PR (to get more context): systemd/systemd#26365 Note: systemd doesn't recommend using systemctl exit code to check unit states: "The mapping of LSB service states to systemd unit states is imperfect, so it is better to not rely on those return values but to look for specific unit states and substates instead." Since we only want to check if the command failed, update our test to check if systemctl returned a non zero code whatever the reason of the failure. Thanks to Yann E. MORIN for the brainstorming! Fixes: https://gitlab.com/buildroot.org/buildroot/-/jobs/4768561464 (TestPolkitSystemd) [1] http://lists.busybox.net/pipermail/buildroot/2023-August/671900.html [2] https://lists.freedesktop.org/archives/systemd-devel/2023-August/049362.html [3] https://git.buildroot.net/buildroot/tree/support/testing/tests/package/test_polkit.py?h=2023.08-rc1#n45 [4] https://github.com/systemd/systemd/blob/v254/man/systemctl.xml#L2612 Signed-off-by: Romain Naour <romain.naour@smile.fr> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

An error reply from polkit is a valid case and should not be propagated as failure of async_polkit_callback(). It should only be saved here. It'll be returned by bus_verify_polkit_async() later, when it's called for the same method again. Follow-up for systemd#26365. (cherry picked from commit 45b1c01)

github-actions bot added util-lib please-review PR is ready for (re-)review by a maintainer labels Feb 8, 2023

yuwata added this to the v254 milestone Feb 10, 2023

dtardon force-pushed the multiple-polkit-calls branch from 6f03519 to bcf3e64 Compare February 10, 2023 11:38

This comment was marked as outdated.

Sign in to view

yuwata reviewed Feb 22, 2023

View reviewed changes

src/shared/bus-polkit.c Outdated Show resolved Hide resolved

yuwata reviewed Feb 22, 2023

View reviewed changes

yuwata requested changes Feb 22, 2023

View reviewed changes

yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Feb 22, 2023

dtardon force-pushed the multiple-polkit-calls branch from bcf3e64 to 4fad040 Compare February 22, 2023 13:31

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Feb 22, 2023

dtardon added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Feb 22, 2023

dtardon force-pushed the multiple-polkit-calls branch from 4fad040 to 225fb0e Compare February 22, 2023 15:10

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Feb 22, 2023

dtardon force-pushed the multiple-polkit-calls branch from 225fb0e to 72abb01 Compare May 5, 2023 08:23

dtardon force-pushed the multiple-polkit-calls branch from 72abb01 to 63ef492 Compare May 26, 2023 13:48

yuwata added the rc-blocker 🚧 PRs and Issues tagged this way are blocking the upcoming rc release! label Jun 1, 2023

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jun 14, 2023

dtardon mentioned this pull request Jun 16, 2023

Some bus-polkit cleanup #28056

Merged

dtardon added 3 commits July 13, 2023 16:00

bus-polkit: use automatic cleanup

10ee141

bus-polkit: drop unused argument

8f7195e

bus-polkit: refactor a bit to avoid goto

0df66e4

dtardon force-pushed the multiple-polkit-calls branch from f4ba6ac to 03830fa Compare July 13, 2023 14:16

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jul 13, 2023

dtardon added 3 commits July 13, 2023 16:29

bus-polkit: describe async. polkit verification

361c4ee

bus-polkit: extract action into a separate struct

c71901b

This is a preparation for later commits.

bus-polkit: parse reply from polkit on receive

959301c

... and store just the result.

dtardon force-pushed the multiple-polkit-calls branch from 03830fa to 969d807 Compare July 13, 2023 14:37

dtardon added 2 commits July 13, 2023 16:40

bus-polkit: avoid extra variable

b1ebc20

dtardon force-pushed the multiple-polkit-calls branch from 969d807 to b1ebc20 Compare July 13, 2023 14:40

bluca approved these changes Jul 13, 2023

View reviewed changes

bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Jul 13, 2023

bluca modified the milestones: v255, v254 Jul 14, 2023

bluca merged commit 0865c46 into systemd:main Jul 14, 2023
40 of 48 checks passed

github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jul 14, 2023

dtardon deleted the multiple-polkit-calls branch July 17, 2023 08:01

poettering reviewed Aug 9, 2023

View reviewed changes

dtardon mentioned this pull request Aug 17, 2023

bus-polkit: don't propagate error from polkit #28867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow D-Bus methods to auth. for more than one polkit action #26365

Allow D-Bus methods to auth. for more than one polkit action #26365

dtardon commented Feb 8, 2023 •

edited by github-actions bot

github-actions bot commented Feb 8, 2023

This comment was marked as outdated.

yuwata Feb 22, 2023

dtardon Feb 22, 2023

yuwata Feb 22, 2023

dtardon Feb 22, 2023

yuwata Feb 22, 2023

dtardon Feb 22, 2023

yuwata Feb 22, 2023

yuwata Feb 22, 2023

dtardon Feb 22, 2023

dtardon commented May 5, 2023

poettering commented Jun 14, 2023

poettering commented Jun 14, 2023

dtardon commented Jun 16, 2023

dtardon commented Jun 16, 2023

bluca commented Jul 11, 2023

dtardon commented Jul 13, 2023

poettering Aug 9, 2023

Allow D-Bus methods to auth. for more than one polkit action #26365

Allow D-Bus methods to auth. for more than one polkit action #26365

Conversation

dtardon commented Feb 8, 2023 • edited by github-actions bot

github-actions bot commented Feb 8, 2023

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtardon commented May 5, 2023

poettering commented Jun 14, 2023

poettering commented Jun 14, 2023

dtardon commented Jun 16, 2023

dtardon commented Jun 16, 2023

bluca commented Jul 11, 2023

dtardon commented Jul 13, 2023

Choose a reason for hiding this comment

dtardon commented Feb 8, 2023 •

edited by github-actions bot