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

Update password_hash.yml #81675

Merged
merged 1 commit into from Apr 23, 2024
Merged

Conversation

artificial-intelligence
Copy link
Contributor

add missing bcrypt docs

add missing bcrypt docs

Signed-off-by: Sven Kieske <kieske@osism.tech>
@ansibot ansibot added the needs_triage Needs a first human triage before being processed. label Sep 11, 2023
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 12, 2023
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Sep 12, 2023
@@ -16,7 +16,7 @@ DOCUMENTATION:
description: Hashing algorithm to use.
type: string
default: sha512
choices: [ md5, blowfish, sha256, sha512 ]
choices: [ md5, md5_crypt, blowfish, bcrypt, sha256, sha256_crypt, sha512, sha512_crypt ]
Copy link
Member

Choose a reason for hiding this comment

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

i would not add the _crypt versions as explicit options, but update the code to try/retry with non _crypt and _crypt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine either way, can you two then maybe talk about this, because you two are imposing conflicting views on me? I just wanted to fix the missing bcrypt.

Copy link
Member

Choose a reason for hiding this comment

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

@artificial-intelligence Please go ahead with @bcoca's review and discard mine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, but:

[...]but update the code to try/retry with non _crypt and _crypt

I'm not sure how this should be implemented here, as I'm just a drive by contributor, I'm not deeply familiar with this codebase.

I see no retry logic at all here, could you give me a hint on how this is supposed to look like?

Maybe I also misunderstood this comment.

Copy link
Member

Choose a reason for hiding this comment

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

to the list, just add bcrypt as you were doing originally,

then in the code we have to do something like:

try:
     function(hashtype)
except ...:
    function(hashtype + '_crypt')

But that might be asking too much of you, just do the first change and we'll add the 2nd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done so. I'm currently not really finding the place where you would add that try except block - I thought I could give it a try - this doesn't seem to be located in the utils/encrypt.py?

Copy link
Member

@bcoca bcoca Sep 18, 2023

Choose a reason for hiding this comment

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

something liek this (untested):

diff --git a/lib/ansible/plugins/filter/core.py b/lib/ansible/plugins/filter/core.py
index 09820faa7c..f3d700f0a8 100644
--- a/lib/ansible/plugins/filter/core.py
+++ b/lib/ansible/plugins/filter/core.py
@@ -292,16 +292,23 @@ def get_encrypted_password(password, hashtype='sha512', salt=None, salt_size=Non
         )
 
     try:
-        return do_encrypt(password, hashtype, salt=salt, salt_size=salt_size, rounds=rounds, ident=ident)
-    except AnsibleError as e:
-        reraise(AnsibleFilterError, AnsibleFilterError(to_native(e), orig_exc=e), sys.exc_info()[2])
+        try:
+            return do_encrypt(password, hashtype, salt=salt, salt_size=salt_size, rounds=rounds, ident=ident)
+        except AnsibleError as e:
+            reraise(AnsibleFilterError, AnsibleFilterError(to_native(e), orig_exc=e), sys.exc_info()[2])
+        except Exception as e:
+            if unknown_passlib_hashtype and not hashtype.endswith('crypt'):
+                return do_encrypt(password, hashtype + '_crypt', salt=salt, salt_size=salt_size, rounds=rounds, ident=ident)
+            raise
+    except AnsibleError:
+        raise
     except Exception as e:
         if unknown_passlib_hashtype:
             # This can occur if passlib.hash has the hashtype attribute, but it has a different signature than the valid choices.
             # In 2.19 this will replace the deprecation warning above and the extra exception handling can be deleted.
             choices = ', '.join(passlib_mapping)
-            raise AnsibleFilterError(f"{hashtype} is not in the list of supported passlib algorithms: {choices}") from e
-        raise
+                raise AnsibleFilterError(f"{hashtype} is not in the list of supported passlib algorithms: {choices}") from e
+            raise AnsibleFilterError("Could not hash the password", orig_exc=e)
 
 

Copy link
Member

Choose a reason for hiding this comment

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

actually, just saw easier/better

diff --git a/lib/ansible/plugins/filter/core.py b/lib/ansible/plugins/filter/core.py
index 09820faa7c..e3bbd7e6df 100644
--- a/lib/ansible/plugins/filter/core.py
+++ b/lib/ansible/plugins/filter/core.py
@@ -284,13 +284,21 @@ def get_encrypted_password(password, hashtype='sha512', salt=None, salt_size=Non
 
     unknown_passlib_hashtype = False
     if PASSLIB_AVAILABLE and hashtype not in passlib_mapping and hashtype not in passlib_mapping.values():
-        unknown_passlib_hashtype = True
-        display.deprecated(
-            f"Checking for unsupported password_hash passlib hashtype '{hashtype}'. "
-            "This will be an error in the future as all supported hashtypes must be documented.",
-            version='2.19'
-        )
+        if hashtype.endswith('crypt'):
+            unknown_passlib_hashtype = True
+        else:
+            crypt_hashtype = hashtype + '_crypt'
+            if crypt_hashtype in passlib_mapping or crypt_hashtype in passlib_mapping.values():
+                hashtype = crypt_hashtype
+            else:
+                unknown_passlib_hashtype = True
 
+        if unknown_passlib_hashtype:
+            display.deprecated(
+                f"Checking for unsupported password_hash passlib hashtype '{hashtype}'. "
+                "This will be an error in the future as all supported hashtypes must be documented.",
+                version='2.19'
+            )
     try:
         return do_encrypt(password, hashtype, salt=salt, salt_size=salt_size, rounds=rounds, ident=ident)
     except AnsibleError as e:
@@ -301,7 +309,7 @@ def get_encrypted_password(password, hashtype='sha512', salt=None, salt_size=Non
             # In 2.19 this will replace the deprecation warning above and the extra exception handling can be deleted.
             choices = ', '.join(passlib_mapping)
             raise AnsibleFilterError(f"{hashtype} is not in the list of supported passlib algorithms: {choices}") from e
-        raise
+        raise AnsibleFilterError('Could not hash password', orig_exc=e)

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Sep 12, 2023
@Akasurde Akasurde requested review from Akasurde and bcoca and removed request for Akasurde September 12, 2023 15:40
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 2, 2023
@Akasurde
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Akasurde Akasurde enabled auto-merge (squash) April 23, 2024 01:18
@Akasurde Akasurde merged commit 33d1224 into ansible:devel Apr 23, 2024
113 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants