classification
Title: urllib2 fails against IIS 6.0 (No support for MD5-sess auth)
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: orsenthil Nosy List: berker.peksag, bwmcadams, christian.heimes, cjw296, deronnax, dieresys, georg.brandl, jjlee, kristjan.jonsson, martin.panter, orsenthil, python-dev, r.david.murray, santoso.wijaya
Priority: normal Keywords: patch

Created on 2008-02-28 18:26 by bwmcadams, last changed 2016-03-08 12:48 by deronnax. This issue is now closed.

Files
File name Uploaded Description Edit
urllib2-support_md5-sess.diff dieresys, 2010-01-21 21:53 add support for MD5-sess review
issue2202_py27.patch santoso.wijaya, 2011-04-11 18:37 review
md5-sess_not_implem_cur.diff deronnax, 2014-09-25 14:57 review
md5-sess_not_implem_27.diff deronnax, 2014-09-25 14:57 review
md5-sess_not_implem_27_v2.diff deronnax, 2016-02-18 08:01 '%s' changed to %r
md5-sess_not_implem_cur_v2.diff deronnax, 2016-02-18 08:02 '%s' changed to %r review
digest_md5sess_unittest.diff deronnax, 2016-02-28 11:50 review
Messages (33)
msg63101 - (view) Author: Brendan W. McAdams (bwmcadams) Date: 2008-02-28 18:26
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)
msg63102 - (view) Author: Brendan W. McAdams (bwmcadams) Date: 2008-02-28 18:27
Sorry, this isn't a crash.  It doesn't crash the interpreter.  I'll
assume "behavior' is the correct categorization.
msg63108 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2008-02-28 21:59
Marking as easy, assuming you add the exception; implementing MD5-sess
is probably not easy.
msg98115 - (view) Author: Manuel Muradás (dieresys) Date: 2010-01-21 21:53
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.
msg109874 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-10 15:04
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
msg133535 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2011-04-11 16:15
Is this moving anywhere?  I was asked to apply this patch locally and am curious as to whether it is moving into the stdlib.
msg133540 - (view) Author: Santoso Wijaya (santoso.wijaya) * Date: 2011-04-11 18:37
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).
msg138847 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2011-06-23 01:27
Just got bitten by this as well, what still needs to happen with the patch?
msg138848 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-06-23 01:44
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.
msg138849 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-06-23 02:11
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.
msg138855 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2011-06-23 07:11
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...
msg138865 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-06-23 12:53
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.
msg138866 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2011-06-23 13:34
...which is, of course, rather disappointing.
When *would* md5-sess land? 2.7? 3.3?!
msg138869 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-06-23 15:46
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 :)
msg138992 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2011-06-24 21:49
Who with and where does the argument need to be had?
msg138995 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-06-24 22:13
Hmm.  How about python-committers?  I would suggest python-dev, but that feels like invoking the dragon :)
msg192499 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-06 23:11
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.
msg220429 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2014-06-13 10:44
Could we at least do something cleaner that let the interpreter raise an UnboundLocalError ? Maybe something like "NotImplemented" ?
msg220433 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-06-13 12:50
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.
msg227540 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2014-09-25 14:57
here is the patch, for the trunk
msg227541 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2014-09-25 14:57
and here for the 2.7 branch
msg227884 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2014-09-30 12:06
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 ?).
msg260423 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-02-18 03:50
up
msg260437 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-02-18 08:01
python 2.7
msg260438 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-02-18 08:02
python current
msg260448 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-18 10:57
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.
msg260971 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-02-28 10:27
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 ?
msg260972 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-02-28 11:50
first draft
msg261163 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-03-03 12:42
I'm waiting for reviews.
msg261252 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-03-06 14:17
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
msg261253 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-03-06 14:27
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
msg261254 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-03-06 14:31
Thanks, Mathieu! I went with a simpler approach to test the code.
msg261356 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-03-08 12:48
Much better indeed. Thanks.
History
Date User Action Args
2016-03-08 12:48:52deronnaxsetmessages: + msg261356
2016-03-06 14:31:42berker.peksagsetstatus: open -> closed

type: enhancement -> behavior

keywords: - needs review
nosy: + berker.peksag
messages: + msg261254
resolution: fixed
stage: patch review -> resolved
2016-03-06 14:27:18python-devsetmessages: + msg261253
2016-03-06 14:17:43python-devsetnosy: + python-dev
messages: + msg261252
2016-03-03 12:42:27deronnaxsetmessages: + msg261163
2016-02-28 11:50:25deronnaxsetfiles: + digest_md5sess_unittest.diff

messages: + msg260972
2016-02-28 10:27:32deronnaxsetmessages: + msg260971
2016-02-18 10:57:47martin.pantersetversions: + Python 2.7, Python 3.5, Python 3.6
nosy: + martin.panter

messages: + msg260448

stage: needs patch -> patch review
2016-02-18 08:02:25deronnaxsetfiles: + md5-sess_not_implem_cur_v2.diff

messages: + msg260438
2016-02-18 08:01:53deronnaxsetfiles: + md5-sess_not_implem_27_v2.diff

messages: + msg260437
2016-02-18 03:50:51deronnaxsetmessages: + msg260423
2014-12-31 16:19:29akuchlingsetnosy: - akuchling
2014-09-30 12:06:15deronnaxsetmessages: + msg227884
2014-09-25 14:57:54deronnaxsetfiles: + md5-sess_not_implem_27.diff

messages: + msg227541
2014-09-25 14:57:34deronnaxsetfiles: + md5-sess_not_implem_cur.diff

messages: + msg227540
versions: - Python 3.4
2014-06-13 12:50:37r.david.murraysetmessages: + msg220433
stage: patch review -> needs patch
2014-06-13 10:44:27deronnaxsetnosy: + deronnax
messages: + msg220429
2014-02-03 17:06:23BreamoreBoysetnosy: - BreamoreBoy
2013-07-06 23:11:37christian.heimessetversions: + Python 3.4, - Python 2.6, Python 3.1, Python 2.7, Python 3.2
nosy: + christian.heimes

messages: + msg192499

type: behavior -> enhancement
resolution: accepted -> (no value)
2013-04-18 05:08:29orsenthilunlinkissue4140 dependencies
2011-06-24 22:13:58r.david.murraysetmessages: + msg138995
2011-06-24 21:49:26cjw296setmessages: + msg138992
2011-06-23 15:46:40r.david.murraysetmessages: + msg138869
2011-06-23 13:34:49cjw296setmessages: + msg138866
2011-06-23 12:53:21r.david.murraysetmessages: + msg138865
2011-06-23 07:11:16cjw296setmessages: + msg138855
2011-06-23 02:11:07orsenthilsetmessages: + msg138849
2011-06-23 01:44:11r.david.murraysetnosy: + r.david.murray
messages: + msg138848
2011-06-23 01:27:18cjw296setkeywords: - easy
nosy: + cjw296
messages: + msg138847

2011-04-11 18:37:31santoso.wijayasetfiles: + issue2202_py27.patch
nosy: + santoso.wijaya
messages: + msg133540

2011-04-11 16:15:37kristjan.jonssonsetnosy: + kristjan.jonsson
messages: + msg133535
2010-07-11 05:20:39orsenthillinkissue4140 dependencies
2010-07-11 05:06:01orsenthilsetassignee: georg.brandl -> orsenthil
resolution: accepted
2010-07-10 15:04:26BreamoreBoysetversions: + Python 3.1, Python 2.7, Python 3.2
nosy: + BreamoreBoy

messages: + msg109874

stage: test needed -> patch review
2010-01-21 21:55:55brian.curtinsetkeywords: + needs review
2010-01-21 21:53:55dieresyssetfiles: + urllib2-support_md5-sess.diff

nosy: + dieresys
messages: + msg98115

keywords: + patch
2009-02-13 02:08:37ajaksu2setnosy: + jjlee
2009-02-12 17:37:27ajaksu2setnosy: + orsenthil
stage: test needed
versions: - Python 2.5
2008-03-20 04:28:00jafosetpriority: normal
assignee: georg.brandl
nosy: + georg.brandl
versions: + Python 2.6
2008-02-28 21:59:34akuchlingsetkeywords: + easy
nosy: + akuchling
messages: + msg63108
2008-02-28 18:27:48bwmcadamssettype: crash -> behavior
messages: + msg63102
2008-02-28 18:26:33bwmcadamscreate