Re: [PATCH v4 2/8] credential: add WWW-Authenticate header to cred requests

From: Matthew John Cheetham <mjcheetham@outlook.com>
To: Victoria Dye <vdye@github.com>,
	Matthew John Cheetham via GitGitGadget  <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Derrick Stolee <derrickstolee@github.com>,
	Lessley Dennington <lessleydennington@gmail.com>,
	M Hickford <mirth.hickford@gmail.com>,
	Jeff Hostetler <git@jeffhostetler.com>,
	Glen Choo <chooglen@google.com>,
	Matthew John Cheetham <mjcheetham@github.com>
Subject: Re: [PATCH v4 2/8] credential: add WWW-Authenticate header to cred requests
Date: Wed, 11 Jan 2023 12:37:14 -0800	[thread overview]
Message-ID: <AS2PR03MB9815AE0DC94C53B59E13C3C9C0FC9@AS2PR03MB9815.eurprd03.prod.outlook.com> (raw)
In-Reply-To: <35352a10-3fc0-365f-11af-b821d114c409@github.com>

On 2022-12-14 15:15, Victoria Dye wrote:

> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Add the value of the WWW-Authenticate response header to credential
>> requests. Credential helpers that understand and support HTTP
>> authentication and authorization can use this standard header (RFC 2616
>> Section 14.47 [1]) to generate valid credentials.
>>
>> WWW-Authenticate headers can contain information pertaining to the
>> authority, authentication mechanism, or extra parameters/scopes that are
>> required.
>>
>> The current I/O format for credential helpers only allows for unique
>> names for properties/attributes, so in order to transmit multiple header
>> values (with a specific order) we introduce a new convention whereby a
>> C-style array syntax is used in the property name to denote multiple
>> ordered values for the same property.
>>
>> In this case we send multiple `wwwauth[]` properties where the order
>> that the repeated attributes appear in the conversation reflects the
>> order that the WWW-Authenticate headers appeared in the HTTP response.
>>
>> [1] https://datatracker.ietf.org/doc/html/rfc2616#section-14.47
> 
> ...
> 
>> +Attributes with keys that end with C-style array brackets `[]` can have
>> +multiple values. Each instance of a multi-valued attribute forms an
>> +ordered list of values - the order of the repeated attributes defines
>> +the order of the values. An empty multi-valued attribute (`key[]=\n`)
>> +acts to clear any previous entries and reset the list.
>> +
> 
> The commit message & documentation changes (here and the 'www-auth[]'
> definition below) are concise, easy-to-understand explanations of what
> you're doing here with the 'www-authenticate' header values.
> 
>>  
>> @@ -160,6 +166,16 @@ empty string.
>>  Components which are missing from the URL (e.g., there is no
>>  username in the example above) will be left unset.
>>  
>> +`wwwauth[]`::
>> +
>> +	When an HTTP response is received by Git that includes one or more
>> +	'WWW-Authenticate' authentication headers, these will be passed by Git
>> +	to credential helpers.
>> +	Each 'WWW-Authenticate' header value is passed as a multi-valued
>> +	attribute 'wwwauth[]', where the order of the attributes is the same as
>> +	they appear in the HTTP response. This attribute is 'one-way' from Git
>> +	to pass additional information to credential helpers.
> 
> nit: if you're trying to get a paragraph break between "...to credential
> helpers." and "Each 'WWW-Authenticate' header value", you need to add an
> explicit break:
> 
> -------- 8< --------
> 
> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
> index bf0de0e940..50759153ef 100644
> --- a/Documentation/git-credential.txt
> +++ b/Documentation/git-credential.txt
> @@ -171,10 +171,11 @@ username in the example above) will be left unset.
>  	When an HTTP response is received by Git that includes one or more
>  	'WWW-Authenticate' authentication headers, these will be passed by Git
>  	to credential helpers.
> -	Each 'WWW-Authenticate' header value is passed as a multi-valued
> -	attribute 'wwwauth[]', where the order of the attributes is the same as
> -	they appear in the HTTP response. This attribute is 'one-way' from Git
> -	to pass additional information to credential helpers.
> ++
> +Each 'WWW-Authenticate' header value is passed as a multi-valued
> +attribute 'wwwauth[]', where the order of the attributes is the same as
> +they appear in the HTTP response. This attribute is 'one-way' from Git
> +to pass additional information to credential helpers.
>  
>  Unrecognised attributes are silently discarded.
>  
> -------- >8 --------
> 
> You can test to see how the docs look by running 'make doc' from the
> repository root and looking at the generated 'git-credential.html' (note
> that, if you've installed Git dependencies with Homebrew, you might need to
> specify 'XML_CATALOG_FILES=$(brew --prefix)/etc/xml/catalog' to get it to
> work).

Thanks! Yes, I was intending there to be a line break. Thanks for the tip;
will be addressed in the next iteration.

>> +
>>  Unrecognised attributes are silently discarded.
>>  
>>  GIT
>> diff --git a/credential.c b/credential.c
>> index 897b4679333..8a3ad6c0ae2 100644
>> --- a/credential.c
>> +++ b/credential.c
>> @@ -263,6 +263,17 @@ static void credential_write_item(FILE *fp, const char *key, const char *value,
>>  	fprintf(fp, "%s=%s\n", key, value);
>>  }
>>  
>> +static void credential_write_strvec(FILE *fp, const char *key,
>> +				    const struct strvec *vec)
>> +{
>> +	int i = 0;
>> +	const char *full_key = xstrfmt("%s[]", key);
>> +	for (; i < vec->nr; i++) {
>> +		credential_write_item(fp, full_key, vec->v[i], 0);
>> +	}
>> +	free((void*)full_key);
>> +}
>> +
>>  void credential_write(const struct credential *c, FILE *fp)
>>  {
>>  	credential_write_item(fp, "protocol", c->protocol, 1);
>> @@ -270,6 +281,7 @@ void credential_write(const struct credential *c, FILE *fp)
>>  	credential_write_item(fp, "path", c->path, 0);
>>  	credential_write_item(fp, "username", c->username, 0);
>>  	credential_write_item(fp, "password", c->password, 0);
>> +	credential_write_strvec(fp, "wwwauth", &c->wwwauth_headers);
> 
> This implementation looks good to me.
> 
>>  }
>>  
>>  static int run_credential_helper(struct credential *c,
> 

Thanks,
Matthew