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

Adds **kwargs parameter to password_hash to support more algorithms. #45692

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

mfuchs
Copy link
Contributor

@mfuchs mfuchs commented Sep 16, 2018

SUMMARY

Adds **kwargs parameter to password_hash to support more algorithms.

This adds support for the bcrypt algorithm as described in the documentation.
The msdcc and msdcc2 algorithm are also supported now.

Fixes #45392

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

utils/encrypt

ANSIBLE VERSION
ansible 2.7.0rc2.post0

@mfuchs
Copy link
Contributor Author

@mfuchs mfuchs commented Sep 16, 2018

@abadger I know you were not a fan of **kwargs in #21215.
But I am not sure if it is good to add even further parameters to the functions (i.e. 'user' and 'ident').

@mscherer
Copy link
Contributor

@mscherer mscherer commented Sep 16, 2018

Please do separate PR if you start to do multiple change, or at least, multiple commits.

@ansibot
Copy link
Contributor

@ansibot ansibot commented Sep 16, 2018

@ansibot ansibot added affects_2.8 bug docs needs_revision support:community support:core labels Sep 16, 2018
lib/ansible/utils/encrypt.py Outdated Show resolved Hide resolved
@@ -170,7 +174,10 @@ def _hash(self, secret, salt, salt_size, rounds):

# starting with passlib 1.7 'using' and 'hash' should be used instead of 'encrypt'
if hasattr(self.crypt_algo, 'hash'):
result = self.crypt_algo.using(**settings).hash(secret)
if self.algorithm in ('msdcc', 'msdcc2'):
result = self.crypt_algo.hash(secret, **settings)
Copy link
Contributor

@mscherer mscherer Sep 16, 2018

Choose a reason for hiding this comment

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

So, this doesn't verify anymore that user is set, unlike the previous PR, I think that should be corrected.

Copy link
Contributor Author

@mfuchs mfuchs Sep 16, 2018

Choose a reason for hiding this comment

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

It is done implicitly by passlib.
So if the user is not set an exception would be thrown, similar to what #45392 experienced.

This is the same behaviour as for all other hashing algorithms.
I.e. the checking of what parameters are supported/needed is done by passlib.

Copy link
Contributor

@mscherer mscherer Sep 16, 2018

Choose a reason for hiding this comment

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

Throwing a exception do not seems like great UX for users.

Copy link
Contributor Author

@mfuchs mfuchs Sep 17, 2018

Choose a reason for hiding this comment

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

I think from a maintenance point it is easier to let passlib handle the parameters.
Otherwise we would have to take care of each algorithm ourselves and have a list of required and optional parameters for them.
It becomes worse if algorithms retrieve new paramters that we already support.
Now iff new algorithms are added to passlib we support them automatically.
Given that they use the 'using' and 'hash' functions.

@abadger
Copy link
Contributor

@abadger abadger commented Sep 16, 2018

@mfuchs
Copy link
Contributor Author

@mfuchs mfuchs commented Sep 16, 2018

@mscherer do you mean splitting it up into two commits: One adding bcrypt support and the other for msdcc/msdcc/2?

@abadger ok, I removed **kwargs again. :)

@mscherer
Copy link
Contributor

@mscherer mscherer commented Sep 16, 2018

Yes, so it is easier to see the meaning of each, and review them.

@mfuchs
Copy link
Contributor Author

@mfuchs mfuchs commented Sep 17, 2018

@mscherer I split the commit into two parts.

@ansibot ansibot added stale_ci stale_review labels Sep 26, 2018
@ansibot ansibot added needs_rebase and removed stale_review labels Oct 12, 2018
@ansibot ansibot removed the support:community label Nov 26, 2018
@ansibot ansibot added the docsite label Dec 4, 2018
@samccann
Copy link
Contributor

@samccann samccann commented Jun 5, 2019

@mfuchs Sorry for the delays. Can you rebase and resolve any open review comments on this PR?

Thanks!

@samccann samccann added the needs_info label Jun 5, 2019
mfuchs added 2 commits Jun 8, 2019
The ident parameter, as described in the documentation, was not supported.
This is done by adding the 'user' parameters for msdcc/msdcc2.
Furthermore msdcc/msdcc2 does not support the 'using' function with passlib
1.7 and newer.
Thus there is specific handling if these algorithms are used.

Fixes ansible#45392
@mfuchs mfuchs force-pushed the bcrypt_msdcc_support branch from 45b4e2a to 8dd74c2 Compare Jun 8, 2019
@mfuchs
Copy link
Contributor Author

@mfuchs mfuchs commented Jun 8, 2019

@samccann rebased and also resolved all comments.

@ansibot ansibot removed needs_info needs_rebase stale_ci labels Jun 8, 2019
@ansibot ansibot added stale_ci stale_review labels Jun 18, 2019
@ansibot ansibot removed the stale_review label Jun 26, 2019
@samccann
Copy link
Contributor

@samccann samccann commented Jul 2, 2019

@mscherer @abadger - can you take another look at this PR to see if everything has been addressed? (and merge if you approve?) Thanks!

@ansibot ansibot added the stale_review label Jul 2, 2019
@@ -848,6 +848,13 @@ Some hash types allow providing a rounds parameter::

{{ 'secretpassword' | password_hash('sha256', 'mysecretsalt', rounds=10000) }}

When`Passlib <https://passlib.readthedocs.io/en/stable/>`_ is installed
`password_hash` supports more crypt schematas::
Copy link
Contributor

@mscherer mscherer Oct 24, 2019

Choose a reason for hiding this comment

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

Small typo (not blocking, but since I review)

@mscherer
Copy link
Contributor

@mscherer mscherer commented Oct 24, 2019

You should also add a changelog fragment (IIRC the new merging rule, I am a bit out of touch)

@ansibot ansibot added core_review and removed needs_revision stale_review labels Oct 24, 2019
@ansibot ansibot added needs_rebase needs_revision and removed core_review labels Dec 19, 2019
@ansibot ansibot added the needs_triage label May 16, 2020
@mkrizek mkrizek removed the needs_triage label May 18, 2020
@tumluliu
Copy link

@tumluliu tumluliu commented Sep 8, 2020

Hi @mfuchs , thanks a lot for the nice job! Is there any news that when this can be merged? The ident param is very required in our use cases

@ansibot ansibot added pre_azp and removed stale_ci labels Dec 5, 2020
@bcoca
Copy link
Member

@bcoca bcoca commented Jan 12, 2022

ident was already added in parallel, if you still wish to add 'user' parameter please update the PR (solve conflicts) and add a changelog.

waiting_on_contributor

@ansibot ansibot added the waiting_on_contributor label May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.8 bug docs docsite has_issue needs_rebase needs_revision pre_azp support:core waiting_on_contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants