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

states.user.present: Make usage of `hash_password` idempotent #47147

Merged
merged 2 commits into from Jun 30, 2018

Conversation

Projects
None yet
4 participants
@eliasp
Member

eliasp commented Apr 18, 2018

What does this PR do?

It fixes the non-idempotent behavior of states.user.present when hash_password: True described in #45939
It targets 2018.3, but due to it's trivial nature, it should be easily cherry-picked for backports to older releases.

What issues does this PR fix or reference?

#45939

Previous Behavior

Every time states.user.present with hash_password: True was used, a new shadow hash was generated based on a new randomly generated salt value for hashing which made this state's behavior idempotent.

New Behavior

Before hashing a password, it checks now whether an existing salt can be retrieved.
If yes, it will be re-used for generating the hash.
If not, it defaults to using a new randomly generated salt.

Tests written?

No

Commits signed with GPG?

Yes

@eliasp

This comment has been minimized.

Member

eliasp commented Apr 18, 2018

@cachedout

This comment has been minimized.

Contributor

cachedout commented Apr 18, 2018

I totally see the case for this but would love to have some tests written around this behavior. Any chance you'd be willing to write some?

@cachedout

This comment has been minimized.

Contributor

cachedout commented Apr 23, 2018

@eliasp Did you see my comment above regarding tests?

@eliasp

This comment has been minimized.

Member

eliasp commented Apr 23, 2018

Sorry, I initially planned on adding some tests, but won't have time right now to complete this.
Feel free to take it from here for now:

  • needs tests
  • when the result of hashing the salt+password differs, re-generate using a new random salt as suggested by @tigpas in #45939 (comment)

@cachedout cachedout requested a review from isbm May 1, 2018

# hash to change each time and thereby making the
# user.present state non-idempotent.
algorithms = {
'1': 'md5',

This comment has been minimized.

@isbm

isbm May 1, 2018

Contributor

But this is insecure. I think we should either kill this option or at least scream all around the place in logs if anyone will use this.

This comment has been minimized.

@eliasp

eliasp May 2, 2018

Member

I do agree that "screaming all around the place in logs" is the most reasonable reaction here…

Added a corresponding warning.

@rallytime rallytime requested a review from isbm May 22, 2018

@isbm

isbm approved these changes May 22, 2018

@rallytime rallytime merged commit 9b364e2 into saltstack:2018.3 Jun 30, 2018

5 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18641 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4589 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9513 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #24761 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #16889 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22468 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21509 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment