[PATCH 0/3] t: improve compatibility with NixOS

[PATCH 0/3] t: improve compatibility with NixOS

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

Hi,

this patch series improves compatibility of our test suite with NixOS.

NixOS is somewhat special compared to more conventional Linux distros
because it doesn't follow the Filesystem Hierarchy Specification for
most of the part. Instead, packages are installed into the Nix store
in `/nix` with hashed paths that change frequently when upgrading the
system to a newer generation. Consequentially, paths cannot be hardcoded
and must instead be computed at runtime.

We have two such issues in our test harness right now:

    - t/lib-httpd searches Apache httpd and its modules directory in a
      list of hardcoded paths.

    - t9164 doesn't propagate PATH to a script and thus cannot find the
      basename(1) utility.

Both of these issues are fixed in this patch series. In addition, this
patch series fixes an upcoming issue in httpd's passwd files caused by
the deprecation of the crypt(3) function.

Patrick

Patrick Steinhardt (3):
  t/lib-httpd: dynamically detect httpd and modules path
  t/lib-httpd: stop using legacy crypt(3) for authentication
  t9164: fix inability to find basename(1) in hooks

 t/lib-httpd.sh                        | 51 ++++++++++++++++++---------
 t/lib-httpd/passwd                    |  2 +-
 t/lib-httpd/proxy-passwd              |  2 +-
 t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++--
 4 files changed, 45 insertions(+), 22 deletions(-)

-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]

In order to set up the Apache httpd server, we need to locate both the
httpd binary and its default module path. This is done with a hardcoded
list of locations that we scan. While this works okayish with distros
that more-or-less follow the Filesystem Hierarchy Standard, it falls
apart on others like NixOS that don't.

While it is possible to specify these paths via `LIB_HTTPD_PATH` and
`LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
to figure out how to set those up. And in fact we can do better by
dynamically detecting both httpd and its module path at runtime:

    - The httpd binary can be located via PATH.

    - The module directory can (in many cases) be derived via the
      `HTTPD_ROOT` compile-time variable.

Refactor the code to do so. If runtime detection of the paths fails we
continue to fall back to the hardcoded list of paths.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd.sh | 51 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 2fb1b2ae561..06038f72607 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,25 +55,42 @@ fi
 
 HTTPD_PARA=""
 
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
-do
-	if test -x "$DEFAULT_HTTPD_PATH"
-	then
-		break
-	fi
-done
-
-for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
-				 '/usr/lib/apache2/modules' \
-				 '/usr/lib64/httpd/modules' \
-				 '/usr/lib/httpd/modules' \
-				 '/usr/libexec/httpd'
-do
-	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+DEFAULT_HTTPD_PATH="$(command -v httpd)"
+if test -z "$DEFAULT_HTTPD_PATH" -o ! -x "$DEFAULT_HTTPD_PATH"
+then
+	for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+	do
+		if test -x "$DEFAULT_HTTPD_PATH"
+		then
+			break
+		fi
+	done
+fi
+
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+	HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+	if test -n "$HTTPD_ROOT" -a -d "$HTTPD_ROOT/modules"
 	then
-		break
+		DEFAULT_HTTPD_MODULE_PATH="$HTTPD_ROOT/modules"
 	fi
-done
+	unset HTTPD_ROOT
+fi
+
+if test -z "$DEFAULT_HTTPD_MODULE_PATH" -o ! -d "$DEFAULT_HTTPD_MODULE_PATH"
+then
+	for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
+					 '/usr/lib/apache2/modules' \
+					 '/usr/lib64/httpd/modules' \
+					 '/usr/lib/httpd/modules' \
+					 '/usr/libexec/httpd'
+	do
+		if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+		then
+			break
+		fi
+	done
+fi
 
 case $(uname) in
 	Darwin)
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> In order to set up the Apache httpd server, we need to locate both the
> httpd binary and its default module path. This is done with a hardcoded
> list of locations that we scan. While this works okayish with distros
> that more-or-less follow the Filesystem Hierarchy Standard, it falls
> apart on others like NixOS that don't.
>
> While it is possible to specify these paths via `LIB_HTTPD_PATH` and
> `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
> to figure out how to set those up. And in fact we can do better by
> dynamically detecting both httpd and its module path at runtime:
>
>     - The httpd binary can be located via PATH.
>
>     - The module directory can (in many cases) be derived via the
>       `HTTPD_ROOT` compile-time variable.
>
> Refactor the code to do so. If runtime detection of the paths fails we
> continue to fall back to the hardcoded list of paths.

Hmph.

I do not think we would want to punish the distros that follow the
FHS that was created explicitly to help developers by standardizing
locations of various things, with an approach this patch takes that
throws everthing with bathwater and rely on $PATH first.

Would it be sufficient to please NixOS if we simply append $(command
-v apache) or whatever after the well known candidate locations?

I know "command -v" is in POSIX, and on both bash and dash (the two
shells most distros use), it works as this patch expects, but its
portability is also a bit worrysome, especially because the whole
point of this patch is to support a platform that is, eh, on the
fringe.

So, I dunno.

Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

On Wed, Nov 08, 2023 at 04:59:46PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > In order to set up the Apache httpd server, we need to locate both the
> > httpd binary and its default module path. This is done with a hardcoded
> > list of locations that we scan. While this works okayish with distros
> > that more-or-less follow the Filesystem Hierarchy Standard, it falls
> > apart on others like NixOS that don't.
> >
> > While it is possible to specify these paths via `LIB_HTTPD_PATH` and
> > `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
> > to figure out how to set those up. And in fact we can do better by
> > dynamically detecting both httpd and its module path at runtime:
> >
> >     - The httpd binary can be located via PATH.
> >
> >     - The module directory can (in many cases) be derived via the
> >       `HTTPD_ROOT` compile-time variable.
> >
> > Refactor the code to do so. If runtime detection of the paths fails we
> > continue to fall back to the hardcoded list of paths.
> 
> Hmph.
> 
> I do not think we would want to punish the distros that follow the
> FHS that was created explicitly to help developers by standardizing
> locations of various things, with an approach this patch takes that
> throws everthing with bathwater and rely on $PATH first.
> 
> Would it be sufficient to please NixOS if we simply append $(command
> -v apache) or whatever after the well known candidate locations?

I was a bit torn myself when writing this. I can also see a potential
future where we would drop the hardcoded list of locations altogether in
favor of always using PATH. After all we already rely on PATH to resolve
other tools as well, so why should httpd be special there?

But in the end I opted to use the more conservative approach of using
both PATH and the static list as I didn't want to break other distros. I
don't mind to make this even more conservative and resolve via PATH as a
last resort, only.

Patrick

> I know "command -v" is in POSIX, and on both bash and dash (the two
> shells most distros use), it works as this patch expects, but its
> portability is also a bit worrysome, especially because the whole
> point of this patch is to support a platform that is, eh, on the
> fringe.
> 
> So, I dunno.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Jeff King]

On Wed, Nov 08, 2023 at 11:42:11AM +0100, Patrick Steinhardt wrote:

> I was a bit torn myself when writing this. I can also see a potential
> future where we would drop the hardcoded list of locations altogether in
> favor of always using PATH. After all we already rely on PATH to resolve
> other tools as well, so why should httpd be special there?

httpd/apache2 is often in sbin, which is not as commonly found in the
PATH of regular users. E.g., this is the case on Debian (it is
/sbin/apache2, and a newly created user will not have that in their
PATH).

-Peff

[PATCH 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

When setting up httpd for our tests, we also install a passwd and
proxy-passwd file that contain the test user's credentials. These
credentials currently use crypt(3) as the password encryption schema.

This schema can be considered deprecated nowadays as it is not safe
anymore. Quoting Apache httpd's documentation [1]:

> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.

This is starting to cause issues in modern Linux distributions. glibc
has deprecated its libcrypt library that used to provide crypt(3) in
favor of the libxcrypt library. This newer replacement provides a
compile time switch to disable insecure password encryption schemata,
which causes crypt(3) to always return `EINVAL`. The end result is that
httpd tests that exercise authentication will fail on distros that use
libxcrypt without these insecure encryption schematas.

Regenerate the passwd files to instead use the default password
encryption schema, which is md5. While it feels kind of funny that an
MD5-based encryption schema should be more secure than anything else, it
is the current default and supported by all platforms. Furthermore, it
really doesn't matter all that much given that these files are only used
for testing purposes anyway.

[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd/passwd       | 2 +-
 t/lib-httpd/proxy-passwd | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When setting up httpd for our tests, we also install a passwd and
> proxy-passwd file that contain the test user's credentials. These
> credentials currently use crypt(3) as the password encryption schema.
>
> This schema can be considered deprecated nowadays as it is not safe
> anymore. Quoting Apache httpd's documentation [1]:
>
>> Unix only. Uses the traditional Unix crypt(3) function with a
>> randomly-generated 32-bit salt (only 12 bits used) and the first 8
>> characters of the password. Insecure.
>
> This is starting to cause issues in modern Linux distributions. glibc
> has deprecated its libcrypt library that used to provide crypt(3) in
> favor of the libxcrypt library. This newer replacement provides a
> compile time switch to disable insecure password encryption schemata,
> which causes crypt(3) to always return `EINVAL`. The end result is that
> httpd tests that exercise authentication will fail on distros that use
> libxcrypt without these insecure encryption schematas.
>
> Regenerate the passwd files to instead use the default password
> encryption schema, which is md5. While it feels kind of funny that an
> MD5-based encryption schema should be more secure than anything else, it
> is the current default and supported by all platforms. Furthermore, it
> really doesn't matter all that much given that these files are only used
> for testing purposes anyway.

This step makes quite a lot of sense, as we are changing this not at
all for security but for portability ;-)

>
> [1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/lib-httpd/passwd       | 2 +-
>  t/lib-httpd/proxy-passwd | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
> index 99a34d64874..d9c122f3482 100644
> --- a/t/lib-httpd/passwd
> +++ b/t/lib-httpd/passwd
> @@ -1 +1 @@
> -user@host:xb4E8pqD81KQs
> +user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
> diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
> index 77c25138e07..2ad7705d9a3 100644
> --- a/t/lib-httpd/proxy-passwd
> +++ b/t/lib-httpd/proxy-passwd
> @@ -1 +1 @@
> -proxuser:2x7tAukjAED5M
> +proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.

[PATCH 3/3] t9164: fix inability to find basename(1) in hooks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

The post-commit hook in t9164 is executed with a default environment.
To work around this issue we already write the current `PATH` value into
the hook script. But this is done at a point where we already tried to
execute non-built-in commands, namely basename(1). While this seems to
work alright on most platforms, it fails on NixOS.

Set the `PATH` variable earlier to fix this issue. Note that we do this
via two separate heredocs. This is done because in the first one we do
want to expand variables, whereas in the second one we don't.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..3b64022dc57 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -47,9 +47,15 @@ setup_hook()
 	echo "cnt=$skip_revs" > "$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
 	hook="$rawsvnrepo/hooks/$hook_type"
-	cat > "$hook" <<- 'EOF1'
+	cat >"$hook" <<-EOF
 		#!/bin/sh
 		set -e
+
+		PATH=\"$PATH\"
+		export PATH
+	EOF
+
+	cat >>"$hook" <<-'EOF'
 		cd "$1/.."  # "$1" is repository location
 		exec >> svn-hook.log 2>&1
 		hook="$(basename "$0")"
@@ -59,11 +65,11 @@ setup_hook()
 		cnt="$(($cnt - 1))"
 		echo "cnt=$cnt" > ./$hook-counter
 		[ "$cnt" = "0" ] || exit 0
-EOF1
+	EOF
+
 	if [ "$hook_type" = "pre-commit" ]; then
 		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
 	else
-		echo "PATH=\"$PATH\"; export PATH" >>"$hook"
 		echo "svnconf=\"$svnconf\"" >>"$hook"
 		cat >>"$hook" <<- 'EOF2'
 			cd work-auto-commits.svn
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 0/3] t: improve compatibility with NixOS

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4332 bytes --]

Hi,

this is the second version of my patch series to improve compatibility
of our tests with NixOS.

There's only a single change compared to v1. As suggested by Junio, we
now use the executable discovered via PATH and the modules detected via
`HTTPD_ROOT` as fallback values if none of our hardcoded paths were
detected. This should further ensure that the refactoring doesn't cause
any issues on platforms that follow the FHS more closely than NixOS
does. It also has the benefit that the change is now a lot more straight
forward.

Patrick

Patrick Steinhardt (3):
  t/lib-httpd: dynamically detect httpd and modules path
  t/lib-httpd: stop using legacy crypt(3) for authentication
  t9164: fix inability to find basename(1) in hooks

 t/lib-httpd.sh                        | 10 ++++++++--
 t/lib-httpd/passwd                    |  2 +-
 t/lib-httpd/proxy-passwd              |  2 +-
 t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
 4 files changed, 19 insertions(+), 7 deletions(-)

Range-diff against v1:
1:  ac059db8fed ! 1:  cee8fbebf84 t/lib-httpd: dynamically detect httpd and modules path
    @@ Commit message
             - The module directory can (in many cases) be derived via the
               `HTTPD_ROOT` compile-time variable.
     
    -    Refactor the code to do so. If runtime detection of the paths fails we
    -    continue to fall back to the hardcoded list of paths.
    +    Amend the code to do so. The new runtime-detected paths will only be
    +    used in case none of the hardcoded paths are usable.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ t/lib-httpd.sh: fi
      HTTPD_PARA=""
      
     -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
    --do
    --	if test -x "$DEFAULT_HTTPD_PATH"
    --	then
    --		break
    --	fi
    --done
    --
    --for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
    --				 '/usr/lib/apache2/modules' \
    --				 '/usr/lib64/httpd/modules' \
    --				 '/usr/lib/httpd/modules' \
    --				 '/usr/libexec/httpd'
    --do
    --	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    -+DEFAULT_HTTPD_PATH="$(command -v httpd)"
    -+if test -z "$DEFAULT_HTTPD_PATH" -o ! -x "$DEFAULT_HTTPD_PATH"
    -+then
    -+	for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
    -+	do
    -+		if test -x "$DEFAULT_HTTPD_PATH"
    -+		then
    -+			break
    -+		fi
    -+	done
    -+fi
    -+
    -+if test -x "$DEFAULT_HTTPD_PATH"
    -+then
    -+	HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
    -+	if test -n "$HTTPD_ROOT" -a -d "$HTTPD_ROOT/modules"
    ++for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
    + do
    + 	if test -x "$DEFAULT_HTTPD_PATH"
      	then
    --		break
    -+		DEFAULT_HTTPD_MODULE_PATH="$HTTPD_ROOT/modules"
    +@@ t/lib-httpd.sh: do
      	fi
    --done
    -+	unset HTTPD_ROOT
    -+fi
    -+
    -+if test -z "$DEFAULT_HTTPD_MODULE_PATH" -o ! -d "$DEFAULT_HTTPD_MODULE_PATH"
    + done
    + 
    ++if test -x "$DEFAULT_HTTPD_PATH"
     +then
    -+	for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
    -+					 '/usr/lib/apache2/modules' \
    -+					 '/usr/lib64/httpd/modules' \
    -+					 '/usr/lib/httpd/modules' \
    -+					 '/usr/libexec/httpd'
    -+	do
    -+		if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    -+		then
    -+			break
    -+		fi
    -+	done
    ++	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
     +fi
    - 
    - case $(uname) in
    - 	Darwin)
    ++
    + for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
    + 				 '/usr/lib/apache2/modules' \
    + 				 '/usr/lib64/httpd/modules' \
    + 				 '/usr/lib/httpd/modules' \
    +-				 '/usr/libexec/httpd'
    ++				 '/usr/libexec/httpd' \
    ++				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
    + do
    + 	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    + 	then
2:  23835763002 = 2:  f4c6c754f1e t/lib-httpd: stop using legacy crypt(3) for authentication
3:  b50e625f967 = 3:  361f1bd9c88 t9164: fix inability to find basename(1) in hooks

base-commit: 98009afd24e2304bf923a64750340423473809ff
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

In order to set up the Apache httpd server, we need to locate both the
httpd binary and its default module path. This is done with a hardcoded
list of locations that we scan. While this works okayish with distros
that more-or-less follow the Filesystem Hierarchy Standard, it falls
apart on others like NixOS that don't.

While it is possible to specify these paths via `LIB_HTTPD_PATH` and
`LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
to figure out how to set those up. And in fact we can do better by
dynamically detecting both httpd and its module path at runtime:

    - The httpd binary can be located via PATH.

    - The module directory can (in many cases) be derived via the
      `HTTPD_ROOT` compile-time variable.

Amend the code to do so. The new runtime-detected paths will only be
used in case none of the hardcoded paths are usable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 5fe3c8ab69d..26c1632ea9a 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,7 +55,7 @@ fi
 
 HTTPD_PARA=""
 
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
 do
 	if test -x "$DEFAULT_HTTPD_PATH"
 	then
@@ -63,11 +63,17 @@ do
 	fi
 done
 
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+fi
+
 for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
 				 '/usr/lib/apache2/modules' \
 				 '/usr/lib64/httpd/modules' \
 				 '/usr/lib/httpd/modules' \
-				 '/usr/libexec/httpd'
+				 '/usr/libexec/httpd' \
+				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
 do
 	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
 	then
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Jeff King]

On Wed, Nov 08, 2023 at 03:57:19PM +0100, Patrick Steinhardt wrote:

> While it is possible to specify these paths via `LIB_HTTPD_PATH` and
> `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
> to figure out how to set those up. And in fact we can do better by
> dynamically detecting both httpd and its module path at runtime:
> 
>     - The httpd binary can be located via PATH.
> 
>     - The module directory can (in many cases) be derived via the
>       `HTTPD_ROOT` compile-time variable.
> 
> Amend the code to do so. The new runtime-detected paths will only be
> used in case none of the hardcoded paths are usable.

If these improve detection on your platform, I think that is a good
thing and they are worth doing. But as a generic mechanism, I have two
comments:

> -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
>  do
>  	if test -x "$DEFAULT_HTTPD_PATH"
>  	then

The binary goes under the name "httpd", but also "apache2". But the PATH
search only looks for "httpd". Should we check "command -v apache2" as
well?

This also means we may run "test -x" on an empty string, but that is
probably OK in practice (we could guard it with "test -n", though).

> +if test -x "$DEFAULT_HTTPD_PATH"
> +then
> +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> +fi

I was really pleased to see this and hoped it could replace the
kitchen-sink list of paths in the hunk below. But sadly I think it
depends on having a configured apache setup. On my Debian system, for
example, I have the "apache2-bin" package installed but not "apache"
(because only the latter actually sets up a system apache daemon, which
I don't want). And thus there is no config:

  $ /usr/sbin/apache2 -D HTTPD_ROOT
  apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory

So without a system config file to act as a template for our custom
config, I think we are stuck with guessing where the installer might
have put them.

-Peff

Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 08, 2023 at 03:57:19PM +0100, Patrick Steinhardt wrote:
>
>> While it is possible to specify these paths via `LIB_HTTPD_PATH` and
>> `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
>> to figure out how to set those up. And in fact we can do better by
>> dynamically detecting both httpd and its module path at runtime:
>> 
>>     - The httpd binary can be located via PATH.
>> 
>>     - The module directory can (in many cases) be derived via the
>>       `HTTPD_ROOT` compile-time variable.
>> 
>> Amend the code to do so. The new runtime-detected paths will only be
>> used in case none of the hardcoded paths are usable.
>
> If these improve detection on your platform, I think that is a good
> thing and they are worth doing. But as a generic mechanism, I have two
> comments:
>
>> -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
>> +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
>>  do
>>  	if test -x "$DEFAULT_HTTPD_PATH"
>>  	then
>
> The binary goes under the name "httpd", but also "apache2". But the PATH
> search only looks for "httpd". Should we check "command -v apache2" as
> well?

Interesting.  If $() were on the right hand side of an assignment,
the command that fails may also something to watch for, but I think
$? from $(command -v no-such-httpd) would be discarded, so it would
be fine in this case.  A trivia I found out today: dash exits with 127
and bash exits with 1 when doing 

    $shell -c 'command -v no-such-httpd'

> This also means we may run "test -x" on an empty string, but that is
> probably OK in practice (we could guard it with "test -n", though).

Good thinking.  We are not worried about exotic or misbehaving
shells in this thread, so hopefully shell built-in 'test' would say
"no" when asked if "" is executable (in other words, "" is not
mistaken as ".").  But an explicit "test -n" would save the readers
by removing the worry.

> ... So without a system config file to act as a template for our custom
> config, I think we are stuck with guessing where the installer might
> have put them.

Right.  Thanks for a careful analysis.

Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2646 bytes --]

On Wed, Nov 08, 2023 at 11:54:26AM -0500, Jeff King wrote:
> On Wed, Nov 08, 2023 at 03:57:19PM +0100, Patrick Steinhardt wrote:
> 
> > While it is possible to specify these paths via `LIB_HTTPD_PATH` and
> > `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
> > to figure out how to set those up. And in fact we can do better by
> > dynamically detecting both httpd and its module path at runtime:
> > 
> >     - The httpd binary can be located via PATH.
> > 
> >     - The module directory can (in many cases) be derived via the
> >       `HTTPD_ROOT` compile-time variable.
> > 
> > Amend the code to do so. The new runtime-detected paths will only be
> > used in case none of the hardcoded paths are usable.
> 
> If these improve detection on your platform, I think that is a good
> thing and they are worth doing. But as a generic mechanism, I have two
> comments:
> 
> > -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> > +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
> >  do
> >  	if test -x "$DEFAULT_HTTPD_PATH"
> >  	then
> 
> The binary goes under the name "httpd", but also "apache2". But the PATH
> search only looks for "httpd". Should we check "command -v apache2" as
> well?

Doesn't hurt to do it.

> This also means we may run "test -x" on an empty string, but that is
> probably OK in practice (we could guard it with "test -n", though).

Fair, will do.

> > +if test -x "$DEFAULT_HTTPD_PATH"
> > +then
> > +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> > +fi
> 
> I was really pleased to see this and hoped it could replace the
> kitchen-sink list of paths in the hunk below. But sadly I think it
> depends on having a configured apache setup. On my Debian system, for
> example, I have the "apache2-bin" package installed but not "apache"
> (because only the latter actually sets up a system apache daemon, which
> I don't want). And thus there is no config:
> 
>   $ /usr/sbin/apache2 -D HTTPD_ROOT
>   apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
> 
> So without a system config file to act as a template for our custom
> config, I think we are stuck with guessing where the installer might
> have put them.

Indeed. Also, paths like `/usr/libexec/apache2` wouldn't be possible to
be discovered via `${HTTPD_ROOT}/modules` anyway. It's kind of a shame
that there is no way (or at least no way I know of) to portably discover
the default modules directory used by httpd.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

When setting up httpd for our tests, we also install a passwd and
proxy-passwd file that contain the test user's credentials. These
credentials currently use crypt(3) as the password encryption schema.

This schema can be considered deprecated nowadays as it is not safe
anymore. Quoting Apache httpd's documentation [1]:

> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.

This is starting to cause issues in modern Linux distributions. glibc
has deprecated its libcrypt library that used to provide crypt(3) in
favor of the libxcrypt library. This newer replacement provides a
compile time switch to disable insecure password encryption schemata,
which causes crypt(3) to always return `EINVAL`. The end result is that
httpd tests that exercise authentication will fail on distros that use
libxcrypt without these insecure encryption schematas.

Regenerate the passwd files to instead use the default password
encryption schema, which is md5. While it feels kind of funny that an
MD5-based encryption schema should be more secure than anything else, it
is the current default and supported by all platforms. Furthermore, it
really doesn't matter all that much given that these files are only used
for testing purposes anyway.

[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd/passwd       | 2 +-
 t/lib-httpd/proxy-passwd | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

[Jeff King]

On Wed, Nov 08, 2023 at 03:57:23PM +0100, Patrick Steinhardt wrote:

> Regenerate the passwd files to instead use the default password
> encryption schema, which is md5. While it feels kind of funny that an
> MD5-based encryption schema should be more secure than anything else, it
> is the current default and supported by all platforms. Furthermore, it
> really doesn't matter all that much given that these files are only used
> for testing purposes anyway.

Thanks for doing this. I died inside a little while adding the
proxy-passwd one recently in 29ae2c9e74 (add basic http proxy tests,
2023-02-16). There I mused about moving to bcrypt in a separate patch,
which I think is probably the least-bad option from a security
perspective. But I agree that md5 is more likely to be available
everywhere, and we certainly don't care about security here.

-Peff

[PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

The post-commit hook in t9164 is executed with a default environment.
To work around this issue we already write the current `PATH` value into
the hook script. But this is done at a point where we already tried to
execute non-built-in commands, namely basename(1). While this seems to
work alright on most platforms, it fails on NixOS.

Set the `PATH` variable earlier to fix this issue. Note that we do this
via two separate heredocs. This is done because in the first one we do
want to expand variables, whereas in the second one we don't.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..3b64022dc57 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -47,9 +47,15 @@ setup_hook()
 	echo "cnt=$skip_revs" > "$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
 	hook="$rawsvnrepo/hooks/$hook_type"
-	cat > "$hook" <<- 'EOF1'
+	cat >"$hook" <<-EOF
 		#!/bin/sh
 		set -e
+
+		PATH=\"$PATH\"
+		export PATH
+	EOF
+
+	cat >>"$hook" <<-'EOF'
 		cd "$1/.."  # "$1" is repository location
 		exec >> svn-hook.log 2>&1
 		hook="$(basename "$0")"
@@ -59,11 +65,11 @@ setup_hook()
 		cnt="$(($cnt - 1))"
 		echo "cnt=$cnt" > ./$hook-counter
 		[ "$cnt" = "0" ] || exit 0
-EOF1
+	EOF
+
 	if [ "$hook_type" = "pre-commit" ]; then
 		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
 	else
-		echo "PATH=\"$PATH\"; export PATH" >>"$hook"
 		echo "svnconf=\"$svnconf\"" >>"$hook"
 		cat >>"$hook" <<- 'EOF2'
 			cd work-auto-commits.svn
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

[Jeff King]

On Wed, Nov 08, 2023 at 03:57:27PM +0100, Patrick Steinhardt wrote:

> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index c8e6c0733f4..3b64022dc57 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -47,9 +47,15 @@ setup_hook()
>  	echo "cnt=$skip_revs" > "$hook_type-counter"
>  	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
>  	hook="$rawsvnrepo/hooks/$hook_type"
> -	cat > "$hook" <<- 'EOF1'
> +	cat >"$hook" <<-EOF
>  		#!/bin/sh
>  		set -e
> +
> +		PATH=\"$PATH\"
> +		export PATH
> +	EOF

The quoting here is weird. The original escaped the double-quotes
because it was echo-ing and interpolated string:

> -		echo "PATH=\"$PATH\"; export PATH" >>"$hook"

But the here-doc interpolation does not treat double-quotes as special,
and you end up with actual double-quote characters at the beginning and
end of your $PATH variable, wrecking those entries (but I guess it works
some of the time depending on whether those parts of your $PATH are
important).

It also interpolates PATH while making the here-doc. That's what you
want to convince the hook's inner shell to use the same PATH as the
outer shell, but it also means that the inner shell will parse and
evaluate it. I don't think anyone is going to inject '"; rm -rf /' into
their own test runs, but it does mean we'd do the wrong thing if their
$PATH contains double-quotes or backslashes.

Certainly the original suffered from the same issue, so it may not be
worth worrying about. But the more robust way to do it is:

  # export ORIG_PATH, which presumably is not cleared the same way PATH
  # is, so that the hook can access it
  ORIG_PATH=$PATH &&
  export ORIG_PATH &&

  cat >"$hook" <<EOF
  # pull the original path from the caller
  PATH=$ORIG_PATH
  export PATH

  ...do other stuff...
  EOF

That's assuming that environment variables make it intact to the hook at
all (it is not clear to me why the original $PATH doesn't). If they
don't, you'd probably have to stash it in a file like:

  echo "$PATH" >orig-path
  cat >"$hook" <<EOF
  PATH="$(cat orig-path)"
  export PATH
  EOF

-Peff

Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... But the more robust way to do it is:
>
>   # export ORIG_PATH, which presumably is not cleared the same way PATH
>   # is, so that the hook can access it
>   ORIG_PATH=$PATH &&
>   export ORIG_PATH &&
>
>   cat >"$hook" <<EOF
>   # pull the original path from the caller
>   PATH=$ORIG_PATH
>   export PATH
>
>   ...do other stuff...
>   EOF
>
> That's assuming that environment variables make it intact to the hook at
> all (it is not clear to me why the original $PATH doesn't).

Yeah, the parenthetical comment points at the crux of the issue.  I
can tell from the patch what issue the platform throws at us we are
trying to work around, but it is frustrating not to know why the
platform does such unpleasant things in the first place.

Thanks.

Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

On Thu, Nov 09, 2023 at 02:43:02AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > ... But the more robust way to do it is:
> >
> >   # export ORIG_PATH, which presumably is not cleared the same way PATH
> >   # is, so that the hook can access it
> >   ORIG_PATH=$PATH &&
> >   export ORIG_PATH &&
> >
> >   cat >"$hook" <<EOF
> >   # pull the original path from the caller
> >   PATH=$ORIG_PATH
> >   export PATH
> >
> >   ...do other stuff...
> >   EOF
> >
> > That's assuming that environment variables make it intact to the hook at
> > all (it is not clear to me why the original $PATH doesn't).
> 
> Yeah, the parenthetical comment points at the crux of the issue.  I
> can tell from the patch what issue the platform throws at us we are
> trying to work around, but it is frustrating not to know why the
> platform does such unpleasant things in the first place.
> 
> Thanks.

Indeed, I should've described this better in the commit message. Quoting
[1]:

> By default, Subversion executes hook scripts with an empty
> environment—that is, no environment variables are set at all, not even
> $PATH (or %PATH%, under Windows). Because of this, many administrators
> are baffled when their hook program runs fine by hand, but doesn't
> work when invoked by Subversion. Administrators have historically
> worked around this problem by manually setting all the environment
> variables their hook scripts need in the scripts themselves.

So it's not an issue of the environment, but rather an implementation
detail of how Subversion hooks work. It's surprising that this does not
fail on other platforms -- maybe the shell has a default PATH there that
allow us to locate basename(1)? I dunno.

Will add to the commit message.

Patrick

[1]: https://svnbook.red-bean.com/en/1.8/svn.reposadmin.create.html#svn.reposadmin.create.hooks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Thu, Nov 09, 2023 at 07:30:01AM +0100, Patrick Steinhardt wrote:
> On Thu, Nov 09, 2023 at 02:43:02AM +0900, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
[snip]
> > By default, Subversion executes hook scripts with an empty
> > environment—that is, no environment variables are set at all, not even
> > $PATH (or %PATH%, under Windows). Because of this, many administrators
> > are baffled when their hook program runs fine by hand, but doesn't
> > work when invoked by Subversion. Administrators have historically
> > worked around this problem by manually setting all the environment
> > variables their hook scripts need in the scripts themselves.
> 
> So it's not an issue of the environment, but rather an implementation
> detail of how Subversion hooks work. It's surprising that this does not
> fail on other platforms -- maybe the shell has a default PATH there that
> allow us to locate basename(1)? I dunno.

Yup, that's in fact it. Bash falls back to a default PATH that can be
queried via getconf(1p), and this works alright on most platforms. Not
on NixOS though.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 0/3] t: improve compatibility with NixOS

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5864 bytes --]

Hi,

this is the third version of my patch series to improve compatibility of
our tests with NixOS.

Changes compared to v2:

    - Patch 1: We now check for both httpd and apache2 binaries via
      PATH.

    - Patch 1: We're a bit more defensive and will check whether
      variables are empty before passing them to either `test -d` or
      `test -x`.

    - Patch 3: Clarified why PATH is unset.

    - Patch 3: Instead of writing PATH into the hook we now write it
      into the `hook-env` configuration file read by Subversion.

Patrick

Patrick Steinhardt (3):
  t/lib-httpd: dynamically detect httpd and modules path
  t/lib-httpd: stop using legacy crypt(3) for authentication
  t9164: fix inability to find basename(1) in Subversion hooks

 t/lib-httpd.sh                        | 17 +++++++++++++----
 t/lib-httpd/passwd                    |  2 +-
 t/lib-httpd/proxy-passwd              |  2 +-
 t/t9164-git-svn-dcommit-concurrent.sh |  9 ++++++++-
 4 files changed, 23 insertions(+), 7 deletions(-)

Range-diff against v2:
1:  cee8fbebf84 ! 1:  e4c75c492dd t/lib-httpd: dynamically detect httpd and modules path
    @@ t/lib-httpd.sh: fi
      HTTPD_PARA=""
      
     -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
    -+for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
    ++for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
    ++			  '/usr/sbin/apache2' \
    ++			  "$(command -v httpd)" \
    ++			  "$(command -v apache2)"
      do
    - 	if test -x "$DEFAULT_HTTPD_PATH"
    +-	if test -x "$DEFAULT_HTTPD_PATH"
    ++	if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
      	then
    -@@ t/lib-httpd.sh: do
    + 		break
      	fi
      done
      
    @@ t/lib-httpd.sh: do
     +				 '/usr/libexec/httpd' \
     +				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
      do
    - 	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    +-	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    ++	if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
      	then
    + 		break
    + 	fi
2:  f4c6c754f1e = 2:  3dce76da477 t/lib-httpd: stop using legacy crypt(3) for authentication
3:  361f1bd9c88 ! 3:  6891e254155 t9164: fix inability to find basename(1) in hooks
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    t9164: fix inability to find basename(1) in hooks
    +    t9164: fix inability to find basename(1) in Subversion hooks
     
    -    The post-commit hook in t9164 is executed with a default environment.
    -    To work around this issue we already write the current `PATH` value into
    -    the hook script. But this is done at a point where we already tried to
    -    execute non-built-in commands, namely basename(1). While this seems to
    -    work alright on most platforms, it fails on NixOS.
    +    Hooks executed by Subversion are spawned with an empty environment. By
    +    default, not even variables like PATH will be propagated to them. In
    +    order to ensure that we're still able to find required executables, we
    +    thus write the current PATH variable into the hook script itself and
    +    then re-export it in t9164.
     
    -    Set the `PATH` variable earlier to fix this issue. Note that we do this
    -    via two separate heredocs. This is done because in the first one we do
    -    want to expand variables, whereas in the second one we don't.
    +    This happens too late in the script though, as we already tried to
    +    execute the basename(1) utility before exporting the PATH variable. This
    +    tends to work on most platforms as the fallback value of PATH for Bash
    +    (see `getconf PATH`) is likely to contain this binary. But on more
    +    exotic platforms like NixOS this is not the case, and thus the test
    +    fails.
    +
    +    While we could work around this issue by simply setting PATH earlier, it
    +    feels fragile to inject a user-controlled value into the script and have
    +    the shell interpret it. Instead, we can refactor the hook setup to write
    +    a `hooks-env` file that configures PATH for us. Like this, Subversion
    +    will know to set up the environment as expected for all hooks.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t9164-git-svn-dcommit-concurrent.sh ##
     @@ t/t9164-git-svn-dcommit-concurrent.sh: setup_hook()
    + 			"passed to setup_hook" >&2 ; return 1; }
      	echo "cnt=$skip_revs" > "$hook_type-counter"
      	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
    - 	hook="$rawsvnrepo/hooks/$hook_type"
    --	cat > "$hook" <<- 'EOF1'
    -+	cat >"$hook" <<-EOF
    - 		#!/bin/sh
    - 		set -e
    -+
    -+		PATH=\"$PATH\"
    -+		export PATH
    -+	EOF
     +
    -+	cat >>"$hook" <<-'EOF'
    - 		cd "$1/.."  # "$1" is repository location
    - 		exec >> svn-hook.log 2>&1
    - 		hook="$(basename "$0")"
    -@@ t/t9164-git-svn-dcommit-concurrent.sh: setup_hook()
    - 		cnt="$(($cnt - 1))"
    - 		echo "cnt=$cnt" > ./$hook-counter
    - 		[ "$cnt" = "0" ] || exit 0
    --EOF1
    ++	# Subversion hooks run with an empty environment by default. We thus
    ++	# need to propagate PATH so that we can find executables.
    ++	cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
    ++	[default]
    ++	PATH = ${PATH}
     +	EOF
     +
    + 	hook="$rawsvnrepo/hooks/$hook_type"
    + 	cat > "$hook" <<- 'EOF1'
    + 		#!/bin/sh
    +@@ t/t9164-git-svn-dcommit-concurrent.sh: EOF1
      	if [ "$hook_type" = "pre-commit" ]; then
      		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
      	else

base-commit: dadef801b365989099a9929e995589e455c51fed
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2159 bytes --]

In order to set up the Apache httpd server, we need to locate both the
httpd binary and its default module path. This is done with a hardcoded
list of locations that we scan. While this works okayish with distros
that more-or-less follow the Filesystem Hierarchy Standard, it falls
apart on others like NixOS that don't.

While it is possible to specify these paths via `LIB_HTTPD_PATH` and
`LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
to figure out how to set those up. And in fact we can do better by
dynamically detecting both httpd and its module path at runtime:

    - The httpd binary can be located via PATH.

    - The module directory can (in many cases) be derived via the
      `HTTPD_ROOT` compile-time variable.

Amend the code to do so. The new runtime-detected paths will only be
used in case none of the hardcoded paths are usable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd.sh | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 5fe3c8ab69d..6ab8f273a3a 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,21 +55,30 @@ fi
 
 HTTPD_PARA=""
 
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
+			  '/usr/sbin/apache2' \
+			  "$(command -v httpd)" \
+			  "$(command -v apache2)"
 do
-	if test -x "$DEFAULT_HTTPD_PATH"
+	if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
 	then
 		break
 	fi
 done
 
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+fi
+
 for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
 				 '/usr/lib/apache2/modules' \
 				 '/usr/lib64/httpd/modules' \
 				 '/usr/lib/httpd/modules' \
-				 '/usr/libexec/httpd'
+				 '/usr/libexec/httpd' \
+				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
 do
-	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+	if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
 	then
 		break
 	fi
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Jeff King]

On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote:

> -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
> +			  '/usr/sbin/apache2' \
> +			  "$(command -v httpd)" \
> +			  "$(command -v apache2)"
>  do
> -	if test -x "$DEFAULT_HTTPD_PATH"
> +	if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"

Sorry to be a pedant, but I'm not sure if we might have portability
problems with "-a". It's an XSI extension, and POSIX labels it as
obsolescent because it can create parsing ambiguities.

We do have a few instances, but only in corners of the test suite that
probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
So maybe not worth worrying about, but it's easy to write it as:

  if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"

-Peff

Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

On Thu, Nov 09, 2023 at 02:32:50AM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote:
> 
> > -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> > +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
> > +			  '/usr/sbin/apache2' \
> > +			  "$(command -v httpd)" \
> > +			  "$(command -v apache2)"
> >  do
> > -	if test -x "$DEFAULT_HTTPD_PATH"
> > +	if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
> 
> Sorry to be a pedant, but I'm not sure if we might have portability
> problems with "-a". It's an XSI extension, and POSIX labels it as
> obsolescent because it can create parsing ambiguities.
> 
> We do have a few instances, but only in corners of the test suite that
> probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
> So maybe not worth worrying about, but it's easy to write it as:

Yeah, I was grepping for it in our codebase and saw other occurrences,
so I assumed it was fair game. If we're going to convert it to the
below, how about I send another patch on top that also converts the
preexisting instances so that the next one grepping for it isn't going
to repeat the same mistake?

Patrick

>   if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
> 
> -Peff

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Yeah, I was grepping for it in our codebase and saw other occurrences,
> so I assumed it was fair game. If we're going to convert it to the
> below, how about I send another patch on top that also converts the
> preexisting instances so that the next one grepping for it isn't going
> to repeat the same mistake?

Yup, an independent clean-up would be fine.  Now we need to find a
way to give better visibility to CodingGuidelines, which already
says this:

 - We do not write our "test" command with "-a" and "-o" and use "&&"
   or "||" to concatenate multiple "test" commands instead, because
   the use of "-a/-o" is often error-prone.  E.g.

     test -n "$x" -a "$a" = "$b"

   is buggy and breaks when $x is "=", but

     test -n "$x" && test "$a" = "$b"

   does not have such a problem.

Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

On Thu, Nov 09, 2023 at 04:46:05PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Yeah, I was grepping for it in our codebase and saw other occurrences,
> > so I assumed it was fair game. If we're going to convert it to the
> > below, how about I send another patch on top that also converts the
> > preexisting instances so that the next one grepping for it isn't going
> > to repeat the same mistake?
> 
> Yup, an independent clean-up would be fine.  Now we need to find a
> way to give better visibility to CodingGuidelines, which already
> says this:

Okay, I'll send one in. Do you want me to send a v4 of this patch series
or will you squash in below changes into patch 1/3?

>  - We do not write our "test" command with "-a" and "-o" and use "&&"
>    or "||" to concatenate multiple "test" commands instead, because
>    the use of "-a/-o" is often error-prone.  E.g.
> 
>      test -n "$x" -a "$a" = "$b"
> 
>    is buggy and breaks when $x is "=", but
> 
>      test -n "$x" && test "$a" = "$b"
> 
>    does not have such a problem.

I did indeed spot this part of our coding style now. I didn't bother to
look farther when I found other examples where we used `-a` and `-o`,
but that issue will be gone once we've dropped all of these usages.

Patrick

-- >8 --

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 6ab8f273a3..0a74922d7f 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -60,7 +60,7 @@ for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
 			  "$(command -v httpd)" \
 			  "$(command -v apache2)"
 do
-	if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
+	if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
 	then
 		break
 	fi
@@ -78,7 +78,7 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
 				 '/usr/libexec/httpd' \
 				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
 do
-	if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
+	if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
 	then
 		break
 	fi


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Jeff King]

On Thu, Nov 09, 2023 at 08:36:53AM +0100, Patrick Steinhardt wrote:

> > Sorry to be a pedant, but I'm not sure if we might have portability
> > problems with "-a". It's an XSI extension, and POSIX labels it as
> > obsolescent because it can create parsing ambiguities.
> > 
> > We do have a few instances, but only in corners of the test suite that
> > probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
> > So maybe not worth worrying about, but it's easy to write it as:
> 
> Yeah, I was grepping for it in our codebase and saw other occurrences,
> so I assumed it was fair game. If we're going to convert it to the
> below, how about I send another patch on top that also converts the
> preexisting instances so that the next one grepping for it isn't going
> to repeat the same mistake?

I would be very happy with that. :)

-Peff

[PATCH v3 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

When setting up httpd for our tests, we also install a passwd and
proxy-passwd file that contain the test user's credentials. These
credentials currently use crypt(3) as the password encryption schema.

This schema can be considered deprecated nowadays as it is not safe
anymore. Quoting Apache httpd's documentation [1]:

> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.

This is starting to cause issues in modern Linux distributions. glibc
has deprecated its libcrypt library that used to provide crypt(3) in
favor of the libxcrypt library. This newer replacement provides a
compile time switch to disable insecure password encryption schemata,
which causes crypt(3) to always return `EINVAL`. The end result is that
httpd tests that exercise authentication will fail on distros that use
libxcrypt without these insecure encryption schematas.

Regenerate the passwd files to instead use the default password
encryption schema, which is md5. While it feels kind of funny that an
MD5-based encryption schema should be more secure than anything else, it
is the current default and supported by all platforms. Furthermore, it
really doesn't matter all that much given that these files are only used
for testing purposes anyway.

[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd/passwd       | 2 +-
 t/lib-httpd/proxy-passwd | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 3/3] t9164: fix inability to find basename(1) in Subversion hooks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

Hooks executed by Subversion are spawned with an empty environment. By
default, not even variables like PATH will be propagated to them. In
order to ensure that we're still able to find required executables, we
thus write the current PATH variable into the hook script itself and
then re-export it in t9164.

This happens too late in the script though, as we already tried to
execute the basename(1) utility before exporting the PATH variable. This
tends to work on most platforms as the fallback value of PATH for Bash
(see `getconf PATH`) is likely to contain this binary. But on more
exotic platforms like NixOS this is not the case, and thus the test
fails.

While we could work around this issue by simply setting PATH earlier, it
feels fragile to inject a user-controlled value into the script and have
the shell interpret it. Instead, we can refactor the hook setup to write
a `hooks-env` file that configures PATH for us. Like this, Subversion
will know to set up the environment as expected for all hooks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..d1dec89c3b7 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -46,6 +46,14 @@ setup_hook()
 			"passed to setup_hook" >&2 ; return 1; }
 	echo "cnt=$skip_revs" > "$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
+
+	# Subversion hooks run with an empty environment by default. We thus
+	# need to propagate PATH so that we can find executables.
+	cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
+	[default]
+	PATH = ${PATH}
+	EOF
+
 	hook="$rawsvnrepo/hooks/$hook_type"
 	cat > "$hook" <<- 'EOF1'
 		#!/bin/sh
@@ -63,7 +71,6 @@ EOF1
 	if [ "$hook_type" = "pre-commit" ]; then
 		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
 	else
-		echo "PATH=\"$PATH\"; export PATH" >>"$hook"
 		echo "svnconf=\"$svnconf\"" >>"$hook"
 		cat >>"$hook" <<- 'EOF2'
 			cd work-auto-commits.svn
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] t9164: fix inability to find basename(1) in Subversion hooks

[Jeff King]

On Thu, Nov 09, 2023 at 08:10:01AM +0100, Patrick Steinhardt wrote:

> +	# Subversion hooks run with an empty environment by default. We thus
> +	# need to propagate PATH so that we can find executables.
> +	cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
> +	[default]
> +	PATH = ${PATH}
> +	EOF

This is so much less ugly than the shell shenanigans discussed in the
earlier round. Thanks for finding it. :)

The ${PATH} here is interpolated by the here-doc. I guess it's possible
somebody's exotic PATH could break the svn conf syntax, but it's
probably not worth worrying about.

-Peff

Re: [PATCH v3 0/3] t: improve compatibility with NixOS

[Jeff King]

On Thu, Nov 09, 2023 at 08:09:47AM +0100, Patrick Steinhardt wrote:

> this is the third version of my patch series to improve compatibility of
> our tests with NixOS.
> 
> Changes compared to v2:
> 
>     - Patch 1: We now check for both httpd and apache2 binaries via
>       PATH.
> 
>     - Patch 1: We're a bit more defensive and will check whether
>       variables are empty before passing them to either `test -d` or
>       `test -x`.
> 
>     - Patch 3: Clarified why PATH is unset.
> 
>     - Patch 3: Instead of writing PATH into the hook we now write it
>       into the `hook-env` configuration file read by Subversion.

Thanks. This looks good to me, modulo a minor shell nit I mentioned in
the first patch.

-Peff

[PATCH v4 0/3] t: improve compatibility with NixOS

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3367 bytes --]

Hi,

this is the fourth version of my patch series to improve compatibility
of our test suite with NixOS.

Three changes compared to v3:

    - Switched from `test <expr> -a <expr>` to `test <expr> && test
      <expr>`.

    - Improved the commit message to explain why the new
      runtime-detected paths are only used as a fallback.

    - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
      2023-11-09), which has been merged to `next` and caused conflicts.

Technically speaking this series also depends on 0763c3a2c4 (http:
update curl http/2 info matching for curl 8.3.0, 2023-09-15), without
which the tests will fail on NixOS machines with a recent libcurl.

Patrick

Patrick Steinhardt (3):
  t/lib-httpd: dynamically detect httpd and modules path
  t/lib-httpd: stop using legacy crypt(3) for authentication
  t9164: fix inability to find basename(1) in Subversion hooks

 t/lib-httpd.sh                        | 17 +++++++++++++----
 t/lib-httpd/passwd                    |  2 +-
 t/lib-httpd/proxy-passwd              |  2 +-
 t/t9164-git-svn-dcommit-concurrent.sh |  9 ++++++++-
 4 files changed, 23 insertions(+), 7 deletions(-)

Range-diff against v3:
1:  e4c75c492dd ! 1:  41b9dada2e0 t/lib-httpd: dynamically detect httpd and modules path
    @@ Commit message
             - The module directory can (in many cases) be derived via the
               `HTTPD_ROOT` compile-time variable.
     
    -    Amend the code to do so. The new runtime-detected paths will only be
    -    used in case none of the hardcoded paths are usable.
    +    Amend the code to do so.
    +
    +    Note that the new runtime-detected paths will only be used as a fallback
    +    in case none of the hardcoded paths are usable. For the PATH lookup this
    +    is because httpd is typically installed into "/usr/sbin", which is often
    +    not included in the user's PATH variable. And the module path detection
    +    relies on a configured httpd installation and may thus not work in all
    +    cases, either.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ t/lib-httpd.sh: fi
     +			  "$(command -v apache2)"
      do
     -	if test -x "$DEFAULT_HTTPD_PATH"
    -+	if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
    ++	if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
      	then
      		break
      	fi
    @@ t/lib-httpd.sh: fi
      				 '/usr/lib/apache2/modules' \
      				 '/usr/lib64/httpd/modules' \
      				 '/usr/lib/httpd/modules' \
    --				 '/usr/libexec/httpd'
    -+				 '/usr/libexec/httpd' \
    + 				 '/usr/libexec/httpd' \
    +-				 '/usr/lib/apache2'
    ++				 '/usr/lib/apache2' \
     +				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
      do
     -	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    -+	if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
    ++	if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
      	then
      		break
      	fi
2:  3dce76da477 = 2:  7d28c32feaa t/lib-httpd: stop using legacy crypt(3) for authentication
3:  6891e254155 = 3:  6a7773aadba t9164: fix inability to find basename(1) in Subversion hooks

base-commit: 0e3b67e2aa25edb7e1a5c999c87b52a7b3a7649a
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]

In order to set up the Apache httpd server, we need to locate both the
httpd binary and its default module path. This is done with a hardcoded
list of locations that we scan. While this works okayish with distros
that more-or-less follow the Filesystem Hierarchy Standard, it falls
apart on others like NixOS that don't.

While it is possible to specify these paths via `LIB_HTTPD_PATH` and
`LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
to figure out how to set those up. And in fact we can do better by
dynamically detecting both httpd and its module path at runtime:

    - The httpd binary can be located via PATH.

    - The module directory can (in many cases) be derived via the
      `HTTPD_ROOT` compile-time variable.

Amend the code to do so.

Note that the new runtime-detected paths will only be used as a fallback
in case none of the hardcoded paths are usable. For the PATH lookup this
is because httpd is typically installed into "/usr/sbin", which is often
not included in the user's PATH variable. And the module path detection
relies on a configured httpd installation and may thus not work in all
cases, either.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd.sh | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 9ea74927c40..f69d0da51d1 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,22 +55,31 @@ fi
 
 HTTPD_PARA=""
 
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
+			  '/usr/sbin/apache2' \
+			  "$(command -v httpd)" \
+			  "$(command -v apache2)"
 do
-	if test -x "$DEFAULT_HTTPD_PATH"
+	if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
 	then
 		break
 	fi
 done
 
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+fi
+
 for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
 				 '/usr/lib/apache2/modules' \
 				 '/usr/lib64/httpd/modules' \
 				 '/usr/lib/httpd/modules' \
 				 '/usr/libexec/httpd' \
-				 '/usr/lib/apache2'
+				 '/usr/lib/apache2' \
+				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
 do
-	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+	if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
 	then
 		break
 	fi
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +if test -x "$DEFAULT_HTTPD_PATH"
> +then
> +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> +fi

With this patch, my test run starts like so:

    rm -f -r 'test-results'
    *** prove ***
    apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
    ...

I find the error message leaking mildly annoying, and would suggest
doing something like the following on top.

diff --git c/t/lib-httpd.sh w/t/lib-httpd.sh
index 0a74922d7f..03493ee72b 100644
--- c/t/lib-httpd.sh
+++ w/t/lib-httpd.sh
@@ -68,7 +68,7 @@ done
 
 if test -x "$DEFAULT_HTTPD_PATH"
 then
-	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V 2>/dev/null | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
 fi
 
 for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \

Re: [PATCH v4 1/3] t/lib-httpd: dynamically detect httpd and modules path

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

On Sat, Nov 11, 2023 at 09:00:09AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +if test -x "$DEFAULT_HTTPD_PATH"
> > +then
> > +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> > +fi
> 
> With this patch, my test run starts like so:
> 
>     rm -f -r 'test-results'
>     *** prove ***
>     apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
>     ...
> 
> I find the error message leaking mildly annoying, and would suggest
> doing something like the following on top.
> 
> diff --git c/t/lib-httpd.sh w/t/lib-httpd.sh
> index 0a74922d7f..03493ee72b 100644
> --- c/t/lib-httpd.sh
> +++ w/t/lib-httpd.sh
> @@ -68,7 +68,7 @@ done
>  
>  if test -x "$DEFAULT_HTTPD_PATH"
>  then
> -	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V 2>/dev/null | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
>  fi
>  
>  for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \

Yup, makes sense. Thanks for the tweak.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

When setting up httpd for our tests, we also install a passwd and
proxy-passwd file that contain the test user's credentials. These
credentials currently use crypt(3) as the password encryption schema.

This schema can be considered deprecated nowadays as it is not safe
anymore. Quoting Apache httpd's documentation [1]:

> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.

This is starting to cause issues in modern Linux distributions. glibc
has deprecated its libcrypt library that used to provide crypt(3) in
favor of the libxcrypt library. This newer replacement provides a
compile time switch to disable insecure password encryption schemata,
which causes crypt(3) to always return `EINVAL`. The end result is that
httpd tests that exercise authentication will fail on distros that use
libxcrypt without these insecure encryption schematas.

Regenerate the passwd files to instead use the default password
encryption schema, which is md5. While it feels kind of funny that an
MD5-based encryption schema should be more secure than anything else, it
is the current default and supported by all platforms. Furthermore, it
really doesn't matter all that much given that these files are only used
for testing purposes anyway.

[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd/passwd       | 2 +-
 t/lib-httpd/proxy-passwd | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 3/3] t9164: fix inability to find basename(1) in Subversion hooks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

Hooks executed by Subversion are spawned with an empty environment. By
default, not even variables like PATH will be propagated to them. In
order to ensure that we're still able to find required executables, we
thus write the current PATH variable into the hook script itself and
then re-export it in t9164.

This happens too late in the script though, as we already tried to
execute the basename(1) utility before exporting the PATH variable. This
tends to work on most platforms as the fallback value of PATH for Bash
(see `getconf PATH`) is likely to contain this binary. But on more
exotic platforms like NixOS this is not the case, and thus the test
fails.

While we could work around this issue by simply setting PATH earlier, it
feels fragile to inject a user-controlled value into the script and have
the shell interpret it. Instead, we can refactor the hook setup to write
a `hooks-env` file that configures PATH for us. Like this, Subversion
will know to set up the environment as expected for all hooks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..d1dec89c3b7 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -46,6 +46,14 @@ setup_hook()
 			"passed to setup_hook" >&2 ; return 1; }
 	echo "cnt=$skip_revs" > "$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
+
+	# Subversion hooks run with an empty environment by default. We thus
+	# need to propagate PATH so that we can find executables.
+	cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
+	[default]
+	PATH = ${PATH}
+	EOF
+
 	hook="$rawsvnrepo/hooks/$hook_type"
 	cat > "$hook" <<- 'EOF1'
 		#!/bin/sh
@@ -63,7 +71,6 @@ EOF1
 	if [ "$hook_type" = "pre-commit" ]; then
 		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
 	else
-		echo "PATH=\"$PATH\"; export PATH" >>"$hook"
 		echo "svnconf=\"$svnconf\"" >>"$hook"
 		cat >>"$hook" <<- 'EOF2'
 			cd work-auto-commits.svn
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 0/3] t: improve compatibility with NixOS

[Jeff King]

On Fri, Nov 10, 2023 at 09:16:56AM +0100, Patrick Steinhardt wrote:

> this is the fourth version of my patch series to improve compatibility
> of our test suite with NixOS.
> 
> Three changes compared to v3:
> 
>     - Switched from `test <expr> -a <expr>` to `test <expr> && test
>       <expr>`.
> 
>     - Improved the commit message to explain why the new
>       runtime-detected paths are only used as a fallback.
> 
>     - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
>       2023-11-09), which has been merged to `next` and caused conflicts.

Thanks, these all look good to me.

-Peff

Re: [PATCH v4 0/3] t: improve compatibility with NixOS

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Three changes compared to v3:
>
>     - Switched from `test <expr> -a <expr>` to `test <expr> && test
>       <expr>`.
>
>     - Improved the commit message to explain why the new
>       runtime-detected paths are only used as a fallback.
>
>     - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
>       2023-11-09), which has been merged to `next` and caused conflicts.

Please, no.  "The other topic has been merged to 'next'" is *not* a
good reason to do this.  Before that, the other topic was in 'seen'
and causing conflicts already, so getting into 'next' did not create
any new reason for rebasing.

I'll manage this time, but please do not do such a rebase unless you
are asked to do so.  The rebase will force me to do (1) detach from
'next' and apply these, (2) discard the result and detach from the
base of where the previous iteration is queued, (3) apply the new
iteration with "am -3" to undo the rebase, before I can compare the
new iteration with the old iteration.

> Technically speaking this series also depends on 0763c3a2c4 (http:
> update curl http/2 info matching for curl 8.3.0, 2023-09-15), without
> which the tests will fail on NixOS machines with a recent libcurl.

Thanks for that note.  This topic has been queued on top of
v2.43.0-rc1 which has 0763c3a2c4, so we'd be safe.

Will queue.

Re: [PATCH v4 0/3] t: improve compatibility with NixOS

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1741 bytes --]

On Sat, Nov 11, 2023 at 09:10:23AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Three changes compared to v3:
> >
> >     - Switched from `test <expr> -a <expr>` to `test <expr> && test
> >       <expr>`.
> >
> >     - Improved the commit message to explain why the new
> >       runtime-detected paths are only used as a fallback.
> >
> >     - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
> >       2023-11-09), which has been merged to `next` and caused conflicts.
> 
> Please, no.  "The other topic has been merged to 'next'" is *not* a
> good reason to do this.  Before that, the other topic was in 'seen'
> and causing conflicts already, so getting into 'next' did not create
> any new reason for rebasing.
> 
> I'll manage this time, but please do not do such a rebase unless you
> are asked to do so.  The rebase will force me to do (1) detach from
> 'next' and apply these, (2) discard the result and detach from the
> base of where the previous iteration is queued, (3) apply the new
> iteration with "am -3" to undo the rebase, before I can compare the
> new iteration with the old iteration.

Fair enough. I assumed that it would ease your workload instead of
creating more work for you. But I'll keep in mind that it doesn't and
refrain from doing this in the future.

> > Technically speaking this series also depends on 0763c3a2c4 (http:
> > update curl http/2 info matching for curl 8.3.0, 2023-09-15), without
> > which the tests will fail on NixOS machines with a recent libcurl.
> 
> Thanks for that note.  This topic has been queued on top of
> v2.43.0-rc1 which has 0763c3a2c4, so we'd be safe.
> 
> Will queue.

Thanks.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 0/3] t: improve compatibility with NixOS

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Fair enough. I assumed that it would ease your workload instead of
> creating more work for you. But I'll keep in mind that it doesn't and
> refrain from doing this in the future.

Thanks.  

With or without conflicts, basing your work on 'next' would not be a
good idea to begin with, as we never merge 'next' down to 'master'
(we only merge individual topics).

The following applies not specifically to you but to all
contributors.

What is helpful is in a tricky case is to start your topic branch
development at an appropriate base (often the tip of 'master', but
for a bugfix the tip of 'maint'), merge in other topics in flight
you depend on that are not in the target base (which limits what you
can depend on if you are writing a bugfix on 'maint'), and then
start building on top of the merge result.  Then communicate how you
built this (artificial) base clearly in your cover letter.

Whether you have "other topics in flight" merged to your base or
not, creating a trial merge of your topic to seen to see how others'
work-in-progress may interact with your work would be a valuable way
to keep aware of what is cooking and how you will be affected.

Thanks.