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/users:added users.allowLoginless #73106
Conversation
Cool, what about allowing multiple users with the same UID? Should we make that a separate option? |
@pstn for that there is already |
@@ -473,15 +475,15 @@ in { | |||
''; | |||
}; | |||
|
|||
# FIXME: obsolete - will remove. |
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.
@edolstra, you put this comment - Is it okay to remove it like this?
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/86 |
Thank you for your contributions.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
||
users.allowLoginless = mkOption { |
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'm not fond of this option name since I don't know what "loginless" means.
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.
Maybe allowNoPasswordLogin
would be better?
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.
What about ssh keys though? Loginless is more accurate, even if not obvious.
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 mean, I can't really parse "loginless". Maybe invert the meaning and rename to requireLoginUser
?
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.
invert
un-invert even. I like this.
Maybe requireLoginCapableUser
if explicit is good.
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 also have a general problem with the usage of the term login here. For example the command to get a root shell in a nixos container without any authentication inside the container is called nixos-container root-login
. You are logging in, you are just not authenticating. Maybe the option should explicitly mention password authentication since this is what it's really about?
Would requirePasswordedUser
be alright?
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.
Would requirePasswordedUser be alright?
An ssh key is also sufficient, so that name is not accurate.
The container profile module can disable this check. I suppose it could be refactored so that all ways to satisfy the check are written in the same way, open for extension, but I don't want this idea to hinder progress.
Ok, I've addressed @edolstra 's concerns. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Please rebase this PR on master. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
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.
Looks alright; 1 comment and needs a rebase.
24edf41
to
ec96cf0
Compare
Evaluation error needs to be resolved. |
a832169
to
910cf2a
Compare
Correct the assertion logic Fixed indentation Better wording od allowLoginless' description Co-authored-by: Eelco Dolstra <edolstra@gmail.com> Better formatting Co-authored-by: Eelco Dolstra <edolstra@gmail.com> allowLoginless -> allowNoPasswordLogin Clarified users.allowNoPasswordLogin's description Clarified assertion expression Co-authored-by: Robert Hensing <roberth@users.noreply.github.com> Reworded assertion message to gude to safer alternative
Thank you! |
Motivation for this change
Fixes #73003
Also made security.initialRootPassword a renamed option
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @