-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-128192: support HTTP sha-256 digest authentication as per RFC-7617 #128193
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
Conversation
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 |
Misc/NEWS.d/next/Core_and_Builtins/2024-12-23-11-14-07.gh-issue-128192.02mEhD.rst
Outdated
Show resolved
Hide resolved
cc @picnixz (cryptography expert) |
…e-128192.02mEhD.rst Co-authored-by: Peter Bierma <[email protected]>
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.
A preliminary round of comments. You can also update "RFC 2617" to "RFC 2617/7616" in the AbstractDigestAuthHandler comment.
Misc/NEWS.d/next/Core_and_Builtins/2024-12-23-11-14-07.gh-issue-128192.02mEhD.rst
Outdated
Show resolved
Hide resolved
handler = AbstractDigestAuthHandler() | ||
class TestDigestAlgorithms(unittest.TestCase): | ||
def setUp(self): | ||
self.handler = AbstractDigestAuthHandler() |
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.
In a follow-up PR (or this one), if you want/can, we can add tests for a full communication round where we request HTTP digest authentication.
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.
yep, next time!
Co-authored-by: Bénédikt Tran <[email protected]>
We are still missing the What's New entry and the modified NEWS entry. |
…e-128192.02mEhD.rst Co-authored-by: Bénédikt Tran <[email protected]>
my bad, fixed |
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.
We may also need an additional .. versionchanged:: next
in the docs to mention SHA-256 in https://docs.python.org/3/library/urllib.request.html#urllib.request.AbstractDigestAuthHandler.
I think we'll also need a follow-up PR to update the docs because they are lacking IMO.
Co-authored-by: Bénédikt Tran <[email protected]>
added
what docs were you thinking of? i can make a new gh issue for it, as well as the complete end-to-end http digest auth test |
None of the methods of AbstractDigestAuthHandler are actually documented so we may want to document them. If someone wants to subclass this interface, they need to know its usage. For now, let's wait for Gregory's feedback. |
Thank you for your contribution Calvin and Gregory for the merge/2nd review. |
|
i'm not surprised at that buildbot error. it is running in an unrealistic config without critical hash functions. we'll ultimately just need to decorate the test to indicate that it requires md5, sha1, and sha256 - skipping it otherwise. (see test_hashlib) |
I'll do it tomorrow (it's almost midnight here and I'm no more on my dev session) or someone else somewhere else can add the decorators. However, maybe we should revert the commit just to avoid other PRs to have the buildbot failure in the meantime? |
No worries. That buildbot is an unstable unsupported configuration, no need to revert anything. It's there solely so that we can use it to identify and clean up test dependency declaration issues eventually. It doesn't block anything. |
…C-7617 (pythonGH-128193) support sha-256 digest authentication Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]> Co-authored-by: Gregory P. Smith <[email protected]>
as mentioned in the issue, other authentication exist, but i don't see them supported out of the box in hashlib.
this all depends if python wishes to support rfc7616