Patchwork url: don't reuse bad credentials for basic/digest auth (issue3210)

login
register
mail settings
Submitter matt.zuba at gmail.com
Date Jan. 4, 2013, 4:25 p.m.
Message ID <93e59ec3adde4189a225.1357316721@matt-desktop>
Download mbox | patch
Permalink /patch/391/
State Changes Requested
Headers show

Comments

matt.zuba at gmail.com - Jan. 4, 2013, 4:25 p.m.
# HG changeset patch
# User Matt Zuba <matt.zuba at gmail.com>
# Date 1357316654 25200
# Node ID 93e59ec3adde4189a2257006796a6ea089c1c682
# Parent  2c1276825e938872ebc099c191eb202f0dbadfcc
url: don't reuse bad credentials for basic/digest auth (issue3210)

The urllib2 library will reuse existing credentials (whether they are good or bad)
on 401 HTTP responses for up to 5 times.  This can cause servers to lock out
users if they have a threshold for failed logins less than 5.  Instead of retrying
bad credentials, remove them from the password store and allow the user to enter
new ones.
Mads Kiilerich - Jan. 4, 2013, 5:04 p.m.
On 01/04/2013 05:25 PM, matt.zuba at gmail.com wrote:
> # HG changeset patch
> # User Matt Zuba <matt.zuba at gmail.com>
> # Date 1357316654 25200
> # Node ID 93e59ec3adde4189a2257006796a6ea089c1c682
> # Parent  2c1276825e938872ebc099c191eb202f0dbadfcc
> url: don't reuse bad credentials for basic/digest auth (issue3210)
>
> The urllib2 library will reuse existing credentials (whether they are good or bad)
> on 401 HTTP responses for up to 5 times.  This can cause servers to lock out
> users if they have a threshold for failed logins less than 5.  Instead of retrying
> bad credentials, remove them from the password store and allow the user to enter
> new ones.

What Python versions has this been tested with? As mentioned before, 
code in this area is quite tricky because it has to work with different 
Python versions which have different implementations and different bugs. 
Please verify that it works as expected on for instance Python 2.4.x, 
2.5.x, 2.6.1, 2.6.2, 2.6.x and 2.7.x. (IIRC it can be tested by fetching 
urllib2.py from the Python repo and storing it temporarily in mercurial/ ).

And please try to add some test coverage for this. userpass.py in 
test-http.t (and hg-textauth) should probably return 
401/HTTP_UNAUTHORIZED instead of 403/HTTP_FORBIDDEN and thus make it 
more easy to test this.

/Mads

Patch

diff --git a/mercurial/url.py b/mercurial/url.py
--- a/mercurial/url.py
+++ b/mercurial/url.py
@@ -57,6 +57,18 @@ 
         return urllib2.HTTPPasswordMgrWithDefaultRealm.find_user_password(
             self, None, authuri)
 
+    def remove_password(self, realm, uri):
+        # uri could be a single URI or a sequence
+        if isinstance(uri, basestring):
+            uri = [uri]
+        if not realm in self.passwd:
+            return
+        for default_port in True, False:
+            reduced_uri = tuple(
+                [self.reduce_uri(u, default_port) for u in uri])
+            if reduced_uri in self.passwd[realm]:
+                del self.passwd[realm][reduced_uri]
+
 class proxyhandler(urllib2.ProxyHandler):
     def __init__(self, ui):
         proxyurl = ui.config("http_proxy", "host") or os.getenv('http_proxy')
@@ -411,6 +423,18 @@ 
                 return
             raise
 
+    def retry_http_digest_auth(self, req, auth):
+        # Remove any username for this realm that might exist
+        # We use 1 here because urllib appears to do one request
+        # to see if Auth is even needed, then does a second request
+        # to actually do the authentication
+        if self.retried > 1:
+            token, challenge = auth.split(' ', 1)
+            chal = urllib2.parse_keqv_list(urllib2.parse_http_list(challenge))
+            self.passwd.remove_password(chal['realm'], req.get_full_url())
+        return urllib2.HTTPDigestAuthHandler.retry_http_digest_auth(
+                        self, req, auth)
+
 class httpbasicauthhandler(urllib2.HTTPBasicAuthHandler):
     def __init__(self, *args, **kwargs):
         urllib2.HTTPBasicAuthHandler.__init__(self, *args, **kwargs)
@@ -430,6 +454,16 @@ 
         return urllib2.HTTPBasicAuthHandler.http_error_auth_reqed(
                         self, auth_header, host, req, headers)
 
+    def retry_http_basic_auth(self, host, req, realm):
+        # Remove any username for this realm that might exist
+        # We use 1 here because urllib appears to do one request
+        # to see if Auth is even needed, then does a second request
+        # to actually do the authentication
+        if self.retried > 1:
+            self.passwd.remove_password(realm, host)
+        return urllib2.HTTPBasicAuthHandler.retry_http_basic_auth(
+                        self, host, req, realm)
+
 handlerfuncs = []
 
 def opener(ui, authinfo=None):