git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 0/4] t/lib-httpd ssl fixes

From
Jeff King <peff@peff.net>
Date
Feb 3, 2023, 17:17 UTC
Message-ID
<Y91Bq+xYUhDnPHfS@coredump.intra.peff.net>
In-Reply-To
<Y9s7vyHKXP+TQPRm@pobox.com>
On Wed, Feb 01, 2023 at 11:27:43PM -0500, Todd Zullinger wrote:
Show 8 quoted lines
> > The final two patches here fix ssl problems I found. The first two
> > patches drop support for older apache. This yields some minor cleanups,
> > and makes the ssl fixes slightly easier. I've cc'd Todd as the last
> > person to express support for Apache 2.2, in 2017. I'm hoping even
> > CentOS has moved on by now, but we'll see. :)
> 
> Heh.  Fortunately, CentOS 6 has been EOL for a few years.
> CentOS 7 has httpd-2.4.6.
Oh, good. So I think we are OK to take these patches, then.
> I applied these patches and ran builds for CentOS/RHEL 7-9
> and Fedora 36-38.  I had not previously run the test suite
> with LIB_HTTPD_SSL=1 and I ran into many, many failures.
> (154 failures across 12 tests, to be precise.)

Oof. I was just focusing on getting a handful of interesting tests to run, and didn't think to try the whole suite.

> I can clean up this diff if you think it's worthwhile.

Yes, please. Your fixes all look to be along the lines I'd expect. I think my patches can stand on their own as an incremental improvement, and then yours can come on top as a separate series.

> It sounds like it may be quite useful for the http/2 tests, but maybe
> LIB_HTTPD_SSL=1 in t5559-http-fetch-smart-http2 is simpler for now?

I'd prefer to avoid requiring ssl support for those tests if we can. Version 2.0.12 of mod_http2 does fix the test failure. There may be some other issues (there's more details in the GitHub issue I linked earlier), but I think it would be enough to stop the immediate pain. I don't know how long it will take before that version makes it into an apache release.

> -- 8< --
A few bits I noticed on your test fixes:
Show 10 quoted lines
> diff --git i/t/lib-httpd.sh w/t/lib-httpd.sh
> index 608949ea80..a4f787f580 100644
> --- i/t/lib-httpd.sh
> +++ w/t/lib-httpd.sh
> @@ -168,7 +168,7 @@ prepare_httpd() {
>  		then
>  			HTTPD_PARA="$HTTPD_PARA -DSVN"
>  			LIB_HTTPD_SVNPATH="$rawsvnrepo"
> -			svnrepo="http://127.0.0.1:$LIB_HTTPD_PORT/"
> +			svnrepo="$HTTPD_PROTO://127.0.0.1:$LIB_HTTPD_PORT/"

This probably should have been $HTTPD_URL all along, which fixes the protocol and avoids hard-coding the loopback address.

Show 10 quoted lines
> diff --git i/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh
> index fbad2d5ff5..b1f414dfe0 100755
> --- i/t/t5541-http-push-smart.sh
> +++ w/t/t5541-http-push-smart.sh
> @@ -122,9 +122,9 @@ test_expect_success 'setup rejected update hook' '
>  
>  	cat >exp <<-EOF
>  	remote: error: hook declined to update refs/heads/dev2
> -	To http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git
> +	To '$HTTPD_PROTO'://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git
Ditto here.
Show 12 quoted lines
> diff --git i/t/t5550-http-fetch-dumb.sh w/t/t5550-http-fetch-dumb.sh
> index 8f182a3cbf..070d04cdce 100755
> --- i/t/t5550-http-fetch-dumb.sh
> +++ w/t/t5550-http-fetch-dumb.sh
> @@ -384,7 +384,7 @@ test_expect_success 'remote-http complains cleanly about malformed urls' '
>  # learned to handle early remote helper failures more cleanly.
>  test_expect_success 'remote-http complains cleanly about empty scheme' '
>  	test_must_fail ok=sigpipe git ls-remote \
> -		http::${HTTPD_URL#http}/dumb/repo.git 2>stderr &&
> +		http::${HTTPD_URL#$HTTPD_PROTO}/dumb/repo.git 2>stderr &&
>  	test_i18ngrep "url has no scheme" stderr
>  '

I wondered if this should also be adjusting "http::" to match the protocol. It doesn't really matter, though. The point of the test is use "http::" to route it to git-remote-curl, and then "://anything" should fail. The fact that it is using $HTTPD_URL in the first place is only so that we might detect an accidental success. We never expect to actually hit the endpoint.

Show 11 quoted lines
> @@ -454,9 +454,9 @@ test_expect_success 'http-alternates triggers not-from-user protocol check' '
>  	echo "$HTTPD_URL/dumb/victim.git/objects" \
>  		>"$evil/objects/info/http-alternates" &&
>  	test_config_global http.followRedirects true &&
> -	test_must_fail git -c protocol.http.allow=user \
> +	test_must_fail git -c protocol.'$HTTPD_PROTO'.allow=user \
>  		clone $HTTPD_URL/dumb/evil.git evil-user &&
> -	git -c protocol.http.allow=always \
> +	git -c protocol.'$HTTPD_PROTO'.allow=always \
>  		clone $HTTPD_URL/dumb/evil.git evil-user
>  '

These single quotes seem unnecessary. We're in a single-quoted string, but it's a test-body that will be eval'd. So the variable can just be referenced in the usual way, just like $HTTPD_URL in the existing context.

Show 10 quoted lines
> diff --git i/t/t5703-upload-pack-ref-in-want.sh w/t/t5703-upload-pack-ref-in-want.sh
> index df74f80061..b365e30eda 100755
> --- i/t/t5703-upload-pack-ref-in-want.sh
> +++ w/t/t5703-upload-pack-ref-in-want.sh
> @@ -450,7 +450,7 @@ test_expect_success 'setup repos for change-while-negotiating test' '
>  		# Local repo with many commits (so that negotiation will take
>  		# more than 1 request/response pair)
>  		rm -rf "$LOCAL_PRISTINE" &&
> -		git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo" "$LOCAL_PRISTINE" &&
> +		git clone "$HTTPD_PROTO://127.0.0.1:$LIB_HTTPD_PORT/smart/repo" "$LOCAL_PRISTINE" &&
Another one that could be $HTTPD_URL.
Show 6 quoted lines
> @@ -462,7 +462,7 @@ test_expect_success 'setup repos for change-while-negotiating test' '
>  		test_commit m3 &&
>  		git tag -d m2 m3
>  	) &&
> -	git -C "$LOCAL_PRISTINE" remote set-url origin "http://127.0.0.1:$LIB_HTTPD_PORT/one_time_perl/repo" &&
> +	git -C "$LOCAL_PRISTINE" remote set-url origin "$HTTPD_PROTO://127.0.0.1:$LIB_HTTPD_PORT/one_time_perl/repo" &&
Likewise.
Show 16 quoted lines
> diff --git i/t/t5812-proto-disable-http.sh w/t/t5812-proto-disable-http.sh
> index d8da5f58d1..9ee5132276 100755
> --- i/t/t5812-proto-disable-http.sh
> +++ w/t/t5812-proto-disable-http.sh
> [...]
> @@ -28,9 +28,9 @@ test_expect_success 'curl limits redirects' '
>  '
>  
>  test_expect_success 'http can be limited to from-user' '
> -	git -c protocol.http.allow=user \
> +	git -c protocol.'$HTTPD_PROTO'.allow=user \
>  		clone "$HTTPD_URL/smart/repo.git" plain.git &&
> -	test_must_fail git -c protocol.http.allow=user \
> +	test_must_fail git -c protocol.'$HTTPD_PROTO'.allow=user \
>  		clone "$HTTPD_URL/smart-redir-perm/repo.git" redir.git
>  '
And another one with funny quotes.
-Peff
Previous: Todd Zullinger
Message 9 of 9 in “t/lib-httpd ssl fixes”
  1. 0/4 t/lib-httpd ssl fixesJeff King, Feb 1, 2023
  2. 1/4 t/lib-httpd: bump required apache version to 2.2Jeff King, Feb 1, 2023
  3. Todd ZullingerFeb 2, 2023
  4. Jeff KingFeb 3, 2023
  5. 2/4 t/lib-httpd: bump required apache version to 2.4Jeff King, Feb 1, 2023
  6. 3/4 t/lib-httpd: drop SSLMutex configJeff King, Feb 1, 2023
  7. 4/4 t/lib-httpd: increase ssl key size to 2048 bitsJeff King, Feb 1, 2023
  8. Todd ZullingerFeb 2, 2023
  9. Jeff KingFeb 3, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.