Add support for (auth-int) quality of protection

[karpetrosyan]

For tests, I used httpbin.

[tomchristie]

Seems okay to me yup, thank for this!

Needs a test case adding to it.

[tomchristie]

Instead of removing this test case, let's change it so that we're testing auth-int.

[tomchristie]

Great. This looks good.

The trade-off that we're making if we pull this in, is that digest auth will then always pre-emptively read the response body. So... no streaming responses if you're using digest auth. Even if it's not an auth-int case.

Is that a trade-off we want to make in order to add this support?

[karpetrosyan]

I considered it, and I believe we can solve this problem in the future, but for now, I think 'auth-int' support is more important than stream reading, which is rarely used in my opinion.

I decided to make this pull request because I used httpx in my application, which sends requests to many servers, and some of them required 'auth-int' authorization, which httpx did not support.

[karpetrosyan]

Also, I believe we can solve this problem by overriding sync_auth_flow and async_auth_flow methods in the DigestAuth class, but is it worth it?

[tomchristie]

Also, I believe we can solve this problem by overriding sync_auth_flow and async_auth_flow methods in the DigestAuth class

True. To do that with a reasonably minimal code change, I think we could do something like this...

requires_request_body = False

def sync_auth_flow(...):
    if auth-int in headers
        self.requires_request_body = True
    return super().sync_auth_flow(...)

async def async_auth_flow(...):
    if auth-int in headers
        self.requires_request_body = True
    return await super().async_auth_flow(...)

...so that we're not having to override the entire methods.

but is it worth it?

I'm not all that keen on the extra complexity, but the above looks like it might be simple enough to be worth the trade off. What are your thoughts on it?

[karpetrosyan]

I believe the only way to override is to copy the entire sync_auth_flow and async_auth_flow to the DigestAuth because we need to override require_response_body when we have access to that response.

Unfortunately, we do not have easy access to that response, and we cannot do so without coping whole methods.

1    def sync_auth_flow(
2       self, request: Request
3    ) -> typing.Generator[Request, Response, None]:
4        """
5        Execute the authentication flow synchronously.
6
7        By default, this defers to `.auth_flow()`. You should override this method
8       when the authentication scheme does I/O and/or uses concurrency primitives.
9       """
10
11        if self.requires_request_body:
12           request.read()
13
14        flow = self.auth_flow(request)
15        request = next(flow)
16
17        while True:
18            response = yield request
19            if self.requires_response_body:
20                response.read()
21
22            try:
23               request = flow.send(response)
24            except StopIteration:
25                break

We can either copy the entire function and change the 19th line, or we can go another way and check for 'auth-int' in the 'Auth.sync auth flow' and 'Auth.async auth flow' like so.

class Auth:
    ...
    def sync_auth_flow(
        if self.requires_request_body:
            request.read()

        flow = self.auth_flow(request)
        request = next(flow)
        is_digest = isinstance(self, DigestAuth)

        while True:
            response = yield request
            if self.requires_response_body or (
                is_digest
                and "www-authenticate" in response.headers
                and "auth-int" in response.headers["www-authenticate"]
            ):
                response.read()

            try:
                request = flow.send(response)
            except StopIteration:
                break

We have a weird require_response_body which is false for DigestAuth but we actually need response body in DigestAuth, so i think we should preefer the second variant because we globally says that this an exception and also we do not need to copy whole sync_auth_flow and async_auth_flow functions.

[tomchristie]

I believe the only way to override is to copy the entire sync_auth_flow and async_auth_flow to the DigestAuth because we need to override require_response_body when we have access to that response.

I see what you mean, yep.

Okay, the additional support of auth-int seems like it's probably? worth the trade-off vs. non-streaming for digest auth cases.

[karpetrosyan]

That appears to be all.

[tomchristie]

Ah right, I've just reviewed and seen commit d846089.
Having digest-auth specific logo in the base Auth classes doesn't really make sense.
I think we probably want this PR prior to that change.

[karpetrosyan]

Did you think we should override entire auth_flow methods, or do you refuse to support auth-int at all?

[karpetrosyan]

Overriding auth_flows is a bad idea in my opinion; there will be four methods with the same body in the _auth.py module.
Rather than copying and pasting these methods, I believe it would be better to not support 'auth-int' at all :D.

[tomchristie]

Rather than copying and pasting these methods, I believe it would be better to not support 'auth-int' at all :D.

I think we either don't support it or we do support it, but just using a plain requires_response_body = True marker like you previously had...

dcca661

(In other words, roll back the d846089 commit)

[karpetrosyan]

Sounds good

[karpetrosyan]

@tomchristie Could you please merge this pull request?

[tomchristie]

Okay... I've been a little slow on this because Ive been weighing it up as a trade-off. Thanks for being patient.
I think we're up for it yup.

For tests, I used httpbin.

Could you show an example of how the requests/responses look with that once we've got this PR in?

[karpetrosyan]

import httpx
auth = httpx.DigestAuth(username="test", password="test")
resp = httpx.get("https://httpbin.org/digest-auth/auth-int/test/test", auth=auth, timeout=1123)
print(resp)
print(resp.history[0].headers)

OUTPUT

<Response [200 OK]>
Headers([('date', 'Thu, 20 Apr 2023 12:37:25 GMT'), ('content-type', 'text/html; charset=utf-8'), ('content-length', '0'), ('connection', 'keep-alive'), ('server', 'gunicorn/19.9.0'), ('www-authenticate', 'Digest realm="me@kennethreitz.com", nonce="82316fa77f70091f777188f4829f118a", qop="auth-int", opaque="a12a059d6e498f16bfc303794c65da51", algorithm=MD5, stale=FALSE'), ('set-cookie', 'stale_after=never; Path=/'), ('set-cookie', 'fake=fake_value; Path=/'), ('access-control-allow-origin', '*'), ('access-control-allow-credentials', 'true')])

[karpetrosyan]

@tomchristie There is a strange issue, but I believe it is a httpbin fault; I tried using original httpx to do this request; I just hardcoded that qop is always equal to auth to avoid the DigestAuth not supported error, but this request still succeeds.

[karpetrosyan]

@tomchristie It appears that httpcore is accepting qop=auth even if it requested qop=auth-int; I tried sending a request with normal qop=auth using original httpx, and it works; however, when I generate the header with normal qop and then change the qop value to qop=auth-int, the request fails.

In the original httpx, I modified those parts of code for local tests.

httpx/httpx/_auth.py

Lines 246 to 248 in 472597f

	return _DigestAuthChallenge(
	realm=realm, nonce=nonce, algorithm=algorithm, opaque=opaque, qop=qop
	)

httpx/httpx/_auth.py

Lines 293 to 294 in 472597f

	if qop:
	format_args["qop"] = b"auth"

This works correct [200]

return _DigestAuthChallenge(
      realm=realm, nonce=nonce, algorithm=algorithm, opaque=opaque, qop=b"auth"
)

if qop:
    format_args["qop"] = b"auth"

This fails [401]

return _DigestAuthChallenge(
      realm=realm, nonce=nonce, algorithm=algorithm, opaque=opaque, qop=b"auth"
)

if qop:
    format_args["qop"] = b"auth-int"

Httpsore simply sees that we sent the request as a normal qop=auth and does us a favor by accepting such a header.
When we process 401 as normal qop=auth and then send it as auth-int, it recognizes that the header is incorrect and returns 401.

[tomchristie]

Okay, I'm going to punt this out to the team...

Here's the trade-off:

+ve: We add auth-int support to our digest auth.
-ve: Our digest auth always internally pre-loads responses, even if the developer is using httpx.stream(...), because auth-int requires it for the integrity protection signing.

I think our options here are either...

• Great, we'll accept this trade-off & pull this into core.
• No thanks, let's have this shared somewhere as a gist or third-party package that we can link to from the docs.

Useful signposts we could follow here: does curl support auth-int? Can we point to existing URLs that require it? Can we easily demo browser behaviour that supports it? Do any of y'all need this or not?

[karpetrosyan]

Curl supports auth-int while well-known Python http libraries such as requests, urllib3, and aiohttp do not.

[karpetrosyan]

I forced pushed into my httpx master to sync it with the real master, but I forgot I had a pull request in my master, so it broke.

If we decide we really want this PR, I will repair it and open the same pull request.

[tomchristie]

Okay thanks, I'd suggest we leave this one as-is unless we've got some good motivation otherwise.