{"thread":{"id":"48642","subject":"[RFC PATCH v1] http: add http.keepRejectedCredentials config","startedAt":"2018-06-04T12:27:01Z","lastAt":"2018-06-08T05:47:50Z","messageCount":6,"participants":["lars.schneider@autodesk.com","Jeff King","Martin-Louis Bright","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"349234","messageId":"20180604122635.95342-1-lars.schneider@autodesk.com","threadId":"48642","inReplyTo":null,"subject":"[RFC PATCH v1] http: add http.keepRejectedCredentials config","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-06-04T12:26:35Z","receivedAt":"2018-06-04T12:27:01Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nIf a Git HTTP server responds with 401 or 407, then Git tells the\ncredential helper to reject and delete the credentials. In general\nthis is good.\n\nHowever, in certain automation environments it is not desired to remove\ncredentials automatically. This is in particular the case if credentials\nare only invalid temporarily (e.g. because of problems in the server's\nauthentication backend).\n\nTherefore, add the config \"http.keepRejectedCredentials\" which tells\nGit to keep invalid credentials if set to \"true\".\n\nIt was considered to disable the credential deletion in credential.c\ndirectly. This approach was not chosen as it could be confusing to\nother callers of credential_reject() if the function does not do what\nits name says (e.g. in imap-send.c).\n\nThe Git-Credential-Manager-for-Windows already implements a similar\nmechanism [1]. This solution aims to enable that feature for all\ncredential helper implementations.\n\n[1] https://github.com/Microsoft/Git-Credential-Manager-for-Windows/blob/0c1af463b33b0a0142f36f99c49ca8f83e86ee43/Shared/Cli/Functions/Common.cs#L484-L504\n\nSigned-off-by: Lars Schneider \n---\n\nNotes:\n Base Ref: master\n Web-Diff: https://github.com/larsxschneider/git/commit/51993c2ff9\n Checkout: git fetch https://github.com/larsxschneider/git keepcreds-v1 && git checkout 51993c2ff9\n\n Documentation/config.txt | 6 ++++++\n http.c | 12 ++++++++++--\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ab641bf5a9..184aee8dbc 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1997,6 +1997,12 @@ http.emptyAuth::\n \ta username in the URL, as libcurl normally requires a username for\n \tauthentication.\n\n+http.keepRejectedCredentials::\n+\tKeep credentials in the credential helper that a Git server responded\n+\tto with 401 (unauthorized) or 407 (proxy authentication required).\n+\tThis can be useful in automation environments where credentials might\n+\tbecome temporarily invalid. The default is `false`.\n+\n http.delegation::\n \tControl GSSAPI credential delegation. The delegation is disabled\n \tby default in libcurl since version 7.21.7. Set parameter to tell\ndiff --git a/http.c b/http.c\nindex b4bfbceaeb..ff6932813f 100644\n--- a/http.c\n+++ b/http.c\n@@ -138,6 +138,7 @@ static int ssl_cert_password_required;\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n static unsigned long http_auth_methods = CURLAUTH_ANY;\n static int http_auth_methods_restricted;\n+static int keep_rejected_credentials = 0;\n /* Modes for which empty_auth cannot actually help us. */\n static unsigned long empty_auth_useless =\n \tCURLAUTH_BASIC\n@@ -403,6 +404,11 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n\n+\tif (!strcmp(\"http.keeprejectedcredentials\", var)) {\n+\t\tkeep_rejected_credentials = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \t/* Fall back on the default ones */\n \treturn git_default_config(var, value, cb);\n }\n@@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results *results)\n \t\treturn HTTP_MISSING_TARGET;\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\tif (!keep_rejected_credentials)\n+\t\t\t\tcredential_reject(&http_auth);\n \t\t\treturn HTTP_NOAUTH;\n \t\t} else {\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n@@ -1485,7 +1492,8 @@ static int handle_curl_result(struct slot_results *results)\n \t\t}\n \t} else {\n \t\tif (results->http_connectcode == 407)\n-\t\t\tcredential_reject(&proxy_auth);\n+\t\t\tif (!keep_rejected_credentials)\n+\t\t\t\tcredential_reject(&proxy_auth);\n #if LIBCURL_VERSION_NUM >= 0x070c00\n \t\tif (!curl_errorstr[0])\n \t\t\tstrlcpy(curl_errorstr,\n\nbase-commit: c2c7d17b030646b40e6764ba34a5ebf66aee77af\n--\n2.17.1\n\n"},{"id":"349252","messageId":"20180604144747.GA27655@sigill.intra.peff.net","threadId":"48642","inReplyTo":"20180604122635.95342-1-lars.schneider@autodesk.com","subject":"Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-04T14:47:47Z","receivedAt":"2018-06-04T14:47:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 04, 2018 at 05:26:35AM -0700, lars.schneider@autodesk.com wrote:\n\n> From: Lars Schneider \n> \n> If a Git HTTP server responds with 401 or 407, then Git tells the\n> credential helper to reject and delete the credentials. In general\n> this is good.\n> \n> However, in certain automation environments it is not desired to remove\n> credentials automatically. This is in particular the case if credentials\n> are only invalid temporarily (e.g. because of problems in the server's\n> authentication backend).\n> \n> Therefore, add the config \"http.keepRejectedCredentials\" which tells\n> Git to keep invalid credentials if set to \"true\".\n\nIt seems like those servers should be returning a value besides \"401\" if\nit's a temporary error.\n\nBut alas, we live in the real world, and your patch seems like a pretty\nsensible workaround for clients. This could be done at the helper layer,\nbut I think in practice doing it here is going to be a lot more\nconvenient (and doesn't preclude helpers having their own logic if\npeople care to extend them in that direction).\n\n> It was considered to disable the credential deletion in credential.c\n> directly. This approach was not chosen as it could be confusing to\n> other callers of credential_reject() if the function does not do what\n> its name says (e.g. in imap-send.c).\n\nYeah, I think \"git credential\" relies on that code, too, and you\nprobably should be able to manually forget a credential at that plumbing\nlayer.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ab641bf5a9..184aee8dbc 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1997,6 +1997,12 @@ http.emptyAuth::\n> \ta username in the URL, as libcurl normally requires a username for\n> \tauthentication.\n> \n> +http.keepRejectedCredentials::\n> +\tKeep credentials in the credential helper that a Git server responded\n> +\tto with 401 (unauthorized) or 407 (proxy authentication required).\n> +\tThis can be useful in automation environments where credentials might\n> +\tbecome temporarily invalid. The default is `false`.\n\nLooks good.\n\n> http.delegation::\n> \tControl GSSAPI credential delegation. The delegation is disabled\n> \tby default in libcurl since version 7.21.7. Set parameter to tell\n> diff --git a/http.c b/http.c\n> index b4bfbceaeb..ff6932813f 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -138,6 +138,7 @@ static int ssl_cert_password_required;\n> #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> static unsigned long http_auth_methods = CURLAUTH_ANY;\n> static int http_auth_methods_restricted;\n> +static int keep_rejected_credentials = 0;\n\nMinor nit, but we usually skip the redundant \"= 0\" for BSS variables.\n\n> @@ -403,6 +404,11 @@ static int http_options(const char *var, const char *value, void *cb)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (!strcmp(\"http.keeprejectedcredentials\", var)) {\n> +\t\tkeep_rejected_credentials = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n> \t/* Fall back on the default ones */\n> \treturn git_default_config(var, value, cb);\n> }\n> @@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results *results)\n> \t\treturn HTTP_MISSING_TARGET;\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\tif (!keep_rejected_credentials)\n> +\t\t\t\tcredential_reject(&http_auth);\n\nThe rest of the patch looks good.\n\nIt's possible we'd eventually want a similar feature for other\nprotocols, like IMAP. And that we'd in the long run prefer to have a\nsingle credential.keepRejected that covers them all. Or maybe not. Given\nthat this is kind of a workaround, people might ultimately want\nprotocol-specific options. So I'm happy to start with \"http\" for now and\ndeal with other protocols down the road (if it's even necessary).\n\nSome scripts that use \"git credential\" may want to support this config\noption, too (I'm thinking of git-remote-mediawiki, which I believe\nuses it for http requests). But those can be added one by one to the\nporcelain scripts.\n\nSo modulo the minor \"= 0\" nit, this all looks good to me.\n\n-Peff\n"},{"id":"349256","messageId":"CAG2PGsoHajiYbS29F2nD+_0i2b4+Min5NR3tQYDb3MH=BW=0Aw@mail.gmail.com","threadId":"48642","inReplyTo":"20180604144747.GA27655@sigill.intra.peff.net","subject":"Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config","fromName":"Martin-Louis Bright","fromEmail":"mlbright@gmail.com","sentAt":"2018-06-04T16:18:59Z","receivedAt":"2018-06-04T16:19:44Z","isPatch":true,"sender":{"key":"mlbright@gmail.com","avatar":null},"body":"Why must the credentials must be deleted after receiving the 401 (or\nany) error? What's the rationale for this?\n\nOn Mon, Jun 4, 2018 at 10:47 AM, Jeff King wrote:\n> On Mon, Jun 04, 2018 at 05:26:35AM -0700, lars.schneider@autodesk.com wrote:\n>\n>> From: Lars Schneider \n>>\n>> If a Git HTTP server responds with 401 or 407, then Git tells the\n>> credential helper to reject and delete the credentials. In general\n>> this is good.\n>>\n>> However, in certain automation environments it is not desired to remove\n>> credentials automatically. This is in particular the case if credentials\n>> are only invalid temporarily (e.g. because of problems in the server's\n>> authentication backend).\n>>\n>> Therefore, add the config \"http.keepRejectedCredentials\" which tells\n>> Git to keep invalid credentials if set to \"true\".\n>\n> It seems like those servers should be returning a value besides \"401\" if\n> it's a temporary error.\n>\n> But alas, we live in the real world, and your patch seems like a pretty\n> sensible workaround for clients. This could be done at the helper layer,\n> but I think in practice doing it here is going to be a lot more\n> convenient (and doesn't preclude helpers having their own logic if\n> people care to extend them in that direction).\n>\n>> It was considered to disable the credential deletion in credential.c\n>> directly. This approach was not chosen as it could be confusing to\n>> other callers of credential_reject() if the function does not do what\n>> its name says (e.g. in imap-send.c).\n>\n> Yeah, I think \"git credential\" relies on that code, too, and you\n> probably should be able to manually forget a credential at that plumbing\n> layer.\n>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index ab641bf5a9..184aee8dbc 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -1997,6 +1997,12 @@ http.emptyAuth::\n>> a username in the URL, as libcurl normally requires a username for\n>> authentication.\n>>\n>> +http.keepRejectedCredentials::\n>> + Keep credentials in the credential helper that a Git server responded\n>> + to with 401 (unauthorized) or 407 (proxy authentication required).\n>> + This can be useful in automation environments where credentials might\n>> + become temporarily invalid. The default is `false`.\n>\n> Looks good.\n>\n>> http.delegation::\n>> Control GSSAPI credential delegation. The delegation is disabled\n>> by default in libcurl since version 7.21.7. Set parameter to tell\n>> diff --git a/http.c b/http.c\n>> index b4bfbceaeb..ff6932813f 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -138,6 +138,7 @@ static int ssl_cert_password_required;\n>> #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n>> static unsigned long http_auth_methods = CURLAUTH_ANY;\n>> static int http_auth_methods_restricted;\n>> +static int keep_rejected_credentials = 0;\n>\n> Minor nit, but we usually skip the redundant \"= 0\" for BSS variables.\n>\n>> @@ -403,6 +404,11 @@ static int http_options(const char *var, const char *value, void *cb)\n>> return 0;\n>> }\n>>\n>> + if (!strcmp(\"http.keeprejectedcredentials\", var)) {\n>> + keep_rejected_credentials = git_config_bool(var, value);\n>> + return 0;\n>> + }\n>> +\n>> /* Fall back on the default ones */\n>> return git_default_config(var, value, cb);\n>> }\n>> @@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results *results)\n>> return HTTP_MISSING_TARGET;\n>> else if (results->http_code == 401) {\n>> if (http_auth.username && http_auth.password) {\n>> - credential_reject(&http_auth);\n>> + if (!keep_rejected_credentials)\n>> + credential_reject(&http_auth);\n>\n> The rest of the patch looks good.\n>\n> It's possible we'd eventually want a similar feature for other\n> protocols, like IMAP. And that we'd in the long run prefer to have a\n> single credential.keepRejected that covers them all. Or maybe not. Given\n> that this is kind of a workaround, people might ultimately want\n> protocol-specific options. So I'm happy to start with \"http\" for now and\n> deal with other protocols down the road (if it's even necessary).\n>\n> Some scripts that use \"git credential\" may want to support this config\n> option, too (I'm thinking of git-remote-mediawiki, which I believe\n> uses it for http requests). But those can be added one by one to the\n> porcelain scripts.\n>\n> So modulo the minor \"= 0\" nit, this all looks good to me.\n>\n> -Peff\n"},{"id":"349294","messageId":"20180604185551.GA4296@sigill.intra.peff.net","threadId":"48642","inReplyTo":"CAG2PGsoHajiYbS29F2nD+_0i2b4+Min5NR3tQYDb3MH=BW=0Aw@mail.gmail.com","subject":"Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-04T18:55:51Z","receivedAt":"2018-06-04T18:55:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 04, 2018 at 12:18:59PM -0400, Martin-Louis Bright wrote:\n\n> Why must the credentials must be deleted after receiving the 401 (or\n> any) error? What's the rationale for this?\n\nBecause Git only tries a single credential per invocation. So if a\nhelper provides one, it doesn't prompt. If you get a 401 and then the\nprogram aborts, invoking it again is just going to try the same\ncredential over and over. Dropping the credential from the helper breaks\nout of that loop.\n\nIn fact, this patch probably should give the user some advice in that\nregard (either in the documentation, or as a warning when we skip the\nrejection). If you _do_ have a bogus credential and set the new option,\nyou'd need to reject it manually (you can do it with \"git credential\nreject\", but it's probably easier to just unset the option temporarily\nand re-invoke the original command).\n\n-Peff\n"},{"id":"349676","messageId":"46F82119-D185-4B41-828B-FC92709CFCDA@gmail.com","threadId":"48642","inReplyTo":"20180604185551.GA4296@sigill.intra.peff.net","subject":"Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-06-08T03:15:16Z","receivedAt":"2018-06-08T03:15:23Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 04 Jun 2018, at 11:55, Jeff King wrote:\n> \n> On Mon, Jun 04, 2018 at 12:18:59PM -0400, Martin-Louis Bright wrote:\n> \n>> Why must the credentials must be deleted after receiving the 401 (or\n>> any) error? What's the rationale for this?\n> \n> Because Git only tries a single credential per invocation. So if a\n> helper provides one, it doesn't prompt. If you get a 401 and then the\n> program aborts, invoking it again is just going to try the same\n> credential over and over. Dropping the credential from the helper breaks\n> out of that loop.\n> \n> In fact, this patch probably should give the user some advice in that\n> regard (either in the documentation, or as a warning when we skip the\n> rejection). If you _do_ have a bogus credential and set the new option,\n> you'd need to reject it manually (you can do it with \"git credential\n> reject\", but it's probably easier to just unset the option temporarily\n> and re-invoke the original command).\n\nI like the advice idea very much!\n\nHow about this?\n\n$ git fetch\nhint: Git has stored invalid credentials.\nhint: Reject them with 'git credential reject' or\nhint: disable the Git config 'http.keepRejectedCredentials'.\nremote: Invalid username or password.\nfatal: Authentication failed for 'https://server.com/myrepo.git/'\n\nI am not really sure about the grammar :-)\n\nThanks,\nLars\n"},{"id":"349679","messageId":"20180608054745.GA2893@sigill.intra.peff.net","threadId":"48642","inReplyTo":"46F82119-D185-4B41-828B-FC92709CFCDA@gmail.com","subject":"Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-08T05:47:45Z","receivedAt":"2018-06-08T05:47:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 07, 2018 at 08:15:16PM -0700, Lars Schneider wrote:\n\n> > In fact, this patch probably should give the user some advice in that\n> > regard (either in the documentation, or as a warning when we skip the\n> > rejection). If you _do_ have a bogus credential and set the new option,\n> > you'd need to reject it manually (you can do it with \"git credential\n> > reject\", but it's probably easier to just unset the option temporarily\n> > and re-invoke the original command).\n> \n> I like the advice idea very much!\n> \n> How about this?\n> \n> $ git fetch\n> hint: Git has stored invalid credentials.\n> hint: Reject them with 'git credential reject' or\n> hint: disable the Git config 'http.keepRejectedCredentials'.\n> remote: Invalid username or password.\n> fatal: Authentication failed for 'https://server.com/myrepo.git/'\n> \n> I am not really sure about the grammar :-)\n\nIt's probably not worth pointing the user at \"git credential reject\",\nsince it's not really meant to be friendly to users. In particular, you\nhave to speak the credential protocol on stdin.\n\nI _think_\n\n echo https://server.com/myrepo.git | git credential reject\n\nmight be enough, but I didn't test. Probably better advice is to just\nrepeat the command. Maybe:\n\n hint: Git kept invalid credentials due to the value of\n hint: http.keepRejectedCredentials. If you wish to drop these\n hint: credentials and be prompted for new ones, re-run your\n hint: command with \"git -c http.keepRejectedCredentials=false\".\n\nor something?\n\n-Peff\n"}]}