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


        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"`.

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.


        📜🤖 Added by blurb_it.

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 ?


        bpo-38686 review fix: remember why.

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


        fix trailing space in Lib/urllib/request.py

Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>

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.
🐍🍒⛏🤖


        bpo-38686: fix HTTP Digest handling in request.py (pythonGH-17045)

* 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 <brandtbucher@gmail.com> (cherry picked from commit 14a89c4) Co-authored-by: PypeBros <PypeBros@users.noreply.github.com>

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.


        bpo-38686: fix HTTP Digest handling in request.py (pythonGH-17045)

* 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 <brandtbucher@gmail.com> (cherry picked from commit 14a89c4) Co-authored-by: PypeBros <PypeBros@users.noreply.github.com>


        bpo-38686: fix HTTP Digest handling in request.py (GH-17045)

* 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 <brandtbucher@gmail.com> (cherry picked from commit 14a89c4) Co-authored-by: PypeBros <PypeBros@users.noreply.github.com>


        bpo-38686: fix HTTP Digest handling in request.py (GH-17045)

* 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 <brandtbucher@gmail.com> (cherry picked from commit 14a89c4) Co-authored-by: PypeBros <PypeBros@users.noreply.github.com>


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

* 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 <brandtbucher@gmail.com>

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 suggested changes Nov 4, 2019

View changes

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

brandtbucher added the type-bugfix 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.

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

5ade571

brandtbucher approved these changes Nov 6, 2019

View changes

orsenthil reviewed Nov 13, 2019

View changes

Lib/urllib/request.py Show resolved Hide resolved

bpo-38686 review fix: remember why.

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

432c7b9

orsenthil approved these changes Nov 22, 2019

View changes

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

brandtbucher suggested changes Nov 22, 2019

View changes

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

brandtbucher approved these changes Nov 22, 2019

View changes

orsenthil approved these changes Nov 22, 2019

View 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

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

python/cpython

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

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

PypeBros commented Nov 4, 2019 •

edited by bedevere-bot

This comment has been minimized.

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

This comment has been minimized.

brandtbucher commented Nov 4, 2019

brandtbucher left a comment

This comment has been minimized.

PypeBros commented Nov 6, 2019

brandtbucher left a comment

This comment has been minimized.

brandtbucher commented Nov 6, 2019

This comment has been minimized.

PypeBros commented Nov 13, 2019

orsenthil left a comment

This comment has been minimized.

PypeBros commented Nov 18, 2019

This comment has been minimized.

PypeBros commented Nov 22, 2019

orsenthil left a comment

This comment has been minimized.

PypeBros commented Nov 22, 2019

This comment has been minimized.

bedevere-bot commented Nov 22, 2019

This comment has been minimized.

miss-islington commented Nov 22, 2019

This comment has been minimized.

bedevere-bot commented Nov 22, 2019

This comment has been minimized.

bedevere-bot commented Nov 22, 2019

Sponsor python/cpython

python/cpython

Join GitHub today

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

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

Conversation

PypeBros commented Nov 4, 2019 • edited by bedevere-bot

This comment has been minimized.

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

CLA Missing

This comment has been minimized.

brandtbucher commented Nov 4, 2019

brandtbucher left a comment

This comment has been minimized.

PypeBros commented Nov 6, 2019

brandtbucher left a comment

This comment has been minimized.

brandtbucher commented Nov 6, 2019

This comment has been minimized.

PypeBros commented Nov 13, 2019

orsenthil left a comment

This comment has been minimized.

PypeBros commented Nov 18, 2019

This comment has been minimized.

PypeBros commented Nov 22, 2019

orsenthil left a comment

This comment has been minimized.

PypeBros commented Nov 22, 2019

This comment has been minimized.

bedevere-bot commented Nov 22, 2019

This comment has been minimized.

miss-islington commented Nov 22, 2019

This comment has been minimized.

bedevere-bot commented Nov 22, 2019

This comment has been minimized.

bedevere-bot commented Nov 22, 2019

PypeBros commented Nov 4, 2019 •

edited by bedevere-bot