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

[Jeff King <peff@peff.net>]

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