[RFC PATCH v1] http: add http.keepRejectedCredentials config

[RFC PATCH v1] http: add http.keepRejectedCredentials config

[lars.schneider]

From: Lars Schneider <larsxschneider@gmail.com>

If a Git HTTP server responds with 401 or 407, then Git tells the
credential helper to reject and delete the credentials. In general
this is good.

However, in certain automation environments it is not desired to remove
credentials automatically. This is in particular the case if credentials
are only invalid temporarily (e.g. because of problems in the server's
authentication backend).

Therefore, add the config "http.keepRejectedCredentials" which tells
Git to keep invalid credentials if set to "true".

It was considered to disable the credential deletion in credential.c
directly. This approach was not chosen as it could be confusing to
other callers of credential_reject() if the function does not do what
its name says (e.g. in imap-send.c).

The Git-Credential-Manager-for-Windows already implements a similar
mechanism [1]. This solution aims to enable that feature for all
credential helper implementations.

[1] https://github.com/Microsoft/Git-Credential-Manager-for-Windows/blob/0c1af463b33b0a0142f36f99c49ca8f83e86ee43/Shared/Cli/Functions/Common.cs#L484-L504

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---

Notes:
    Base Ref: master
    Web-Diff: https://github.com/larsxschneider/git/commit/51993c2ff9
    Checkout: git fetch https://github.com/larsxschneider/git keepcreds-v1 && git checkout 51993c2ff9

 Documentation/config.txt |  6 ++++++
 http.c                   | 12 ++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a9..184aee8dbc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1997,6 +1997,12 @@ http.emptyAuth::
 	a username in the URL, as libcurl normally requires a username for
 	authentication.

+http.keepRejectedCredentials::
+	Keep credentials in the credential helper that a Git server responded
+	to with 401 (unauthorized) or 407 (proxy authentication required).
+	This can be useful in automation environments where credentials might
+	become temporarily invalid. The default is `false`.
+
 http.delegation::
 	Control GSSAPI credential delegation. The delegation is disabled
 	by default in libcurl since version 7.21.7. Set parameter to tell
diff --git a/http.c b/http.c
index b4bfbceaeb..ff6932813f 100644
--- a/http.c
+++ b/http.c
@@ -138,6 +138,7 @@ static int ssl_cert_password_required;
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
 static int http_auth_methods_restricted;
+static int keep_rejected_credentials = 0;
 /* Modes for which empty_auth cannot actually help us. */
 static unsigned long empty_auth_useless =
 	CURLAUTH_BASIC
@@ -403,6 +404,11 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}

+	if (!strcmp("http.keeprejectedcredentials", var)) {
+		keep_rejected_credentials = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
@@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results *results)
 		return HTTP_MISSING_TARGET;
 	else if (results->http_code == 401) {
 		if (http_auth.username && http_auth.password) {
-			credential_reject(&http_auth);
+			if (!keep_rejected_credentials)
+				credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
@@ -1485,7 +1492,8 @@ static int handle_curl_result(struct slot_results *results)
 		}
 	} else {
 		if (results->http_connectcode == 407)
-			credential_reject(&proxy_auth);
+			if (!keep_rejected_credentials)
+				credential_reject(&proxy_auth);
 #if LIBCURL_VERSION_NUM >= 0x070c00
 		if (!curl_errorstr[0])
 			strlcpy(curl_errorstr,

base-commit: c2c7d17b030646b40e6764ba34a5ebf66aee77af
--
2.17.1

Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config

[Jeff King]

On Mon, Jun 04, 2018 at 05:26:35AM -0700, lars.schneider@autodesk.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> If a Git HTTP server responds with 401 or 407, then Git tells the
> credential helper to reject and delete the credentials. In general
> this is good.
> 
> However, in certain automation environments it is not desired to remove
> credentials automatically. This is in particular the case if credentials
> are only invalid temporarily (e.g. because of problems in the server's
> authentication backend).
> 
> Therefore, add the config "http.keepRejectedCredentials" which tells
> Git to keep invalid credentials if set to "true".

It seems like those servers should be returning a value besides "401" if
it's a temporary error.

But alas, we live in the real world, and your patch seems like a pretty
sensible workaround for clients. This could be done at the helper layer,
but I think in practice doing it here is going to be a lot more
convenient (and doesn't preclude helpers having their own logic if
people care to extend them in that direction).

> It was considered to disable the credential deletion in credential.c
> directly. This approach was not chosen as it could be confusing to
> other callers of credential_reject() if the function does not do what
> its name says (e.g. in imap-send.c).

Yeah, I think "git credential" relies on that code, too, and you
probably should be able to manually forget a credential at that plumbing
layer.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ab641bf5a9..184aee8dbc 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1997,6 +1997,12 @@ http.emptyAuth::
>  	a username in the URL, as libcurl normally requires a username for
>  	authentication.
> 
> +http.keepRejectedCredentials::
> +	Keep credentials in the credential helper that a Git server responded
> +	to with 401 (unauthorized) or 407 (proxy authentication required).
> +	This can be useful in automation environments where credentials might
> +	become temporarily invalid. The default is `false`.

Looks good.

>  http.delegation::
>  	Control GSSAPI credential delegation. The delegation is disabled
>  	by default in libcurl since version 7.21.7. Set parameter to tell
> diff --git a/http.c b/http.c
> index b4bfbceaeb..ff6932813f 100644
> --- a/http.c
> +++ b/http.c
> @@ -138,6 +138,7 @@ static int ssl_cert_password_required;
>  #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
>  static unsigned long http_auth_methods = CURLAUTH_ANY;
>  static int http_auth_methods_restricted;
> +static int keep_rejected_credentials = 0;

Minor nit, but we usually skip the redundant "= 0" for BSS variables.

> @@ -403,6 +404,11 @@ static int http_options(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
> 
> +	if (!strcmp("http.keeprejectedcredentials", var)) {
> +		keep_rejected_credentials = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>  	/* Fall back on the default ones */
>  	return git_default_config(var, value, cb);
>  }
> @@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results *results)
>  		return HTTP_MISSING_TARGET;
>  	else if (results->http_code == 401) {
>  		if (http_auth.username && http_auth.password) {
> -			credential_reject(&http_auth);
> +			if (!keep_rejected_credentials)
> +				credential_reject(&http_auth);

The rest of the patch looks good.

It's possible we'd eventually want a similar feature for other
protocols, like IMAP. And that we'd in the long run prefer to have a
single credential.keepRejected that covers them all. Or maybe not. Given
that this is kind of a workaround, people might ultimately want
protocol-specific options. So I'm happy to start with "http" for now and
deal with other protocols down the road (if it's even necessary).

Some scripts that use "git credential" may want to support this config
option, too (I'm thinking of git-remote-mediawiki, which I believe
uses it for http requests). But those can be added one by one to the
porcelain scripts.

So modulo the minor "= 0" nit, this all looks good to me.

-Peff

Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config

[Martin-Louis Bright]

Why must the credentials must be deleted after receiving the 401 (or
any) error? What's the rationale for this?

On Mon, Jun 4, 2018 at 10:47 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Jun 04, 2018 at 05:26:35AM -0700, lars.schneider@autodesk.com wrote:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> If a Git HTTP server responds with 401 or 407, then Git tells the
>> credential helper to reject and delete the credentials. In general
>> this is good.
>>
>> However, in certain automation environments it is not desired to remove
>> credentials automatically. This is in particular the case if credentials
>> are only invalid temporarily (e.g. because of problems in the server's
>> authentication backend).
>>
>> Therefore, add the config "http.keepRejectedCredentials" which tells
>> Git to keep invalid credentials if set to "true".
>
> It seems like those servers should be returning a value besides "401" if
> it's a temporary error.
>
> But alas, we live in the real world, and your patch seems like a pretty
> sensible workaround for clients. This could be done at the helper layer,
> but I think in practice doing it here is going to be a lot more
> convenient (and doesn't preclude helpers having their own logic if
> people care to extend them in that direction).
>
>> It was considered to disable the credential deletion in credential.c
>> directly. This approach was not chosen as it could be confusing to
>> other callers of credential_reject() if the function does not do what
>> its name says (e.g. in imap-send.c).
>
> Yeah, I think "git credential" relies on that code, too, and you
> probably should be able to manually forget a credential at that plumbing
> layer.
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index ab641bf5a9..184aee8dbc 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1997,6 +1997,12 @@ http.emptyAuth::
>>       a username in the URL, as libcurl normally requires a username for
>>       authentication.
>>
>> +http.keepRejectedCredentials::
>> +     Keep credentials in the credential helper that a Git server responded
>> +     to with 401 (unauthorized) or 407 (proxy authentication required).
>> +     This can be useful in automation environments where credentials might
>> +     become temporarily invalid. The default is `false`.
>
> Looks good.
>
>>  http.delegation::
>>       Control GSSAPI credential delegation. The delegation is disabled
>>       by default in libcurl since version 7.21.7. Set parameter to tell
>> diff --git a/http.c b/http.c
>> index b4bfbceaeb..ff6932813f 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -138,6 +138,7 @@ static int ssl_cert_password_required;
>>  #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
>>  static unsigned long http_auth_methods = CURLAUTH_ANY;
>>  static int http_auth_methods_restricted;
>> +static int keep_rejected_credentials = 0;
>
> Minor nit, but we usually skip the redundant "= 0" for BSS variables.
>
>> @@ -403,6 +404,11 @@ static int http_options(const char *var, const char *value, void *cb)
>>               return 0;
>>       }
>>
>> +     if (!strcmp("http.keeprejectedcredentials", var)) {
>> +             keep_rejected_credentials = git_config_bool(var, value);
>> +             return 0;
>> +     }
>> +
>>       /* Fall back on the default ones */
>>       return git_default_config(var, value, cb);
>>  }
>> @@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results *results)
>>               return HTTP_MISSING_TARGET;
>>       else if (results->http_code == 401) {
>>               if (http_auth.username && http_auth.password) {
>> -                     credential_reject(&http_auth);
>> +                     if (!keep_rejected_credentials)
>> +                             credential_reject(&http_auth);
>
> The rest of the patch looks good.
>
> It's possible we'd eventually want a similar feature for other
> protocols, like IMAP. And that we'd in the long run prefer to have a
> single credential.keepRejected that covers them all. Or maybe not. Given
> that this is kind of a workaround, people might ultimately want
> protocol-specific options. So I'm happy to start with "http" for now and
> deal with other protocols down the road (if it's even necessary).
>
> Some scripts that use "git credential" may want to support this config
> option, too (I'm thinking of git-remote-mediawiki, which I believe
> uses it for http requests). But those can be added one by one to the
> porcelain scripts.
>
> So modulo the minor "= 0" nit, this all looks good to me.
>
> -Peff

Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config

[Jeff King]

On Mon, Jun 04, 2018 at 12:18:59PM -0400, Martin-Louis Bright wrote:

> Why must the credentials must be deleted after receiving the 401 (or
> any) error? What's the rationale for this?

Because Git only tries a single credential per invocation. So if a
helper provides one, it doesn't prompt. If you get a 401 and then the
program aborts, invoking it again is just going to try the same
credential over and over. Dropping the credential from the helper breaks
out of that loop.

In fact, this patch probably should give the user some advice in that
regard (either in the documentation, or as a warning when we skip the
rejection). If you _do_ have a bogus credential and set the new option,
you'd need to reject it manually (you can do it with "git credential
reject", but it's probably easier to just unset the option temporarily
and re-invoke the original command).

-Peff

Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config

[Lars Schneider]

> On 04 Jun 2018, at 11:55, Jeff King <peff@peff.net> wrote:
> 
> On Mon, Jun 04, 2018 at 12:18:59PM -0400, Martin-Louis Bright wrote:
> 
>> Why must the credentials must be deleted after receiving the 401 (or
>> any) error? What's the rationale for this?
> 
> Because Git only tries a single credential per invocation. So if a
> helper provides one, it doesn't prompt. If you get a 401 and then the
> program aborts, invoking it again is just going to try the same
> credential over and over. Dropping the credential from the helper breaks
> out of that loop.
> 
> In fact, this patch probably should give the user some advice in that
> regard (either in the documentation, or as a warning when we skip the
> rejection). If you _do_ have a bogus credential and set the new option,
> you'd need to reject it manually (you can do it with "git credential
> reject", but it's probably easier to just unset the option temporarily
> and re-invoke the original command).

I like the advice idea very much!

How about this?

$ git fetch
hint: Git has stored invalid credentials.
hint: Reject them with 'git credential reject' or
hint: disable the Git config 'http.keepRejectedCredentials'.
remote: Invalid username or password.
fatal: Authentication failed for 'https://server.com/myrepo.git/'

I am not really sure about the grammar :-)

Thanks,
Lars

Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config

[Jeff King]

On Thu, Jun 07, 2018 at 08:15:16PM -0700, Lars Schneider wrote:

> > In fact, this patch probably should give the user some advice in that
> > regard (either in the documentation, or as a warning when we skip the
> > rejection). If you _do_ have a bogus credential and set the new option,
> > you'd need to reject it manually (you can do it with "git credential
> > reject", but it's probably easier to just unset the option temporarily
> > and re-invoke the original command).
> 
> I like the advice idea very much!
> 
> How about this?
> 
> $ git fetch
> hint: Git has stored invalid credentials.
> hint: Reject them with 'git credential reject' or
> hint: disable the Git config 'http.keepRejectedCredentials'.
> remote: Invalid username or password.
> fatal: Authentication failed for 'https://server.com/myrepo.git/'
> 
> I am not really sure about the grammar :-)

It's probably not worth pointing the user at "git credential reject",
since it's not really meant to be friendly to users. In particular, you
have to speak the credential protocol on stdin.

I _think_

  echo https://server.com/myrepo.git | git credential reject

might be enough, but I didn't test. Probably better advice is to just
repeat the command. Maybe:

  hint: Git kept invalid credentials due to the value of
  hint: http.keepRejectedCredentials. If you wish to drop these
  hint: credentials and be prompted for new ones, re-run your
  hint: command with "git -c http.keepRejectedCredentials=false".

or something?

-Peff