gh-136134: Fallback to next auth method when CRAM-MD5 fails due to unsupported hash (e.g. FIPS) #136188
Conversation
…e to unsupported hash (e.g. FIPS)
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Lib/smtplib.py
Outdated
| @@ -743,6 +743,10 @@ def login(self, user, password, *, initial_response_ok=True): | |||
| return (code, resp) | |||
| except SMTPAuthenticationError as e: | |||
| last_exception = e | |||
| except ValueError as e: | |||
| last_exception = e | |||
| if 'unsupported' in str(e).lower(): | |||
There was a problem hiding this comment.
why do we care about this specific error? shouldn't the next method be tried regardless of the nature of the error?
There was a problem hiding this comment.
In general, it makes sense to fallback regardless of the error. However, in this case, we handle this specific ValueError to deal with environments where FIPS mode is enabled, which disables certain hashing algorithms like MD5.
In FIPS mode, CRAM-MD5 will raise a ValueError from the underlying OpenSSL implementation (e.g. [digital envelope routines] unsupported). Since this isn't a misconfiguration or a generic programming error, but rather an expected, reproducible environment-specific issue, we explicitly catch it and fallback silently to the next available method (e.g. LOGIN).
There was a problem hiding this comment.
I see your point. I had seen the error only on Linux container with a specific version of openssl. My worry is that the fix will be rendered ineffective if openssl changes the error string or returns a different error on a different OS/platform.
There was a problem hiding this comment.
I don't think so, we are saving the error, and try another method, if none of them resolve, we will return the latest error.
However, I also don't like this kind of fix, I didn't found another better way to solve this.
And the error is not only related with that docker image.
There was a problem hiding this comment.
We should do it differently. Relying on the content of an exception message is a bad idea as it can evolve in the future. Instead, locate where we call HMAC and wrap that call, then raise an appropriate exception and catch that one instead.
If the call is too deep in the stack we should find another way.
There was a problem hiding this comment.
Thanks for the suggestion @picnixz, I’ve applied the proposed approach by wrapping the HMAC call directly and raising a dedicated SMTPAuthHashUnsupportedError instead of relying on the exception message. I agree with this approach.
If there’s anything you’d like to adjust in naming or behavior, I’ll be happy to apply it.
…M-MD5 Wraps the HMAC call in auth_cram_md5 and raises a dedicated exception to avoid relying on error message content. Also updates the relevant tests.
Lib/smtplib.py
Outdated
| @@ -743,13 +750,12 @@ def login(self, user, password, *, initial_response_ok=True): | |||
| return (code, resp) | |||
| except SMTPAuthenticationError as e: | |||
| last_exception = e | |||
| except ValueError as e: | |||
| except SMTPAuthHashUnsupportedError as e: | |||
| # Some environments (e.g., FIPS) disable certain hashing algorithms like MD5, | |||
| # which are required by CRAM-MD5. This raises a ValueError when trying to use HMAC. | |||
There was a problem hiding this comment.
name of the exception in the comment needs update
There was a problem hiding this comment.
Good catch, tks
I’ve updated the comment to reflect the correct exception name
| return self.user + " " + hmac.HMAC( | ||
| self.password.encode('ascii'), challenge, 'md5').hexdigest() | ||
| except ValueError as e: | ||
| raise SMTPAuthHashUnsupportedError(f'CRAM-MD5 failed: {e}') from e |
There was a problem hiding this comment.
Separate all calls and only wrap the hmac.HMAC call in a try-catch. TiA
| @@ -1570,5 +1570,60 @@ def testAUTH_PLAIN_initial_response_auth(self): | |||
| self.assertEqual(code, 235) | |||
|
|
|||
|
|
|||
| class TestSMTPLoginValueError(unittest.TestCase): | |||
| def broken_hmac(*args, **kwargs): | |||
There was a problem hiding this comment.
Make it an external function, and mock the entire class instead.
| @@ -743,6 +750,12 @@ def login(self, user, password, *, initial_response_ok=True): | |||
| return (code, resp) | |||
| except SMTPAuthenticationError as e: | |||
| last_exception = e | |||
| except SMTPAuthHashUnsupportedError as e: | |||
| # Some environments (e.g., FIPS) disable certain hashing algorithms like MD5, | |||
There was a problem hiding this comment.
This comment is too verbose. Just write something like "skip unsupported HMAC-HASH algorithms"
This change improves the
smtplib.SMTP.login()method to handle the case where CRAM-MD5 fails due to missing or unsupported MD5 digest (e.g. in FIPS-compliant environments). Instead of crashing with aValueError, the client now skips CRAM-MD5 and tries the next available mechanism.A regression test has been added to verify this fallback behavior.
Closes: gh-136134