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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos-install: ask which admin account to set password for #124602

Closed
wants to merge 4 commits into from

Conversation

misuzu
Copy link
Contributor

@misuzu misuzu commented May 27, 2021

Motivation for this change

Fixes: #97422
Fixes: #104343

Tested with jane user uncommented in generated configuration.nix:

  • New nixos-install with new nixpkgs - promts for username, jane or root.
    If users.mutableUsers is set to false or one of users have delared password in config, it doesn't asks any passwords.
  • New nixos-install with old nixpkgs - promts for username, only root.
  • Old nixos-install with new nixpkgs - asks for root password, just like before.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

@misuzu misuzu force-pushed the nixos-install-passwd branch 3 times, most recently from fb0f29d to 1aa3888 Compare May 27, 2021 20:52
@misuzu misuzu marked this pull request as ready for review May 27, 2021 21:00
@misuzu misuzu requested a review from jtojnar May 27, 2021 21:00
@misuzu misuzu requested a review from ryantm as a code owner May 27, 2021 21:26
@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 124602 at e6a544ea run on x86_64-linux 1

2 packages built successfully:
  • nixos-install-tools
  • tests.trivial

Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Looks good.

nixos/modules/installer/tools/nixos-install.sh Outdated Show resolved Hide resolved
@jtojnar jtojnar requested a review from edolstra May 28, 2021 01:21
@roberth
Copy link
Member

roberth commented May 28, 2021

--no-root-password should become --no-set-password, but we'll want to accept the old syntax as well, not to break the install automation scripts out there.

@roberth
Copy link
Member

roberth commented May 28, 2021

@GrahamcOfBorg test installer.simple installer.lvm installer.simpleSpecialised

(kind of random selection; most variations are about file systems, block devices, boot loaders, which don't matter here)

@edolstra
Copy link
Member

What if I have a lot of declarative users? Seems potentially undesirable to prompt for passwords for all of them.

@misuzu
Copy link
Contributor Author

misuzu commented May 28, 2021

What if I have a lot of declarative users? Seems potentially undesirable to prompt for passwords for all of them.

I can add --only-root-password or something to ask only root password.

@dotlambda
Copy link
Member

--no-root-password should become --no-set-password, but we'll want to accept the old syntax as well, not to break the install automation scripts out there.

Isn't the purpose of --no-root-password to prevent logging into the root account by means other than sudo? The semantics should remain the same.

@jtojnar
Copy link
Member

jtojnar commented May 28, 2021

Alternative would be prompting for each user:

Found the following users without passwords: root, adam, eve
Do you want to set password for root?
[Yes, no] n
Do you want to set password for root?
[Yes, no] 
setting adam password...

@edolstra
Copy link
Member

In any case we don't need to ask passwords for all declarative users. We only need to ensure that at least one wheel user has a password.

@misuzu
Copy link
Contributor Author

misuzu commented May 28, 2021

--no-root-password should become --no-set-password, but we'll want to accept the old syntax as well, not to break the install automation scripts out there.

Isn't the purpose of --no-root-password to prevent logging into the root account by means other than sudo? The semantics should remain the same.

I've reverted this change. nixos-install < /dev/null can still be used for unattended installations.

Alternative would be prompting for each user:

It's now prompting for each user if password is needed.

In any case we don't need to ask passwords for all declarative users. We only need to ensure that at least one wheel user has a password.

Should I then filter users by membership in wheel group?

@jtojnar
Copy link
Member

jtojnar commented May 28, 2021

I've reverted this change. nixos-install < /dev/null can still be used for unattended installations.

It would still be nice to have explicit --no-interaction option then (so that it can be listed on help page).

nixos/modules/installer/tools/nixos-install.sh Outdated Show resolved Hide resolved
nixos/modules/installer/tools/nixos-install.sh Outdated Show resolved Hide resolved
nixos/modules/installer/tools/nixos-install.sh Outdated Show resolved Hide resolved
nixos/modules/installer/tools/nixos-install.sh Outdated Show resolved Hide resolved
nixos/modules/installer/tools/nixos-install.sh Outdated Show resolved Hide resolved
nixos/modules/installer/tools/nixos-install.sh Outdated Show resolved Hide resolved
@dasJ
Copy link
Member

dasJ commented Nov 26, 2021

Tbh, I don't see the point in this at all and I just see myself annoyed by having to specify n 20 times when recovering production servers or having to look up the new option from the manual every time.

@misuzu
Copy link
Contributor Author

misuzu commented Nov 26, 2021

Tbh, I don't see the point in this at all and I just see myself annoyed by having to specify n 20 times when recovering production servers or having to look up the new option from the manual every time.

There is a linked issue with main pain points: #97422
What would you recommend doing instead? I really don't like the current state of things, it's an awful experience when onboarding people.

@dasJ
Copy link
Member

dasJ commented Nov 26, 2021

What if we figured out which users already have passwords by checking $target/etc/shadow (if it exists → it's a reinstall of an existing system) and ask for a password if none of the wheel members have one? If the file doesn't exist it's a new installation and asking for the password may be the correct thing even if I'd prefer opting out by configuration.nix. I'm thinking something around

You seem to have no user with a password in the wheel group and your root user appears to have no password.
This means you won't be able to gain root permissions on your new system. Please supply the name of a user in
the wheel group username and a new password. Supplying no username will cause no password to be set.
Valid users are: janne, deployment-user, root
Username [root]:
Password:
Password again:

I actually dislike the current way where it asks for the root password by default even if you have already set one.
Btw this should probably also handle security.sudo.wheelNeedsPassword and security.sudo.enable.

@misuzu misuzu changed the title nixos-install: ask password for all users, not just root nixos-install: ask which admin account to set password for Nov 26, 2021
@misuzu
Copy link
Contributor Author

misuzu commented Nov 26, 2021

by checking $target/etc/shadow (if it exists → it's a reinstall of an existing system)

/etc/shadow is created when nixos-enter is executed, so it's really unreliable indicator.

@misuzu misuzu requested a review from dasJ November 26, 2021 17:41
@misuzu misuzu force-pushed the nixos-install-passwd branch 2 times, most recently from 8bf3288 to ca99e84 Compare May 23, 2022 19:29
@misuzu
Copy link
Contributor Author

misuzu commented May 23, 2022

@ofborg test installer.simple installer.lvm installer.simpleSpecialised

@misuzu
Copy link
Contributor Author

misuzu commented Jun 1, 2022

installer.simpleSpecialised test is already broken on master

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/905

Copy link
Member

@teutat3s teutat3s left a comment

Choose a reason for hiding this comment

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

Tested the following locally, to reproduce:

git clone https://github.com/NixOS/nixpkgs && cd nixpkgs
git fetch origin pull/124602/head:docs-user-onboarding
git checkout docs-user-onboarding
nix-build nixos/tests/installer.nix -A simple

Result:

...
(finished: waiting for the VM to power off, in 1.21 seconds)
(finished: run the VM test script, in 1888.63 seconds)
test script finished in 1888.63s
cleanup
(finished: cleanup, in 0.00 seconds)
/nix/store/nd6f5f05vs5gz9jbxbcgzkvagay50kca-vm-test-run-installer-simple

@misuzu
Copy link
Contributor Author

misuzu commented Jul 28, 2022

Feel free to pick this up.

@misuzu misuzu closed this Jul 28, 2022
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/low-effort-and-high-impact-tasks/21095/45

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/where-did-you-get-stuck-in-the-nix-ecosystem-tell-me-your-story/31415/13

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.

Slightly confusing user management in the NixOS installation guide