Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a digest authentication helper #2213

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 179 additions & 1 deletion aiohttp/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@
import time
import warnings
import weakref
import hashlib
from collections import namedtuple
from math import ceil
from pathlib import Path
from time import gmtime
from urllib.parse import quote
from urllib.parse import quote, urlparse

from async_timeout import timeout

Expand Down Expand Up @@ -216,6 +217,183 @@ def encode(self):
return 'Basic %s' % base64.b64encode(creds).decode(self.encoding)


def parse_pair(pair):
if '=' not in pair:
return pair, None

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


def parse_key_value_list(header):
return {
key: value for key, value in
map(parse_pair, header.split(' '))
Copy link
Member

Choose a reason for hiding this comment

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

Please replace map with a nested comprehension.
It's a part of aiohtp preferred codestyle.

}


class DigestAuth():
"""HTTP digest authentication helper.
The work here is based off of https://github.com/requests/requests/blob/v2.18.4/requests/auth.py.

:param str username: Username or login
Copy link
Member

Choose a reason for hiding this comment

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

Don't use autodocs markup in headers -- we don't use autodoc facility anyway.

:param str password: Password
:param ClientSession session: Session to use digest auth
"""

def __init__(self, username, password, session):
self.username = username
self.password = password
self.last_nonce = ''
self.nonce_count = 0
self.challenge = None
self.num_401 = 0
self.args = {}
self.session = session

@asyncio.coroutine
Copy link
Contributor

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.

def request(self, method, url, *, headers=None, **kwargs):
if headers is None:
headers = {}

# Save the args so we can re-run the request
self.args = {
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

I think this applies to self.challenge as well.

'method': method,
'url': url,
'headers': headers,
'kwargs': kwargs
}

if self.challenge:
headers[hdrs.AUTHORIZATION] = self.build_digest_header(
method.upper(), url
)

response = yield from self.session.request(
method, url, headers=headers, **kwargs
)

# If the response is not in the 400 range, do not try digest
# authentication.
if not 400 <= response.status < 500:
Copy link
Member

Choose a reason for hiding this comment

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

So why do we try to auth in case of...503?

Copy link
Author

Choose a reason for hiding this comment

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

It wouldn't try to do auth in the in the case of a 503. It will only try to perform digest authentication if the response status is from 400 up to but not including 500. Perhaps this would be clearer?

        # Only try performing digest authentication if the response status is
        # from 400 to 500.
        if 400 <= response.status < 500:
            return await self._handle_401(response)
        
        return response

As for why we don't just check for 401, you can see the discussion here: psf/requests#3772.

Copy link
Author

Choose a reason for hiding this comment

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

You'll notice the behavior should be equivalent but perhaps this is more readable.

self.num_401 = 1
return response

return (yield from self.handle_401(response))

def build_digest_header(self, method, url):
"""
:rtype: str
"""

realm = self.challenge['realm']
nonce = self.challenge['nonce']
qop = self.challenge.get('qop')
algorithm = self.challenge.get('algorithm', 'MD5').upper()
opaque = self.challenge.get('opaque')

# lambdas assume digest modules are imported at the top level
if algorithm == 'MD5' or algorithm == 'MD5-SESS':
hash_fn = hashlib.md5
elif algorithm == 'SHA':
hash_fn = hashlib.sha1
else:
return ''

def hash_utf8(x):
if isinstance(x, str):
x = x.encode('utf-8')
return hash_fn(x).hexdigest()

KD = lambda s, d: hash_utf8('%s:%s' % (s, d))

parsed = urlparse(url)
Copy link
Member

Choose a reason for hiding this comment

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

We have yarl. Why url parse?

Copy link
Author

Choose a reason for hiding this comment

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

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

#: path is request-uri defined in RFC 2616 which should not be empty
path = parsed.path or "/"
if parsed.query:
path += '?' + parsed.query

A1 = '%s:%s:%s' % (self.username, realm, self.password)
Copy link
Member

Choose a reason for hiding this comment

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

Same question about meaningless names.

A2 = '%s:%s' % (method, path)

HA1 = hash_utf8(A1)
HA2 = hash_utf8(A2)

if nonce == self.last_nonce:
self.nonce_count += 1
else:
self.nonce_count = 1
Copy link
Member

Choose a reason for hiding this comment

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

This branch is redundant. 0 +1 equals 1.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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


self.last_nonce = nonce

ncvalue = '%08x' % self.nonce_count

k = str(self.nonce_count).encode('utf-8')
k += nonce.encode('utf-8')
k += time.ctime().encode('utf-8')
k += os.urandom(8)
Copy link
Member

Choose a reason for hiding this comment

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

How about join?

cnonce = (hashlib.sha1(k).hexdigest()[:16])

if algorithm == 'MD5-SESS':
HA1 = hash_utf8('%s:%s:%s' % (HA1, nonce, cnonce))

if not qop:
respdig = KD(HA1, '%s:%s' % (nonce, HA2))
elif qop == 'auth' or 'auth' in qop.split(','):
noncebit = '%s:%s:%s:%s:%s' % (
nonce, ncvalue, cnonce, 'auth', HA2
)
respdig = KD(HA1, noncebit)
else:
return ''

base = 'username="%s", realm="%s", nonce="%s", uri="%s", ' \
'response="%s"' % (self.username, realm, nonce, path, respdig)
if opaque:
base += ', opaque="%s"' % opaque
if algorithm:
base += ', algorithm="%s"' % algorithm
if qop:
base += ', qop="auth", nc=%s, cnonce="%s"' % (ncvalue, cnonce)

return 'Digest %s' % base

@asyncio.coroutine
def handle_401(self, response):
"""
Takes the given response and tries digest-auth, if needed.
:rtype: ClientResponse
"""
auth_header = response.headers.get('www-authenticate', '')

if 'digest' in auth_header.lower() and self.num_401 < 2:

self.num_401 += 1
pattern = re.compile(r'digest ', flags=re.IGNORECASE)
self.challenge = parse_key_value_list(
pattern.sub('', auth_header, count=1)
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like you want to strip first word from string if it's match digest in lower case. No reason for regexp here.

)

return (yield from self.request(
self.args['method'],
self.args['url'],
headers=self.args['headers'],
**self.args['kwargs'],
))

self.num_401 = 1
return response


if PY_352:
Copy link
Member

Choose a reason for hiding this comment

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

No longer actual. We require 3.5.3+ on master.

def create_future(loop):
return loop.create_future()
Expand Down