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

[Junio C Hamano <gitster@pobox.com>]

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.

Other than that, looking great.  Thanks for working on this.