[BUG?] google code http auth weirdness

[BUG?] google code http auth weirdness

[Jeff King]

I tried pushing to a repository at Google Code for the first time today,
and I encountered some weird behavior with respect to asking for
credentials.

If I use the url "https://code.google.com/r/repo/", everything works; I
get prompted for my username/password.

But if I instead use the url "https://myuser@code.google.com/r/repo/",
it doesn't work. I get:

  $ git push
  fatal: remote error: Invalid username/password.
  You may need to use your generated googlecode.com password; see
  https://code.google.com/hosting/settings

without any prompt at all. I bisected it to 986bbc0 (http: don't always
prompt for password, 2011-11-04), but I think that is a red herring. It
worked before that commit because we always asked for the password,
before we even talked to the server.

After that commit, we should be reacting to an HTTP 401. But it seems that
Google Code's 401 behavior is odd. When t5540 checks this, the
conversation goes something like:

  1. We do a GET with no "Authorization" header.

  2. The server returns a 401, asking for credentials.

  3. Curl repeats the GET request, putting the username into the
     Authorization header.

  4. The server returns a 401, again, as our credential is not
     sufficient.

  5. Curl returns the 401 to git; git prompts for the credential, feeds
     it to curl, and then repeats the request. It works.

But with Google Code, the first three steps are identical. But then for
step 4, the server returns this:

  < HTTP/1.1 200 OK
  < Content-Type: application/x-git-receive-pack-advertisement
  < X-Content-Type-Options: nosniff
  < Date: Fri, 15 Mar 2013 11:43:14 GMT
  < Server: git_frontend
  < Content-Length: 175
  < X-XSS-Protection: 1; mode=block
  < 
  * Connection #0 to host code.google.com left intact
  packet:          git< # service=git-receive-pack
  packet:          git< 0000
  packet:          git< ERR Invalid username/password [...]

That seems kind of crazy to me. It's generating an HTTP 200 just to tell
us the credentials are wrong. Which kind of makes sense; it's the only
way to convince the git client to show a custom message when it aborts
(rather than producing its own client-side "authorization failed"
message). But it takes the retry decision out of the client's hands. And
in this case, it is wrong; the failed credential is a result of curl
trying the username only, and git never even gets a chance to provide
the real credential.

Technically this did work before 986bbc0, so it could be considered a
regression in git, but I really think that Google Code is in the wrong
here. It should either:

  1. Always return a 401 for bad credentials. This means it would lose
     the custom message. But I think that is a good indication that the
     client should be putting more effort into showing the body of the
     401. Probably not all the time, as we do not want to spew HTML at
     the user, but we could perhaps relay error bodies if the
     content-type is "application/x-git-error" or something.

     The downside is that even if we make such a client change and the
     the Google Code server change sit's behavior, people on older git
     clients will lose the benefit of the message.

  2. Treat a credential with a non-empty username and an empty password
     specially, and return a 401. This covers the specific case of
     https://user@host/, but continues to show the custom message when
     the user provides a wrong password. It does mean that the custom
     message will not be shown if the user actually enters a blank
     password at the prompt (but it will still be shown if they get
     prompted for both username and password and leave both blank).

     So it's a little hacky, but I think it would work OK in practice.

What do you think?

-Peff

Re: [BUG?] google code http auth weirdness

[Shawn Pearce]

On Fri, Mar 15, 2013 at 4:59 AM, Jeff King <peff@peff.net> wrote:
> I tried pushing to a repository at Google Code for the first time today,
> and I encountered some weird behavior with respect to asking for
> credentials.
>
> If I use the url "https://code.google.com/r/repo/", everything works; I
> get prompted for my username/password.
>
> But if I instead use the url "https://myuser@code.google.com/r/repo/",
> it doesn't work. I get:
>
>   $ git push
>   fatal: remote error: Invalid username/password.
>   You may need to use your generated googlecode.com password; see
>   https://code.google.com/hosting/settings
>
> without any prompt at all. I bisected it to 986bbc0 (http: don't always
> prompt for password, 2011-11-04), but I think that is a red herring. It
> worked before that commit because we always asked for the password,
> before we even talked to the server.
>
> After that commit, we should be reacting to an HTTP 401. But it seems that
> Google Code's 401 behavior is odd. When t5540 checks this, the
> conversation goes something like:
>
>   1. We do a GET with no "Authorization" header.
>
>   2. The server returns a 401, asking for credentials.
>
>   3. Curl repeats the GET request, putting the username into the
>      Authorization header.
>
>   4. The server returns a 401, again, as our credential is not
>      sufficient.
>
>   5. Curl returns the 401 to git; git prompts for the credential, feeds
>      it to curl, and then repeats the request. It works.
>
> But with Google Code, the first three steps are identical. But then for
> step 4, the server returns this:
>
>   < HTTP/1.1 200 OK
>   < Content-Type: application/x-git-receive-pack-advertisement
>   < X-Content-Type-Options: nosniff
>   < Date: Fri, 15 Mar 2013 11:43:14 GMT
>   < Server: git_frontend
>   < Content-Length: 175
>   < X-XSS-Protection: 1; mode=block
>   <
>   * Connection #0 to host code.google.com left intact
>   packet:          git< # service=git-receive-pack
>   packet:          git< 0000
>   packet:          git< ERR Invalid username/password [...]
>
> That seems kind of crazy to me. It's generating an HTTP 200 just to tell
> us the credentials are wrong. Which kind of makes sense; it's the only
> way to convince the git client to show a custom message when it aborts
> (rather than producing its own client-side "authorization failed"
> message).

Actually the reason here wasn't to use a custom message. It was to
avoid the client from entering into the old /info/refs fallback path
that existed between 703e6e76 (Git 1.6.6.2) and 6ac964a62 (Git
1.7.12.3). During these versions non-200 responses from the smart
request usually led to a useless error in the client. I suggested to
the Google Code team when they implemented Git support to use a 200
response with the ERR header to give the end-user something easier to
understand than what the Git client have otherwise printed.

But in hindsight maybe I should have told them to always return 401
and let the client handle the error.

FWIW this same "misfeature" probably exists in Gerrit Code Review and
does exist on {gerrit,android}.googlesource.com because it also came
from me.

> But it takes the retry decision out of the client's hands. And
> in this case, it is wrong; the failed credential is a result of curl
> trying the username only, and git never even gets a chance to provide
> the real credential.
>
> Technically this did work before 986bbc0, so it could be considered a
> regression in git, but I really think that Google Code is in the wrong
> here. It should either:
>
>   1. Always return a 401 for bad credentials. This means it would lose
>      the custom message. But I think that is a good indication that the
>      client should be putting more effort into showing the body of the
>      401. Probably not all the time, as we do not want to spew HTML at
>      the user, but we could perhaps relay error bodies if the
>      content-type is "application/x-git-error" or something.
>
>      The downside is that even if we make such a client change and the
>      the Google Code server change sit's behavior, people on older git
>      clients will lose the benefit of the message.

I don't think this is the way to handle errors in the protocol. I
prefer the existing approach of sending a 200 OK with the ERR header
to indicate the message to show to the client. It works since 1.6.6
when smart HTTP was introduced, and it has a very specific meaning.
The 200 is stating the transport worked, and the ERR is stating the
Git operation did not. :-)

Where we have an issue is on a recoverable authentication error.
Recoverable authentication errors should use 401 so the client can try
again. I don't know how older (e.g. 1.6.6) clients behave here with a
401 response. I guess I should crack out a 1.6.6 build and test.

>   2. Treat a credential with a non-empty username and an empty password
>      specially, and return a 401. This covers the specific case of
>      https://user@host/, but continues to show the custom message when
>      the user provides a wrong password. It does mean that the custom
>      message will not be shown if the user actually enters a blank
>      password at the prompt (but it will still be shown if they get
>      prompted for both username and password and leave both blank).
>
>      So it's a little hacky, but I think it would work OK in practice.

I don't work on Google Code, but I have asked the team to consider
making at least this change.

We haven't deployed it yet, but I made the change for
{android,gerrit}.googlesource.com, and should try to get the same
thing into Gerrit Code Review.

Re: [BUG?] google code http auth weirdness

[Jeff King]

On Sat, Mar 16, 2013 at 06:11:32PM -0700, Shawn O. Pearce wrote:

> > That seems kind of crazy to me. It's generating an HTTP 200 just to tell
> > us the credentials are wrong. Which kind of makes sense; it's the only
> > way to convince the git client to show a custom message when it aborts
> > (rather than producing its own client-side "authorization failed"
> > message).
> 
> Actually the reason here wasn't to use a custom message. It was to
> avoid the client from entering into the old /info/refs fallback path
> that existed between 703e6e76 (Git 1.6.6.2) and 6ac964a62 (Git
> 1.7.12.3). During these versions non-200 responses from the smart
> request usually led to a useless error in the client. I suggested to
> the Google Code team when they implemented Git support to use a 200
> response with the ERR header to give the end-user something easier to
> understand than what the Git client have otherwise printed.

OK, that makes sense. I do wonder, though, if the loss of the custom
message will be bad for Google Code. The current message is:

  fatal: remote error: Invalid username/password.
  You may need to use your generated googlecode.com password; see
  https://code.google.com/hosting/settings

which is an important clue (this being the first time I had used Google
Code to authenticate, I was quite surprised that the password was
different from the web page's password, and that pointer helped me
figure it out more quickly).

> >   1. Always return a 401 for bad credentials. This means it would lose
> >      the custom message. But I think that is a good indication that the
> >      client should be putting more effort into showing the body of the
> >      401. Probably not all the time, as we do not want to spew HTML at
> >      the user, but we could perhaps relay error bodies if the
> >      content-type is "application/x-git-error" or something.
> >
> >      The downside is that even if we make such a client change and the
> >      the Google Code server change sit's behavior, people on older git
> >      clients will lose the benefit of the message.
> 
> I don't think this is the way to handle errors in the protocol. I
> prefer the existing approach of sending a 200 OK with the ERR header
> to indicate the message to show to the client. It works since 1.6.6
> when smart HTTP was introduced, and it has a very specific meaning.
> The 200 is stating the transport worked, and the ERR is stating the
> Git operation did not. :-)

Sorry, I should have been more clear. I mean only to use it for
transport errors, not git errors. So we should not in general be
converting 200 responses into something else (with the exception of the
401, because it does have a transport meaning). But when we _do_ issue
an unsuccessful HTTP code, we should include a message that can be
relayed to the user.

> Where we have an issue is on a recoverable authentication error.
> Recoverable authentication errors should use 401 so the client can try
> again. I don't know how older (e.g. 1.6.6) clients behave here with a
> 401 response. I guess I should crack out a 1.6.6 build and test.

Before 42653c0 (Prompt for a username when an HTTP request 401s,
2010-04-01), in v1.7.1.1, I think you just get something like:

  error: 401 Authorization failed
  fatal: HTTP request failed

> I don't work on Google Code, but I have asked the team to consider
> making at least this change.

Thanks for looking into it.

-Peff