http: reauthenticate on 401 Unauthorized
RFC PATCH 1/1 http: reauthenticate on 401 Unauthorized
From: M Hickford <[email protected]>
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.
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.
---
[RFC] http: reauthenticate on 401 Unauthorized
cc. Jeff King [email protected]
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(-)
credential.c +1 -0
@@ -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" );
credential.h +3 -1
@@ -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;
http.c +27 -3
@@ -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; } int http_get_strbuf(const char *url,
t/t5551-http-fetch-smart.sh +2 -8
@@ -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
t/t5563-simple-http-auth.sh +3 -0
@@ -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
Signature · 1 line
gitgitgadget
Quoted 5 lines · M Hickford via GitGitGadget
From: M Hickford <[email protected]>
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?
Quoted 8 lines
To spare the user from retrying the command, in case of HTTP 401 Unauthorized, call
credential fillagain 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.
credential.h struct credential { · line 139
Quoted 12 lines
credential.h +3
-1@@ -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".
Quoted 7 lines
credential.h +0
-0char *username;char *password;http.c +0
-0
http.c static int handle_curl_result(struct slot_results *results) · line 1739
Quoted 9 lines
http.c +5
-1@@ -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"?
http.c static int http_request_reauth(const char *url, · line 2134
Quoted 10 lines
http.c +3
-0}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.
Quoted 10 lines · M Hickford via GitGitGadget
From: M Hickford <[email protected]>
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 fillagain 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..
Quoted 6 lines
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"
Quoted 95 lines
Signed-off-by M Hickford <[email protected]>---
[RFC] http: reauthenticate on 401 Unauthorizedcc. Jeff King [email protected]
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/1521credential.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(-)credential.c +1
-0@@ -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" );credential.h +3
-1@@ -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;http.c +20
-2@@ -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);
http.c static int http_request_reauth(const char *url, · line 2193
Quoted 10 lines
http.c +7
-1+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
Quoted 37 lines
http.c +0
-0int http_get_strbuf(const char *url,t/t5551-http-fetch-smart.sh +2
-8@@ -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@hostt/t5563-simple-http-auth.sh +3
-0@@ -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
Quoted 1 line
base-commit:
fe86abd7511a9a6862d5706c6fa1d9b57a63ba09
Thanks,
Matthew