bpo-38686: fix HTTP Digest handling in request.py #17045

PypeBros · 2019-11-04T15:28:34Z

There is a bug triggered when server replies to a request with WWW-Authenticate: Digest where qop="auth,auth-int" rather than mere qop="auth". Having both auth and auth-int is legitimate according to the qop-options rule in §3.2.1 of https://www.ietf.org/rfc/rfc2617.txt:

 qop-options       = "qop" "=" <"> 1#qop-value <">
 qop-value         = "auth" | "auth-int" | token
qop-options: [...] If present, it is a quoted string of one or more tokens indicating the "quality of protection" values supported by the server. The value "auth" indicates authentication; the value "auth-int" indicates authentication with integrity protection

This is description confirmed by the definition of the [n]#[m]rule extended-BNF pattern defined in §2.1 of https://www.ietf.org/rfc/rfc2616.txt as 'a comma-separated list of rule with at least n and at most m items'.

When this reply is parsed by get_authorization, request.py only tests for identity with 'auth', failing to recognize it as one of the supported modes the server announced, and claims that "qop 'auth,auth-int' is not supported".

https://bugs.python.org/issue38686

There is a bug triggered when server replies to a request with `WWW-Authenticate: Digest` where `qop="auth,auth-int"` rather than mere `qop="auth"`. Having both `auth` and `auth-int` is legitimate according to the `qop-options` rule in §3.2.1 of [[https://www.ietf.org/rfc/rfc2617.txt|RFC 2617]]: > qop-options = "qop" "=" <"> 1#qop-value <"> > qop-value = "auth" | "auth-int" | token > **qop-options**: [...] If present, it is a quoted string **of one or more** tokens indicating the "quality of protection" values supported by the server. The value `"auth"` indicates authentication; the value `"auth-int"` indicates authentication with integrity protection This is description confirmed by the definition of the [//n//]`#`[//m//]//rule// extended-BNF pattern defined in §2.1 of [[https://www.ietf.org/rfc/rfc2616.txt|RFC 2616]] as 'a comma-separated list of //rule// with at least //n// and at most //m// items'. When this reply is parsed by `get_authorization`, request.py only tests for identity with `'auth'`, failing to recognize it as one of the supported modes the server announced, and claims that `"qop 'auth,auth-int' is not supported"`.

the-knights-who-say-ni · 2019-11-04T15:28:38Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@PypeBros

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

brandtbucher · 2019-11-04T17:31:45Z

Thanks for your time @PypeBros, and welcome to CPython! 😎

I assume that you've seen the bot's message about the CLA? Also, if the issue will properly link if you add "bpo-" to the start of this PR's title!

brandtbucher

I'm not a urllib expert, but the code itself looks good.

This PR should also have a NEWS entry. Just something simple, like:

Added support for multiple ``qop`` values in :class:`urllib.request.AbstractDigestAuthHandler`.

I think this also needs a test or two, in Lib/test/test_urllib2.py.

PypeBros · 2019-11-06T16:11:57Z

@brandtbucher : thanks for the welcome and for the help.
I had a look at test_urllib2, but I'm unsure I could write appropriate test cases myself as I don't even see code testing how the DigestAuthHandler would process a valid 'Digest' reply, only code that check it wouldn't prevent BasicAuthHandler from grabbing basic authentication requests.

brandtbucher

Ah, I see there's not many tests for this class at all. I'll leave that for the core reviewer to decide!

brandtbucher · 2019-11-06T19:26:43Z

CC @orsenthil

PypeBros · 2019-11-13T09:36:31Z

Any follow up ? Afaik, in its current state, it makes python clients unable to inter-operate securely with gSOAP clients.

orsenthil

Hi @PypeBros - Thank you for your contribution.

Please see my review comments on this ticket and let me know what your thoughts are.

Lib/urllib/request.py

PypeBros · 2019-11-18T08:05:52Z

@orsenthil, does the replies above address your concern with the proposed change ?

PypeBros · 2019-11-22T17:14:41Z

here you are. I haven't repeated the "XXX: implement auth-int", as this is already on line 1154

orsenthil

Thank you!

PypeBros · 2019-11-22T17:28:10Z

anybody understands why https://travis-ci.org/python/cpython/jobs/615649384?utm_medium=notification&utm_source=github_status failed with "Please fix the 1 file(s) with whitespace issues" ?

Lib/urllib/request.py

Co-Authored-By: Brandt Bucher <[email protected]>

bedevere-bot · 2019-11-22T23:19:11Z

@orsenthil: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2019-11-22T23:19:11Z

Thanks @PypeBros for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

* fix HTTP Digest handling in request.py There is a bug triggered when server replies to a request with `WWW-Authenticate: Digest` where `qop="auth,auth-int"` rather than mere `qop="auth"`. Having both `auth` and `auth-int` is legitimate according to the `qop-options` rule in §3.2.1 of [[https://www.ietf.org/rfc/rfc2617.txt|RFC 2617]]: > qop-options = "qop" "=" <"> 1GH-qop-value <"> > qop-value = "auth" | "auth-int" | token > **qop-options**: [...] If present, it is a quoted string **of one or more** tokens indicating the "quality of protection" values supported by the server. The value `"auth"` indicates authentication; the value `"auth-int"` indicates authentication with integrity protection This is description confirmed by the definition of the [//n//]`GH-`[//m//]//rule// extended-BNF pattern defined in §2.1 of [[https://www.ietf.org/rfc/rfc2616.txt|RFC 2616]] as 'a comma-separated list of //rule// with at least //n// and at most //m// items'. When this reply is parsed by `get_authorization`, request.py only tests for identity with `'auth'`, failing to recognize it as one of the supported modes the server announced, and claims that `"qop 'auth,auth-int' is not supported"`. * 📜🤖 Added by blurb_it. * bpo-38686 review fix: remember why. * fix trailing space in Lib/urllib/request.py Co-Authored-By: Brandt Bucher <[email protected]> (cherry picked from commit 14a89c4) Co-authored-by: PypeBros <[email protected]>

bedevere-bot · 2019-11-22T23:19:21Z

GH-17357 is a backport of this pull request to the 3.8 branch.

bedevere-bot · 2019-11-22T23:19:27Z

GH-17358 is a backport of this pull request to the 3.7 branch.

* fix HTTP Digest handling in request.py There is a bug triggered when server replies to a request with `WWW-Authenticate: Digest` where `qop="auth,auth-int"` rather than mere `qop="auth"`. Having both `auth` and `auth-int` is legitimate according to the `qop-options` rule in §3.2.1 of [[https://www.ietf.org/rfc/rfc2617.txt|RFC 2617]]: > qop-options = "qop" "=" <"> 1GH-qop-value <"> > qop-value = "auth" | "auth-int" | token > **qop-options**: [...] If present, it is a quoted string **of one or more** tokens indicating the "quality of protection" values supported by the server. The value `"auth"` indicates authentication; the value `"auth-int"` indicates authentication with integrity protection This is description confirmed by the definition of the [//n//]`GH-`[//m//]//rule// extended-BNF pattern defined in §2.1 of [[https://www.ietf.org/rfc/rfc2616.txt|RFC 2616]] as 'a comma-separated list of //rule// with at least //n// and at most //m// items'. When this reply is parsed by `get_authorization`, request.py only tests for identity with `'auth'`, failing to recognize it as one of the supported modes the server announced, and claims that `"qop 'auth,auth-int' is not supported"`. * 📜🤖 Added by blurb_it. * bpo-38686 review fix: remember why. * fix trailing space in Lib/urllib/request.py Co-Authored-By: Brandt Bucher <[email protected]> (cherry picked from commit 14a89c4) Co-authored-by: PypeBros <[email protected]>

* fix HTTP Digest handling in request.py There is a bug triggered when server replies to a request with `WWW-Authenticate: Digest` where `qop="auth,auth-int"` rather than mere `qop="auth"`. Having both `auth` and `auth-int` is legitimate according to the `qop-options` rule in §3.2.1 of [[https://www.ietf.org/rfc/rfc2617.txt|RFC 2617]]: > qop-options = "qop" "=" <"> 1#qop-value <"> > qop-value = "auth" | "auth-int" | token > **qop-options**: [...] If present, it is a quoted string **of one or more** tokens indicating the "quality of protection" values supported by the server. The value `"auth"` indicates authentication; the value `"auth-int"` indicates authentication with integrity protection This is description confirmed by the definition of the [//n//]`#`[//m//]//rule// extended-BNF pattern defined in §2.1 of [[https://www.ietf.org/rfc/rfc2616.txt|RFC 2616]] as 'a comma-separated list of //rule// with at least //n// and at most //m// items'. When this reply is parsed by `get_authorization`, request.py only tests for identity with `'auth'`, failing to recognize it as one of the supported modes the server announced, and claims that `"qop 'auth,auth-int' is not supported"`. * 📜🤖 Added by blurb_it. * bpo-38686 review fix: remember why. * fix trailing space in Lib/urllib/request.py Co-Authored-By: Brandt Bucher <[email protected]>

the-knights-who-say-ni added the CLA not signed label Nov 4, 2019

bedevere-bot added the awaiting review label Nov 4, 2019

brandtbucher requested changes Nov 4, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Nov 4, 2019

brandtbucher added the type-bug An unexpected behavior, bug, or error label Nov 4, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Nov 5, 2019

PypeBros changed the title ~~38686: fix HTTP Digest handling in request.py~~ bpo-38686: fix HTTP Digest handling in request.py Nov 5, 2019

📜🤖 Added by blurb_it.

5ade571

brandtbucher approved these changes Nov 6, 2019

View reviewed changes

orsenthil reviewed Nov 13, 2019

View reviewed changes

Lib/urllib/request.py Show resolved Hide resolved

bpo-38686 review fix: remember why.

432c7b9

orsenthil approved these changes Nov 22, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Nov 22, 2019

brandtbucher requested changes Nov 22, 2019

View reviewed changes

Lib/urllib/request.py Outdated Show resolved Hide resolved

fix trailing space in Lib/urllib/request.py

95c6eef

Co-Authored-By: Brandt Bucher <[email protected]>

brandtbucher approved these changes Nov 22, 2019

View reviewed changes

orsenthil approved these changes Nov 22, 2019

View reviewed changes

orsenthil added the needs backport to 3.7 label Nov 22, 2019

orsenthil added the needs backport to 3.8 label Nov 22, 2019

orsenthil merged commit 14a89c4 into python:master Nov 22, 2019

bedevere-bot removed the awaiting merge label Nov 22, 2019

bedevere-bot removed the needs backport to 3.8 label Nov 22, 2019

bedevere-bot removed the needs backport to 3.7 label Nov 22, 2019

Uh oh!

bpo-38686: fix HTTP Digest handling in request.py #17045

bpo-38686: fix HTTP Digest handling in request.py #17045

Uh oh!

Conversation

PypeBros commented Nov 4, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Nov 4, 2019

CLA Missing

Uh oh!

brandtbucher commented Nov 4, 2019

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

PypeBros commented Nov 6, 2019

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

brandtbucher commented Nov 6, 2019

Uh oh!

PypeBros commented Nov 13, 2019

Uh oh!

orsenthil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PypeBros commented Nov 18, 2019

Uh oh!

PypeBros commented Nov 22, 2019

Uh oh!

orsenthil left a comment

Choose a reason for hiding this comment

Uh oh!

PypeBros commented Nov 22, 2019

Uh oh!

Uh oh!

bedevere-bot commented Nov 22, 2019

Uh oh!

miss-islington commented Nov 22, 2019

Uh oh!

bedevere-bot commented Nov 22, 2019

Uh oh!

bedevere-bot commented Nov 22, 2019

Uh oh!

Uh oh!

PypeBros commented Nov 4, 2019 •

edited by bedevere-bot

Loading