Re: Git 2.23.0-rc0 HTTP authentication failure - error message change

On Wed, May 19, 2021 at 12:12:52AM +0000, brian m. carlson wrote:

quoted

But imagine we didn't get a username/password in the URL. The first
request will return REAUTH because of this moved code path (just as it
would have before, because http.auth.{username,password} are not set).
And then we'll get a credential from the user or from a helper and try
again. But this time, if we fail, we'll return HTTP_REAUTH again! We
never hit the "if (http_auth.username && http_auth.password)" check at
all. And hence we never return HTTP_NOAUTH (which gives us the more
useful "authentication failed" message), nor the credential_reject()
line (which informs helpers to stop caching a known-bad password).

I think what we'd want to do in this case is to only call HTTP_REAUTH if
we actually cleared CURLAUTH_GSSNEGOTIATE.  Maybe something like this:
[...]

Yeah, that was my instinct, too, but...

quoted

I suspect we could hack around it by pessimistically guessing that
GSSNEGOTIATE was the problem. But I'm worried that making that work
would require up to three requests (one to find out we need auth, one to
remove the GSSNEGOTIATE bit, and one to retry with a username/password).
That seems like punishing people with servers that don't even care about
Negotiate for no reason.

I think my proposal above does that, but I'm not sure.  If Negotiate
wasn't set, we won't need to make a third request, since we'll have
known the supported mechanisms as part of the original 401.  If they do
support both, then three requests will be required if they have to fall
back to Basic auth, but then they're only paying the price for the
environment they have.

If we aren't already reading the supported mechanisms out of the initial
401, then we'll need the third request, but that would be silly and we
should just avoid doing that.

Yeah, I was worried that just clearing the bit results in the extra
round-trip. I think we do clear bits based on what the other side showed
us. That's the:

  http_auth_methods &= results->auth_avail;

in the code being discussed. But it seems like we'd want to do that as
part of setting the "used negotiate" flag in your sample patch. I.e.,:

  if (http_auth_methods & results->auth_avail & CURLAUTH_GSSNEGOTIATE)
          used_negotiate = 1;

But it's entirely possible I don't understand the subtleties around
unsetting GSSNEGOTIATE in the first place (it's not something I've ever
used myself).

quoted

So perhaps somebody can come up with something clever, but I suspect we
may need to just revert this for the v2.32 release, and re-break the
case that 1b0d9545bb8 was trying to solve.

Yeah, I think this is the right solution for the problem until somebody
with a suitable mixed auth environment shows up and can test.  Your
patches seemed reasonable and, as always, well explained.

Thanks for taking a look!

-Peff