-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos-install: don't prompt for passwords when mutableUsers = false #104343
Conversation
@@ -200,7 +200,7 @@ fi | |||
# Ask the user to set a root password, but only if the passwd command | |||
# exists (i.e. when mutable user accounts are enabled). | |||
if [[ -z $noRootPasswd ]] && [ -t 0 ]; then | |||
if nixos-enter --root "$mountPoint" -c 'test -e /nix/var/nix/profiles/system/sw/bin/passwd'; then | |||
if jq -e '.mutableUsers' "$system/users-groups.json" > /dev/null; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should also have a fallback to cover when the json file doesnt exist, for users installing an old nixpkgs with a new nixos-install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, thanks Clever. I made the logic here a bit more robust against that.
87,640 PRs / issues later :) I just tried this patch on a fresh install on my new laptop, and it worked a treat. |
@@ -200,7 +200,7 @@ fi | |||
# Ask the user to set a root password, but only if the passwd command | |||
# exists (i.e. when mutable user accounts are enabled). | |||
if [[ -z $noRootPasswd ]] && [ -t 0 ]; then | |||
if nixos-enter --root "$mountPoint" -c 'test -e /nix/var/nix/profiles/system/sw/bin/passwd'; then | |||
if jq -e '.mutableUsers' "$system/users-groups.json" > /dev/null; then | |||
set +e | |||
nixos-enter --root "$mountPoint" -c 'echo "setting root password..." && /nix/var/nix/profiles/system/sw/bin/passwd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might as well ask to set passwords for all isNormalUser
users in users-groups.json
without initialHashedPassword
, hashedPassword
, initialPassword
, password
or passwordFile
set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is in scope for this PR. Didn't the installer used to do that, and are we sure it doesn't still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only asks password for root. You have to log in as root on tty to set user password after installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should probably be a feature ticket and second PR, since it is adding new behavior -- and not necessarily trivial behavior, either, since:
- a reasonable implementation might should also work with subsequently added users, after installation
- it should work in a headless environment without a tty
This fixes a regression reported in NixOS#95778, caused by NixOS#16703.
b26835e
to
0b36e65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me. I'd love if the commit message actually had a sentence about the motivation. The references are nice but having another piece of information there could be helpful while "digging through the archives".
I am uneasy approving it as I haven't tested it and apparently we have zero tests that verify that the password prompt during installation actually works. If it isn't too much to ask could you add that bit to one (some?) of the tests?
@andir I'd be happy to! I'm not super sure how to go about writing that test, but I'll take a look. Thanks. |
I marked this as stale due to inactivity. → More info |
:( Why did you close it? |
Can we reopen this @grahamc? It's still relevant and an easy fix 🙂 |
This fixes a regression reported in #95778, caused by #16703.
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)