FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-128192: support HTTP sha-256 digest authentication as per RFC-7617 by calvinbui · Pull Request #128193 · python/cpython · GitHub

/ cpython Public

gh-128192: support HTTP sha-256 digest authentication as per RFC-7617 - #128193

Merged
gpshead merged 15 commits into
python:mainfrom
calvinbui:patch-1
Dec 28, 2024
Merged

gh-128192: support HTTP sha-256 digest authentication as per RFC-7617#128193
gpshead merged 15 commits into
python:mainfrom
calvinbui:patch-1

Conversation

calvinbui commented Dec 23, 2024
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

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

ghost commented Dec 23, 2024
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

bedevere-app Bot commented Dec 23, 2024

Copy link
Copy Markdown

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 skip news label instead.

ZeroIntensity commented Dec 23, 2024
edited
Loading

Copy link
Copy Markdown
Member

cc @picnixz (cryptography expert)

…e-128192.02mEhD.rst

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Comment thread Lib/urllib/request.py

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A preliminary round of comments. You can also update "RFC 2617" to "RFC 2617/7616" in the AbstractDigestAuthHandler comment.

Comment thread Lib/urllib/request.py
Comment thread Lib/urllib/request.py
gpshead self-requested a review December 24, 2024 19:02
picnixz self-requested a review December 28, 2024 09:36
Comment thread Lib/test/test_urllib2.py
Comment thread Lib/test/test_urllib2.py
handler = AbstractDigestAuthHandler()
class TestDigestAlgorithms(unittest.TestCase):
def setUp(self):
self.handler = AbstractDigestAuthHandler()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yep, next time!

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz commented Dec 28, 2024

Copy link
Copy Markdown
Member

We are still missing the What's New entry and the modified NEWS entry.

calvinbui and others added 2 commits December 28, 2024 21:12
…e-128192.02mEhD.rst

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Copy link
Copy Markdown
Contributor Author

We are still missing the What's New entry and the modified NEWS entry.

my bad, fixed

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Doc/whatsnew/3.14.rst Outdated

Copy link
Copy Markdown
Contributor Author

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.

added

I think we'll also need a follow-up PR to update the docs because they are lacking IMO.

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

picnixz commented Dec 28, 2024
edited
Loading

Copy link
Copy Markdown
Member

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.

picnixz changed the title gh-128192: support sha-256 digest authentication gh-128192: support HTTP sha-256 digest authentication as per RFC-7617 Dec 28, 2024
Comment thread Lib/urllib/request.py Outdated
gpshead self-assigned this Dec 28, 2024
gpshead added type-feature A feature request or enhancement 3.14 bugs and security fixes labels Dec 28, 2024
gpshead enabled auto-merge (squash) December 28, 2024 20:47
gpshead merged commit f9a5a3a into python:main Dec 28, 2024

picnixz commented Dec 28, 2024
edited
Loading

Copy link
Copy Markdown
Member

Thank you for your contribution Calvin and Gregory for the merge/2nd review.

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 RHEL8 FIPS Only Blake2 Builtin Hash 3.x has failed when building commit f9a5a3a.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/469/builds/9772) and take a look at the build logs.
  4. Check if the failure is related to this commit (f9a5a3a) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/469/builds/9772

Failed tests:

  • test_urllib2

Failed subtests:

  • test_md5_algorithm - test.test_urllib2.TestDigestAlgorithms.test_md5_algorithm

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/test/test_urllib2.py", line 1972, in test_md5_algorithm
    self.assertEqual(H("foo"), "acbd18db4cc2f85cedef654fccc4a4d8")
                     ~^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/urllib/request.py", line 1182, in <lambda>
    H = lambda x: hashlib.md5(x.encode("ascii")).hexdigest()
                  ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
ValueError: [digital envelope routines: EVP_DigestInit_ex] disabled for FIPS

gpshead commented Dec 28, 2024

Copy link
Copy Markdown
Member

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)

picnixz commented Dec 28, 2024

Copy link
Copy Markdown
Member

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?

gpshead commented Dec 28, 2024

Copy link
Copy Markdown
Member

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.

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
…C-7617 (pythonGH-128193)

support sha-256 digest authentication

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.14 bugs and security fixes type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL