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.

Sign up for GitHub

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

Jump to bottom

HTTP Digest authentication support #131

Open
wants to merge 52 commits into
base: trunk
Choose a base branch
from
Open

HTTP Digest authentication support #131

wants to merge 52 commits into from

Conversation

jameshilliard
Copy link
Contributor

This is basically just #111 cherry-picked with merge conflicts fixed and POST Digest Authentication fixed.

@codecov-io
Copy link

codecov-io commented Aug 1, 2016 •
edited by codecov bot

Codecov Report

Merging #131 into master will decrease coverage by 0.93%.
The diff coverage is 88.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
- Coverage   98.86%   97.92%   -0.94%     
==========================================
  Files          26       26              
  Lines        2285     2510     +225     
  Branches      165      183      +18     
==========================================
+ Hits         2259     2458     +199     
- Misses         14       32      +18     
- Partials       12       20       +8
Impacted Files Coverage Δ
src/treq/test/test_treq_integration.py 96.56% <100%> (+1.27%) ⬆️
src/treq/test/test_auth.py 100% <100%> (ø) ⬆️
src/treq/auth.py 84.65% <82.46%> (-15.35%) ⬇️
src/treq/client.py 98.47% <0%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d45c82...e0d7208. Read the comment docs.

dreid
dreid reviewed Sep 9, 2016
View reviewed changes
treq/auth.py Outdated
return hashlib.sha1(x).hexdigest()


def build_digest_authentication_header(agent, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be public, if it needs to share data with the agent it should probably just be a private method on the agent.

@dreid
Copy link
Contributor

dreid commented Sep 9, 2016

The tests fail on Python 3, looks like you're trying to use a str as the header instead of a bytes.

@jameshilliard
Refactor build_digest_authentication_header as a private method of _R…
a627f0b 
…equestDigestAuthenticationAgent without kwargs
@jameshilliard
Copy link
Contributor Author

@dreid How would I got about fixing the issue with the header being a str instead of bytes? I'm having trouble debugging that.

dreid
dreid reviewed Sep 11, 2016
View reviewed changes
treq/auth.py Outdated
# We support only "auth" QoP as defined in rfc-2617 or rfc-2069
raise UnknownQopForDigestAuth(digest_authentication_params['qop'])
digest_authentication_header = self._build_digest_authentication_header(
self,
Copy link
Contributor

Choose a reason for hiding this comment

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

This self (and the agent argument in _build_digest_authentication_header are redundant.

@dreid
Copy link
Contributor

dreid commented Sep 11, 2016

@jameshilliard

How would I got about fixing the issue with the header being a str instead of bytes? I'm having trouble debugging that.

Start by using byte strings (b"") everywhere internally.

@jameshilliard
Copy link
Contributor Author

@dreid I think I've got everything converted to using byte strings, now I'm getting 401 errors instead(I verified against a production server to confirm it's not just the tests that are failing) for python 3. Any idea what might be causing authentication to fail?

@markrwilliams markrwilliams mentioned this pull request Sep 25, 2016
Closed
twm
twm reviewed Nov 23, 2022
View reviewed changes
Copy link
Contributor

@twm twm left a comment

Choose a reason for hiding this comment

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

Thank you for reviving this! I did just a quick skim, no verification of the algorithms. I'm not feeling well so sorry if my comments are terse, but I wanted to get you some quick feedback.

docs/howto.rst Outdated Show resolved Hide resolved
docs/index.rst
@@ -71,7 +71,7 @@ Here is a list of `requests`_ features and their status in treq.
+----------------------------------+----------+----------+
| Basic Authentication | yes | yes |
+----------------------------------+----------+----------+
| Digest Authentication | yes | no |
| Digest Authentication | yes | yes |
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

src/treq/auth.py Outdated Show resolved Hide resolved
src/treq/test/test_auth.py Outdated Show resolved Hide resolved
src/treq/test/test_auth.py Outdated Show resolved Hide resolved
src/treq/auth.py Show resolved Hide resolved
src/treq/test/test_treq_integration.py Outdated Show resolved Hide resolved
src/treq/test/test_treq_integration.py Outdated Show resolved Hide resolved
src/treq/test/test_treq_integration.py Outdated Show resolved Hide resolved
src/treq/test/test_treq_integration.py Outdated Show resolved Hide resolved
@twm twm removed the abandoned This PR has code, but the author doesn't have time to address feedback. Newcomers, pick these up! label Nov 23, 2022
glyph
glyph reviewed Nov 25, 2022
View reviewed changes
Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

Sorry, I still have a few concerns about security here that I want to make sure are addressed in a comment or something. And we should really avoid adding new magic byte sequences without an accompanying Enum somewhere so that callers can type-check their invocations.

src/treq/auth.py Outdated Show resolved Hide resolved
src/treq/auth.py Show resolved Hide resolved
src/treq/auth.py Outdated Show resolved Hide resolved
src/treq/test/test_auth.py Show resolved Hide resolved
src/treq/test/test_auth.py Outdated Show resolved Hide resolved
src/treq/test/test_treq_integration.py Outdated Show resolved Hide resolved
@jameshilliard jameshilliard force-pushed the HTTPDigestAuth branch 2 times, most recently from a947a7e to 4283a72 Compare November 26, 2022 02:59
@jameshilliard
Copy link
Contributor Author

Sorry, I still have a few concerns about security here that I want to make sure are addressed in a comment or something.

I refactored the header builder based on the latest version in requests so it shouldn't be any worse than that version and appears to be unlikely to be a viable attack vector in practice here in general.

And we should really avoid adding new magic byte sequences without an accompanying Enum somewhere so that callers can type-check their invocations.

I changed algorithm to use an Enum.

@jameshilliard
Copy link
Contributor Author

@glyph Merge conflicts fixed.

@glyph
Copy link
Member

glyph commented May 2, 2023

Thanks for updating this @jameshilliard !

@jameshilliard
Copy link
Contributor Author

@glyph Is this good to merge now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twm twm twm left review comments

@glyph glyph glyph left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jameshilliard @codecov-io @dreid @glyph @twm @mksh