Re: [PATCH] Fix https interactive authentication problem

[Jeff King <peff@peff.net>]

On Wed, Nov 23, 2011 at 07:58:02AM +0000, Steinmann  Ruedi wrote:

> Cloning a repository over https works fine when the username/password is
> given in the URL. But if it is queried interactively, an error occurs(see below).
> I found that the username/password is not set when a connection is reused.
> 
> With this patch, the username/password is set whenever a connection is reused.
> 
> Sample output showing the error:
> 
> git clone https://n.ethz.ch/student/...
> Cloning into ...
> Username:
> Password:
> error: Unable to get pack file https://n.ethz.ch/student/.../objects/pack/pack-1ced2ebff0c9fc1f07e0c7cc9dd3fc75f6ac6962.pack
> The requested URL returned error: 401
> ...
> error: Fetch failed.

Hmm. What version of git are you using?

We used to always set up the auth information in get_curl_handle, so all
handles had any auth information that we already got. But we removed
that recently in 986bbc0 (http: don't always prompt for password,
2011-11-04), which is in v1.7.8-rc1 and later.

I'm wondering if this was an unintended side effect of that patch.

Usually git will notice the 401 and retry with credentials (requesting
them from the user if necessary). That used to not work for all requests
but I fixed that in 8d677ed (http: retry authentication failures for all
http requests, 2011-07-18), which is also in v1.7.8-rc*.

However, it looks like the packfile-fetching code for dumb http does not
actually go through the usual http_request method, and does not
recognize the 401.

So if you are using v1.7.8-rc*, then I think that is the issue. And your
patch is just undoing what 8d677ed did. But the right fix is to refactor
the packfile-fetching function on top of the other http-fetching code,
so it can get the benefit of that logic.

If you are using an older version of git (pre-8d677ed), then one guess
about what is happening is a race condition with setting up multiple
curl handles:

  1. we get curl handle 1 from get_curl_handle, calling
     init_curl_http_auth. But we have no username in the url, so we
     assume no credentials are needed.

  2. we get curl handle 2 from get_curl_handle, as above.

  3. we make a request on handle one, get a 401, and ask for
     credentials.

  4. we make a request for a packfile on handle 2, but we never copied
     the auth from step (3) into the curl handle from step (2), and it
     fails.

(I haven't looked closely at our usage of curl_multi_*, so I am not 100%
sure that this can even happen. So consider it just a theory).

If this is the case, your fix works because it calls init_curl_http_auth
more often (i.e., before making any request, not just when we create the
handle). But that means it is also undoing what 8d677ed did; we will ask
the user for credentials before actually seeing a 401.

We could still fix this by teaching the packfile-fetching code to
respect 401s. However, it does incur an unnecessary round-trip (we
already know from the other request that credentials are required, but
we don't bother to use them).

So I think the logic you want instead is "if we already have
credentials, use them with this curl slot, but otherwise don't bother
the user". Which would mean splitting the prompting bit out of
init_curl_http_auth.

-Peff