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

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

[Jeff King]

While chasing down a possible HTTP/2 problem in our test suite (which
turns out not to be a Git bug, I think), I tried running the tests with
LIB_HTTPD_SSL=1, as TLS affects HTTP/2 upgrade.

Sadly, apache would not start at all with our ssl config. It looks like
this has probably been broken for many years, depending on your apache
and openssl versions.

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. :)

  [1/4]: t/lib-httpd: bump required apache version to 2.2
  [2/4]: t/lib-httpd: bump required apache version to 2.4
  [3/4]: t/lib-httpd: drop SSLMutex config
  [4/4]: t/lib-httpd: increase ssl key size to 2048 bits

 t/lib-httpd.sh          | 11 +++++++----
 t/lib-httpd/apache.conf | 31 ++++---------------------------
 t/lib-httpd/ssl.cnf     |  2 +-
 3 files changed, 12 insertions(+), 32 deletions(-)

-Peff

[PATCH 1/4] t/lib-httpd: bump required apache version to 2.2

[Jeff King]

Apache 2.2 was released in 2005, almost 18 years ago. We can probably
assume that people are running a version at least that old (and the
stakes for removing it are fairly low, as the worst case is that they
would not run the http tests against their ancient version).

Dropping support for the older versions cleans up the config file a
little, and will also enable us to bump the required version further
(with more cleanups) in a future patch.

Note that the file actually checks for version 2.1. In apache's
versioning scheme, odd numbered versions are for development and even
numbers are for stable releases. So 2.1 and 2.2 are effectively the same
from our perspective.

Older versions would just fail to start, which would generally cause us
to skip the tests. However, we do have version detection code in
lib-httpd.sh which produces a nicer error message, so let's update that,
too. I didn't bother handling the case of "3.0", etc. Apache has been on
2.x for 21 years, with no signs of bumping the major version.  And if
they eventually do, I suspect there will be enough breaking changes that
we'd need to update more than just the numeric version check. We can
worry about that hypothetical when it happens.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd.sh          | 11 +++++++----
 t/lib-httpd/apache.conf |  8 --------
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 608949ea80..8fc411ff41 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -99,16 +99,19 @@ then
 fi
 
 HTTPD_VERSION=$($LIB_HTTPD_PATH -v | \
-	sed -n 's/^Server version: Apache\/\([0-9]*\)\..*$/\1/p; q')
+	sed -n 's/^Server version: Apache\/\([0-9.]*\).*$/\1/p; q')
+HTTPD_VERSION_MAJOR=$(echo $HTTPD_VERSION | cut -d. -f1)
+HTTPD_VERSION_MINOR=$(echo $HTTPD_VERSION | cut -d. -f2)
 
-if test -n "$HTTPD_VERSION"
+if test -n "$HTTPD_VERSION_MAJOR"
 then
 	if test -z "$LIB_HTTPD_MODULE_PATH"
 	then
-		if ! test $HTTPD_VERSION -ge 2
+		if ! test "$HTTPD_VERSION_MAJOR" -eq 2 ||
+		   ! test "$HTTPD_VERSION_MINOR" -ge 2
 		then
 			test_skip_or_die GIT_TEST_HTTPD \
-				"at least Apache version 2 is required"
+				"at least Apache version 2.2 is required"
 		fi
 		if ! test -d "$DEFAULT_HTTPD_MODULE_PATH"
 		then
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 0294739a77..35f5e28507 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -38,13 +38,6 @@ Protocols h2c
 LockFile accept.lock
 </IfVersion>
 
-<IfVersion < 2.1>
-<IfModule !mod_auth.c>
-	LoadModule auth_module modules/mod_auth.so
-</IfModule>
-</IfVersion>
-
-<IfVersion >= 2.1>
 <IfModule !mod_auth_basic.c>
 	LoadModule auth_basic_module modules/mod_auth_basic.so
 </IfModule>
@@ -57,7 +50,6 @@ LockFile accept.lock
 <IfModule !mod_authz_host.c>
 	LoadModule authz_host_module modules/mod_authz_host.so
 </IfModule>
-</IfVersion>
 
 <IfVersion >= 2.4>
 <IfModule !mod_authn_core.c>
-- 
2.39.1.767.gb4615b3bd3

Re: [PATCH 1/4] t/lib-httpd: bump required apache version to 2.2

[Todd Zullinger]

Jeff King wrote:
> Apache 2.2 was released in 2005, almost 18 years ago. We can probably
> assume that people are running a version at least that old (and the
> stakes for removing it are fairly low, as the worst case is that they
> would not run the http tests against their ancient version).
> 
> Dropping support for the older versions cleans up the config file a
> little, and will also enable us to bump the required version further
> (with more cleanups) in a future patch.
> 
> Note that the file actually checks for version 2.1. In apache's
> versioning scheme, odd numbered versions are for development and even
> numbers are for stable releases. So 2.1 and 2.2 are effectively the same
> from our perspective.
> 
> Older versions would just fail to start, which would generally cause us
> to skip the tests. However, we do have version detection code in
> lib-httpd.sh which produces a nicer error message, so let's update that,
> too. I didn't bother handling the case of "3.0", etc. Apache has been on
> 2.x for 21 years, with no signs of bumping the major version.  And if
> they eventually do, I suspect there will be enough breaking changes that
> we'd need to update more than just the numeric version check. We can
> worry about that hypothetical when it happens.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/lib-httpd.sh          | 11 +++++++----
>  t/lib-httpd/apache.conf |  8 --------
>  2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index 608949ea80..8fc411ff41 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -99,16 +99,19 @@ then
>  fi
>  
>  HTTPD_VERSION=$($LIB_HTTPD_PATH -v | \

Trivial, but is it worth getting rid of the unnecessary
backslash, while you're improving things here?  Maybe that's
a mild tangent for code that's otherwise not adjusted very
often?

The backslash was present when lib-httpd.sh was added in
faa4bc35a0 (http-push: add regression tests, 2008-02-27).
The line was last touched in e429dfd5e4 (t/lib-httpd.sh: use
the $( ... ) construct for command substitution,
2015-12-22).

> -	sed -n 's/^Server version: Apache\/\([0-9]*\)\..*$/\1/p; q')
> +	sed -n 's/^Server version: Apache\/\([0-9.]*\).*$/\1/p; q')
> +HTTPD_VERSION_MAJOR=$(echo $HTTPD_VERSION | cut -d. -f1)
> +HTTPD_VERSION_MINOR=$(echo $HTTPD_VERSION | cut -d. -f2)

-- 
Todd

Re: [PATCH 1/4] t/lib-httpd: bump required apache version to 2.2

[Jeff King]

On Wed, Feb 01, 2023 at 11:39:37PM -0500, Todd Zullinger wrote:

> > diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> > index 608949ea80..8fc411ff41 100644
> > --- a/t/lib-httpd.sh
> > +++ b/t/lib-httpd.sh
> > @@ -99,16 +99,19 @@ then
> >  fi
> >  
> >  HTTPD_VERSION=$($LIB_HTTPD_PATH -v | \
> 
> Trivial, but is it worth getting rid of the unnecessary
> backslash, while you're improving things here?  Maybe that's
> a mild tangent for code that's otherwise not adjusted very
> often?

Heh, I didn't even notice. I don't think it's a big deal either way. It
definitely doesn't match our style, and I don't mind if we fix it on
top, but it could just be left alone.

It might make more sense as part of a broader cleanup. Running:

  git grep '| \\'

turns up a number of hits.

-Peff

[PATCH 2/4] t/lib-httpd: bump required apache version to 2.4

[Jeff King]

Apache 2.4 has been out since early 2012, almost 11 years. And its
predecessor, 2.2, has been out of support since its last release in
2017, over 5 years ago. The last mention on the mailing list was from
around the same time, in this thread:

  https://lore.kernel.org/git/20171231023234.21215-1-tmz@pobox.com/

We can probably assume that 2.4 is available everywhere. And the stakes
are fairly low, as the worst case is that such a platform would skip the
http tests.

This lets us clean up a few minor version checks in the config file, but
also revert f1f2b45be0 (tests: adjust the configuration for Apache 2.2,
2016-05-09). Its technique isn't _too_ bad, but certainly required a bit
more explanation than the 2.4 version it replaced. I manually confirmed
that the test in t5551 still behaves as expected (if you replace
"cadabra" with "foo", the server correctly rejects the request).

It will also help future patches which will no longer have to deal with
conditional config for this old version.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd.sh          |  4 ++--
 t/lib-httpd/apache.conf | 22 ++++------------------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 8fc411ff41..5d2d56c445 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -108,10 +108,10 @@ then
 	if test -z "$LIB_HTTPD_MODULE_PATH"
 	then
 		if ! test "$HTTPD_VERSION_MAJOR" -eq 2 ||
-		   ! test "$HTTPD_VERSION_MINOR" -ge 2
+		   ! test "$HTTPD_VERSION_MINOR" -ge 4
 		then
 			test_skip_or_die GIT_TEST_HTTPD \
-				"at least Apache version 2.2 is required"
+				"at least Apache version 2.4 is required"
 		fi
 		if ! test -d "$DEFAULT_HTTPD_MODULE_PATH"
 		then
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 35f5e28507..332617f10d 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -34,10 +34,6 @@ LoadModule http2_module modules/mod_http2.so
 Protocols h2c
 </IfDefine>
 
-<IfVersion < 2.4>
-LockFile accept.lock
-</IfVersion>
-
 <IfModule !mod_auth_basic.c>
 	LoadModule auth_basic_module modules/mod_auth_basic.so
 </IfModule>
@@ -51,7 +47,6 @@ LockFile accept.lock
 	LoadModule authz_host_module modules/mod_authz_host.so
 </IfModule>
 
-<IfVersion >= 2.4>
 <IfModule !mod_authn_core.c>
 	LoadModule authn_core_module modules/mod_authn_core.so
 </IfModule>
@@ -75,7 +70,6 @@ LockFile accept.lock
 	LoadModule mpm_prefork_module modules/mod_mpm_prefork.so
 </IfModule>
 </IfDefine>
-</IfVersion>
 
 PassEnv GIT_VALGRIND
 PassEnv GIT_VALGRIND_OPTIONS
@@ -115,6 +109,10 @@ Alias /auth/dumb/ www/auth/dumb/
 	Header set Set-Cookie name=value
 </LocationMatch>
 <LocationMatch /smart_headers/>
+	<RequireAll>
+		Require expr %{HTTP:x-magic-one} == 'abra'
+		Require expr %{HTTP:x-magic-two} == 'cadabra'
+	</RequireAll>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
@@ -197,18 +195,6 @@ RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
 RewriteRule ^/redir-objects/(.*/info/refs)$ /dumb/$1 [PT]
 RewriteRule ^/redir-objects/(.*/objects/.*)$ /dumb/$1 [R=301]
 
-# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
-# And as RewriteCond does not allow testing for non-matches, we match
-# the desired case first (one has abra, two has cadabra), and let it
-# pass by marking the RewriteRule as [L], "last rule, do not process
-# any other matching RewriteRules after this"), and then have another
-# RewriteRule that matches all other cases and lets them fail via '[F]',
-# "fail the request".
-RewriteCond %{HTTP:x-magic-one} =abra
-RewriteCond %{HTTP:x-magic-two} =cadabra
-RewriteRule ^/smart_headers/.* - [L]
-RewriteRule ^/smart_headers/.* - [F]
-
 <IfDefine SSL>
 LoadModule ssl_module modules/mod_ssl.so
 
-- 
2.39.1.767.gb4615b3bd3

[PATCH 3/4] t/lib-httpd: drop SSLMutex config

[Jeff King]

The SSL config enabled by setting LIB_HTTPD_SSL does not work with
Apache versions greater than 2.2, as more recent versions complain about
the SSLMutex directive. According to
https://httpd.apache.org/docs/current/upgrading.html:

  Directives AcceptMutex, LockFile, RewriteLock, SSLMutex,
  SSLStaplingMutex, and WatchdogMutexPath have been replaced with a
  single Mutex directive. You will need to evaluate any use of these
  removed directives in your 2.2 configuration to determine if they can
  just be deleted or will need to be replaced using Mutex.

Deleting this line will just use the system default, which seems
sensible. The original came as part of faa4bc35a0 (http-push: add
regression tests, 2008-02-27), but no specific reason is given there (or
on the mailing list) for its presence.

Signed-off-by: Jeff King <peff@peff.net>
---
If we can't drop support for 2.2 in patch 2, then this would need to be
adjusted with an IfVersion.

 t/lib-httpd/apache.conf | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 332617f10d..51a4fbcf62 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -203,7 +203,6 @@ SSLCertificateKeyFile httpd.pem
 SSLRandomSeed startup file:/dev/urandom 512
 SSLRandomSeed connect file:/dev/urandom 512
 SSLSessionCache none
-SSLMutex file:ssl_mutex
 SSLEngine On
 </IfDefine>
 
-- 
2.39.1.767.gb4615b3bd3

[PATCH 4/4] t/lib-httpd: increase ssl key size to 2048 bits

[Jeff King]

Recent versions of openssl will refuse to work with 1024-bit RSA keys,
as they are considered insecure. I didn't track down the exact version
in which the defaults were tightened, but the Debian-package openssl 3.0
on my system yields:

  $ LIB_HTTPD_SSL=1 ./t5551-http-fetch-smart.sh -v -i
  [...]
  SSL Library Error: error:0A00018F:SSL routines::ee key too small
  1..0 # SKIP web server setup failed

This could probably be overcome with configuration, but that's likely
to be a headache (especially if it requires touching /etc/openssl).
Let's just pick a key size that's less outrageously out of date.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd/ssl.cnf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-httpd/ssl.cnf b/t/lib-httpd/ssl.cnf
index 6dab2579cb..812e8253f0 100644
--- a/t/lib-httpd/ssl.cnf
+++ b/t/lib-httpd/ssl.cnf
@@ -1,7 +1,7 @@
 RANDFILE                = $ENV::RANDFILE_PATH
 
 [ req ]
-default_bits            = 1024
+default_bits            = 2048
 distinguished_name      = req_distinguished_name
 prompt                  = no
 [ req_distinguished_name ]
-- 
2.39.1.767.gb4615b3bd3

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

[Todd Zullinger]

Jeff King wrote:
> While chasing down a possible HTTP/2 problem in our test suite (which
> turns out not to be a Git bug, I think), I tried running the tests with
> LIB_HTTPD_SSL=1, as TLS affects HTTP/2 upgrade.
> 
> Sadly, apache would not start at all with our ssl config. It looks like
> this has probably been broken for many years, depending on your apache
> and openssl versions.
> 
> 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.

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.)

None of the failures were due to the httpd config, so the
changes here seem fine. :)

The below diff is what I applied to resolve all but 21
git-svn failures:

Test Summary Report
-------------------
t9118-git-svn-funky-branch-names.sh              (Wstat: 256 (exited 1) Tests: 5 Failed: 4)
  Failed tests:  2-5
  Non-zero exit status: 1
t9115-git-svn-dcommit-funky-renames.sh           (Wstat: 256 (exited 1) Tests: 12 Failed: 11)
  Failed tests:  2-12
  Non-zero exit status: 1
t9120-git-svn-clone-with-percent-escapes.sh      (Wstat: 256 (exited 1) Tests: 8 Failed: 5)
  Failed tests:  2, 5-8
  Non-zero exit status: 1
t9142-git-svn-shallow-clone.sh                   (Wstat: 256 (exited 1) Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=983, Tests=27522, 353 wallclock secs (11.78 usr  2.61 sys + 935.44 cusr 1358.38 csys = 2308.21 CPU)

I didn't feel like digging into the git-svn code and the
subversion bindings to try and fix those tests.  It's likely
a relatively simple matter to someone who knows them better
than I do, hopefully.

I can clean up this diff if you think it's worthwhile.  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?

-- 8< --
diff --git i/t/lib-git-svn.sh w/t/lib-git-svn.sh
index ea28971e8e..f636bcafe4 100644
--- i/t/lib-git-svn.sh
+++ w/t/lib-git-svn.sh
@@ -67,7 +67,7 @@ svn_cmd () {
 		svn
 		return
 	fi
-	svn "$orig_svncmd" --config-dir "$svnconf" "$@"
+	svn --non-interactive --trust-server-cert "$orig_svncmd" --config-dir "$svnconf" "$@"
 }
 
 maybe_start_httpd () {
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/"
 			svnrepo="$svnrepo$LIB_HTTPD_SVN"
 			export LIB_HTTPD_SVN LIB_HTTPD_SVNPATH
 		fi
@@ -280,11 +280,11 @@ expect_askpass() {
 		none)
 			;;
 		pass)
-			echo "askpass: Password for 'http://$2@$dest': "
+			echo "askpass: Password for '$HTTPD_PROTO://$2@$dest': "
 			;;
 		both)
-			echo "askpass: Username for 'http://$dest': "
-			echo "askpass: Password for 'http://$2@$dest': "
+			echo "askpass: Username for '$HTTPD_PROTO://$dest': "
+			echo "askpass: Password for '$HTTPD_PROTO://$2@$dest': "
 			;;
 		*)
 			false
diff --git i/t/t5411-proc-receive-hook.sh w/t/t5411-proc-receive-hook.sh
index 92cf52c6d4..c85f4668c4 100755
--- i/t/t5411-proc-receive-hook.sh
+++ w/t/t5411-proc-receive-hook.sh
@@ -63,7 +63,7 @@ run_proc_receive_hook_test() {
 	case $1 in
 	http)
 		PROTOCOL="HTTP protocol"
-		URL_PREFIX="http://.*"
+		URL_PREFIX="$HTTPD_PROTO://.*"
 		;;
 	local)
 		PROTOCOL="builtin protocol"
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
 	 ! [remote rejected] dev2 -> dev2 (hook declined)
-	error: failed to push some refs to '\''http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git'\''
+	error: failed to push some refs to '\'$HTTPD_PROTO'://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git'\''
 	EOF
 '
 
diff --git i/t/t5548-push-porcelain.sh w/t/t5548-push-porcelain.sh
index 6282728eaf..18e47c8a30 100755
--- i/t/t5548-push-porcelain.sh
+++ w/t/t5548-push-porcelain.sh
@@ -84,7 +84,7 @@ run_git_push_porcelain_output_test() {
 	case $1 in
 	http)
 		PROTOCOL="HTTP protocol"
-		URL_PREFIX="http://.*"
+		URL_PREFIX="$HTTPD_PROTO://.*"
 		;;
 	file)
 		PROTOCOL="builtin protocol"
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
 '
 
@@ -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
 '
 
diff --git i/t/t5561-http-backend.sh w/t/t5561-http-backend.sh
index 9c57d84315..b3f5759972 100755
--- i/t/t5561-http-backend.sh
+++ w/t/t5561-http-backend.sh
@@ -15,7 +15,7 @@ fi
 start_httpd
 
 GET() {
-	curl --include "$HTTPD_URL/$SMART/repo.git/$1" >out &&
+	curl --include --insecure "$HTTPD_URL/$SMART/repo.git/$1" >out &&
 	tr '\015' Q <out |
 	sed '
 		s/Q$//
@@ -26,7 +26,7 @@ GET() {
 }
 
 POST() {
-	curl --include --data "$2" \
+	curl --include --insecure --data "$2" \
 	--header "Content-Type: application/x-$1-request" \
 	"$HTTPD_URL/smart/repo.git/$1" >out &&
 	tr '\015' Q <out |
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" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
 		test_commit_bulk --id=s 33 &&
@@ -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" &&
 	git -C "$LOCAL_PRISTINE" config protocol.version 2
 '
 
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
@@ -14,7 +14,7 @@ test_expect_success 'create git-accessible repo' '
 	git -C "$bare" config http.receivepack true
 '
 
-test_proto "smart http" http "$HTTPD_URL/smart/repo.git"
+test_proto "smart http" $HTTPD_PROTO "$HTTPD_URL/smart/repo.git"
 
 test_expect_success 'http(s) transport respects GIT_ALLOW_PROTOCOL' '
 	test_must_fail env GIT_ALLOW_PROTOCOL=http:https \
@@ -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
 '
 
-- 8< --

-- 
Todd

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

[Jeff King]

On Wed, Feb 01, 2023 at 11:27:43PM -0500, Todd Zullinger wrote:

> > 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:

> 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.

> 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.

> 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.

> @@ -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.

> 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.

> @@ -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.

> 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