This is terrifying. @ethomson?

Hmm, that's very interesting. So yes, you're very much right that we should be re-posting the form data if we received a 401 on the original POST. This was sort of sneaky because we assume that we would never be in a situation where this mattered: the assumption is that a GET of /info/refs (which is the first step in any push/fetch) would authenticate you and then you could prompty POST to the upload pack / receive pack handlers on the same session without requiring re-authentication.

If I had to guess: Apache is tearing down the connection between the GET and the POST for some reason. I don't really know why it would do that but it's not really wrong to do that, per se. Just annoying (and costly for any session-based authentication mechanism like NTLM or Negotiate).

I'd love to see a fiddler capture, if you don't mind. It's possible that we're not sending keep-alive headers correctly for some reason, or that we're tearing down the connection... (Less likely is that it's not treating a kept-alive session as authenticated, which would be a bug in their implementation.)

In any case, this is a very valid bug that I regret. I think that the really proper support would be to send a POST with Expect: continue and then do our authentication dance until we get a 100 and then post the data.

This should be the standards-compliant way to accomplish this same thing you're doing, but I'm not sure how to coerce winhttp into doing this offhand. (I'm actually pretty shocked that winhttp didn't do this for us.)

I took a Fiddler trace and stripped out the hostnames and tickets and things. Should I just paste it here as a comment?

I get Connection: Keep-Alive on every request and response, so I don’t think Apache is closing the connection. I did have that misconfigured on the server at one point, which actually caused the initial GET to go into an infinite loop because it wouldn’t send the credentials on replay.

It does seem like mod_auth_kerb treats the request as authenticated instead of the connection. I haven’t found any hard documentation, but I’ve found a few references that make me think that's expected:

Generally IIS only needs 1 Spnego token per connection, while mod_auth_kerb in apache wants one per request

at http://kerberos.996246.n3.nabble.com/resend-spnego-token-td21160.html, and

It's clear that the expected authentication persistence behavior hasn't been documented in RFC 4559. Microsoft has implemented per-connection authentication in their products, whereas Tomcat and mod_auth_kerb have per-request authentication.

at http://code.google.com/p/serf/issues/detail?id=89.

Web browsers seem to send the Authorization header on subsequent requests to avoid the round-trip, but I couldn’t find any way to make WinHttp do that.

I can start looking into the Expect: continue approach you suggest, but I’m new to both WinHttp and Kerberos and very very rusty at C, so I’d appreciate any help you can provide.

Thanks!

Should I just paste it here as a comment?

That works. You can also create a Gist if you feel the paste is too large. https://gist.github.com/

We deal with this in git-core, too. Ideally you would send an Expect line, but so many servers are flaky with them that we do not by default. Instead, we handle re-POSTing ourselves. When the response is too large to fit in our buffer (1MB by default), we POST a "probe" packet consisting of a single pkt-line flush, to cheaply deal with any authentication or rejection issues. I don't see any similar code in libgit2 (but I might just be missing it; your http implementation is quite different from ours).

The relevant code in git-core is in post_rpc.

Also see git/git@c80d96c for Expect handling.

Boom, thanks @peff.

@chescock Let's do what git.git is doing then, which is just a refinement of your existing implementation, I think. I hope I didn't lead you too far astray with the expect/continue suggestion.

Happy to provide any help you need.

If I'm reading the code correctly, the only change I need to make to match the behavior of probe_rpc is to replace the empty HTTP request body with “0000”, which I assume makes it a valid empty git request. Is that right?

post_rpc calls probe_rpc in a loop until it succeeds, but if I try something equivalent with WinHttp it forgets the authentication after the successful empty request and the actual request fails again.

The other difference is that I'm currently doing this for all requests instead of just large ones. I think WinHttp will resend the request body automatically if we pass the data in WinHttpSendRequest instead of calling WinHttpWriteData afterwards. That could work for small requests that use winhttp_stream_write_single, and would remove the need for the if statement I added to winhttp_action. The buffer needs to be available until WinHttpReceiveResponse is called and the one there now isn’t, so I would need to make a copy. Should I make that change as well?

@ethomson Does this change look reasonable with the updates? Is there anything else you think I should fix?

Sorry! I have been slammed with other things this week.

So the WinHTTP transport layer is not the only place where this code makes sense. Now that we have stateful authentication in the regular HTTP transport, we'll want to do the same sort of dance there.

I wonder if it makes sense (or is possible) to move this up a level so that both transport layers can benefit from this logic for dealing with deficient servers.

It should definitely be possible to do this in the smart protocol layer. We already need to perform special handling if the transport is stateless/rpc in some places, so we should be able to send a flush packet before we start the push proper to make sure the authentication happens early.

Sorry for the delay in responding! I was out on vacation last week and I’m still catching up.

I’ll start looking into how I could implement this in the higher layer. I’m a little more nervous about making a larger change, though, since I’m new to the codebase and extremely rusty at C.

I’m not entirely convinced that this is actually a general concern as opposed to just handling quirks in the WinHTTP API. In the upload-pack case we can just give WinHTTP the entire request body and it will resend it as needed, but other transports might need a flush packet there as well. Most web browsers appear to resend the Authorization header on subsequent requests so that they don’t get 401 responses every time, and a transport that did that wouldn’t need to send the flush packet first.

Seems this effort got stuck a little bit? It would be great to have this solved in the next version that will be published, as the Kerberos authentication simply doesn't work with Apache and mod_auth_kerb.

I assume there mp workaround that can be applied on Apache configuration to get this working?

Closed with #5286