Re: [PATCH v2] remote-curl: Use auth for probe_rpc() requests too

> Aaron Plattner <aplattner@nvidia.com> writes:
> 
>> Subject: Re: [PATCH v2] remote-curl: Use auth for probe_rpc() requests too
> 
> Micronit.  "Use" -> "use" (see "git shortlog --no-merges -200
> master" and notice that the usual "the first word of the sentence is
> upcased" rule does not typically apply to the word after "<area>: "
> prefix).

>> If a large request requires post_rpc() to call probe_rpc(), the latter
>> does not use the authorization credentials used for other requests. If
>> this fails with an HTTP 401 error and http_auth.multistage isn't set,
>> then the whole request just fails.
> 
>> For example, using git-credential-msal [1], the following attempt to clone a
>> large repository fails partway through because the initial request to download
>> the commit history and promisor packs succeeds, but the
>> subsequent request to download the blobs needed to construct the working
>> tree fails with a 401 error and the checkout fails.
>>
>> (lines removed for brevity)
>>
>>    git clone --filter=blob:none https://secure-server.example/repo
>>    ...
>>    11:07:34.725262 http.c:849              => Send header: POST repo/git-upload-pack HTTP/1.1
>>    11:07:34.725279 http.c:849              => Send header: Content-Type: application/x-git-upload-pack-request
>>    11:07:34.761407 http.c:849              <= Recv header: HTTP/1.1 401 Unauthorized
>>    11:07:34.761443 http.c:890              == Info: Bearer authentication problem, ignoring.
>>    11:07:34.761453 http.c:849              <= Recv header: WWW-Authenticate: Bearer error="invalid_request", error_description="No bearer token found in the request", msal-tenant-id="<tenant>", msal-client-id="<client>"
>>    11:07:34.761509 http.c:890              == Info: The requested URL returned error: 401
>>    11:07:34.761530 http.c:890              == Info: closing connection #0
> 
> Excellent illustration here.
> 
>>    11:07:34.761913 run-command.c:673       trace: run_command: 'git credential-cache erase'
>>    11:07:34.761927 run-command.c:765       trace: start_command: /bin/sh -c 'git credential-cache erase' 'git credential-cache erase'
>>    11:07:34.768069 git.c:502               trace: built-in: git credential-cache erase
>>    11:07:34.768690 run-command.c:673       trace: run_command: 'git credential-msal erase'
>>    11:07:34.768713 run-command.c:765       trace: start_command: /bin/sh -c 'git credential-msal erase' 'git credential-msal erase'
>>    11:07:34.772742 git.c:808               trace: exec: git-credential-msal erase
>>    11:07:34.772783 run-command.c:673       trace: run_command: git-credential-msal erase
>>    11:07:34.772819 run-command.c:765       trace: start_command: /usr/bin/git-credential-msal erase
>>    error: RPC failed; HTTP 401 curl 22 The requested URL returned error: 401
>>    fatal: unable to write request to remote: Broken pipe
>>    fatal: could not fetch c4fff0229c9be06ecf576356a4d39a8a755b8d81 from promisor remote
>>    warning: Clone succeeded, but checkout failed.
>>    You can inspect what was checked out with 'git status'
>>    and retry with 'git restore --source=HEAD :/'
>>
>> In this case, the HTTP_REAUTH retry logic is not used because the
>> credential helper didn't set the 'continue' flag, so
>> http_auth.multistage is false and handle_curl_result() fails with
>> HTTP_NOAUTH instead.
>>
>> Fix the immediate problem by including the authorization headers in the
>> probe_rpc() request as well.
> 
> Great.
> 
>> diff --git a/remote-curl.c b/remote-curl.c
>> index 69f919454a..1d0ae72521 100644
>> --- a/remote-curl.c
>> +++ b/remote-curl.c
>> @@ -877,6 +877,8 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>>   	headers = curl_slist_append(headers, rpc->hdr_content_type);
>>   	headers = curl_slist_append(headers, rpc->hdr_accept);
>>   
>> +	headers = http_append_auth_header(&http_auth, headers);
>> +
> 
> Is http_auth headers so different from the content-type and accept
> in the larger picture to warrant the blank line before this new
> call?  If not, you probably would want to have these three
> assignments to "headers" that accumulates the header lines together
> in a single block of three lines without any blank line in between.

>>   	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0L);
>>   	curl_easy_setopt(slot->curl, CURLOPT_POST, 1L);
>>   	curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
>> diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh
>> index c1febbae9d..adc210cdd3 100755
>> --- a/t/t5563-simple-http-auth.sh
>> +++ b/t/t5563-simple-http-auth.sh
>> @@ -63,6 +63,26 @@ test_expect_success 'setup repository' '
>>   	git push --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
>>   '
>>   
>> +test_expect_success 'setup large repository for probe_rpc testing' '
>> +	rm -rf large.git &&
>> +	git init large.git &&
>> +	(
>> +		cd large.git &&
>> +		git config set maintenance.auto false &&
>> +		git commit --allow-empty --message "initial" &&
>> +		# Create many refs to trigger probe_rpc, which is called when
>> +		# the request body is larger than http.postBuffer.
>> +		#
>> +		# In the test later, http.postBuffer is set to 70000. Each
>> +		# "want" line is ~45 bytes, so we need at least 70000/45 = ~1600
>> +		# refs
>> +		printf "create refs/heads/branch-%d @\n" $(test_seq 2000) |
>> +		    git update-ref --stdin
>> +	) &&
> 
> Hopefully, $(test_seq 2000) would not bust $(sysconf ARG_MAX), which
> could be as low as 4KB, on any system we care about.  If not, of
> course we could
> 
> 	test_seq 2000 |
> 	xargs printf "create ...\n" |
> 	git update-ref --stdin
> 
> which probably is not all that more expensive than what you wrote above.