nixos/users-groups: do not check validity of special hashes #91238

rnhmjoj · 2020-06-21T15:56:50Z

Motivation for this change

Fixup of PR #83171

Things done

Tested using sandboxing (nix.useSandbox on NixOS)
Built manual
Tested via one or more NixOS test(s): `nixosTests.mutableUsers``
Ensured that relevant documentation is up to date
Fits CONTRIBUTING.md.

Profpatsch · 2020-06-22T17:34:10Z

nixos/modules/config/users-groups.nix

+      [ "users" "users" "root" "initialHashedPassword" ]
+      (cfg: if cfg.security.initialHashedPassword == "!"
+            then null
+            else cfg.security.initialHashedPassword))


This means that the change is backwards-compatible with people setting security.initialRootPassword, right?

Yes, it is. Also this option has been hidden since 2014, so the impact should be small.

Profpatsch · 2020-06-22T17:37:12Z

nixos/modules/config/users-groups.nix

@@ -588,7 +593,7 @@ in {
             || cfg.group == "wheel"
             || elem "wheel" cfg.extraGroups)
            &&
-            ((cfg.hashedPassword != null && cfg.hashedPassword != "!")
+            (cfg.hashedPassword != null


I’m super uncomfortable with this line, somebody else will have to confirm that it’s valid.

There are two possible cases I can think of:

using the deprecated security.initialRootPassword "!" will be turned into a null, so it's covered

using users.users.<name>.hashedPassword or similar, "!" will issue the warning about possibly being locked out.

But yes, it would be better is someone familiar with this module would comment.

Looks to me like @edolstra is the only one who touched this code in the past.

Yeah, user using users.users.<name>.hashedPassword that is not a hash will lock themselves out if they miss the warning so the assertion still makes sense. It is even too narrow here – I use * as a root’s hash and this would not save me. Not sure we can extend it much further without causing false positives, though.

jtojnar

We should definitely add an assertion that hashedPassword cannot contain : since that will definitely break the shadowfile as reported in Enforce no ":" in "hashedPassword" (perhaps a check) #12008
There are other commonly used schemes for hashedPassword. Personally, I set it to * for root, since hash prefixed with ! means the account is locked according to shadow(5). Different systems might support special sequences with even wilder semantics like !! or *NS* https://unix.stackexchange.com/questions/252016/difference-between-vs-vs-in-etc-shadow

jtojnar · 2020-06-23T14:27:39Z

nixos/modules/config/users-groups.nix

@@ -588,7 +593,7 @@ in {
             || cfg.group == "wheel"
             || elem "wheel" cfg.extraGroups)
            &&
-            ((cfg.hashedPassword != null && cfg.hashedPassword != "!")
+            (cfg.hashedPassword != null


Yeah, user using users.users.<name>.hashedPassword that is not a hash will lock themselves out if they miss the warning so the assertion still makes sense. It is even too narrow here – I use * as a root’s hash and this would not save me. Not sure we can extend it much further without causing false positives, though.

rnhmjoj · 2020-06-23T15:26:58Z

There are other commonly used schemes for hashedPassword. Personally, I set it to * for root, since hash prefixed with ! means the account is locked according to shadow(5). Different systems might support special sequences with even wilder semantics like !! or NS

What is the point of setting the hash to "!" when null is the default (and the update-users-groups.pl script locks users with "!" when the hash is null)? It's not a rhetoric question, I'm honestly asking.

If these special hashes are common I'll add exceptions, but to me it feel like an implementation detail that shouldn't leak to the NixOS configuration: the options users.users don't mention /etc/shadow anywhere.

jtojnar · 2020-06-23T17:26:37Z

What is the point of setting the hash to "!" when null is the default (and the update-users-groups.pl script locks users with "!" when the hash is null)? It's not a rhetoric question, I'm honestly asking.

I did not know that before reviewing the code. I would expect null to map to empty string (i.e. no password) so I set the hash to *. Also reading the shadow(5), using * makes more sense to me than !.

If these special hashes are common I'll add exceptions, but to me it feel like an implementation detail that shouldn't leak to the NixOS configuration: the options users.users don't mention /etc/shadow anywhere.

Yeah, this it a personal preference. Sometimes I like it when NixOS modules map directly to UNIX concepts and do not add much special logic. Especially, if I was familiar with the config before switching to NixOS.

rnhmjoj · 2020-06-25T08:26:50Z

I added a check for "!", "!!" and "*" in the assertion and the warning as you requested.
I also added another assertion for avoiding ":"s in the password hash, since these are guaranteed to break havoc.
The main concerns should be answered.

nixos/modules/config/users-groups.nix

Profpatsch

As far as I can see, this is fine now

DIzFer · 2020-07-03T18:53:23Z

I seem to not understand what this PR is achieving on my system: I'm now getting the intended warning for a hashedPassword == "!"... which is exactly what I expect users.users.root.hashedPasswords to be. Am I expected (and doomed) to ignore a warning on every rebuild until the end of times?

rnhmjoj · 2020-07-03T19:13:44Z

No, you're not supposed to get that warning and this PR will fix it. The warning is intended to check valid hashes but in the original PR (#83171) I missed the edge cases of "!", "*" and others.

nh2 · 2020-07-03T23:51:05Z

Confirming that commit 470ce47 triggers a spurious warning, e.g. for users.extraUsers.root.password = ""; or users.extraUsers.root.password = "a"; (i came here via bisection of it).

rnhmjoj · 2020-07-04T07:53:15Z

@nh2 Note that It's not root.password, which is never checked, but root.initialHashedPassword set to "!".

This option has been deprecated for a long time because is redundant (users.users.root.initialHashedPassword exists). Moreover, being of type string, it required to handle the special value "!" separately, instead of using just `null`.

This explanation was contained in the description of security.initialRootPassword but got lost when it was deprecated a long ago (f496c3c) and removed.

rnhmjoj requested a review from infinisil June 21, 2020 15:56

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jun 21, 2020

Profpatsch reviewed Jun 22, 2020

View reviewed changes

jtojnar reviewed Jun 23, 2020

View reviewed changes

rnhmjoj force-pushed the users branch from 67fb3f0 to a313ed9 Compare June 25, 2020 00:05

rnhmjoj referenced this pull request Jun 25, 2020

nixos/users: validate password hashes

470ce47

Profpatsch reviewed Jun 25, 2020

View reviewed changes

nixos/modules/config/users-groups.nix Outdated Show resolved Hide resolved

Profpatsch reviewed Jun 25, 2020

View reviewed changes

nixos/modules/config/users-groups.nix Outdated Show resolved Hide resolved

nixos/modules/config/users-groups.nix Outdated Show resolved Hide resolved

rnhmjoj force-pushed the users branch from a313ed9 to d9deb4f Compare June 25, 2020 17:35

Profpatsch approved these changes Jul 1, 2020

View reviewed changes

rnhmjoj force-pushed the users branch from d9deb4f to ce62296 Compare July 3, 2020 19:16

rnhmjoj force-pushed the users branch from ce62296 to 37a188b Compare July 4, 2020 08:11

rnhmjoj added 5 commits July 4, 2020 12:14

nixos/users-groups: clearly document special hash values

900ae97

This explanation was contained in the description of security.initialRootPassword but got lost when it was deprecated a long ago (f496c3c) and removed.

nixos/users-groups: do not check validity of empty hashes

751c2ed

nixos/users-groups: add assertion for ":" in hashes

99899e2

nixos/users-groups: handle password hashes with special meaning

c37347a

rnhmjoj force-pushed the users branch from 37a188b to c37347a Compare July 4, 2020 10:22

rnhmjoj merged commit 01c4a38 into NixOS:master Jul 7, 2020

RaghavSood mentioned this pull request Jul 11, 2020

nixpkgs-unstable: attribute 'initialHashedPassword' missing, even when it is provided #92910

Closed

chkno mentioned this pull request Jan 3, 2022

No way to disable the at-least-one-priviledged-user check #153288

Open

rnhmjoj deleted the users branch July 10, 2023 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/users-groups: do not check validity of special hashes #91238

nixos/users-groups: do not check validity of special hashes #91238

rnhmjoj commented Jun 21, 2020 •

edited

Profpatsch Jun 22, 2020

rnhmjoj Jun 22, 2020

Profpatsch Jun 22, 2020

rnhmjoj Jun 23, 2020

Profpatsch Jun 23, 2020

jtojnar Jun 23, 2020

jtojnar left a comment

jtojnar Jun 23, 2020

rnhmjoj commented Jun 23, 2020

jtojnar commented Jun 23, 2020

rnhmjoj commented Jun 25, 2020

Profpatsch left a comment

DIzFer commented Jul 3, 2020

rnhmjoj commented Jul 3, 2020

nh2 commented Jul 3, 2020

rnhmjoj commented Jul 4, 2020

nixos/users-groups: do not check validity of special hashes #91238

nixos/users-groups: do not check validity of special hashes #91238

Conversation

rnhmjoj commented Jun 21, 2020 • edited

Motivation for this change

Things done

Profpatsch Jun 22, 2020

Choose a reason for hiding this comment

rnhmjoj Jun 22, 2020

Choose a reason for hiding this comment

Profpatsch Jun 22, 2020

Choose a reason for hiding this comment

rnhmjoj Jun 23, 2020

Choose a reason for hiding this comment

Profpatsch Jun 23, 2020

Choose a reason for hiding this comment

jtojnar Jun 23, 2020

Choose a reason for hiding this comment

jtojnar left a comment

Choose a reason for hiding this comment

jtojnar Jun 23, 2020

Choose a reason for hiding this comment

rnhmjoj commented Jun 23, 2020

jtojnar commented Jun 23, 2020

rnhmjoj commented Jun 25, 2020

Profpatsch left a comment

Choose a reason for hiding this comment

DIzFer commented Jul 3, 2020

rnhmjoj commented Jul 3, 2020

nh2 commented Jul 3, 2020

rnhmjoj commented Jul 4, 2020

rnhmjoj commented Jun 21, 2020 •

edited