{"thread":{"id":"59849","subject":"[PATCH] [RFC] http: reauthenticate on 401 Unauthorized","startedAt":"2023-06-11T09:05:58Z","lastAt":"2023-06-12T23:12:28Z","messageCount":3,"participants":["M Hickford via GitGitGadget","Junio C Hamano","Matthew John Cheetham"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478221","messageId":"pull.1521.git.git.1686474351611.gitgitgadget@gmail.com","threadId":"59849","inReplyTo":null,"subject":"[PATCH] [RFC] http: reauthenticate on 401 Unauthorized","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-11T09:05:51Z","receivedAt":"2023-06-11T09:05:58Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford <mirth.hickford@gmail.com>\n\nA credential helper may return a bad credential if the user's password\nhas changed or a personal access token has expired. The user gets\nan HTTP 401 Unauthorized error. The user invariably retries the command.\n\nTo spare the user from retrying the command, in case of HTTP 401\nUnauthorized, call `credential fill` again and reauthenticate. This will\nsucceed if a helper generates a fresh credential or the user enters a\nvalid password.\n\nKeep current behaviour of asking user for username and password at\nmost once. Sanity check that second credential differs from first before\ntrying it.\n\nAlternatives considered: add a string 'source' field to credential\nstruct that records which helper (or getpass) populated credential.\n\nSigned-off-by: M Hickford <mirth.hickford@gmail.com>\n---\n    [RFC] http: reauthenticate on 401 Unauthorized\n    \n    cc. Jeff King peff@peff.net\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1521%2Fhickford%2Freauth-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1521/hickford/reauth-v1\nPull-Request: https://github.com/git/git/pull/1521\n\n credential.c                |  1 +\n credential.h                |  4 +++-\n http.c                      | 30 +++++++++++++++++++++++++++---\n t/t5551-http-fetch-smart.sh | 10 ++--------\n t/t5563-simple-http-auth.sh |  3 +++\n 5 files changed, 36 insertions(+), 12 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 023b59d5711..00fea51800c 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -379,6 +379,7 @@ void credential_fill(struct credential *c)\n \t\t\t    c->helpers.items[i].string);\n \t}\n \n+\tc->getpass = 1;\n \tcredential_getpass(c);\n \tif (!c->username && !c->password)\n \t\tdie(\"unable to get password from user\");\ndiff --git a/credential.h b/credential.h\nindex b8e2936d1dc..c176b05981a 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -134,7 +134,9 @@ struct credential {\n \t\t configured:1,\n \t\t quit:1,\n \t\t use_http_path:1,\n-\t\t username_from_proto:1;\n+\t\t username_from_proto:1,\n+\t\t /* Whether the user has been prompted for username or password. */\n+\t\t getpass:1;\n \n \tchar *username;\n \tchar *password;\ndiff --git a/http.c b/http.c\nindex bb58bb3e6a3..d2897c4d9d1 100644\n--- a/http.c\n+++ b/http.c\n@@ -1732,7 +1732,11 @@ static int handle_curl_result(struct slot_results *results)\n \telse if (results->http_code == 401) {\n \t\tif (http_auth.username && http_auth.password) {\n \t\t\tcredential_reject(&http_auth);\n-\t\t\treturn HTTP_NOAUTH;\n+\t\t\tif (http_auth.getpass) {\n+\t\t\t\t/* Previously prompted user, don't prompt again. */\n+\t\t\t\treturn HTTP_NOAUTH;\n+\t\t\t}\n+\t\t\treturn HTTP_REAUTH;\n \t\t} else {\n \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n \t\t\tif (results->auth_avail) {\n@@ -2125,6 +2129,9 @@ static int http_request_reauth(const char *url,\n \t\t\t       struct http_get_options *options)\n {\n \tint ret = http_request(url, result, target, options);\n+\tint reauth = 0;\n+\tchar* first_username;\n+\tchar* first_password;\n \n \tif (ret != HTTP_OK && ret != HTTP_REAUTH)\n \t\treturn ret;\n@@ -2140,6 +2147,9 @@ static int http_request_reauth(const char *url,\n \tif (ret != HTTP_REAUTH)\n \t\treturn ret;\n \n+\tcredential_fill(&http_auth);\n+\n+reauth:\n \t/*\n \t * The previous request may have put cruft into our output stream; we\n \t * should clear it out before making our next request.\n@@ -2163,9 +2173,23 @@ static int http_request_reauth(const char *url,\n \t\tBUG(\"Unknown http_request target\");\n \t}\n \n-\tcredential_fill(&http_auth);\n+\tfirst_username = xstrdup(http_auth.username);\n+\tfirst_password = xstrdup(http_auth.password);\n+\tret = http_request(url, result, target, options);\n+\tif (ret == HTTP_REAUTH && reauth++ == 0) {\n+\t\tcredential_fill(&http_auth);\n+\t\t/* Sanity check that second credential differs from first. */\n+\t\tif (strcmp(first_username, http_auth.username)\n+\t\t|| strcmp(first_password, http_auth.password)) {\n+\t\t\tfree(first_username);\n+\t\t\tfree(first_password);\n+\t\t\tgoto reauth;\n+\t\t}\n+\t}\n \n-\treturn http_request(url, result, target, options);\n+\tfree(first_username);\n+\tfree(first_password);\n+\treturn ret;\n }\n \n int http_get_strbuf(const char *url,\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 21b7767cbd3..be2e76133c1 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -589,14 +589,8 @@ test_expect_success 'http auth forgets bogus credentials' '\n \t\techo \"password=bogus\"\n \t} | git credential approve &&\n \n-\t# we expect this to use the bogus values and fail, never even\n-\t# prompting the user...\n-\tset_askpass user@host pass@host &&\n-\ttest_must_fail git ls-remote \"$HTTPD_URL/auth/smart/repo.git\" >/dev/null &&\n-\texpect_askpass none &&\n-\n-\t# ...but now we should have forgotten the bad value, causing\n-\t# us to prompt the user again.\n+\t# we expect this to use the bogus values and fail, forget the bad value,\n+\t# then reauthenticate, prompting the user\n \tset_askpass user@host pass@host &&\n \tgit ls-remote \"$HTTPD_URL/auth/smart/repo.git\" >/dev/null &&\n \texpect_askpass both user@host\ndiff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh\nindex ab8a721ccc7..1e7e426bc65 100755\n--- a/t/t5563-simple-http-auth.sh\n+++ b/t/t5563-simple-http-auth.sh\n@@ -111,6 +111,9 @@ test_expect_success 'access using basic auth invalid credentials' '\n \tprotocol=http\n \thost=$HTTPD_DEST\n \twwwauth[]=Basic realm=\"example.com\"\n+\tprotocol=http\n+\thost=$HTTPD_DEST\n+\twwwauth[]=Basic realm=\"example.com\"\n \tEOF\n \n \texpect_credential_query erase <<-EOF\n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n-- \ngitgitgadget\n"},{"id":"478312","messageId":"xmqqmt147362.fsf@gitster.g","threadId":"59849","inReplyTo":"pull.1521.git.git.1686474351611.gitgitgadget@gmail.com","subject":"Re: [PATCH] [RFC] http: reauthenticate on 401 Unauthorized","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-12T21:59:17Z","receivedAt":"2023-06-12T21:59:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"M Hickford via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: M Hickford <mirth.hickford@gmail.com>\n>\n> A credential helper may return a bad credential if the user's password\n> has changed or a personal access token has expired. The user gets\n> an HTTP 401 Unauthorized error. The user invariably retries the command.\n\n... and no matter how many times the user retries, the command will\nnever succeed?  Is that the problem the patch tries to solve?\n\n> To spare the user from retrying the command, in case of HTTP 401\n> Unauthorized, call `credential fill` again and reauthenticate. This will\n> succeed if a helper generates a fresh credential or the user enters a\n> valid password.\n>\n> Keep current behaviour of asking user for username and password at\n> most once. Sanity check that second credential differs from first before\n> trying it.\n\nSoon after changing the password is probably the time it is more\nlikely that you would mistype your password, than after you got used\nto typing it over and over again.  I can understand the wish to\navoid asking for correct password forever, but giving just one\nattempt feels a bit cruel for that reason.\n\n> diff --git a/credential.h b/credential.h\n> index b8e2936d1dc..c176b05981a 100644\n> --- a/credential.h\n> +++ b/credential.h\n> @@ -134,7 +134,9 @@ struct credential {\n>  \t\t configured:1,\n>  \t\t quit:1,\n>  \t\t use_http_path:1,\n> -\t\t username_from_proto:1;\n> +\t\t username_from_proto:1,\n> +\t\t /* Whether the user has been prompted for username or password. */\n> +\t\t getpass:1;\n\nMental note: the comment here says \"prompted\".\n\n>  \tchar *username;\n>  \tchar *password;\n> diff --git a/http.c b/http.c\n> index bb58bb3e6a3..d2897c4d9d1 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1732,7 +1732,11 @@ static int handle_curl_result(struct slot_results *results)\n>  \telse if (results->http_code == 401) {\n>  \t\tif (http_auth.username && http_auth.password) {\n>  \t\t\tcredential_reject(&http_auth);\n> -\t\t\treturn HTTP_NOAUTH;\n> +\t\t\tif (http_auth.getpass) {\n> +\t\t\t\t/* Previously prompted user, don't prompt again. */\n> +\t\t\t\treturn HTTP_NOAUTH;\n> +\t\t\t}\n> +\t\t\treturn HTTP_REAUTH;\n\nAnd here we also see \"prompted\" again.  Perhaps it will help make\nthe result easier to read if we renamed the new member from\n\"getpass\" to another phrase that contains \"prompt\"?\n\n>  \t\t} else {\n>  \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n>  \t\t\tif (results->auth_avail) {\n> @@ -2125,6 +2129,9 @@ static int http_request_reauth(const char *url,\n>  \t\t\t       struct http_get_options *options)\n>  {\n>  \tint ret = http_request(url, result, target, options);\n> +\tint reauth = 0;\n> +\tchar* first_username;\n> +\tchar* first_password;\n\nIn our codebase, asterisk sticks to the variable, not type.\n\nThanks.\n"},{"id":"478318","messageId":"AS2PR03MB981571E7A85446AC0E1CCE0CC054A@AS2PR03MB9815.eurprd03.prod.outlook.com","threadId":"59849","inReplyTo":"pull.1521.git.git.1686474351611.gitgitgadget@gmail.com","subject":"Re: [PATCH] [RFC] http: reauthenticate on 401 Unauthorized","fromName":"Matthew John Cheetham","fromEmail":"mjcheetham@outlook.com","sentAt":"2023-06-12T23:12:14Z","receivedAt":"2023-06-12T23:12:28Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"On 2023-06-11 02:05, M Hickford via GitGitGadget wrote:\n\n> From: M Hickford <mirth.hickford@gmail.com>\n> \n> A credential helper may return a bad credential if the user's password\n> has changed or a personal access token has expired. The user gets\n> an HTTP 401 Unauthorized error. The user invariably retries the command.\n> \n> To spare the user from retrying the command, in case of HTTP 401\n> Unauthorized, call `credential fill` again and reauthenticate. This will\n> succeed if a helper generates a fresh credential or the user enters a\n> valid password.\n\nAdding a retry mechanism is something I'd love to see! I've had some\nthoughts on this for a while, so apologies for a bit of a brain dump..\n\n> Keep current behaviour of asking user for username and password at\n> most once. Sanity check that second credential differs from first before\n> trying it.\n> \n> Alternatives considered: add a string 'source' field to credential\n> struct that records which helper (or getpass) populated credential.\n\nI think having some affinity/knowledge of which helper was called is very\nhelpful and important. In this RFC you only permit one retry attempt,\nwhich could be something that the helper itself may wish to specify\n(including zero retries).\n\nWhen I was working on the WWW-Authenticate series, one thing I'd played\nabout with was having helpers respond in the `get` response some hints\nor features that modify how Git would subsequently call the *same* helper\nwith `erase` or `store`, including retry.\n\nExample:\n\n  protocol=https\n  host=example.com\n  username=test\n  password=secret-value\n  can_retry=1\n\n'Storage'-only helpers would not be able to service a 2nd `get` request\nafter the first `get`, since the subsequent `erase` would delete the bad\ncredential, so always retrying for all helpers can also be wasteful in a \ndifferent way. Retries would definately be useful for credential-\ngenerating helpers however.\n\nTo be able to issue these retries, wouldn't it make sense for Git to\nconsult the same helper that responded in the case of multiple configured\nhelpers? Likewise if one helper responds early in the chain and returns a\nbad credential, do we want to call `erase` on all helpers in the chain\n(including ones that were never given a chance to respond)?\n\nOne more thought about adding a retry mechanism.. if a non-trivial helper\nhas a set of possibly valid credentials (say, for multiple accounts or\ncontexts) for the same host, today we'd have to ask the user to pick one\nto use. If we had the ability to retry we could use our best-guess and if\nthis was incorrect only then prompt the user to select an account/context.\nBUT.. since we'd get an erase in the middle (`get` -> `erase` -> `get`)\nhow would we know to ignore this request?\n\nAre we in the 1st or 2nd `get` call?\n\nFor this we'd need to allow helpers to somehow re-establish state between\n`get` and `erase/store` calls. We could achieve this multiple ways.\nOne option could be adding some unique request ID that Git provides to\nhelpers, to allow them to save state and 'connect-the-dots'.\nAnother would be to allow helpers to encode state that Git should echo\nback between calls. The latter would too require helper affinity however.\n\nIf Git learned such a retry feature, this should also be advertised to\nthe helpers in the first `get` call to they know the possible  change in\nbehaviour, or can opt-in to using the such mechanisms. Something like:\n\n  _supported_options=retry foo bar\n  protocol=https\n  host=example.com\n  wwwauth[]=Basic realm=\"example.com\"\n  wwwauth[]=Bearer authorize_uri=https://id.example.com/oauth scopes=\"a, b\"\n\n\n> Signed-off-by: M Hickford <mirth.hickford@gmail.com>\n> ---\n>     [RFC] http: reauthenticate on 401 Unauthorized\n>     \n>     cc. Jeff King peff@peff.net\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1521%2Fhickford%2Freauth-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1521/hickford/reauth-v1\n> Pull-Request: https://github.com/git/git/pull/1521\n> \n>  credential.c                |  1 +\n>  credential.h                |  4 +++-\n>  http.c                      | 30 +++++++++++++++++++++++++++---\n>  t/t5551-http-fetch-smart.sh | 10 ++--------\n>  t/t5563-simple-http-auth.sh |  3 +++\n>  5 files changed, 36 insertions(+), 12 deletions(-)\n> \n> diff --git a/credential.c b/credential.c\n> index 023b59d5711..00fea51800c 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -379,6 +379,7 @@ void credential_fill(struct credential *c)\n>  \t\t\t    c->helpers.items[i].string);\n>  \t}\n>  \n> +\tc->getpass = 1;\n>  \tcredential_getpass(c);\n>  \tif (!c->username && !c->password)\n>  \t\tdie(\"unable to get password from user\");\n> diff --git a/credential.h b/credential.h\n> index b8e2936d1dc..c176b05981a 100644\n> --- a/credential.h\n> +++ b/credential.h\n> @@ -134,7 +134,9 @@ struct credential {\n>  \t\t configured:1,\n>  \t\t quit:1,\n>  \t\t use_http_path:1,\n> -\t\t username_from_proto:1;\n> +\t\t username_from_proto:1,\n> +\t\t /* Whether the user has been prompted for username or password. */\n> +\t\t getpass:1;\n>  \n>  \tchar *username;\n>  \tchar *password;\n> diff --git a/http.c b/http.c\n> index bb58bb3e6a3..d2897c4d9d1 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1732,7 +1732,11 @@ static int handle_curl_result(struct slot_results *results)\n>  \telse if (results->http_code == 401) {\n>  \t\tif (http_auth.username && http_auth.password) {\n>  \t\t\tcredential_reject(&http_auth);\n> -\t\t\treturn HTTP_NOAUTH;\n> +\t\t\tif (http_auth.getpass) {\n> +\t\t\t\t/* Previously prompted user, don't prompt again. */\n> +\t\t\t\treturn HTTP_NOAUTH;\n> +\t\t\t}\n> +\t\t\treturn HTTP_REAUTH;\n>  \t\t} else {\n>  \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n>  \t\t\tif (results->auth_avail) {\n> @@ -2125,6 +2129,9 @@ static int http_request_reauth(const char *url,\n>  \t\t\t       struct http_get_options *options)\n>  {\n>  \tint ret = http_request(url, result, target, options);\n> +\tint reauth = 0;\n> +\tchar* first_username;\n> +\tchar* first_password;\n>  \n>  \tif (ret != HTTP_OK && ret != HTTP_REAUTH)\n>  \t\treturn ret;\n> @@ -2140,6 +2147,9 @@ static int http_request_reauth(const char *url,\n>  \tif (ret != HTTP_REAUTH)\n>  \t\treturn ret;\n>  \n> +\tcredential_fill(&http_auth);\n> +\n> +reauth:\n>  \t/*\n>  \t * The previous request may have put cruft into our output stream; we\n>  \t * should clear it out before making our next request.\n> @@ -2163,9 +2173,23 @@ static int http_request_reauth(const char *url,\n>  \t\tBUG(\"Unknown http_request target\");\n>  \t}\n>  \n> -\tcredential_fill(&http_auth);\n> +\tfirst_username = xstrdup(http_auth.username);\n> +\tfirst_password = xstrdup(http_auth.password);\n> +\tret = http_request(url, result, target, options);\n> +\tif (ret == HTTP_REAUTH && reauth++ == 0) {\n> +\t\tcredential_fill(&http_auth);\n> +\t\t/* Sanity check that second credential differs from first. */\n> +\t\tif (strcmp(first_username, http_auth.username)\n> +\t\t|| strcmp(first_password, http_auth.password)) {\n> +\t\t\tfree(first_username);\n> +\t\t\tfree(first_password);\n> +\t\t\tgoto reauth;\n> +\t\t}\n> +\t}\n>  \n> -\treturn http_request(url, result, target, options);\n> +\tfree(first_username);\n> +\tfree(first_password);\n> +\treturn ret;\n>  }\n\nDoes updating only `http_request_reauth` cover all our bases here?\nI know there are several other code paths that do not use the `http_get_*`\nfunctions but still invoke `credential_fill` and `run_slot` / `run_one_slot`\n(which then calls `handle_curl_result`).\n\nFor example:\n\n- the `imap-send` command\n- callers of `post_rpc`\n- callers of `http_init` when the `proactive_auth` arg is true\n\n>  int http_get_strbuf(const char *url,\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index 21b7767cbd3..be2e76133c1 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -589,14 +589,8 @@ test_expect_success 'http auth forgets bogus credentials' '\n>  \t\techo \"password=bogus\"\n>  \t} | git credential approve &&\n>  \n> -\t# we expect this to use the bogus values and fail, never even\n> -\t# prompting the user...\n> -\tset_askpass user@host pass@host &&\n> -\ttest_must_fail git ls-remote \"$HTTPD_URL/auth/smart/repo.git\" >/dev/null &&\n> -\texpect_askpass none &&\n> -\n> -\t# ...but now we should have forgotten the bad value, causing\n> -\t# us to prompt the user again.\n> +\t# we expect this to use the bogus values and fail, forget the bad value,\n> +\t# then reauthenticate, prompting the user\n>  \tset_askpass user@host pass@host &&\n>  \tgit ls-remote \"$HTTPD_URL/auth/smart/repo.git\" >/dev/null &&\n>  \texpect_askpass both user@host\n> diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh\n> index ab8a721ccc7..1e7e426bc65 100755\n> --- a/t/t5563-simple-http-auth.sh\n> +++ b/t/t5563-simple-http-auth.sh\n> @@ -111,6 +111,9 @@ test_expect_success 'access using basic auth invalid credentials' '\n>  \tprotocol=http\n>  \thost=$HTTPD_DEST\n>  \twwwauth[]=Basic realm=\"example.com\"\n> +\tprotocol=http\n> +\thost=$HTTPD_DEST\n> +\twwwauth[]=Basic realm=\"example.com\"\n>  \tEOF\n>  \n>  \texpect_credential_query erase <<-EOF\n> \n> base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n\nThanks,\nMatthew\n"}]}