Patchwork [1,of,2] lfs: disable all authentication except Basic for HTTP(S) connections

login
register
mail settings
Submitter Matt Harbison
Date Feb. 7, 2019, 4:49 a.m.
Message ID <847900c2b99e6659cc50.1549514989@Envy>
Download mbox | patch
Permalink /patch/38516/
State Accepted
Headers show

Comments

Matt Harbison - Feb. 7, 2019, 4:49 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1549510249 18000
#      Wed Feb 06 22:30:49 2019 -0500
# Node ID 847900c2b99e6659cc50353ddab4f980905675c4
# Parent  349c8879becd13143db74894490299b56524d1e1
lfs: disable all authentication except Basic for HTTP(S) connections

I ran into a problem pushing to an old Apache server- the normal outgoing
traffic occurred, the Batch API request and response occurred, and then things
suddenly halted.  5 minutes later, a 500 was returned, and the server log had a
timeout reading 32K from `self._req.bodyfh` in hgweb.request.sendresponse().
Watching in WireShark, the Batch API got a 401, retried properly, then proceeded
to PUT the blob (without authentication headers).  This got a 401, but the
client never retried with authentication.  Worse, the blob was sent over the
wire in the failed attempt.

This kills digests for both the Batch API and the Transfer API.  While in theory
we could have the Batch API provide external URLs to a place that supports Basic
Authentication, the LFS spec actually calls out using Basic Authentication[1].
It's not clear to me if they've been able to shoehorn in other methods.  But
let's keep it simple until somebody needs it.

If we only had to support python2, we could just not add the handler for digest
authentication.  However in python3, AbstractBasicAuthHandler raises ValueError
if it sees a scheme other than Basic.  So we need to intercept all other schemes
before it gets to that point.

# no-check-commit because of urllib2.OpenerDirector foo_bar calling conventions

[1] https://github.com/git-lfs/git-lfs/blob/master/docs/api/authentication.md
Matt Harbison - Feb. 7, 2019, 5:02 a.m.
On Wed, 06 Feb 2019 23:49:49 -0500, Matt Harbison <mharbison72@gmail.com>  
wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1549510249 18000
> #      Wed Feb 06 22:30:49 2019 -0500
> # Node ID 847900c2b99e6659cc50353ddab4f980905675c4
> # Parent  349c8879becd13143db74894490299b56524d1e1
> lfs: disable all authentication except Basic for HTTP(S) connections
>

> diff --git a/hgext/lfs/blobstore.py b/hgext/lfs/blobstore.py
> --- a/hgext/lfs/blobstore.py
> +++ b/hgext/lfs/blobstore.py
> @@ -264,6 +264,24 @@ def _urlerrorreason(urlerror):
>      else:
>          return stringutil.forcebytestr(urlerror)
>  +class lfsauthhandler(util.urlreq.basehandler):
> +    handler_order = 480  # Before HTTPDigestAuthHandler (== 490)
> +
> +    def http_error_401(self, req, fp, code, msg, headers):
> +        """Enforces that any authentication performed is HTTP Basic
> +        Authentication.  No authentication is also acceptable.
> +        """
> +        authreq = headers.get(r'www-authenticate', None)
> +        if authreq:
> +            scheme = authreq.split()[0]
> +
> +            if scheme.lower() != r'basic':
> +                msg = _(b'the server must support Basic Authentication')
> +                raise util.urlerr.httperror(req.get_full_url(), code,
> +                                            encoding.strfromlocal(msg),  
> headers,
> +                                            fp)
> +        return None
> +

I noticed if `fp` is replaced with None, the following stacktrace is spit  
out.  I don't understand the forcebytestr() magic here, and it doesn't  
quite seem like a None check is missing.  I think I saw an instance of  
HttpError being thrown without this from urllib2.

+  Traceback (most recent call last):\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\hg", line 43, in <module>\r (esc)
+      dispatch.run()\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
99, in run\r (esc)
+      status = dispatch(req)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
225, in dispatch\r (esc)
+      ret = _runcatch(req) or 0\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
376, in _runcatch\r (esc)
+      return _callcatch(ui, _runcatchfunc)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
384, in _callcatch\r (esc)
+      return scmutil.callcatch(ui, func)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\scmutil.py", line  
165, in callcatch\r (esc)
+      return func()\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
367, in _runcatchfunc\r (esc)
+      return _dispatch(req)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
1021, in _dispatch\r (esc)
+      cmdpats, cmdoptions)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
756, in runcommand\r (esc)
+      ret = _runcommand(ui, options, cmd, d)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
1030, in _runcommand\r (esc)
+      return cmdfunc()\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\dispatch.py", line  
1018, in <lambda>\r (esc)
+      d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)\r  
(esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\util.py", line 1676,  
in check\r (esc)
+      return func(*args, **kwargs)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\commands.py", line  
4634, in push\r (esc)
+      opargs=opargs)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\hgext\\lfs\\wrapper.py", line  
340, in push\r (esc)
+      return orig(repo, remote, *args, **kwargs)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\exchange.py", line  
566, in push\r (esc)
+      _pushbundle2(pushop)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\exchange.py", line  
1138, in _pushbundle2\r (esc)
+      ret = partgen(pushop, bundler)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\exchange.py", line  
911, in _pushb2ctx\r (esc)
+      pushop.repo.prepushoutgoinghooks(pushop)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\util.py", line 3123,  
in __call__\r (esc)
+      results.append(hook(*args))\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\hgext\\lfs\\wrapper.py", line  
317, in prepush\r (esc)
+      return uploadblobsfromrevs(pushop.repo, pushop.outgoing.missing)\r  
(esc)
+    File "c:\\Users\\Matt\\projects\\hg\\hgext\\lfs\\wrapper.py", line  
308, in uploadblobsfromrevs\r (esc)
+      uploadblobs(repo, pointers)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\hgext\\lfs\\wrapper.py", line  
423, in uploadblobs\r (esc)
+      remoteblob.writebatch(pointers, repo.svfs.lfslocalblobstore)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\hgext\\lfs\\blobstore.py", line  
301, in writebatch\r (esc)
+      self._batch(_deduplicate(pointers), fromstore, b'upload')\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\hgext\\lfs\\blobstore.py", line  
494, in _batch\r (esc)
+      response = self._batchrequest(pointers, action)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\hgext\\lfs\\blobstore.py", line  
335, in _batchrequest\r (esc)
+      _(b'LFS HTTP error: %s') % stringutil.forcebytestr(ex),\r (esc)
+    File  
"c:\\Users\\Matt\\projects\\hg\\mercurial\\utils\\stringutil.py", line  
617, in forcebytestr\r (esc)
+      return pycompat.bytestr(obj)\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\pycompat.py", line  
199, in __new__\r (esc)
+      and not hasattr(s, u'__bytes__')):  # hasattr-py3-only\r (esc)
+    File "c:\\Users\\Matt\\projects\\hg\\mercurial\\pycompat.py", line  
272, in w\r (esc)
+      return f(object, sysstr(name), *args)\r (esc)
+    File "C:\\Program Files\\Python37\\Lib\\tempfile.py", line 475, in  
__getattr__\r (esc)
+      file = self.__dict__['file']\r (esc)
+  KeyError: 'file'\r (esc)
+  [1]
Yuya Nishihara - Feb. 7, 2019, 12:08 p.m.
On Wed, 06 Feb 2019 23:49:49 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1549510249 18000
> #      Wed Feb 06 22:30:49 2019 -0500
> # Node ID 847900c2b99e6659cc50353ddab4f980905675c4
> # Parent  349c8879becd13143db74894490299b56524d1e1
> lfs: disable all authentication except Basic for HTTP(S) connections

Queued, thanks.

Patch

diff --git a/hgext/lfs/blobstore.py b/hgext/lfs/blobstore.py
--- a/hgext/lfs/blobstore.py
+++ b/hgext/lfs/blobstore.py
@@ -264,6 +264,24 @@  def _urlerrorreason(urlerror):
     else:
         return stringutil.forcebytestr(urlerror)
 
+class lfsauthhandler(util.urlreq.basehandler):
+    handler_order = 480  # Before HTTPDigestAuthHandler (== 490)
+
+    def http_error_401(self, req, fp, code, msg, headers):
+        """Enforces that any authentication performed is HTTP Basic
+        Authentication.  No authentication is also acceptable.
+        """
+        authreq = headers.get(r'www-authenticate', None)
+        if authreq:
+            scheme = authreq.split()[0]
+
+            if scheme.lower() != r'basic':
+                msg = _(b'the server must support Basic Authentication')
+                raise util.urlerr.httperror(req.get_full_url(), code,
+                                            encoding.strfromlocal(msg), headers,
+                                            fp)
+        return None
+
 class _gitlfsremote(object):
 
     def __init__(self, repo, url):
@@ -275,6 +293,7 @@  class _gitlfsremote(object):
         if not useragent:
             useragent = b'git-lfs/2.3.4 (Mercurial %s)' % util.version()
         self.urlopener = urlmod.opener(ui, authinfo, useragent)
+        self.urlopener.add_handler(lfsauthhandler())
         self.retry = ui.configint(b'lfs', b'retry')
 
     def writebatch(self, pointers, fromstore):
diff --git a/tests/test-lfs-serve-access.t b/tests/test-lfs-serve-access.t
--- a/tests/test-lfs-serve-access.t
+++ b/tests/test-lfs-serve-access.t
@@ -421,6 +421,32 @@  the GET/PUT request.
 
   $ echo 'another blob' > auth_clone/lfs.blob
   $ hg -R auth_clone ci -Aqm 'add blob'
+
+  $ cat > use_digests.py << EOF
+  > from mercurial import (
+  >     exthelper,
+  >     url,
+  > )
+  > 
+  > eh = exthelper.exthelper()
+  > uisetup = eh.finaluisetup
+  > 
+  > @eh.wrapfunction(url, 'opener')
+  > def urlopener(orig, *args, **kwargs):
+  >     opener = orig(*args, **kwargs)
+  >     opener.addheaders.append((r'X-HgTest-AuthType', r'Digest'))
+  >     return opener
+  > EOF
+
+Test that Digest Auth fails gracefully before testing the successful Basic Auth
+
+  $ hg -R auth_clone push --config extensions.x=use_digests.py
+  pushing to http://localhost:$HGPORT1/
+  searching for changes
+  abort: LFS HTTP error: HTTP Error 401: the server must support Basic Authentication!
+  (api=http://localhost:$HGPORT1/.git/info/lfs/objects/batch, action=upload)
+  [255]
+
   $ hg -R auth_clone --debug push | egrep '^[{}]|  '
   {
     "objects": [
@@ -452,6 +478,19 @@  the GET/PUT request.
   $LOCALIP - - [$LOGDATE$] "POST /.git/info/lfs/objects/batch HTTP/1.1" 401 - (glob)
   $LOCALIP - - [$LOGDATE$] "POST /.git/info/lfs/objects/batch HTTP/1.1" 200 - (glob)
   $LOCALIP - - [$LOGDATE$] "GET /.hg/lfs/objects/276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d HTTP/1.1" 200 - (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=capabilities HTTP/1.1" 401 - x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=capabilities HTTP/1.1" 200 - x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=batch HTTP/1.1" 401 - x-hgarg-1:cmds=heads+%3Bknown+nodes%3D525251863cad618e55d483555f3d00a2ca99597e+4d9397055dc0c205f3132f331f36353ab1a525a3 x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=batch HTTP/1.1" 200 - x-hgarg-1:cmds=heads+%3Bknown+nodes%3D525251863cad618e55d483555f3d00a2ca99597e+4d9397055dc0c205f3132f331f36353ab1a525a3 x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=phases x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=phases x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=branchmap HTTP/1.1" 401 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=branchmap HTTP/1.1" 200 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull x-hgtest-authtype:Digest (glob)
+  $LOCALIP - - [$LOGDATE$] "POST /.git/info/lfs/objects/batch HTTP/1.1" 401 - x-hgtest-authtype:Digest (glob)
   $LOCALIP - - [$LOGDATE$] "GET /?cmd=capabilities HTTP/1.1" 401 - (glob)
   $LOCALIP - - [$LOGDATE$] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
   $LOCALIP - - [$LOGDATE$] "GET /?cmd=batch HTTP/1.1" 200 - x-hgarg-1:cmds=heads+%3Bknown+nodes%3D525251863cad618e55d483555f3d00a2ca99597e+4d9397055dc0c205f3132f331f36353ab1a525a3 x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull (glob)