Re: [PATCH] http: preserve wwwauth_headers across redirects

> Aaron Plattner <aplattner@nvidia.com> writes:
> 
>> diff --git a/http.c b/http.c
>> index ea9b16861b..cac8c9bfc9 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -2425,7 +2425,21 @@ static int http_request_recoverable(const char *url,
>>   	if (options->effective_url && options->base_url) {
>>   		if (update_url_from_redirect(options->base_url,
>>   					     url, options->effective_url)) {
>> +			struct strvec wwwauth_headers = STRVEC_INIT;
>> +
>> +			/*
>> +			 * Preserve wwwauth_headers across the call to
>> +			 * credential_from_url(): if the effective URL doesn't
>> +			 * specify its own credentials, a credential helper
>> +			 * might need the wwwauth[] array from the server's
>> +			 * redirect response in order to authenticate.
>> +			 */
>> +			strvec_pushv(&wwwauth_headers,
>> +				     http_auth.wwwauth_headers.v);
>>   			credential_from_url(&http_auth, options->base_url->buf);
>> +			strvec_pushv(&http_auth.wwwauth_headers,
>> +				     wwwauth_headers.v);
>> +			strvec_clear(&wwwauth_headers);
>>   			url = options->effective_url->buf;
>>   		}
>>   	}
> 
> As strvec_pushv() makes copies of the strings contained in .v[]
> array, the above will
> 
>   - make a deep copy of http_auth.wwwauth_headers.v[] and store it away
>     in wwwauth_headers.v[];
> 
>   - let credential_from_url() get rid of
>     http_auth.wwwauth_headers.v[] (the original is freed here, but we
>     have a deep copy stashed away safely), and perhaps add some of
>     its own there; then
> 
>   - add what we stashed away back to http_auth.wwwauth_headers.v[].
> 
> So it does not leak and it does not have use-after-free, either,
> which is good, even though it may be a bit inefficient having to
> copy these strings so many times.

> I briefly wondered if it is unconditionally adding back the original
> wwwauth_headers always the right thing to do, but I think this is
> good.  In the context of http_request_recovorable(), the redirect
> has already happened, and the request to the redirect target has
> failed with a 401. The wwwauth_headers currently in http_auth were
> populated from this 401 response from the redirect target. Since we
> are updating http_auth's URL to match this redirect target (in order
> to query the helper for the correct host), the headers we currently
> have are the active challenges for this new URL. Thus, they must be
> preserved and passed to the helper.

> A few design questions that came to my mind are:
> 
>   - Is wwwauth_headers the _only_ thing that needs to be preserved in
>     the existing credential in http_auth?  Will it stay to be the
>     only thing, or will we need to rethink what this patch did in the
>     future when we add such a new member to "struct credential"?
> 
>   - If we need to preserve some other members in "struct credential",
>     or if we add such members to the struct in the future, what would
>     be the recommended way to extend what this patch does to cover?
> 
> If we add new members in the future to store other transient
> response-based authentication state (e.g. Authentication-Info
> headers, or proxy authentication states), they will be wiped by
> credential_from_url() and will need to be preserved the same way,
> no?  This observation and thought experiment may hint that the
> manual save-and-restore approach is not robust against future
> extensions of struct credential.
> 
> The current approach of manually saving and restoring
> wwwauth_headers in http.c creates a tight coupling between the HTTP
> layer and the internals of struct credential. If new transient
> fields are added in the future, developers must remember to update
> http.c to preserve them, which may be error-prone.
> 
> I wonder if it would make the design more robust and future-proof to
> encapsulate this logic in credential.c instead.  For example, we
> could introduce a helper function:
> 
>      void credential_update_url(struct credential *c, const char *url)
> 
> that does what the new code added around credential_from_url() by
> this patch does, perhaps?