-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
Give warning if user inputs not encrypted password to user module #43615
Conversation
Check the password format and notify user if they input unencrypted password.
The test
|
lib/ansible/modules/system/user.py
Outdated
else: | ||
maybe_invalid = True | ||
if maybe_invalid: | ||
self.module.warn("The iunput password seems not been hashed, " |
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.
"iunput" --> "input"
This is a very nice sanity check. Can you add some integration tests for this as well? |
Since some testing platfrom has no passlib installed
Tests look good. Thanks! Please create a changelog fragment and I'll get this merged (sorry I forgot to mention that earlier). See fragments for examples. |
@samdoran I have created a changelog fragment, could you review again? Thanks! |
lib/ansible/modules/system/user.py
Outdated
maybe_invalid = True | ||
if maybe_invalid: | ||
self.module.warn("The input password seems not been hashed, " | ||
"please note that 'password' argument requires an encrypted value or the password will not work properly.") |
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.
The English on this sentence needs to be reworked. "The input password appears not to have been hashed." "The 'password' argument must be encrypted for this module to work properly."
lib/ansible/modules/system/user.py
Outdated
else: | ||
fields = self.module.params['password'].split("$") | ||
if len(fields) >= 3: | ||
# contains character outside crypt constrain |
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.
"outside the crypto constraint"
lib/ansible/modules/system/user.py
Outdated
if self.module.params['password'] and self.platform != 'Darwin': | ||
maybe_invalid = False | ||
# : for delimiter, * for disable user, ! for lock user | ||
# these character are invalid in password |
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.
s/character/characters
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.
"the password"
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.
Thanks for reworking the words
@@ -0,0 +1,3 @@ | |||
--- | |||
minor_changes: | |||
- user module - add a sanity check for user password and a more helpful warning message (https://github.com/ansible/ansible/pull/43615) |
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.
Sorry one more nit: "a user's password"
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.
No problem!
@ericwb Fixed the grammar mistake. |
@samdoran Need any other change? |
SUMMARY
user
module doesn't check the input password and return very little information if users input unencrypted password (which is wrong), it's hard for users to figure out what's wrong. This PR check the format of the input password and give a warning if the password is not encrypted.Fix #28772
ISSUE TYPE
COMPONENT NAME
/lib/ansible/modules/system/user.py
ANSIBLE VERSION