Digest auth helper #10725

TimMenninger · 2025-04-15T23:32:32Z

What do these changes do?

This adds a DigestAuth class that can be used in lieu of BasicAuth for digest authorization.

Note that this is just me taking a crack at polishing off the 3.12 milestone because it has features I'm interested in. I am reserving the right to abandon this if the PR is too far off from the vision of the maintainers and/or is outright wrong.

Are there changes in behavior for the user?

There are no changes to default behavior. It will continue to use BasicAuth, and existing uses of BasicAuth should be intact.

Is it a substantial burden for the maintainers to support this?

Hopefully this won't be too horrible to maintain. The few burdens I can think of are:

changing the BasicAuth interface will necessitate changing DigestAuth
if there's any future functionality we want to add, but that's only a burden if it's non-backward-compatible which seems rare (or it'd break the internet)

Related issue number

Continuation of #2213

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES/ folder
- name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)
- if you don't have an issue number, change it to the pull request
  number after creating the PR
  - .bugfix: A bug fix for something the maintainers deemed an
    improper undesired behavior that got corrected to match
    pre-agreed expectations.
  - .feature: A new behavior, public APIs. That sort of stuff.
  - .deprecation: A declaration of future API removals and breaking
    changes in behavior.
  - .breaking: When something public is removed in a breaking way.
    Could be deprecated in an earlier release.
  - .doc: Notable updates to the documentation structure or build
    process.
  - .packaging: Notes for downstreams about unobvious side effects
    and tooling. Changes in the test invocation considerations and
    runtime assumptions.
  - .contrib: Stuff that affects the contributor experience. e.g.
    Running tests, building the docs, setting up the development
    environment.
  - .misc: Changes that are hard to assign to any of the above
    categories.
- Make sure to use full sentences with correct case and punctuation,
  for example:
```
Fixed issue with non-ascii contents in doctest text files
-- by :user:`contributor-gh-handle`.
```
  Use the past tense or the present tense a non-imperative mood,
  referring to what's changed compared to the last released version
  of this project.

This is the initial work by jf from aio-libs#2213, which I rebased onto the tip of master. It is fully brought back to life in subsequent commits.

This completes the implementation of the digest helper, implementing for additional hashing algorithms sha256 and sha512, as well as auth-int qop. It implements tests and documents the features. The DigestAuth feature was shimmed into the existing API so it could be used in place of BasicAuth in the auth field of ClientSession and ClientRequest.

aiohttp/helpers.py

@@ -53,7 +57,7 @@
 from propcache.api import under_cached_property as reify
 from yarl import URL

-from . import hdrs
+from . import client_exceptions, hdrs


bdraco · 2025-04-15T23:39:35Z

aiohttp/helpers.py

+    #   key1="value1", key2=value2, key3="some value, with, commas"
+    #
+    # This regex attempts to parse that out
+    pattern = re.compile(


Please make this a constant so its not compiled every time

TimMenninger commented 7 minutes ago

...

bdraco now

so quick

bdraco · 2025-04-15T23:39:46Z

aiohttp/helpers.py

+        #                                           +--> at least one char req'd
+    )
+
+    header_pairs = {}


Missing type here

bdraco · 2025-04-15T23:42:03Z

We had talked about making a more generic client middleware, but we never moved that forward. I'm inclined to accept this since we would very much like to have digest auth support in 3.x, and move to using client middleware for auth in 4.x since it would be a breaking change anyways.

bdraco · 2025-04-15T23:45:01Z

aiohttp/helpers.py

+        if "nonce" not in self.ctx.challenge:
+            raise client_exceptions.ClientError("Challenge is missing nonce")
+
+        realm: str = self.ctx.challenge.get("realm", "")


nit: personal preference here would be to assign challenge = self.ctx.challenge so we don't have self.ctx.challenge on every line

Do you also feel this way in the authenticate function where I set all the values?

I think if #10725 (comment) would be implemented it would negate the the concern in another way

codspeed-hq · 2025-04-15T23:45:16Z

CodSpeed Performance Report

Merging #10725 will not alter performance

_{Comparing TimMenninger:digest-auth-helper (e6f53a9) with master (d912123)}

Summary

✅ 55 untouched benchmarks

TimMenninger · 2025-04-15T23:46:00Z

We had talked about making a more generic client middleware, but we never moved that forward. I'm inclined to accept this since we would very much like to have digest auth support in 3.x, and move to using client middleware for auth in 4.x since it would be a breaking change anyways.

I agree with that - I didn't see any other good way of doing this without breaking backward compatibility. Almost requested punting this one to 4.x but it ended up not being horrible for a first attempt. @feus4177 did most of the heavy lifting anyway.

bdraco · 2025-04-15T23:47:12Z

aiohttp/helpers.py

+        def H(x: str) -> str:
+            return hash_fn(x.encode()).hexdigest()
+
+        def KD(s: str, d: str) -> str:
+            return H(f"{s}:{d}")


Please give these docstrings. and link to sections in the RFC that define that these are so anyone who comes across this code later will know what this is doing

https://datatracker.ietf.org/doc/html/rfc2617#section-3.2.2.2

bdraco · 2025-04-15T23:48:13Z

aiohttp/helpers.py

+        pairs = [
+            f'username="{self.login}"',
+            f'realm="{realm}"',
+            f'nonce="{nonce}"',
+            f'uri="{path}"',
+            f'response="{response_digest}"',
+            f'algorithm="{algorithm}"',
+        ]
+        if opaque:
+            pairs.append(f'opaque="{opaque}"')
+        if qop:
+            pairs.append(f'qop="{qop}"')
+            pairs.append(f"nc={ncvalue}")
+            pairs.append(f'cnonce="{cnonce}"')


TODO: These need to be check for possible injection attacks in the event any of this could be user provided (it may be fine, but we need to analyze and think about it)

bdraco · 2025-04-15T23:48:58Z

aiohttp/helpers.py

+            if "realm" in header_pairs and header_pairs["realm"]:
+                self.ctx.challenge["realm"] = header_pairs["realm"]
+            if "nonce" in header_pairs and header_pairs["nonce"]:
+                self.ctx.challenge["nonce"] = header_pairs["nonce"]
+            if "qop" in header_pairs and header_pairs["qop"]:
+                self.ctx.challenge["qop"] = header_pairs["qop"]
+            if "algorithm" in header_pairs and header_pairs["algorithm"]:
+                self.ctx.challenge["algorithm"] = header_pairs["algorithm"]
+            if "opaque" in header_pairs and header_pairs["opaque"]:
+                self.ctx.challenge["opaque"] = header_pairs["opaque"]


Maybe could be written as a loop with a constant tuple as iteration source

bdraco · 2025-04-15T23:50:30Z

aiohttp/helpers.py

+    def __init__(self) -> None:
+        super().__init__()
+        self.init = False


probably could make init a classvar here and than drop __init__

bdraco · 2025-04-15T23:51:05Z

aiohttp/helpers.py

+    ...
+
+
+class DigestAuthContext(threading.local):


I'm a bit confused why we would need thread local storage since aiohttp is an asyncio library, we shouldn't have multiple threads to worry about.

Dreamsorcerer · 2025-04-15T23:52:40Z

We had talked about making a more generic client middleware, but we never moved that forward. I'm inclined to accept this since we would very much like to have digest auth support in 3.x, and move to using client middleware for auth in 4.x since it would be a breaking change anyways.

I was hoping to work on that reasonably soon. This PR would add several extra hooks throughout the code that we'd need to remember to remove again. It'd also be confusing to users if we add this and then deprecate it only a release or 2 later.

My current understanding is that the middlewares would not be a breaking change and they could be backported. Then BasicAuth would become deprecated in favour of the middlewares.

bdraco · 2025-04-15T23:56:38Z

We had talked about making a more generic client middleware, but we never moved that forward. I'm inclined to accept this since we would very much like to have digest auth support in 3.x, and move to using client middleware for auth in 4.x since it would be a breaking change anyways.

I was hoping to work on that reasonably soon. This PR would add several extra hooks throughout the code that we'd need to remember to remove again. It'd also be confusing to users if we add this and then deprecate it only a release or 2 later.

My current understanding is that the middlewares would not be a breaking change and they could be backported. Then BasicAuth would become deprecated in favour of the middlewares.

I'd be much happier with client middleware vs this implementation. Seems like that is closer than I expected so I'd probably stop working on this PR but keep it warm in case we run into any snags with client middleware that would prevent it from being available in 3.12.

bdraco · 2025-04-15T23:57:20Z

For clarity: Once we have the client middleware we could add a digest auth client middleware (probably much of this could be reused)

TimMenninger · 2025-04-16T00:02:45Z

I'd be much happier with client middleware vs this implementation. Seems like that is closer than I expected so I'd probably stop working on this PR but keep it warm in case we run into any snags with client middleware that would prevent it from being available in 3.12.

Okay I am going to put this down for now. I really only picked it up in an effort to move 3.12 along. Feel free to ping me if/when we're ready to pick back up.

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 15, 2025

feus4177 and others added 3 commits April 15, 2025 16:33

Added a digest authentication helper (1/2)

3cd9feb

This is the initial work by jf from aio-libs#2213, which I rebased onto the tip of master. It is fully brought back to life in subsequent commits.

FIXUP: revert proxy_auth back to BasicAuth

cd59ebd

github-advanced-security bot found potential problems Apr 15, 2025

View reviewed changes

FIXUP: added CHANGES file

22825c1

TimMenninger force-pushed the digest-auth-helper branch from 9927632 to 22825c1 Compare April 15, 2025 23:34

bdraco added the backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot label Apr 15, 2025

FIXUP: spelling

e6f53a9

bdraco reviewed Apr 15, 2025

View reviewed changes

aiohttp/helpers.py

# +--> at least one char req'd

)

header_pairs = {}

Copy link

Member

bdraco Apr 15, 2025

Choose a reason for hiding this comment

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

Missing type here

bdraco reviewed Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Digest auth helper #10725

Digest auth helper #10725

TimMenninger commented Apr 15, 2025

Check notice

bdraco Apr 15, 2025

TimMenninger Apr 15, 2025

bdraco Apr 15, 2025

bdraco commented Apr 15, 2025

bdraco Apr 15, 2025

TimMenninger Apr 15, 2025

bdraco Apr 15, 2025

codspeed-hq bot commented Apr 15, 2025 •

edited

Loading

TimMenninger commented Apr 15, 2025

bdraco Apr 15, 2025

bdraco Apr 15, 2025 •

edited

Loading

bdraco Apr 15, 2025

bdraco Apr 15, 2025

bdraco Apr 15, 2025 •

edited

Loading

Dreamsorcerer commented Apr 15, 2025

bdraco commented Apr 15, 2025

bdraco commented Apr 15, 2025

TimMenninger commented Apr 16, 2025

Digest auth helper #10725

Are you sure you want to change the base?

Digest auth helper #10725

Conversation

TimMenninger commented Apr 15, 2025

What do these changes do?

Are there changes in behavior for the user?

Is it a substantial burden for the maintainers to support this?

Related issue number

Checklist

Check notice

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdraco commented Apr 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codspeed-hq bot commented Apr 15, 2025 • edited Loading

Merging #10725 will not alter performance

Summary

TimMenninger commented Apr 15, 2025

Choose a reason for hiding this comment

bdraco Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdraco Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

Dreamsorcerer commented Apr 15, 2025

bdraco commented Apr 15, 2025

bdraco commented Apr 15, 2025

TimMenninger commented Apr 16, 2025

codspeed-hq bot commented Apr 15, 2025 •

edited

Loading

bdraco Apr 15, 2025 •

edited

Loading

bdraco Apr 15, 2025 •

edited

Loading