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
urllib2 fails against IIS 6.0 (No support for MD5-sess auth) #46455
Comments
URLLIB2 seems to have issues attempting to digest authenticate against a 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 This could be easily patched, but support for MD5-sess would be great as In my estimation, at the least, urllib2 shouldn't crap out in such an I'd suggest changing line 970 (The # XXX MD5-sess line) to: raise Exception, "Unsupported Digest Authentication Algorithm '%s'" % |
Sorry, this isn't a crash. It doesn't crash the interpreter. I'll |
Marking as easy, assuming you add the exception; implementing MD5-sess |
Here is a patch for supporting MD5-sess, following RFC2617 specification. Some comments/warnings:
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 ...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. |
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': New changeset 046c75c9cd33 by Berker Peksag in branch 'default': |
New changeset d135799e952b by Berker Peksag in branch '2.7': |
Thanks, Mathieu! I went with a simpler approach to test the code. |
Much better indeed. Thanks. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: