logind: always check for inhibitor locks#30307
Conversation
github-actions
bot
added
documentation
tests
please-review
|
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. |
bluca
force-pushed
the
enforce_inhibitors
branch
from
77f62eb to
bf3beb8
Compare
YHNdnzj
left a comment
YHNdnzj
left a comment
There was a problem hiding this comment.
I don't think we should restrict rebooting with --force (only once) in this way. PowerOff and similar methods already require CAP_SYS_BOOT to operate, and adding those checks in systemctl doesn't block people from calling the methods directly using busctl. This doesn't seem to introduce security improvements, but merely prohibits privileged users from issuing a method through systemctl, which is confusing.
--force --force is kinda destructive, and I'm not convinced that you should do/recommand that just to bypass inhibitors.
It's not a security feature, as privilege levels are the same. It's for safety: evidently people want to be able to avoid accidentally issueing reboot commands, given the horrendous hacks that are shipped for this purpose, and this is necessary to provide a full alternative to be able to deprecate them. It's intentionally behind a config option disabled by default: if you choose to enable it, then you sign up for this. Otherwise, previous behavior continues to apply unchanged. I also want to extend this to pid1 dbus methods, but it's a bit more complicated due to the client/server relationship so I'll keep it for later. |
bluca
force-pushed
the
enforce_inhibitors
branch
from
bf3beb8 to
d998aab
Compare
Yeah, but the main point is that Maybe, just disallow the use of |
bluca
force-pushed
the
enforce_inhibitors
branch
from
d998aab to
467a3ce
Compare
It's a pre-existing option, and as such we shouldn't hide it - it's there, so it should be documented. I've reworded to list several other safer options first, and to make clear that it's a last resort and will likely cause data loss when used. |
bluca
force-pushed
the
enforce_inhibitors
branch
2 times, most recently
from
f80ed4b to
b70a6fa
Compare
|
@mrc0mmand looks new, but I don't think it's related? |
That's ... concerning. Lemme restart the job. |
|
Ah, it failed to reboot the machine after relabeling: Could that be related to the change? |
|
Yes, that could, let me check |
bluca
force-pushed
the
enforce_inhibitors
branch
from
b70a6fa to
bc32682
Compare
|
hmpf, I really don't like this approach. I hate config options which totally redefine what things mean. In particular as some distris will just set this and others won't and then noone knows what an inhibitor actzally means. I also think it's a really bad idea to allow unpriv users to just block reboots done by root. That's totally not acceptable. This discussion has been going on for a while, and I know I changed my mind in some parts on this a couple of times, but maybe a solution like this might work:
|
It doesn't really redefine it, it makes it stricter, to cover some extra corner cases. The protocol, meaning, tooling, etc are the same. We got tons of options that make existing things stricter on demand. No distro would set this by default as it would break a tons of things, and it wouldn't make sense anyway. This is a config option for machine owners and admins, not a build option for distro builders.
Unpriv users are not allowed to block, only to delay. That is already the case today. The only difference here is that root decides that it wants to impose additional restrictions on itself by itself. It can also be backed out at any time as documented - it is not a security feature.
This would require a new API, and all applications to buy into that new API. That doesn't help at all to solve the problem I'm trying to solve: how can a machine owner decide to make things stricter for itself, regardless of what the application wants to do. This is not about applications making things stricter for the machine owner, that is not the problem being solved here. |
No, it does not. Because I suggested the new flag if not configured specifically defaults to whether the client has privs or not. This would mean that magically all such locks created by priv userspace become truly blocking, but those by unpriv locks are not. |
Didn't you say Debian builds something like this manually? So what is this PR about? I assumed this was something DEbian needs? if not, what is this for? |
That would break backward compatibility
Yes, for users on Debian to enable if/when they need it, not for the distribution to enable it by default. This is what it replaces: https://sources.debian.org/src/molly-guard/0.8.1/debian/control/ |
bluca
force-pushed
the
enforce_inhibitors
branch
from
bc32682 to
f4f7841
Compare
So would your change. Sorry, I am not convinced that inhibitor semantics should be up for configuration of the admin. I think a more selective change might be OK, as I proposed, but it would have to adjust things for all cases, not dependent on config. Sorry, but new config options for this are just bad. This makes no sense.
I do no see the relationship of that to inhibitors? That tool appears to interactively ask for confirmation, asking for a hostname to be typed in? this is nothing inhibitors could be used for. |
Yes I wanted to make it a config option because that is also fixed beforehand - if it is set, you know the behaviour is reliable. Not ideal, but fine as a compromise. A flag from the client side is not the same: when you successfully get a lock, you don't know if later another clients comes in and passes a flag via D-Bus that makes your lock go ignored. And adding one more problem of the exact same type of the many problems that already exist is not really a compromise - it does not solve the issue at hand - seriously I do not know how else to explain this: the problem is the huge variability that comes in depending on internal details of whichever client happens to call the inhibited operation. Adding one more dimension to that variability is not a generous compromise, is a step in the wrong direction that makes the existing situation even worse than it is, as now there's one more thing that can go wrong and take your successfully acquired lock away from you, that you have no way whatsoever of knowing in advance. How would that help solve the problem? |
Well, this proposal is about making existing users of inhibitors behave consistently/reliably. I.e. if the inhibitor cannot be acquired, the owner/user/taker should get an error, and based on that it could choose whether to carry on without inhibitor (if the job is interruptible) or to bail out if not. IOW, to actually do this right, If there's need for |
|
Ok on top of the SD_LOGIND_SKIP_INHIBITORS reboot flag that I already had added, I have now added two new modes 'block-weak' and 'delay-weak' as requested, that make the new lock behave as it does currently |
Hmm, I don't see the point for delay-weak? delay inhibitors are inherently weak? BTW, to make things actually reliable, SD_LOGIND_SKIP_INHIBITORS should only try to skip |
|
The request was to be able to skip/ignore everything, so I've implemented it as that. I think there's been enough back and forth, I just want to get this merged |
But this just brings the problem you're trying to resolve back, no? Now the clients that do hold an block inhibitor can still be skipped. The idea is that the block inhibitors, once acquired, will definitely not be skipped. And whether you can acquire one is authorized through polkit. OTOH, the weak ones can be skipped when a flag is specified and the caller is privileged, and the required privilege is relatively lower. The delay one you can aleays get, but others are always allowed to shortcut things. |
Yes, I am well aware - but @poettering doesn't want to have it any other way as per last meeting, and is adamant this "weak" version needs to be available too in parallel. I do not like it, but I want to get this merged. At least the default now will be sane and work in all cases as a reasonable person expects. And if one wants to take a lock that doesn't actually lock, the option is there. |
That's not the way I understand @poettering? The weak ones are enough IIUC? |
keszybz
left a comment
keszybz
left a comment
There was a problem hiding this comment.
LGTM apart from cosmetic stuff.
I tested this in a VM, and it all seems to work as expected, incl. the new weak inhibitors.
My understanding is the the latest version implements what we agreed on in the meeting. To quote Lennart (from memory, but I think I got the gist correctly): as a user I want to take an inhibitor that blocks other users, but not actions that I initiate myself, nor actions initiated by the system. The weak inhibitors implement exactly that. |
|
noble-s390x testbed is busted, not related. |
Currently inhibitors are bypassed unless an explicit request is made to check for them, or even in that case when the requestor is root or the same uid as the holder of the lock. But in many cases this makes it impractical to rely on inhibitor locks. For example, in Debian there are several convoluted and archaic workarounds that divert systemctl/reboot to some hacky custom scripts to try and enforce blocking accidental reboots, when it's not expected that the requestor will remember to specify the command line option to enable checking for active inhibitor locks. Also in many cases one wants to ensure that locks taken by a user are respected by actions initiated by that same user. Change logind so that inhibitors checks are not skipped in these cases, and systemctl so that locks are checked in order to show a friendly error message rather than "permission denied". Add new block-weak and delay-weak modes that keep the previous behaviour unchanged.
| <literal>shutdown</literal>.</para></listitem> | ||
| Note that <literal>delay</literal> is only available for <literal>sleep</literal> and | ||
| <literal>shutdown</literal>. In addition, the weak variants will automatically and silently be | ||
| bypassed under some circumstances.</para></listitem> |
There was a problem hiding this comment.
that's awfully vague. please clarify this.
delay inhibitors are already kinda weak: they have a timeout anyway, that's enough. and we always applied them anyway regardless who is asking for a shutdown. Please do not add a weak flavour of delay inhibitors. they make no sense.
| This mode is only available for _sleep_ and _shutdown_ locks. | ||
|
|
||
| 3. _block-weak_ and _delay-weak_ that work as the non-weak counterparts, but that in addition may be ignored | ||
| automatically and silently under certain circumstances, unlike the formers which are always respected. |
There was a problem hiding this comment.
please add proper documentation, i.e. which "certain circumstances" those are. we need to tell people which ones to take.
There was a problem hiding this comment.
Looks like the conditions are:
Invoker of the command is root (geteuid() == 0), invoker is the same UID as the inhibitor (geteuid() == uid), or invoker is not on a terminal (?!) (!on_tty)
| performs a userspace reboot only. <constant>SD_LOGIND_SOFT_REBOOT_IF_NEXTROOT_SET_UP</constant> and | ||
| flags. Since systemd version 256 <constant>SD_LOGIND_ROOT_CHECK_INHIBITORS</constant> (0x01) is deprecated, | ||
| and active inhibitors are always honoured by default for privileged users too, and a new flag | ||
| <constant>SD_LOGIND_SKIP_INHIBITORS</constant> (0x04) can be specified to bypass inhibitors. When |
There was a problem hiding this comment.
please be explicit here, that this is limited to privileged clients, and that this bypasses block and block-weak inhibitors (and has no effect on delay inhibitors).
| /* We don't check polkit for root here, because you can't be more privileged than root */ | ||
| if (uid == 0 && (flags & SD_LOGIND_ROOT_CHECK_INHIBITORS)) | ||
| if (!FLAGS_SET(flags, SD_LOGIND_SKIP_INHIBITORS)) | ||
| return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, |
There was a problem hiding this comment.
this should return a proper dbus error btw, that says BlockedByInhibitor... (yes, pre-existing issue i know, but please fix if you touch this)
|
sigh. sorry, but this wasn't ready. what was the need to hurry this in? you knew this was a contentious issue... |
6 participants
Currently inhibitors are bypassed unless an explicit request is made to
check for them, or even in that case when the requestor is root or the
same uid as the holder of the lock.
But in many cases this makes it impractical to rely on inhibitor locks.
For example, in Debian there are several convoluted and archaic
workarounds that divert systemctl/reboot to some hacky custom scripts
to try and enforce blocking accidental reboots, when it's not expected
that the requestor will remember to specify the command line option
to enable checking for active inhibitor locks.
Also in many cases one wants to ensure that locks taken by a user are
respected by actions initiated by that same user.
Change logind so that inhibitors checks are not skipped in these
cases, and systemctl so that locks are checked in order to show a
friendly error message rather than "permission denied".