Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion nixos/modules/security/pam.nix
Original file line number Diff line number Diff line change
Expand Up @@ -2554,7 +2554,18 @@ in
setuid = true;
owner = "root";
group = "root";
source = "${package}/bin/unix_chkpwd";
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";
Comment on lines +2557 to +2568
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

};
};

Expand Down
84 changes: 18 additions & 66 deletions nixos/modules/services/display-managers/gdm.nix
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,6 @@ let
exec "$@"
'';

# 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
'';

Comment on lines -28 to -43
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

defaultSessionName = config.services.displayManager.defaultSession;

setSessionScript = pkgs.callPackage ../x11/display-managers/account-service-util.nix { };
Expand Down Expand Up @@ -188,33 +172,7 @@ in

services.xserver.displayManager.lightdm.enable = false;

users.users = lib.mkMerge [
{
gdm = {
name = "gdm";
uid = config.ids.uids.gdm;
group = "gdm";
description = "GDM user";
};

gdm-greeter = {
isSystemUser = true;
uid = 60578;
group = "gdm";
home = "/run/gdm";
};
}

(lib.genAttrs' [ 1 2 3 4 ] (
i:
lib.nameValuePair "gdm-greeter-${toString i}" {
isSystemUser = true;
uid = 60578 + i;
group = "gdm";
home = "/run/gdm-${toString i}";
}
))
];
services.userdbd.enable = true;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.


users.groups.gdm.gid = config.ids.gids.gdm;

Expand Down Expand Up @@ -248,25 +206,16 @@ in
GDM_X_SESSION_WRAPPER = "${xSessionWrapper}";
};
execCmd = "exec ${gdm}/bin/gdm";
preStart = lib.optionalString (defaultSessionName != null) ''
# Set default session in session chooser to a specified values – basically ignore session history.
${setSessionScript}/bin/set-session ${config.services.displayManager.sessionData.autologinSession}
'';
preStart =
# sleep to avoid a race condition where userdb allocation isn't available yet, and gdm fails to start
"sleep 1"
Comment on lines +209 to +211
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

+ lib.optionalString (defaultSessionName != null) ''
# Set default session in session chooser to a specified values – basically ignore session history.
${setSessionScript}/bin/set-session ${config.services.displayManager.sessionData.autologinSession}
'';
};
};

systemd.tmpfiles.rules = [
"d /run/gdm/.config 0711 gdm gdm"
]
++ lib.optionals config.services.pulseaudio.enable [
"d /run/gdm/.config/pulse 0711 gdm gdm"
"L+ /run/gdm/.config/pulse/${pulseConfig.name} - - - - ${pulseConfig}"
]
++ lib.optionals config.services.gnome.gnome-initial-setup.enable [
# Create stamp file for gnome-initial-setup to prevent it starting in GDM.
"f /run/gdm/.config/gnome-initial-setup-done 0711 gdm gdm - yes"
];

# Otherwise GDM will not be able to start correctly and display Wayland sessions
systemd.packages = [
gdm
Expand All @@ -286,6 +235,8 @@ in
systemd.services.display-manager.wants = [
# Because sd_login_monitor_new requires /run/systemd/machines
"systemd-machined.service"
# to allocate dynamic gdm-greeter{-N} users
"systemd-userdbd.service"
# setSessionScript wants AccountsService
"accounts-daemon.service"
Comment on lines 240 to 241
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.)

];
Expand All @@ -294,6 +245,7 @@ in
"rc-local.service"
"systemd-machined.service"
"systemd-user-sessions.service"
"systemd-userdbd.service"
"plymouth-quit.service"
"plymouth-start.service"
];
Expand Down Expand Up @@ -397,6 +349,13 @@ in
"gdm"
];
}
{
name = "env";
control = "required";
modulePath = "${config.security.pam.package}/lib/security/pam_env.so";
settings.conffile = "/etc/pam/environment";
settings.readenv = 0;
}
Comment on lines +352 to +358
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why the move here? (Not doubting it, but some detail in the commit message would be great.)

{
name = "permit";
control = "optional";
Expand Down Expand Up @@ -445,13 +404,6 @@ in
"gdm"
];
}
{
name = "env";
control = "required";
modulePath = "${config.security.pam.package}/lib/security/pam_env.so";
settings.conffile = "/etc/pam/environment";
settings.readenv = 0;
}
{
name = "systemd";
control = "optional";
Expand Down
4 changes: 0 additions & 4 deletions pkgs/by-name/gd/gdm/package.nix
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,6 @@ stdenv.mkDerivation (finalAttrs: {
# installed (mostly just because .passthru.tests can make use of it).
substituteInPlace meson.build \
--replace-fail "dconf_prefix = dconf_dep.get_variable(pkgconfig: 'prefix')" "dconf_prefix = gdm_prefix"

# Disable userdb dynamic users for now
substituteInPlace meson.build \
--replace-fail 'have_userdb = libsystemd_dep' 'have_userdb = false #'
'';

doInstallCheck = true;
Expand Down
Loading