URLLIB2 seems to have issues attempting to digest authenticate against a
Windows-based IIS 6.x server.  The IIS server requests MD5-sess support,
and URLLIB2 throws an exception that ... far from explains this:

Traceback (most recent call last):
  File "/usr/lib64/python2.5/runpy.py", line 95, in run_module
    filename, loader, alter_sys)
  File "/usr/lib64/python2.5/runpy.py", line 52, in _run_module_code
    mod_name, mod_fname, mod_loader)
  File "/usr/lib64/python2.5/runpy.py", line 32, in _run_code
    exec code in run_globals
  File "/srv/pubPal/test/test_auth.py", line 16, in <module>
    print opener.open(uri)
  File "/usr/lib64/python2.5/urllib2.py", line 380, in open
    response = meth(req, response)
  File "/usr/lib64/python2.5/urllib2.py", line 491, in http_response
    'http', request, response, code, msg, hdrs)
  File "/usr/lib64/python2.5/urllib2.py", line 412, in error
    result = self._call_chain(*args)
  File "/usr/lib64/python2.5/urllib2.py", line 353, in _call_chain
    result = func(*args)
  File "/usr/lib64/python2.5/urllib2.py", line 992, in http_error_401
    host, req, headers)
  File "/usr/lib64/python2.5/urllib2.py", line 884, in http_error_auth_reqed
    return self.retry_http_digest_auth(req, authreq)
  File "/usr/lib64/python2.5/urllib2.py", line 889, in
retry_http_digest_auth
    auth = self.get_authorization(req, chal)
  File "/usr/lib64/python2.5/urllib2.py", line 920, in get_authorization
    H, KD = self.get_algorithm_impls(algorithm)
  File "/usr/lib64/python2.5/urllib2.py", line 972, in get_algorithm_impls
    return H, KD
UnboundLocalError: local variable 'H' referenced before assignment


The offending code is urllib2.py line 972:

    def get_algorithm_impls(self, algorithm):
        # lambdas assume digest modules are imported at the top level
        if algorithm == 'MD5':
            H = lambda x: hashlib.md5(x).hexdigest()
        elif algorithm == 'SHA':
            H = lambda x: hashlib.sha1(x).hexdigest()
        # XXX MD5-sess
        KD = lambda s, d: H("%s:%s" % (s, d))
        return H, KD

I'm not sure what's meant by the # XXX MD5-sess comment there... But
what ends up happening is that MD5-sess matches neither the if or the
elif, and when return attempts to return, H is undefined.

This could be easily patched, but support for MD5-sess would be great as
well.  

In my estimation, at the least, urllib2 shouldn't crap out in such an
obscure way when encountering an unsupported algorithm.

I'd suggest changing line 970 (The # XXX MD5-sess line) to:

 raise Exception, "Unsupported Digest Authentication Algorithm '%s'" %
(algorithm)

Sorry, this isn't a crash.  It doesn't crash the interpreter.  I'll
assume "behavior' is the correct categorization.

Marking as easy, assuming you add the exception; implementing MD5-sess
is probably not easy.

Here is a patch for supporting MD5-sess, following RFC2617 specification.

Some comments/warnings:
 * I've only tested the patch against IIS 6.0. I don't know about other servers supporting MD5-sess.
 * IIS 6.0 expects the User Agent to send the URI (in the Authorization header) without the query string.
 * This patch doesn't add support for Digest sessions. For each request we make, we get a new [401|407] message with a new nonce (depending if we're talking about a proxy with digest authentication or a web server). Then we generate another authenticated request using that nonce. For Digest sessions to be fully supported, we should be adding an [WWW|Proxy]-Authenticate header in each following request we made to the server using the last nonce. This includes both MD5-sess and MD5 digest implementations.
 * A1 is being recalculated for every request. Given the above, this is not a real problem.

I'll open a new ticket regarding Digest sessions.

I tried to test the patch but got:-
c:\Python26>python.exe lib\test\test_urllib2_localnet.py
Traceback (most recent call last):
  File "lib\test\test_urllib2_localnet.py", line 325, in <module>
    class DigestMD5sessClientAuthTests(BaseTestCase):
NameError: name 'BaseTestCase' is not defined

Is this moving anywhere?  I was asked to apply this patch locally and am curious as to whether it is moving into the stdlib.

Adding some minor fixes to the original patch to apply cleanly to current 2.7 tip.

Also, add a conditional branch in the case that the server returns none of the supported algorithm strings, to raise a ValueError (the previous patch adds support for 'MD5-sess' but the original non-descript stack trace would be seen again with some other unsupported algorithm string).

Just got bitten by this as well, what still needs to happen with the patch?

One question is whether this is a bug fix or a feature request.

Other than that, I'd like to see the test classes collapsed into a single test class, considering that each one only has a single test in it.  Probably ProxyAuthTests should be refactored so that the stuff that is currently in setUp is a method that gets called with appropriate parameters instead, and all the new tests moved on to ProxyAuthTests.

Also, a version of the patch for 3.x would be most helpful, since it won't port cleanly due to the renamings.

Yes, it is a feature. Sorry that I have not paid attention to this. The Windows (IIS) part led me to delay as I did not have any to test. Let me take this up and see through it in 3.3.

Hmm, I'd argue it's a bug:

  File "/usr/lib64/python2.5/urllib2.py", line 972, in get_algorithm_impls
    return H, KD
UnboundLocalError: local variable 'H' referenced before assignment

...does not say anything like:

"The digest authentication scheme you have requested is not supported"

Now, as to whether it's a bug that 'MD5-sess' isn't supported is a tougher call. The XXX indicates the intention was certainly for it to be supported...

Oh, the bad error message is definitely a bug.  The question is whether we can also add md5-sess support while fixing it.  Sounds like Senthil thinks no, in which case this issue needs to be split into two parts.

...which is, of course, rather disappointing.
When *would* md5-sess land? 2.7? 3.3?!

3.3.

IMO this is in the grey area between feature and bug fix.  I think it is possible to argue that it can be treated as a bug fix, but I think we need opinions from other developers if we want to try to go that route.

The reason I think it can be argued that it can be treated as a bug fix is that (if I understand correctly) there is no difference in the application code.  The only difference is whether or not one can successfully communicate with IIS 6.  That may not be a sufficient argument, but it is an argument :)

Who with and where does the argument need to be had?

Hmm.  How about python-committers?  I would suggest python-dev, but that feels like invoking the dragon :)

Python 3.3 has been released a while ago and is in bug fix mode.

Personally I'm against this enhancement. It adds a digest auth scheme that is based on a broken and insecure hashing algorithm.

Could we at least do something cleaner that let the interpreter raise an UnboundLocalError ? Maybe something like "NotImplemented" ?

Sure.  Would you like to propose a patch?  It does seem that NotImplementedError would be the most appropriate.  It could give Christian's reason why it is not implemented.

here is the patch, for the trunk

and here for the 2.7 branch

But I think md5-sess should really be integrated. It's a standard mechanism described by a RFC (https://www.ietf.org/rfc/rfc2617.txt), and people need it, however insecure it may be (aren't other method (md5) insecure too ?).

up

python 2.7

python current

I do wonder if NotImplementedError is the right exception. According to the documentation it is derived from RuntimeError and is meant for abstract methods, i.e. it is a programmer error. Other cases in urllib.request raise ValueError (e.g. AbstractBasicAuthHandler), which is fairly normal for protocol errors like this.

Also it would be nice to add a test case for the bug fix.

I can see in the tests (test_urllib2_localnet.py) that Digest auth is tested only through "ProxyAuthTests". Is that sufficient ? Isn't it a bug ? If no, should I add the test in that class ?

first draft

I'm waiting for reviews.

New changeset 22eab50cb585 by Berker Peksag in branch '3.5':
Issue #2202: Fix UnboundLocalError in AbstractDigestAuthHandler.get_algorithm_impls
https://hg.python.org/cpython/rev/22eab50cb585

New changeset 046c75c9cd33 by Berker Peksag in branch 'default':
Issue #2202: Fix UnboundLocalError in AbstractDigestAuthHandler.get_algorithm_impls
https://hg.python.org/cpython/rev/046c75c9cd33

New changeset d135799e952b by Berker Peksag in branch '2.7':
Issue #2202: Fix UnboundLocalError in AbstractDigestAuthHandler.get_algorithm_impls
https://hg.python.org/cpython/rev/d135799e952b

Thanks, Mathieu! I went with a simpler approach to test the code.

Much better indeed. Thanks.