| Message ID | pull.1521.git.git.1686474351611.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] http: reauthenticate on 401 Unauthorized | expand |
"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.
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
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; } 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