From 52ce5caf366985c13f501f49473078b35296281b Mon Sep 17 00:00:00 2001 From: fidgetingbits Date: Sun, 15 Dec 2024 12:21:32 +0800 Subject: [PATCH 1/4] nixos/users-groups: Correct and refactor password override documentation Testing showed that the existing documentation regarding password override ordering was incorrect. This commit corrects the errors and refactors the way the text is constructed to make updating future ordering changes significantly easier. --- nixos/modules/config/users-groups.nix | 107 ++++++++++++++++++-------- 1 file changed, 73 insertions(+), 34 deletions(-) diff --git a/nixos/modules/config/users-groups.nix b/nixos/modules/config/users-groups.nix index 845f9fdaf68e..d225da551663 100644 --- a/nixos/modules/config/users-groups.nix +++ b/nixos/modules/config/users-groups.nix @@ -53,37 +53,75 @@ let "*" # password unset ]); + overrideOrderMutable = '' + {option}`initialHashedPassword` -> {option}`initialPassword` -> {option}`hashedPassword` -> {option}`password` -> {option}`hashedPasswordFile` + ''; + + overrideOrderImmutable = '' + {option}`initialHashedPassword` -> {option}`hashedPassword` -> {option}`initialPassword` -> {option}`password` -> {option}`hashedPasswordFile` + ''; + + overrideOrderText = isMutable: '' + If the option {option}`users.mutableUsers` is + `${if isMutable then "true" else "false"}`, then the order of precedence is as shown + below, where values on the left are overridden by values on the right: + ${if isMutable then overrideOrderMutable else overrideOrderImmutable} + ''; + + multiplePasswordsWarning = '' + If multiple of these password options are set at the same time then a + specific order of precedence is followed, which can lead to surprising + results. The order of precedence differs depending on whether the + {option}`users.mutableUsers` option is set. + ''; + + overrideDescription = '' + ${multiplePasswordsWarning} + + ${overrideOrderText false} + + ${overrideOrderText true} + ''; + passwordDescription = '' - The options {option}`hashedPassword`, - {option}`password` and {option}`hashedPasswordFile` - controls what password is set for the user. - {option}`hashedPassword` overrides both - {option}`password` and {option}`hashedPasswordFile`. - {option}`password` overrides {option}`hashedPasswordFile`. - If none of these three options are set, no password is assigned to - the user, and the user will not be able to do password logins. - If the option {option}`users.mutableUsers` is true, the - password defined in one of the three options will only be set when - the user is created for the first time. After that, you are free to - change the password with the ordinary user management commands. If - {option}`users.mutableUsers` is false, you cannot change - user passwords, they will always be set according to the password - options. + The {option}`initialHashedPassword`, {option}`hashedPassword`, + {option}`initialPassword`, {option}`password` and + {option}`hashedPasswordFile` options all control what password is set for + the user. + + In a system where [](#opt-systemd.sysusers.enable) is `false`, typically + only one of {option}`hashedPassword`, {option}`password`, or + {option}`hashedPasswordFile` will be set. + + In a system where [](#opt-systemd.sysusers.enable) is `true`, typically + only one of {option}`initialPassword`, {option}`initialHashedPassword`, + or {option}`hashedPasswordFile` will be set. + + If the option {option}`users.mutableUsers` is true, the password defined + in one of the above password options will only be set when the user is + created for the first time. After that, you are free to change the + password with the ordinary user management commands. If + {option}`users.mutableUsers` is false, you cannot change user passwords, + they will always be set according to the password options. + + If none of the password options are set, then no password is assigned to + the user, and the user will not be able to do password-based logins. + + ${overrideDescription} ''; hashedPasswordDescription = '' To generate a hashed password run `mkpasswd`. - If set to an empty string (`""`), this user will - be able to log in without being asked for a password (but not via remote - services such as SSH, or indirectly via {command}`su` or - {command}`sudo`). This should only be used for e.g. bootable - live systems. Note: this is different from setting an empty password, - which can be achieved using {option}`users.users..password`. + If set to an empty string (`""`), this user will be able to log in without + being asked for a password (but not via remote services such as SSH, or + indirectly via {command}`su` or {command}`sudo`). This should only be used + for e.g. bootable live systems. Note: this is different from setting an + empty password, which can be achieved using + {option}`users.users..password`. - If set to `null` (default) this user will not - be able to log in using a password (i.e. via {command}`login` - command). + If set to `null` (default) this user will not be able to log in using a + password (i.e. via {command}`login` command). ''; userOpts = { name, config, ... }: { @@ -281,6 +319,7 @@ let default = null; description = '' Specifies the hashed password for the user. + ${passwordDescription} ${hashedPasswordDescription} ''; @@ -294,6 +333,7 @@ let Warning: do not set confidential information here because it is world-readable in the Nix store. This option should only be used for public accounts. + ${passwordDescription} ''; }; @@ -307,6 +347,7 @@ let password. The password file is read on each system activation. The file should contain exactly one line, which should be the password in an encrypted form that is suitable for the `chpasswd -e` command. + ${passwordDescription} ''; }; @@ -329,9 +370,7 @@ let {command}`passwd` command. Otherwise, it's equivalent to setting the {option}`hashedPassword` option. - Note that the {option}`hashedPassword` option will override - this option if both are set. - + ${passwordDescription} ${hashedPasswordDescription} ''; }; @@ -351,8 +390,7 @@ let used for guest accounts or passwords that will be changed promptly. - Note that the {option}`password` option will override this - option if both are set. + ${passwordDescription} ''; }; @@ -960,11 +998,12 @@ in { (filter (x: x != null) (map (flip getAttr user) passwordOptions)); in optional (!unambiguousPasswordConfiguration) '' The user '${user.name}' has multiple of the options - `hashedPassword`, `password`, `hashedPasswordFile`, `initialPassword` - & `initialHashedPassword` set to a non-null value. - The options silently discard others by the order of precedence - given above which can lead to surprising results. To resolve this warning, - set at most one of the options above to a non-`null` value. + `initialHashedPassword`, `hashedPassword`, `initialPassword`, `password` + & `hashedPasswordFile` set to a non-null value. + + ${multiplePasswordsWarning} + + ${overrideOrderText cfg.mutableUsers} The values of these options are: ${concatMapStringsSep From b84fb1e5cdc27d9f2103d260398567116daa5ce4 Mon Sep 17 00:00:00 2001 From: fidgetingbits Date: Sun, 15 Dec 2024 12:22:02 +0800 Subject: [PATCH 2/4] nixos/tests: Add two new tests for password option override ordering This commit adds two new tests to show that the ordering of password overrides documentation in nixos/modules/config/user-groups.nix is correct. The override behavior differs depending on whether a system has systemd-sysusers enabled, so there are two tests. --- nixos/tests/all-tests.nix | 2 + .../password-option-override-ordering.nix | 171 ++++++++++++++++++ ...sers-password-option-override-ordering.nix | 77 ++++++++ 3 files changed, 250 insertions(+) create mode 100644 nixos/tests/password-option-override-ordering.nix create mode 100644 nixos/tests/systemd-sysusers-password-option-override-ordering.nix diff --git a/nixos/tests/all-tests.nix b/nixos/tests/all-tests.nix index 37e005f128a2..6029033f0eb3 100644 --- a/nixos/tests/all-tests.nix +++ b/nixos/tests/all-tests.nix @@ -770,6 +770,7 @@ in { pantheon = handleTest ./pantheon.nix {}; paperless = handleTest ./paperless.nix {}; parsedmarc = handleTest ./parsedmarc {}; + password-option-override-ordering = handleTest ./password-option-override-ordering.nix {}; pdns-recursor = handleTest ./pdns-recursor.nix {}; peerflix = handleTest ./peerflix.nix {}; peering-manager = handleTest ./web-apps/peering-manager.nix {}; @@ -1013,6 +1014,7 @@ in { systemd-sysupdate = runTest ./systemd-sysupdate.nix; systemd-sysusers-mutable = runTest ./systemd-sysusers-mutable.nix; systemd-sysusers-immutable = runTest ./systemd-sysusers-immutable.nix; + systemd-sysusers-password-option-override-ordering = runTest ./systemd-sysusers-password-option-override-ordering.nix; systemd-timesyncd = handleTest ./systemd-timesyncd.nix {}; systemd-timesyncd-nscd-dnssec = handleTest ./systemd-timesyncd-nscd-dnssec.nix {}; systemd-user-linger = handleTest ./systemd-user-linger.nix {}; diff --git a/nixos/tests/password-option-override-ordering.nix b/nixos/tests/password-option-override-ordering.nix new file mode 100644 index 000000000000..5b06ab0bdbff --- /dev/null +++ b/nixos/tests/password-option-override-ordering.nix @@ -0,0 +1,171 @@ +let + password1 = "foobar"; + password2 = "helloworld"; + hashed_bcrypt = "$2b$05$8xIEflrk2RxQtcVXbGIxs.Vl0x7dF1/JSv3cyX6JJt0npzkTCWvxK"; # fnord + hashed_yeshash = "$y$j9T$d8Z4EAf8P1SvM/aDFbxMS0$VnTXMp/Hnc7QdCBEaLTq5ZFOAFo2/PM0/xEAFuOE88."; # fnord + hashed_sha512crypt = "$6$ymzs8WINZ5wGwQcV$VC2S0cQiX8NVukOLymysTPn4v1zJoJp3NGyhnqyv/dAf4NWZsBWYveQcj6gEJr4ZUjRBRjM0Pj1L8TCQ8hUUp0"; # meow +in + +import ./make-test-python.nix ( + { pkgs, ... }: + { + name = "password-option-override-ordering"; + meta = with pkgs.lib.maintainers; { + maintainers = [ fidgetingbits ]; + }; + + nodes = + let + # The following users are expected to have the same behavior between immutable and mutable systems + # NOTE: Below given A -> B it implies B overrides A . Each entry below builds off the next + users = { + # mutable true/false: initialHashedPassword -> hashedPassword + fran = { + isNormalUser = true; + initialHashedPassword = hashed_yeshash; + hashedPassword = hashed_sha512crypt; + }; + + # mutable false: initialHashedPassword -> hashedPassword -> initialPassword + # mutable true: initialHashedPassword -> initialPassword -> hashedPassword + greg = { + isNormalUser = true; + hashedPassword = hashed_sha512crypt; + initialPassword = password1; + }; + + # mutable false: initialHashedPassword -> hashedPassword -> initialPassword -> password + # mutable true: initialHashedPassword -> initialPassword -> hashedPassword -> password + egon = { + isNormalUser = true; + initialPassword = password2; + password = password1; + }; + + # mutable true/false: hashedPassword -> password + # NOTE: minor duplication of test above, but to verify no initialXXX use is consistent + alice = { + isNormalUser = true; + hashedPassword = hashed_sha512crypt; + password = password1; + }; + + # mutable false: initialHashedPassword -> hashedPassword -> initialPassword -> password -> hashedPasswordFile + # mutable true: initialHashedPassword -> initialPassword -> hashedPassword -> password -> hashedPasswordFile + bob = { + isNormalUser = true; + hashedPassword = hashed_sha512crypt; + password = password1; + hashedPasswordFile = (pkgs.writeText "hashed_bcrypt" hashed_bcrypt).outPath; # Expect override of everything above + }; + + # Show hashedPassword -> password -> hashedPasswordFile -> initialPassword is false + # to explicitly show the following lib.trace warning in users-groups.nix (which was + # the wording prior to PR 310484) is in fact wrong: + # ``` + # The user 'root' has multiple of the options + # `hashedPassword`, `password`, `hashedPasswordFile`, `initialPassword` + # & `initialHashedPassword` set to a non-null value. + # The options silently discard others by the order of precedence + # given above which can lead to surprising results. To resolve this warning, + # set at most one of the options above to a non-`null` value. + # ``` + cat = { + isNormalUser = true; + hashedPassword = hashed_sha512crypt; + password = password1; + hashedPasswordFile = (pkgs.writeText "hashed_bcrypt" hashed_bcrypt).outPath; + initialPassword = password2; # lib.trace message implies this overrides everything above + }; + + # Show hashedPassword -> password -> hashedPasswordFile -> initialHashedPassword is false + # to also explicitly show the lib.trace explained above (see cat user) is wrong + dan = { + isNormalUser = true; + hashedPassword = hashed_sha512crypt; + initialPassword = password2; + password = password1; + hashedPasswordFile = (pkgs.writeText "hashed_bcrypt" hashed_bcrypt).outPath; + initialHashedPassword = hashed_yeshash; # lib.trace message implies this overrides everything above + }; + }; + + mkTestMachine = mutable: { + environment.systemPackages = [ pkgs.shadow ]; + users = { + mutableUsers = mutable; + inherit users; + }; + }; + in + { + immutable = mkTestMachine false; + mutable = mkTestMachine true; + }; + + testScript = '' + import crypt + + def assert_password_match(machine, username, password): + shadow_entry = machine.succeed(f"getent shadow {username}") + print(shadow_entry) + hash = shadow_entry.split(":")[1] + seed = "$".join(hash.split("$")[:-1]) + assert crypt.crypt(password, seed) == hash, f"{username} user password does not match" + + with subtest("alice user has correct password"): + for machine in machines: + assert_password_match(machine, "alice", "${password1}") + assert "${hashed_sha512crypt}" not in machine.succeed("getent shadow alice"), f"{machine}: alice user password is not correct" + + with subtest("bob user has correct password"): + for machine in machines: + print(machine.succeed("getent shadow bob")) + assert "${hashed_bcrypt}" in machine.succeed("getent shadow bob"), f"{machine}: bob user password is not correct" + + with subtest("cat user has correct password"): + for machine in machines: + print(machine.succeed("getent shadow cat")) + assert "${hashed_bcrypt}" in machine.succeed("getent shadow cat"), f"{machine}: cat user password is not correct" + + with subtest("dan user has correct password"): + for machine in machines: + print(machine.succeed("getent shadow dan")) + assert "${hashed_bcrypt}" in machine.succeed("getent shadow dan"), f"{machine}: dan user password is not correct" + + with subtest("greg user has correct password"): + print(mutable.succeed("getent shadow greg")) + assert "${hashed_sha512crypt}" in mutable.succeed("getent shadow greg"), "greg user password is not correct" + + assert_password_match(immutable, "greg", "${password1}") + assert "${hashed_sha512crypt}" not in immutable.succeed("getent shadow greg"), "greg user password is not correct" + + for machine in machines: + machine.wait_for_unit("multi-user.target") + machine.wait_until_succeeds("pgrep -f 'agetty.*tty1'") + + def check_login(machine: Machine, tty_number: str, username: str, password: str): + machine.send_key(f"alt-f{tty_number}") + machine.wait_until_succeeds(f"[ $(fgconsole) = {tty_number} ]") + machine.wait_for_unit(f"getty@tty{tty_number}.service") + machine.wait_until_succeeds(f"pgrep -f 'agetty.*tty{tty_number}'") + machine.wait_until_tty_matches(tty_number, "login: ") + machine.send_chars(f"{username}\n") + machine.wait_until_tty_matches(tty_number, f"login: {username}") + machine.wait_until_succeeds("pgrep login") + machine.wait_until_tty_matches(tty_number, "Password: ") + machine.send_chars(f"{password}\n") + machine.send_chars(f"whoami > /tmp/{tty_number}\n") + machine.wait_for_file(f"/tmp/{tty_number}") + assert username in machine.succeed(f"cat /tmp/{tty_number}"), f"{machine}: {username} password is not correct" + + with subtest("Test initialPassword override"): + for machine in machines: + check_login(machine, "2", "egon", "${password1}") + + with subtest("Test initialHashedPassword override"): + for machine in machines: + check_login(machine, "3", "fran", "meow") + ''; + } +) diff --git a/nixos/tests/systemd-sysusers-password-option-override-ordering.nix b/nixos/tests/systemd-sysusers-password-option-override-ordering.nix new file mode 100644 index 000000000000..4cb13512549c --- /dev/null +++ b/nixos/tests/systemd-sysusers-password-option-override-ordering.nix @@ -0,0 +1,77 @@ +{ + lib, + pkgs ? import ../.., + ... +}: +let + password = "test"; + password1 = "test1"; + hashedPassword = "$y$j9T$wLgKY231.8j.ciV2MfEXe1$P0k5j3bCwHgnwW0Ive3w4knrgpiA4TzhCYLAnHvDZ51"; # test + hashedPassword1 = "$y$j9T$s8TyQJtNImvobhGM5Nlez0$3E8/O8EVGuA4sr1OQmrzi8GrRcy/AEhj454JjAn72A2"; # test + hashed_sha512crypt = "$6$ymzs8WINZ5wGwQcV$VC2S0cQiX8NVukOLymysTPn4v1zJoJp3NGyhnqyv/dAf4NWZsBWYveQcj6gEJr4ZUjRBRjM0Pj1L8TCQ8hUUp0"; # meow + + hashedPasswordFile = pkgs.writeText "hashed-password" hashedPassword1; +in +{ + name = "systemd-sysusers-password-option-override-ordering"; + + meta.maintainers = with lib.maintainers; [ fidgetingbits ]; + + nodes.machine = { + systemd.sysusers.enable = true; + system.etc.overlay.enable = true; + boot.initrd.systemd.enable = true; + + users.mutableUsers = true; + + # NOTE: Below given A -> B it implies B overrides A . Each entry below builds off the next + + users.users.root = { + hashedPasswordFile = lib.mkForce null; + initialHashedPassword = password; + }; + + users.groups.test = { }; + + # initialPassword -> initialHashedPassword + users.users.alice = { + isSystemUser = true; + group = "test"; + initialPassword = password; + initialHashedPassword = hashedPassword; + }; + + # initialPassword -> initialHashedPassword -> hashedPasswordFile + users.users.bob = { + isSystemUser = true; + group = "test"; + initialPassword = password; + initialHashedPassword = hashedPassword; + hashedPasswordFile = hashedPasswordFile.outPath; + }; + }; + + testScript = '' + machine.wait_for_unit("systemd-sysusers.service") + + with subtest("systemd-sysusers.service contains the credentials"): + sysusers_service = machine.succeed("systemctl cat systemd-sysusers.service") + print(sysusers_service) + assert "SetCredential=passwd.plaintext-password.alice:${password}" in sysusers_service + + with subtest("Correct mode on the password files"): + assert machine.succeed("stat -c '%a' /etc/passwd") == "644\n" + assert machine.succeed("stat -c '%a' /etc/group") == "644\n" + assert machine.succeed("stat -c '%a' /etc/shadow") == "0\n" + assert machine.succeed("stat -c '%a' /etc/gshadow") == "0\n" + + with subtest("alice user has correct password"): + print(machine.succeed("getent shadow alice")) + assert "${hashedPassword}" in machine.succeed("getent shadow alice"), "alice user password is not correct" + + with subtest("bob user has new password after switching to new generation"): + print(machine.succeed("getent passwd bob")) + print(machine.succeed("getent shadow bob")) + assert "${hashedPassword1}" in machine.succeed("getent shadow bob"), "bob user password is not correct" + ''; +} From cb9b3fd78877f2bb984d92d7ee50fe5fbb479e44 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sun, 22 Dec 2024 15:32:56 +0100 Subject: [PATCH 3/4] nixos/modules: remove a few whitespaces from the pw override parts Otherwise the evaluation warnings have a two or even three lines of whitespace between paragraphs. --- nixos/modules/config/users-groups.nix | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/nixos/modules/config/users-groups.nix b/nixos/modules/config/users-groups.nix index d225da551663..38d40e99abd6 100644 --- a/nixos/modules/config/users-groups.nix +++ b/nixos/modules/config/users-groups.nix @@ -53,13 +53,9 @@ let "*" # password unset ]); - overrideOrderMutable = '' - {option}`initialHashedPassword` -> {option}`initialPassword` -> {option}`hashedPassword` -> {option}`password` -> {option}`hashedPasswordFile` - ''; + overrideOrderMutable = ''{option}`initialHashedPassword` -> {option}`initialPassword` -> {option}`hashedPassword` -> {option}`password` -> {option}`hashedPasswordFile`''; - overrideOrderImmutable = '' - {option}`initialHashedPassword` -> {option}`hashedPassword` -> {option}`initialPassword` -> {option}`password` -> {option}`hashedPasswordFile` - ''; + overrideOrderImmutable = ''{option}`initialHashedPassword` -> {option}`hashedPassword` -> {option}`initialPassword` -> {option}`password` -> {option}`hashedPasswordFile`''; overrideOrderText = isMutable: '' If the option {option}`users.mutableUsers` is @@ -1002,9 +998,7 @@ in { & `hashedPasswordFile` set to a non-null value. ${multiplePasswordsWarning} - ${overrideOrderText cfg.mutableUsers} - The values of these options are: ${concatMapStringsSep "\n" From da2c826e1afe3ad1cfbc394dcd411bab2206ae78 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sun, 22 Dec 2024 15:35:41 +0100 Subject: [PATCH 4/4] nixos/tests/systemd-sysusers-password-option-override-ordering: clarify SetCredential check It was kinda weird to assert that the clear-text password was in the unit when the hashed password was the effective one. This change makes it explicit that both are in there and the latter takes precedence. --- .../systemd-sysusers-password-option-override-ordering.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nixos/tests/systemd-sysusers-password-option-override-ordering.nix b/nixos/tests/systemd-sysusers-password-option-override-ordering.nix index 4cb13512549c..4622459e77df 100644 --- a/nixos/tests/systemd-sysusers-password-option-override-ordering.nix +++ b/nixos/tests/systemd-sysusers-password-option-override-ordering.nix @@ -5,10 +5,8 @@ }: let password = "test"; - password1 = "test1"; hashedPassword = "$y$j9T$wLgKY231.8j.ciV2MfEXe1$P0k5j3bCwHgnwW0Ive3w4knrgpiA4TzhCYLAnHvDZ51"; # test hashedPassword1 = "$y$j9T$s8TyQJtNImvobhGM5Nlez0$3E8/O8EVGuA4sr1OQmrzi8GrRcy/AEhj454JjAn72A2"; # test - hashed_sha512crypt = "$6$ymzs8WINZ5wGwQcV$VC2S0cQiX8NVukOLymysTPn4v1zJoJp3NGyhnqyv/dAf4NWZsBWYveQcj6gEJr4ZUjRBRjM0Pj1L8TCQ8hUUp0"; # meow hashedPasswordFile = pkgs.writeText "hashed-password" hashedPassword1; in @@ -57,7 +55,9 @@ in with subtest("systemd-sysusers.service contains the credentials"): sysusers_service = machine.succeed("systemctl cat systemd-sysusers.service") print(sysusers_service) + # Both are in the unit, but the hashed password takes precedence as shown below. assert "SetCredential=passwd.plaintext-password.alice:${password}" in sysusers_service + assert "SetCredential=passwd.hashed-password.alice:${hashedPassword}" in sysusers_service with subtest("Correct mode on the password files"): assert machine.succeed("stat -c '%a' /etc/passwd") == "644\n"