Added a digest authentication helper #2213

feus4177 · 2017-08-20T01:28:04Z

What do these changes do?

Added a digest authentication helper.

Are there changes in behavior for the user?

None

Related issue number

Resolves #4939

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_id>.<type> for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

I didn't go through the checklist. I just wanted to see if this is something you are interested in incorporating into the aiohttp package. To be able to use this DigestAuth helper like the aiohttp BasicAuth helper would take some restructuring so I decided to keep it as isolated as possible right now. Let me know what you think and I can make any changes as necessary.

asvetlov · 2017-08-20T06:36:30Z

Sorry, I don't follow how to use the helper.
Full test coverage is required also.

feus4177 · 2017-08-20T18:24:42Z

Sorry, I meant to add this snippet as well.

import aiohttp
import asyncio

client = aiohttp.ClientSession()
auth = aiohttp.auth.DigestAuth('usr', 'psswd', client)

async def fetch(url, **kwargs):
    return await auth.request('GET', url, **kwargs)


loop = asyncio.get_event_loop()
response = loop.run_until_complete(fetch('http://httpbin.org/digest-auth/auth/usr/psswd/MD5/never'))

feus4177 · 2017-08-20T18:29:33Z

Obviously, I can add tests, documentation, etc. Before I go through all that though, I just wanted to see if there is any interest in it.

asvetlov · 2017-09-17T08:53:11Z

Well, idea is good but it should be proved by test suite.

oleksandr-kuzmenko · 2017-10-11T22:06:26Z

This is a good idea!
But I think it would be better to add digest authentication support to standard client, something like:

                                     |
                                     v
async with aiohttp.ClientSession(digest_auth=...) as session:
    async with session.get('https://api.github.com/events') as resp:
        print(resp.status)

kxepal · 2017-10-12T01:33:18Z

@Alxpy
Yeap, that's looks simpler for users. But it will overload ClientSession signature too much. Because digest and basic auths are not the only around. Having sort-of decorators / middlewares instead gives you more control: you can implement own auth method like this without touching aiohttp sources. And aiohttp developers wouldn't have to maintain such a jack of all trades object.

oleksandr-kuzmenko · 2017-10-12T11:21:46Z

@kxepal yes, that makes sense, I agree

feus4177 · 2017-10-30T00:17:10Z

That was the kind of discussion I was looking for. It sounds like you guys would prefer to keep it separated like it currently is. I'll go ahead and add the tests and documentation and then update the pull request. Thanks.

redixin · 2017-10-30T00:55:58Z

aiohttp/helpers.py

+        self.args = {}
+        self.session = session
+
+    @asyncio.coroutine


Nobody uses this syntax here anymore.

…ion of method in docs

feus4177 · 2017-11-18T22:02:24Z

So I updated the pull request to following the contributing guidelines (test, docs, etc.). Since it has been a while I also went ahead and merged the latest from aio-libs/aiohttp:master into the pull request. It looks like you guys switched over to the async/await syntax so I did that as well. However, half of all the tests are failing now. It looks like it's that way for all the tests though, so I'm assuming it isn't an issue. Let me know if there is anything else you want me to do.

kxepal · 2017-11-18T21:46:49Z

aiohttp/helpers.py

+        def KD(s, d):
+            return H('%s:%s' % (s, d))
+
+        parsed = urlparse(url)


We have yarl. Why url parse?

I was unfamiliar with yarl; I will switch it over.

kxepal · 2017-11-18T21:48:06Z

aiohttp/helpers.py

+        if nonce == self.last_nonce:
+            self.nonce_count += 1
+        else:
+            self.nonce_count = 1


This branch is redundant. 0 +1 equals 1.

If you re-use the instance, the nonce_count count can grow without bound. The nonce count is important in preventing replay attacks. There probably aren't too many cases where the nonce will need to reset but perhaps in the event of some failed messages.

I should clarify, if you re-use the instance of DigestAuth for performing multiple requests.

kxepal · 2017-11-18T21:48:35Z

aiohttp/helpers.py

+        k = str(self.nonce_count).encode()
+        k += nonce.encode()
+        k += time.ctime().encode()
+        k += os.urandom(8)


How about join?

kxepal · 2017-11-18T21:49:43Z

aiohttp/helpers.py

+            )
+            respdig = KD(HA1, noncebit)
+        else:
+            raise client_exceptions.ClientError(


qop is known quite early above. How about to fail fast?

There would have to be some duplication of logic around qop and I didn't think that the performance was that big of an issue. I can move it though.

kxepal · 2017-11-18T21:50:54Z

aiohttp/helpers.py

+        return response
+
+
+if PY_352:


No longer actual. We require 3.5.3+ on master.

kxepal · 2017-11-18T22:04:10Z

tests/test_helpers.py

+    def handler(request):
+        realm = '[email protected]'
+        if 'Authorization' in request.headers:
+            pattern = re.compile(r'digest ', flags=re.IGNORECASE)


triple times.

kxepal · 2017-11-18T22:04:23Z

tests/test_helpers.py

+    def handler(request):
+        realm = '[email protected]'
+        if 'Authorization' in request.headers:
+            pattern = re.compile(r'digest ', flags=re.IGNORECASE)


ok, multiple ones.

kxepal · 2017-11-18T22:05:19Z

tests/test_helpers.py

+@asyncio.coroutine
+def test_digest_auth_MD5_sess(loop, test_client):
+    opaque = binascii.hexlify(os.urandom(16))
+    username = 'usr'


wh usr nd psswd whl username nd password wrwhr ls?

I was trying to demonstrate that the helper isn't just hardcoded to work with one username and password. Although, this really doesn't challenge it at all. Once you decide what you want to do about the string escaping I can change this to include something more difficult like spaces and quotes.

kxepal · 2017-11-18T22:05:47Z

tests/test_helpers.py

+    def handler(request):
+        realm = '[email protected]'
+        if 'Authorization' in request.headers:
+            pattern = re.compile(r'digest ', flags=re.IGNORECASE)


kxepal · 2017-11-18T22:06:21Z

tests/test_helpers.py

+    def handler(request):
+        realm = '[email protected]'
+        if 'Authorization' in request.headers:
+            pattern = re.compile(r'digest ', flags=re.IGNORECASE)


ok, tests are the same as the regular code - please, show it some love in the name of maintenance.

asvetlov · 2017-11-22T17:03:38Z

Tests still are red :(

WGH- · 2018-05-22T19:11:48Z

aiohttp/helpers.py

+            headers = {}
+
+        # Save the args so we can re-run the request
+        self.args = {


Why do you store last request as a class attribute? Won't it cause everything to break when DigestAuth instance is used by multiple coroutines simultaneously?

Yeah, that's a good point. We can just store it as a local variable and then pass it to _handle_401.

I think this applies to self.challenge as well.

WGH- · 2018-05-26T12:43:21Z

aiohttp/helpers.py

+
+        # Only try performing digest authentication if the response status is
+        # from 400 to 500.
+        if 400 <= response.status < 500:


In one of my tests, when username-password pair was incorrect, this led to infinite recursion. I think the fix is to call _handle_401 only if self.challenge is None.

WGH- · 2018-07-29T21:28:36Z

aiohttp/helpers.py

+            nonce,
+            time.ctime(),
+            os.urandom(8).decode(errors='ignore'),
+        ]).encode()


os.urandom(8).decode(errors='ignore') looks really odd. It basically discards characters that cannot be decoded as UTF-8, so the string is usually shorter than 8 characters. This would be better, I think:

cnonce_data = ''.join([ str(self.nonce_count), nonce, time.ctime(), ]).encode() cnonce_data += os.urandom(8)

RouquinBlanc · 2018-09-06T13:40:33Z

Hi guys,

What do you think the status of this pull request is and do you have a rough idea if it is going to be ready any time soon-ish? It seems quite close from an external eye...

At the moment AFAIK there is no pure asyncio framework supporting digest authentication, you have to either go back to tornado or to give up on asyncio and look at requests...

Thanks!

asvetlov · 2018-09-10T08:25:38Z

I like the idea but have a feeling that we need to use slightly different and not existing yet API for it.

DavHau · 2018-09-13T20:05:03Z

At the moment AFAIK there is no pure asyncio framework supporting digest authentication, you have to either go back to tornado or to give up on asyncio and look at requests...

@RouquinBlanc
This is not true. There is:
Pulsar: https://github.com/quantmind/pulsar (file upload in combination with DigestAuth is broken at the time of writing)
or
yieldfromrequests: https://github.com/rdbhost/yieldfromrequests (not updated since 4 years, but works for me. Only usable with old generator style async syntax. write yourself a wrapper ;) )

I'm also really looking forward to have DigestAuth available in aiohttp since it is my favorite async http lib.

sonicblind · 2019-01-17T21:49:32Z

@asvetlov
I found a small bug related to Digest Auth Header in helpers.py.

The Digest header can include spaces in realm name, for example:
Digest realm="iMC RESTful Web Services", qop="auth", nonce="MTU0....=="

My suggestion as per below:

def parse_key_value_list(header):
    return {
        key: value for key, value in
#       [parse_pair(header_pair) for header_pair in header.split(' ')]
        [parse_pair(header_pair) for header_pair in header.split(', ')]
    }

But this is probably also not bulletproof as the realm could include also comma.
So maybe header.split('", ') would work even better.

And if the above make sense, then the "remove trailing comma" section in parse_pair(pair) should also be updated accordingly:

def parse_pair(pair):
    key, value = pair.split('=', 1)

    # If it has a trailing comma, remove it.
#   if value[-1] == ',':
#       value = value[:-1]

    # If it is quoted, then remove them.
    if value[0] == value[-1] == '"':
        value = value[1:-1]
    
    return key, value

asvetlov · 2019-01-17T22:02:12Z

@sonicblind thanks for the comment.
The PR doesn't go to merge but I'd like to keep it open as a reference for eventual new implementation built from scratch.

bdraco · 2020-03-04T06:38:44Z

Thanks for working on this. @feus4177 @asvetlov Is there a shot that support for digest auth is forthcoming any time soon?

kiknaio · 2020-07-24T06:40:17Z

Still nothing new. I think it would be amazing if you complete this feature request.

svisser · 2020-09-30T13:44:16Z

docs/client_reference.rst

+
+.. class:: DigestAuth(login, password', session)
+
+   HTTP digest authentication helper. Unlike :class:`DigestAuth`, this helper


Unlike BasicAuth ?

CLAassistant · 2020-11-16T10:17:31Z

All committers have signed the CLA.

ascott18 · 2021-12-29T20:43:16Z

aiohttp/helpers.py

+def parse_key_value_list(header):
+    return {
+        key: value for key, value in
+        [parse_pair(header_pair) for header_pair in header.split(' ')]


Splitting on space does not work because values can be quoted strings that contain spaces.

E.g. 'realm="Login to AMC072A731527963121B", qop="auth", nonce="839506863", opaque="f5a15b51387c97add984d4d948213e563fe3eede"'

skewty · 2022-03-14T20:21:44Z

I still don't see aiohttp.helpers.DigestAuth so it would seem this never got merged. Can we get this merged?

flashnuke · 2022-12-10T19:37:31Z

This would be of tremendous value... any chance of merging?

webknjaz · 2024-01-29T01:56:18Z

Hey @feus4177, are you planning to resume this effort? It looks like a lot of reviews accumulated over the years. Plus, there's merge conflicts that would have to be addressed.

bdraco · 2024-01-29T02:28:56Z

It would be great to get this one implemented as its one of the key drivers for Home Assistant devs choosing alternate (usually slower) libraries because the device they are integrating needs digest auth.

feus4177 · 2024-01-29T03:25:58Z

I have no intention of continuing this work. I tried to get feedback on the overall approach of the work before I invested a bunch of time and effort into finalizing the details: #2213 (comment). After some small feedback I went through and cleaned everything up, made sure all the tests were passing and everything. Then I was hit with this: #2213 (comment). I didn't enjoy jumping through all the hoops just to then revisit the initial feedback I was trying to solicit. I have no intention of going through all that work again knowing that it will all be in vain if the owners decide that they still don't like the architecture of the solution.

bdraco · 2024-01-29T18:18:52Z

I have no intention of continuing this work

The current authentication interface design needs to accommodate additional future methods, and a redesign is required. Unfortunately, this conclusion was realized late in the review process, and I'm sure you wanted to do something other than sign up to do that redesign. I can understand how that is frustrating.

Thank you for your contributions thus far and for following up on this PR.

feus4177 · 2024-01-31T18:04:18Z

@bdraco, I appreciate your understanding. I also think a redesign is completely reasonable. I just think it's up to the maintainers at this point.

Dreamsorcerer · 2024-02-02T13:19:04Z

The current authentication interface design needs to accommodate additional future methods, and a redesign is required.

I think there's a discussion elsewhere, but I noted that auth in other libraries often allows arbitrary functions to run and modify the request. I suggested we could just call this a middleware, and make it work very similarly to the server-side middlewares. Then people can use it for various auth methods or whatever else they might want to do.

bdraco · 2024-11-09T14:07:42Z

I started thinking about this in #9732

Added a digest authentication helper

2753eae

redixin reviewed Oct 30, 2017

View reviewed changes

aiohttp/helpers.py Outdated

self.args = {}

self.session = session

@asyncio.coroutine

Copy link

Contributor

redixin Oct 30, 2017

Choose a reason for hiding this comment

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

Nobody uses this syntax here anymore.

feus4177 added 5 commits November 13, 2017 22:31

Added tests and documentation

393c2b9

Added previous param and fixed tests to get 100% coverage of digest auth

a3352b8

Added myself to CONTRIBUTORS, added news fragment, and fixed indentat…

c688ee1

…ion of method in docs

Merge branch 'master' into master

26a6064

Switched from @asyncio.coroutine/yield to async/await

483239f

kxepal reviewed Nov 18, 2017

View reviewed changes

feus4177 and others added 10 commits November 18, 2017 17:27

Got rid of some scratch variables and did a little renaming

224174f

Fail fast for qop

22f2c9d

Fixed a string encoding problem

cf9b333

replaced regex

c47672f

Updated docs as per PR comments

6e0b534

Can't assume there will always be two parts

bc15623

Switched over to yarl

847de7b

Tried to factor out repetition in digest auth tests

e454032

Swapped some logic around

a995be5

Merge branch 'master' into master

c3e8a33

feus4177 added 2 commits November 22, 2017 16:52

Must have added this in when trying to fix merge conflicts

587a54b

Merge branch 'master' of github.com:feus4177/aiohttp

e800176

WGH- reviewed May 22, 2018

View reviewed changes

WGH- reviewed May 26, 2018

View reviewed changes

WGH- reviewed Jul 29, 2018

View reviewed changes

lalanza808 mentioned this pull request Apr 27, 2019

[enhancment] asyncio support could be useful monero-ecosystem/monero-python#50

Open

xiangyan99 mentioned this pull request Dec 18, 2019

Add digest proxy auth support for aiohttp Azure/azure-sdk-for-python#5268

Closed

svisser reviewed Sep 30, 2020

View reviewed changes

webknjaz mentioned this pull request Oct 3, 2020

support for digest authentication #4939

Open

ascott18 reviewed Dec 29, 2021

View reviewed changes

ascott18 mentioned this pull request Jan 10, 2022

Improvements to Dahua/Amcrest module keshavdv/unifi-cam-proxy#135

Closed

WhyNotHugo mentioned this pull request Dec 13, 2022

vdirsyncer 0.19: digest auth broken pimutils/vdirsyncer#1015

Closed

bdraco added this to the 3.11 milestone Aug 10, 2024

bdraco modified the milestones: 3.11, 3.12 Oct 28, 2024


		.. class:: DigestAuth(login, password', session)

		HTTP digest authentication helper. Unlike :class:`DigestAuth`, this helper

Added a digest authentication helper #2213

Are you sure you want to change the base?

Added a digest authentication helper #2213

Conversation

feus4177 commented Aug 20, 2017 • edited by webknjaz Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

asvetlov commented Aug 20, 2017

feus4177 commented Aug 20, 2017

feus4177 commented Aug 20, 2017 • edited Loading

asvetlov commented Sep 17, 2017

oleksandr-kuzmenko commented Oct 11, 2017 • edited Loading

kxepal commented Oct 12, 2017

oleksandr-kuzmenko commented Oct 12, 2017

feus4177 commented Oct 30, 2017

Choose a reason for hiding this comment

feus4177 commented Nov 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RouquinBlanc commented Sep 6, 2018 • edited Loading

asvetlov commented Sep 10, 2018

DavHau commented Sep 13, 2018

sonicblind commented Jan 17, 2019

asvetlov commented Jan 17, 2019

bdraco commented Mar 4, 2020

kiknaio commented Jul 24, 2020

Choose a reason for hiding this comment

CLAassistant commented Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

skewty commented Mar 14, 2022

flashnuke commented Dec 10, 2022

webknjaz commented Jan 29, 2024

bdraco commented Jan 29, 2024

feus4177 commented Jan 29, 2024

bdraco commented Jan 29, 2024

feus4177 commented Jan 31, 2024

Dreamsorcerer commented Feb 2, 2024

bdraco commented Nov 9, 2024

feus4177 commented Aug 20, 2017 •

edited by webknjaz

Loading

feus4177 commented Aug 20, 2017 •

edited

Loading

oleksandr-kuzmenko commented Oct 11, 2017 •

edited

Loading

RouquinBlanc commented Sep 6, 2018 •

edited

Loading

CLAassistant commented Nov 16, 2020 •

edited

Loading