gdm: use systemd-userdbd#519148
Conversation
b2083fb to
b16c200
Compare
b16c200 to
d0e9d50
Compare
|
Turning this into a draft since we won't use this approach for 26.05, I'll elaborate/sum up the matrix discussion on the issue. |
emilazy
left a comment
There was a problem hiding this comment.
Thanks for taking this up! This should probably target master if it’s going to land after branch‐off.
I do think we need to handle the warning somehow – it’s probably not load‐bearing for these users to be detected as system users, so @ElvishJerricco’s suggestion of just moving the warning to modules that have issues as a result of it (e.g. homed at least) might be okay. But I think it’s worth trying putting the Nix build user information in /etc/userdb drop‐in files instead. I believe that nss-systemd(8) will synthesize PAM records for them even when userdb is not enabled, and it seems like both update-users-groups.pl and userborn refuse to create users with UID ≥ 30000, so we could likely do that unconditionally without any meaningful fallout.
It would require either filtering out those users out from users.users in nixos/modules/config/users-groups.nix, nixos/modules/system/boot/systemd/sysusers.nix, and nixos/modules/services/system/userborn.nix (either with a special case or an option like asUserdbDropin or something), or dropping them from users.users entirely, though. The latter is probably mostly fine, since they’re already not present with auto-allocate-uids, but I can imagine it breaking something – e.g. people adding supplementary groups to Nix build users to poke holes in the sandbox. So that might be best to avoid.
(Another caveat: Doing drop‐ins like this would theoretically expose us to the thing userborn guards against, where UID reuse can lead to security issues due to file ownership. There shouldn’t be anything persistent owned by build users to begin with, and nothing else (except perhaps systemd-sysusers?) will allocate UIDs in that range, though, so it’s probably not a real issue.)
Another potential option: systemd reserves the range 61184–65519 for dynamically allocated users (DynamicUser=, and I’d guess also systemd-nsresourced, which Nix implementations have looked at using for user allocation). According to that page, 60000 is apparently also the limit that standard adduser(8) avoids using UIDs above.
That page also says the following:
Note for the
DynamicUser=and thesystemd-nspawnallocation ranges: when a UID allocation takes place NSS is checked for collisions first, and a different UID is picked if an entry is found. Thus, the user database is used as synchronization mechanism to ensure exclusive ownership of UIDs and UID ranges. To ensure compatibility with other subsystems allocating from the same ranges it is hence essential that they ensure that whatever they pick shows up in the user/group databases, either by providing an NSS module, or by adding entries directly to/etc/passwdand/etc/group.
I’m not sure whether “other subsystems” means other systemd subsystems (i.e. they still claim exclusive ownership of the range), or whether they anticipate sharing it with other things in general, but we do essentially use the nixbld users as a range of UIDs to allocate transient dynamic users from; “dynamic” may be a more accurate classification than “system”. So it sounds like one option is that we could just move build users to the 61184–65519 range, while keeping them in the normal /etc/passwd files, and hopefully things would Just Work?
| source = | ||
| if (config.system.nssModules.path != [ ]) then | ||
| pkgs.runCommand "unix_chkpwd-with-nssModules" | ||
| { | ||
| nativeBuildInputs = [ pkgs.makeBinaryWrapper ]; | ||
| } | ||
| '' | ||
| makeWrapper ${package}/bin/unix_chkpwd $out \ | ||
| --prefix LD_LIBRARY_PATH : ${config.system.nssModules.path} | ||
| '' | ||
| else | ||
| "${package}/bin/unix_chkpwd"; |
There was a problem hiding this comment.
The LD_LIBRARY_PATH here makes me a little nervous and I wonder if this can’t be done in a better way. cc @Majiir @LordGrimmauld
| # Solves problems like: | ||
| # https://wiki.archlinux.org/index.php/Talk:Bluetooth_headset#GDMs_pulseaudio_instance_captures_bluetooth_headset | ||
| # Instead of blacklisting plugins, we use Fedora's PulseAudio configuration for GDM: | ||
| # https://src.fedoraproject.org/rpms/gdm/blob/master/f/default.pa-for-gdm | ||
| pulseConfig = pkgs.writeText "default.pa" '' | ||
| load-module module-device-restore | ||
| load-module module-card-restore | ||
| load-module module-udev-detect | ||
| load-module module-native-protocol-unix | ||
| load-module module-default-device-restore | ||
| load-module module-always-sink | ||
| load-module module-intended-roles | ||
| load-module module-suspend-on-idle | ||
| load-module module-position-event-sounds | ||
| ''; | ||
|
|
There was a problem hiding this comment.
Why do this and the corresponding tmpfiles settings get removed in “nixos/gdm: use systemd-userdbd to allocate gdm-greeter users”? Is PulseAudio no longer being used? That would make sense, but is that tied to systemd-userdb? More explanation in the commit message, or splitting out into a separate commit (or even PR?) if it’s independent, would be great.
There was a problem hiding this comment.
Users need to use services.pipewire.enable = lib.mkForce false; to disable pipewire now, and the linked Fedora configuration has also since been removed: https://src.fedoraproject.org/rpms/gdm/c/29b8d24d031de486e8f74a1263d63a2595fd1a8f?branch=f43
If we want to keep supporting this config for the minority of users opting out of pipewire-by-default, we'd have to somehow create those files after their homes are allocated. I don't mind if we somehow keep it working, but I didn't want to have dead code sticking around that wasn't doing anything.
and I'll clean up the commits.
There was a problem hiding this comment.
Oh yeah I don’t think we need to support Pulse here (or at all really). I just wasn’t sure why it was in that commit.
| } | ||
| )) | ||
| ]; | ||
| services.userdbd.enable = true; |
There was a problem hiding this comment.
This could perhaps use a comment, and I wonder if we shouldn’t put this in nixos/modules/services/desktop-managers/gnome.nix instead/as well?
I suppose it’d have to be “as well”, since you can use GDM without GNOME. But it seems plausible that other GNOME components will begin to want this too, as they move off AccountsService, if they haven’t already. nixos/modules/services/desktop-managers/gnome.nix sets services.accounts-daemon.enable – not sure if that’s still required, I don’t know what the status of the migration is.
| preStart = | ||
| # sleep to avoid a race condition where userdb allocation isn't available yet, and gdm fails to start | ||
| "sleep 1" |
There was a problem hiding this comment.
This is no good. We have to fix the race condition here rather than hacking around it like this. This looks to me like the systemd dependency ordering isn’t quite right.
What does the upstream gdm.service do here? There’s a desire to move away from our weird services.displayManager.generic infrastructure to using upstream units, with overrides if necessary; maybe this would be a good opportunity to do that if it doesn’t have this problem. See https://github.com/NixOS/nixpkgs/blob/13c1852f63a76fd3a651ba2d5cf038d6671a04c0/nixos/modules/services/display-managers/plasma-login-manager.nix for an example.
There was a problem hiding this comment.
yeah, not happy with it, I needed it for my VM to work reliably, without having to restart display-manager occasionally. I'll look into it some more.
| # setSessionScript wants AccountsService | ||
| "accounts-daemon.service" |
There was a problem hiding this comment.
I am wondering if our setSessionScript thing even still works, given that upstream is moving from AccountsService to userdb and this apparently needs AccountsService.
It’s a bit gross in general. Couldn’t we set an autologin session with a dconf setting or drop‐in file or something? (I know this is a bit orthogonal to this PR in general, but it ties in with using the upstream gdm.service and the move away from AccountsService.)
| { | ||
| name = "env"; | ||
| control = "required"; | ||
| modulePath = "${config.security.pam.package}/lib/security/pam_env.so"; | ||
| settings.conffile = "/etc/pam/environment"; | ||
| settings.readenv = 0; | ||
| } |
There was a problem hiding this comment.
Why the move here? (Not doubting it, but some detail in the commit message would be great.)
An alternative to #517777 by fixing #458058. Thanks to @nikamarisa for the hint with the pam wrapper (#458058 (comment))
I should note that this shows the evaluation warning
nixpkgs/nixos/modules/system/boot/systemd/userdbd.nix
Line 39 in 1c5f5f0
nixbldusers with a GNOME DE, I don't know if we shouldsilenceHighSystemUsersfor users.Things done
nixosTests.gnome)passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.