Add KeyringMode unit property to fix cryptsetup key caching#6832
Add KeyringMode unit property to fix cryptsetup key caching#6832keszybz merged 2 commits intosystemd:masterfrom
Conversation
keszybz
left a comment
There was a problem hiding this comment.
Looks superficially OK, but I think the keyring should be called "keyring" everywhere, and the mode should be "KeyringMode" [https://en.wikipedia.org/wiki/Keyring_(cryptography)].
man/systemd.exec.xml
Outdated
| <row> | ||
| <entry>@keyring</entry> | ||
| <entry>Kernel keyring access (<citerefentry project='man-pages'><refentrytitle>keyctl</refentrytitle><manvolnum>2</manvolnum></citerefentry> and related calls)</entry> | ||
| <entry>Kernel key ring access (<citerefentry project='man-pages'><refentrytitle>keyctl</refentrytitle><manvolnum>2</manvolnum></citerefentry> and related calls)</entry> |
There was a problem hiding this comment.
"keyring" is a valid word nowadays.
There was a problem hiding this comment.
hmm, i checked merriem webster specifically, and it insisted on "key ring"... But i figure if it's "keyring" according to wikipedia in respect to cryptography, i figure that's fine too
There was a problem hiding this comment.
Yeah, I think it'd be confusing. Internally in the code we have keyring in various places, so adding key_ring in others is inconsistent.
src/core/execute.c
Outdated
| uid_t saved_uid; | ||
| gid_t saved_gid; | ||
|
|
||
| /* Acquiring a reference to the user keyring is nasty. We briefly change identity in oder to get things |
711b106 to
e393667
Compare
|
force pushed a new version that makes the KeyRing → Keyring change, as @keszybz suggested (and the typo fix too). Please have another look! |
|
Whoa, isn't this missing some .gperf changes? |
|
hmm, indeed. not sure where that commit got lost |
…yring setup
Usually, it's a good thing that we isolate the kernel session keyring
for the various services and disconnect them from the user keyring.
However, in case of the cryptsetup key caching we actually want that
multiple instances of the cryptsetup service can share the keys in the
root user's user keyring, hence we need to be able to disable this logic
for them.
This adds KeyringMode=inherit|private|shared:
inherit: don't do any keyring magic (this is the default in systemd --user)
private: a private keyring as before (default in systemd --system)
shared: the new setting
…yring We want that cryptsetup can cache keys between multiple invocations, and it does so via the root user's user keyring, hence let's share it among services. Replaces: systemd#6286
e393667 to
0b1f68a
Compare
|
somehow the unit file change got lost in my rebase orgy. Readded that stuff from scracth now. Please have a look. |
|
|
|
I booted my notebook with this, everything seems to work nicely. |
|
Looks good, thanks a lot! |
|
|
||
| r = -errno; | ||
|
|
||
| (void) setreuid(saved_uid, -1); |
There was a problem hiding this comment.
The setreiud(2) manpage includes a warning against calling setreuid(2) without checking the error return:
Note: there are cases where setreuid() can fail even when the
caller is UID 0; it is a grave security error to omit
checking for a failure return from setreuid().
There was a problem hiding this comment.
Well, this is a fatal error path already. Generally, when an error path itself results in another error you're fucked. In this case we do the best we can, and make sure to return the first error that happens and the caller will eventually exit due to this.
Mostly an implementation of what was proposed here:
#6286 (comment)
I am not sure if this fixes all the key caching issues reported. @eworm-de can I bribe you into testing this PR and checking if this fixes the caching issues for you properly?