[RFC] http: reauthenticate on 401 Unauthorized

[hickford]

cc. Jeff King peff@peff.net

cc: Matthew John Cheetham mjcheetham@outlook.com

[gitgitgadget-git]

There are issues in commit 982b6f2:
[RFC] http: reauthenticate on 401 Unauthorized
Commit not signed off

[hickford]

Failing tests in

t5551-http-fetch-smart
t5559-http-fetch-smart-http2
t5563-simple-http-auth

[hickford]

/submit

[gitgitgadget-git]

Submitted as pull.1521.git.git.1686474351611.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1521/hickford/reauth-v1

To fetch this version to local tag pr-git-1521/hickford/reauth-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1521/hickford/reauth-v1

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: M Hickford <mirth.hickford@gmail.com>
>
> A credential helper may return a bad credential if the user's password
> has changed or a personal access token has expired. The user gets
> an HTTP 401 Unauthorized error. The user invariably retries the command.

... and no matter how many times the user retries, the command will
never succeed?  Is that the problem the patch tries to solve?

> To spare the user from retrying the command, in case of HTTP 401
> Unauthorized, call `credential fill` again and reauthenticate. This will
> succeed if a helper generates a fresh credential or the user enters a
> valid password.
>
> Keep current behaviour of asking user for username and password at
> most once. Sanity check that second credential differs from first before
> trying it.

Soon after changing the password is probably the time it is more
likely that you would mistype your password, than after you got used
to typing it over and over again.  I can understand the wish to
avoid asking for correct password forever, but giving just one
attempt feels a bit cruel for that reason.

> diff --git a/credential.h b/credential.h
> index b8e2936d1dc..c176b05981a 100644
> --- a/credential.h
> +++ b/credential.h
> @@ -134,7 +134,9 @@ struct credential {
>  		 configured:1,
>  		 quit:1,
>  		 use_http_path:1,
> -		 username_from_proto:1;
> +		 username_from_proto:1,
> +		 /* Whether the user has been prompted for username or password. */
> +		 getpass:1;

Mental note: the comment here says "prompted".

>  	char *username;
>  	char *password;
> diff --git a/http.c b/http.c
> index bb58bb3e6a3..d2897c4d9d1 100644
> --- a/http.c
> +++ b/http.c
> @@ -1732,7 +1732,11 @@ static int handle_curl_result(struct slot_results *results)
>  	else if (results->http_code == 401) {
>  		if (http_auth.username && http_auth.password) {
>  			credential_reject(&http_auth);
> -			return HTTP_NOAUTH;
> +			if (http_auth.getpass) {
> +				/* Previously prompted user, don't prompt again. */
> +				return HTTP_NOAUTH;
> +			}
> +			return HTTP_REAUTH;

And here we also see "prompted" again.  Perhaps it will help make
the result easier to read if we renamed the new member from
"getpass" to another phrase that contains "prompt"?

>  		} else {
>  			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
>  			if (results->auth_avail) {
> @@ -2125,6 +2129,9 @@ static int http_request_reauth(const char *url,
>  			       struct http_get_options *options)
>  {
>  	int ret = http_request(url, result, target, options);
> +	int reauth = 0;
> +	char* first_username;
> +	char* first_password;

In our codebase, asterisk sticks to the variable, not type.

Thanks.

[gitgitgadget-git]

On the Git mailing list, Matthew John Cheetham wrote (reply to this):

On 2023-06-11 02:05, M Hickford via GitGitGadget wrote:

> From: M Hickford <mirth.hickford@gmail.com>
> 
> A credential helper may return a bad credential if the user's password
> has changed or a personal access token has expired. The user gets
> an HTTP 401 Unauthorized error. The user invariably retries the command.
> 
> To spare the user from retrying the command, in case of HTTP 401
> Unauthorized, call `credential fill` again and reauthenticate. This will
> succeed if a helper generates a fresh credential or the user enters a
> valid password.

Adding a retry mechanism is something I'd love to see! I've had some
thoughts on this for a while, so apologies for a bit of a brain dump..

> Keep current behaviour of asking user for username and password at
> most once. Sanity check that second credential differs from first before
> trying it.
> 
> Alternatives considered: add a string 'source' field to credential
> struct that records which helper (or getpass) populated credential.

I think having some affinity/knowledge of which helper was called is very
helpful and important. In this RFC you only permit one retry attempt,
which could be something that the helper itself may wish to specify
(including zero retries).

When I was working on the WWW-Authenticate series, one thing I'd played
about with was having helpers respond in the `get` response some hints
or features that modify how Git would subsequently call the *same* helper
with `erase` or `store`, including retry.

Example:

  protocol=https
  host=example.com
  username=test
  password=secret-value
  can_retry=1

'Storage'-only helpers would not be able to service a 2nd `get` request
after the first `get`, since the subsequent `erase` would delete the bad
credential, so always retrying for all helpers can also be wasteful in a 
different way. Retries would definately be useful for credential-
generating helpers however.

To be able to issue these retries, wouldn't it make sense for Git to
consult the same helper that responded in the case of multiple configured
helpers? Likewise if one helper responds early in the chain and returns a
bad credential, do we want to call `erase` on all helpers in the chain
(including ones that were never given a chance to respond)?

One more thought about adding a retry mechanism.. if a non-trivial helper
has a set of possibly valid credentials (say, for multiple accounts or
contexts) for the same host, today we'd have to ask the user to pick one
to use. If we had the ability to retry we could use our best-guess and if
this was incorrect only then prompt the user to select an account/context.
BUT.. since we'd get an erase in the middle (`get` -> `erase` -> `get`)
how would we know to ignore this request?

Are we in the 1st or 2nd `get` call?

For this we'd need to allow helpers to somehow re-establish state between
`get` and `erase/store` calls. We could achieve this multiple ways.
One option could be adding some unique request ID that Git provides to
helpers, to allow them to save state and 'connect-the-dots'.
Another would be to allow helpers to encode state that Git should echo
back between calls. The latter would too require helper affinity however.

If Git learned such a retry feature, this should also be advertised to
the helpers in the first `get` call to they know the possible  change in
behaviour, or can opt-in to using the such mechanisms. Something like:

  _supported_options=retry foo bar
  protocol=https
  host=example.com
  wwwauth[]=Basic realm="example.com"
  wwwauth[]=Bearer authorize_uri=https://id.example.com/oauth scopes="a, b"


> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
>     [RFC] http: reauthenticate on 401 Unauthorized
>     
>     cc. Jeff King peff@peff.net
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1521%2Fhickford%2Freauth-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1521/hickford/reauth-v1
> Pull-Request: https://github.com/git/git/pull/1521
> 
>  credential.c                |  1 +
>  credential.h                |  4 +++-
>  http.c                      | 30 +++++++++++++++++++++++++++---
>  t/t5551-http-fetch-smart.sh | 10 ++--------
>  t/t5563-simple-http-auth.sh |  3 +++
>  5 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/credential.c b/credential.c
> index 023b59d5711..00fea51800c 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -379,6 +379,7 @@ void credential_fill(struct credential *c)
>  			    c->helpers.items[i].string);
>  	}
>  
> +	c->getpass = 1;
>  	credential_getpass(c);
>  	if (!c->username && !c->password)
>  		die("unable to get password from user");
> diff --git a/credential.h b/credential.h
> index b8e2936d1dc..c176b05981a 100644
> --- a/credential.h
> +++ b/credential.h
> @@ -134,7 +134,9 @@ struct credential {
>  		 configured:1,
>  		 quit:1,
>  		 use_http_path:1,
> -		 username_from_proto:1;
> +		 username_from_proto:1,
> +		 /* Whether the user has been prompted for username or password. */
> +		 getpass:1;
>  
>  	char *username;
>  	char *password;
> diff --git a/http.c b/http.c
> index bb58bb3e6a3..d2897c4d9d1 100644
> --- a/http.c
> +++ b/http.c
> @@ -1732,7 +1732,11 @@ static int handle_curl_result(struct slot_results *results)
>  	else if (results->http_code == 401) {
>  		if (http_auth.username && http_auth.password) {
>  			credential_reject(&http_auth);
> -			return HTTP_NOAUTH;
> +			if (http_auth.getpass) {
> +				/* Previously prompted user, don't prompt again. */
> +				return HTTP_NOAUTH;
> +			}
> +			return HTTP_REAUTH;
>  		} else {
>  			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
>  			if (results->auth_avail) {
> @@ -2125,6 +2129,9 @@ static int http_request_reauth(const char *url,
>  			       struct http_get_options *options)
>  {
>  	int ret = http_request(url, result, target, options);
> +	int reauth = 0;
> +	char* first_username;
> +	char* first_password;
>  
>  	if (ret != HTTP_OK && ret != HTTP_REAUTH)
>  		return ret;
> @@ -2140,6 +2147,9 @@ static int http_request_reauth(const char *url,
>  	if (ret != HTTP_REAUTH)
>  		return ret;
>  
> +	credential_fill(&http_auth);
> +
> +reauth:
>  	/*
>  	 * The previous request may have put cruft into our output stream; we
>  	 * should clear it out before making our next request.
> @@ -2163,9 +2173,23 @@ static int http_request_reauth(const char *url,
>  		BUG("Unknown http_request target");
>  	}
>  
> -	credential_fill(&http_auth);
> +	first_username = xstrdup(http_auth.username);
> +	first_password = xstrdup(http_auth.password);
> +	ret = http_request(url, result, target, options);
> +	if (ret == HTTP_REAUTH && reauth++ == 0) {
> +		credential_fill(&http_auth);
> +		/* Sanity check that second credential differs from first. */
> +		if (strcmp(first_username, http_auth.username)
> +		|| strcmp(first_password, http_auth.password)) {
> +			free(first_username);
> +			free(first_password);
> +			goto reauth;
> +		}
> +	}
>  
> -	return http_request(url, result, target, options);
> +	free(first_username);
> +	free(first_password);
> +	return ret;
>  }

Does updating only `http_request_reauth` cover all our bases here?
I know there are several other code paths that do not use the `http_get_*`
functions but still invoke `credential_fill` and `run_slot` / `run_one_slot`
(which then calls `handle_curl_result`).

For example:

- the `imap-send` command
- callers of `post_rpc`
- callers of `http_init` when the `proactive_auth` arg is true

>  int http_get_strbuf(const char *url,
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 21b7767cbd3..be2e76133c1 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -589,14 +589,8 @@ test_expect_success 'http auth forgets bogus credentials' '
>  		echo "password=bogus"
>  	} | git credential approve &&
>  
> -	# we expect this to use the bogus values and fail, never even
> -	# prompting the user...
> -	set_askpass user@host pass@host &&
> -	test_must_fail git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null &&
> -	expect_askpass none &&
> -
> -	# ...but now we should have forgotten the bad value, causing
> -	# us to prompt the user again.
> +	# we expect this to use the bogus values and fail, forget the bad value,
> +	# then reauthenticate, prompting the user
>  	set_askpass user@host pass@host &&
>  	git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null &&
>  	expect_askpass both user@host
> diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh
> index ab8a721ccc7..1e7e426bc65 100755
> --- a/t/t5563-simple-http-auth.sh
> +++ b/t/t5563-simple-http-auth.sh
> @@ -111,6 +111,9 @@ test_expect_success 'access using basic auth invalid credentials' '
>  	protocol=http
>  	host=$HTTPD_DEST
>  	wwwauth[]=Basic realm="example.com"
> +	protocol=http
> +	host=$HTTPD_DEST
> +	wwwauth[]=Basic realm="example.com"
>  	EOF
>  
>  	expect_credential_query erase <<-EOF
> 
> base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09

Thanks,
Matthew

[gitgitgadget-git]

User Matthew John Cheetham <mjcheetham@outlook.com> has been added to the cc: list.