Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/users-groups: warn on ambiguous password settings #287506

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 9, 2024

Description of changes

After 4b12800 it took me a while in a test setup to find out why root didn't have the password anymore I declared in my config.

Because of that I got reminded how the order of preference works for the password options:

hashedPassword > password > hashedPasswordFile

If the user is new, initialPassword & initialHashedPassword are also relevant. Also, the override is silent in contrast to any other conflicting definition in NixOS.

To make this less surprising I decided to warn in such a case - assertions would probably break too much that technically works as intended.

Also removed the initialHashedPassword for root. This would cause a warning whenever you set something in your own config and a ! is added automatically by users-groups.pl.

systemd-sysusers also seems to implement these precedence rules, so having the warning for that case also seems useful.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

After 4b12800 it took me a while in a
test setup to find out why `root` didn't have the password anymore I
declared in my config.

Because of that I got reminded how the order of preference works for the
password options:

    hashedPassword > password > hashedPasswordFile

If the user is new, initialPassword & initialHashedPassword are also
relevant. Also, the override is silent in contrast to any other
conflicting definition in NixOS.

To make this less surprising I decided to warn in such a case -
assertions would probably break too much that technically works as
intended.

Also removed the `initialHashedPassword` for `root`. This would cause a
warning whenever you set something in your own config and a `!` is added
automatically by `users-groups.pl`.

`systemd-sysusers` also seems to implement these precedence rules, so
having the warning for that case also seems useful.
@lheckemann lheckemann merged commit 5863c27 into NixOS:master Feb 16, 2024
22 checks passed
@Ma27 Ma27 deleted the warn-user-password-options branch February 17, 2024 12:52
@uninsane
Copy link
Contributor

uninsane commented Feb 18, 2024

i'm getting this warning now, but unless the behavior of these options has changed over the last 18 months or so i'm very intentionally setting both options:

users.users.colin = {
  # initial password is empty, in case anything goes wrong.
  # if `colin-passwd` (a password hash) is successfully found/decrypted, that becomes the password at boot.
  initialPassword = "";
  hashedPasswordFile = config.sops.secrets.colin-passwd.path;
};

the setup for providing the hashed password file here requires that sops (secrets provisioning) and impermanence and any other relevant activation script is is aware of eachother and ordered correctly. if not, and i only provide hashedPasswordFile, then a bad update locks me out. and so, i quite intentionally leave myself this escape hatch after having benefited from it in the past.

warning message is a great idea because plenty of people experience exactly the reverse of me here: but is there a way i can silence the warning for my own usecase?

thanks... & sorry to be "that spacebar heater guy"
XKCD "workflow" comic

@Ma27
Copy link
Member Author

Ma27 commented Feb 20, 2024

So I did a small test on one of my VMs. First, I created a user like this:

{
  users.users.snens = {
    password = "abcdefg";
    isNormalUser = true;
  };
}

As expected, the user snens has the password abcdefg.
Then, I changed the expression like this:

{
  users.users.snens = {
    hashedPasswordFile = "/this/path/does/not/exist";
    isNormalUser = true;
  };
}

The activation script warned me about the file not existing, but activation went through. After that the password was still abcdefg, so I wasn't locked out of the user.

Are you maybe using impermanence and/or mutable users?

but is there a way i can silence the warning for my own usecase

I'm not aware of being able to silence the warning in particular. You can forcefully disable all warnings via warnings = lib.mkForce [];, but I'm pretty sure that this is not want you want.

But that's a good point, maybe it should be possible to acknowledge/silence certain warnings, but this is something that should be fixed at a larger scale. cc @infinisil @roberth for that.

@uninsane
Copy link
Contributor

Are you maybe using impermanence and/or mutable users?

immutable users: no. impermanence: yes. unfortunately a different one from the nix-community impermanence module most are using, so i can't share an easily reproducible config. it's good to know the users activation script is resilient against misconfigured hashedPasswordFile outside of impermanence though.

maybe i'll toy with providing an options.warnings.apply function that filters out specific warnings. i think option type merging works in a way that allows me to do that externally, or at least, it'd be good for me to learn if that's possible :)

@Ma27
Copy link
Member Author

Ma27 commented Feb 20, 2024

unfortunately a different one from the nix-community impermanence module most are using, so i can't share an easily reproducible config

I asked specifically for impermanence because it's not possible to persist /etc/shadow & /etc/passwd there. IIRC the impermanence script would need to run before the users-groups activation script, but that's not possible for reasons I forgot.

Do you have a similar issue? Because if you actually can save your /etc/shadow & /etc/passwd across reboots, then I wouldn't expect you to run into this issue at all: users-groups doesn't seem to touch the password field in shadow if it can't read from the file as demonstrated above, so the old one is kept.

@uninsane
Copy link
Contributor

@Ma27 i'm fairly certain /etc/shadow & /etc/passwd are recreated on my system each time it boots. because mutableUsers = false, there's no reason to want to persist these.

for the record, options.warnings.apply does work as a backdoor if absolutely necessary. not that i'd recommend it, but that does give less urgency to figuring out the right UX around all this:

{ lib, ... }:
let
  ignored = [ "some\nwarning\ni don't want to see anymore" ];
in {
  options = {
    warnings = lib.mkOption {
      apply = builtins.filter (w: !(builtins.elem w ignored));
    };
  };
}

@infinisil
Copy link
Member

But that's a good point, maybe it should be possible to acknowledge/silence certain warnings, but this is something that should be fixed at a larger scale. cc @infinisil @roberth for that.

I tried in #97023, but it had to be reverted. There's also some links to related issues/PRs there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants