[PATCH nft] tests/shell: allow running tests as non-root users

[PATCH nft] tests/shell: allow running tests as non-root users

[Thomas Haller]

Allow to opt-out from the have-real-root check via

  NFT_TEST_ROOTLESS=1 ./run-tests.sh

For that to be useful, we must also unshare the PID and user namespace
and map the root user inside that namespace.

With that, inside the namespace we look like root. Including having
CAP_NET_ADMIN for the new net namespace. This allows to run the tests as
rootless and most tests will pass.

Note that some tests will fail as rootless. For example, the socket
buffers are limited by /proc/sys/net/core/{wmem_max,rmem_max}, which is
not namespaced. When running as real-root, nftables can raise them
beyond those limits and the tests will pass. Rootless cannot do that and
the test may fail.

Test that don't work without real root should check for
[ "$NFT_TEST_HAVE_REALROOT" != 1 ] and skip gracefully.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 57 +++++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 12 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index b66ef4fa4d1f..96dd0b0c2fd6 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -1,9 +1,18 @@
 #!/bin/bash
 
-# Configuration
-TESTDIR="./$(dirname $0)/testcases"
-SRC_NFT="$(dirname $0)/../../src/nft"
-DIFF=$(which diff)
+# Environment variables for the user:
+# NFT=<path>                   Path to the nft executable.
+# NFT_TEST_ROOTLESS=0|1:       Whether the test allows to run as rootless. Usually the test
+#                              will require real root permissions. You can set NFT_TEST_ROOTLESS=1
+#                              to run rootless in a separate namespace.
+# NFT_TEST_HAVE_REALROOT=0|1:  To indicate whether the test has real root permissions.
+#                              Usually, you don't need this and it gets autodetected.
+#                              Note that without real root, certain tests may not work.
+#                              For example, due to limited /proc/sys/net/core/{wmem_max,rmem_max}.
+#                              Such test should check for [ "$NFT_TEST_HAVE_REALROOT" != 1 ] and
+#                              skip (not fail) in such an environment.
+# NFT_TEST_NO_UNSHARE=0|1:     Usually, the test will run in a separate namespace.
+#                              You can opt-out from that by setting NFT_TEST_NO_UNSHARE=1.
 
 msg_error() {
 	echo "E: $1 ..." >&2
@@ -18,18 +27,42 @@ msg_info() {
 	echo "I: $1"
 }
 
-if [ "$(id -u)" != "0" ] ; then
+if [ "$NFT_TEST_HAVE_REALROOT" = "" ] ; then
+	# The caller can set NFT_TEST_HAVE_REALROOT to indicate us whether we
+	# have real root. They usually don't need, and we detect it now based
+	# on `id -u`. Note that we may unshare below, so the check inside the
+	# new namespace won't be conclusive. We thus only detect once and export
+	# the result.
+	export NFT_TEST_HAVE_REALROOT="$(test "$(id -u)" = "0" && echo 1 || echo 0)"
+fi
+
+if [ "$NFT_TEST_HAVE_REALROOT" != 1 -a "$NFT_TEST_ROOTLESS" != 1 ] ; then
+	# By default, we require real-root, unless the user explicitly opts-in
+	# to proceed via NFT_TEST_ROOTLESS=1.
 	msg_error "this requires root!"
 fi
 
-if [ "${1}" != "run" ]; then
-	if unshare -f -n true; then
-		unshare -n "${0}" run $@
-		exit $?
-	fi
-	msg_warn "cannot run in own namespace, connectivity might break"
+if [ "$NFT_TEST_NO_UNSHARE" = 1 ]; then
+	# The user opts-out from unshare. Proceed without.
+	:
+elif [ "$_NFT_TEST_IS_UNSHARED" = 1 ]; then
+	# We are inside the unshared environment. Don't unshare again.
+	# Continue.
+	:
+else
+	# Unshare. This works both as rootless and with real root. Inside
+	# the user namespace we will map the root user, so we appear to have
+	# root. Check for [ "$NFT_TEST_HAVE_REALROOT" != 1 ] to know when
+	# not having real root.
+	export _NFT_TEST_IS_UNSHARED=1
+	unshare -f --map-root-user -U -p -n "$0" "$@"
+	exit $?
 fi
-shift
+
+# Configuration
+TESTDIR="./$(dirname $0)/testcases"
+SRC_NFT="$(dirname $0)/../../src/nft"
+DIFF=$(which diff)
 
 [ -z "$NFT" ] && NFT=$SRC_NFT
 ${NFT} > /dev/null 2>&1
-- 
2.41.0

Re: [PATCH nft] tests/shell: allow running tests as non-root users

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> Allow to opt-out from the have-real-root check via
> 
>   NFT_TEST_ROOTLESS=1 ./run-tests.sh

I don't like this.  But its a step in the right direction.

To me run-tests.sh has following issues/pain points:
 - test duration is huge (>10m with debug kernels)
 - all tests run in same netns
 - tries to unloads kernel modules after each test

The need for uid 0 wasn't big on my problem list so far because
I mostly run the tests in a VM.  But I agree its an issue for
auto-build systems / CI and the like.

> For that to be useful, we must also unshare the PID and user namespace
> and map the root user inside that namespace.

Are you sure PIDNS unshare is needed for this?

> Test that don't work without real root should check for
> [ "$NFT_TEST_HAVE_REALROOT" != 1 ] and skip gracefully.

Thats fine, see my recent RFC to add such environment
variables to check if a particular feature is supported or not.

What I don't like here is the NFT_TEST_ROOTLESS environment
variable to alter behaviour of run-tests.sh behavior, but see below.

> -if [ "$(id -u)" != "0" ] ; then
> +if [ "$NFT_TEST_HAVE_REALROOT" = "" ] ; then
> +	# The caller can set NFT_TEST_HAVE_REALROOT to indicate us whether we
> +	# have real root. They usually don't need, and we detect it now based
> +	# on `id -u`. Note that we may unshare below, so the check inside the
> +	# new namespace won't be conclusive. We thus only detect once and export
> +	# the result.
> +	export NFT_TEST_HAVE_REALROOT="$(test "$(id -u)" = "0" && echo 1 || echo 0)"
> +fi
> +

Why not get rid of the check?  Just auto-switch to unpriv userns and
error out if that fails.  You could just print a warning/notice here and
then try userns mode.  And/or print a notice at the together with the
test summary.

> +if [ "$NFT_TEST_NO_UNSHARE" = 1 ]; then
> +	# The user opts-out from unshare. Proceed without.

Whats the use case?  If there is a good one, then i'd prefer a command
line switch rather than environment.

I think long term all of the following would be good to have:

1. run each test in its own netns
2. get rid of the forced 'nft flush ruleset' and the rmmod calls
3. Explore parallelisation of tests to reduce total test time
4. Add a SKIP return value, that tells that the test did not run
  (or some other means that allows run-tests.sh to figure out that
   a particular test did not run because its known to not work on
   current configuration).

This would avoid false-positive 'all tests passed' when in reality
some test had to 'exit 0' because of a missing feature or lack of real
root.

Alternatively we could just make these tests fail and leave it to the
user to figure it out, the normal expectation is for all tests to pass,
its mostly when run-tests.sh is run on older kernel releases when it
starts acting up.

Re: [PATCH nft] tests/shell: allow running tests as non-root users

[Thomas Haller]

On Thu, 2023-08-31 at 18:08 +0200, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > Allow to opt-out from the have-real-root check via
> > 
> >   NFT_TEST_ROOTLESS=1 ./run-tests.sh
> 
> I don't like this.  But its a step in the right direction.
> 
> To me run-tests.sh has following issues/pain points:
>  - test duration is huge (>10m with debug kernels)
>  - all tests run in same netns
>  - tries to unloads kernel modules after each test
> 
> The need for uid 0 wasn't big on my problem list so far because
> I mostly run the tests in a VM.  But I agree its an issue for
> auto-build systems / CI and the like.

It also means there is not simple "run-tests" script without setting up
a VM/container. I personally avoid to run `sudo some/script` from the
internet (although, I execute build scripts from the internet are
normal user).

Running a substantial subset of tests as non-root, seems essential to
me.

It may not be essential to everybody, but to some.

> 
> > For that to be useful, we must also unshare the PID and user
> > namespace
> > and map the root user inside that namespace.
> 
> Are you sure PIDNS unshare is needed for this?

Probably not, but does it hurt? I think it should be

  unshare -f -p -U -n --mount-proc --map-root-user \
      bash -xc 'whoami; ip link; sleep 1 & ps aux'

> 
> > Test that don't work without real root should check for
> > [ "$NFT_TEST_HAVE_REALROOT" != 1 ] and skip gracefully.
> 
> Thats fine, see my recent RFC to add such environment
> variables to check if a particular feature is supported or not.
> 
> What I don't like here is the NFT_TEST_ROOTLESS environment
> variable to alter behaviour of run-tests.sh behavior, but see below.

If you don't run with real-root, then you are maybe not testing the
full thing and miss something. Hence, you have to opt-in to the new
rootless functionality with NFT_TEST_ROOTLESS=1.

The check is to preserve the previous behavior, which failed without
real-root.


> > -if [ "$(id -u)" != "0" ] ; then
> > +if [ "$NFT_TEST_HAVE_REALROOT" = "" ] ; then
> > +       # The caller can set NFT_TEST_HAVE_REALROOT to indicate us
> > whether we
> > +       # have real root. They usually don't need, and we detect it
> > now based
> > +       # on `id -u`. Note that we may unshare below, so the check
> > inside the
> > +       # new namespace won't be conclusive. We thus only detect
> > once and export
> > +       # the result.
> > +       export NFT_TEST_HAVE_REALROOT="$(test "$(id -u)" = "0" &&
> > echo 1 || echo 0)"
> > +fi
> > +
> 
> Why not get rid of the check?  Just auto-switch to unpriv userns and
> error out if that fails.  You could just print a warning/notice here
> and
> then try userns mode.  And/or print a notice at the together with the
> test summary.

Which check? About NFT_TEST_HAVE_REALROOT?

We need that, to detect real-root before unshare. Since we already need
it internally, it's also documented so that the user could override it.
For example, if you develop inside a

     podman run --privileged -ti fedora:latest

then `id -u` would wrongly indicate that the test has real-root. You
can override that with `export  NFT_TEST_HAVE_REALROOT=0` to run in
rootless mode.


> 
> > +if [ "$NFT_TEST_NO_UNSHARE" = 1 ]; then
> > +       # The user opts-out from unshare. Proceed without.
> 
> Whats the use case?  If there is a good one, then i'd prefer a
> command
> line switch rather than environment.

I think command line switches are inferior. They are cumbersome to
implement (in the script, the order of options surprisingly matters).
They are also more limited in their usage. The script should integrate
with a `make check-xyz` step or be called from others scripts, where
the command line is not directly editable. Also, the name
NFT_TEST_NO_UNSHARE is something unique that you can grep for (unlike
the "-g" command line switch).

The variable NFT_TEST_NO_UNSHARE is there, because the previous code
also expected a failure to `unshare`, then printed a warning and
proceeded without separate namespace. I think that fallback is
undesirable, I would not want to run the test accidentally in the main
namespace.

Now, by default it always tries to unshare, and aborts on failure. The
variable is there to opt-out from that.


> 
> I think long term all of the following would be good to have:
> 
> 1. run each test in its own netns
> 2. get rid of the forced 'nft flush ruleset' and the rmmod calls
> 3. Explore parallelisation of tests to reduce total test time
> 4. Add a SKIP return value, that tells that the test did not run
>   (or some other means that allows run-tests.sh to figure out that
>    a particular test did not run because its known to not work on
>    current configuration).
> 
> This would avoid false-positive 'all tests passed' when in reality
> some test had to 'exit 0' because of a missing feature or lack of
> real
> root.
> 
> Alternatively we could just make these tests fail and leave it to the
> user to figure it out, the normal expectation is for all tests to
> pass,
> its mostly when run-tests.sh is run on older kernel releases when it
> starts acting up.
> 

These are very good points.

Parallel execution maybe happen by hooking the tests up as individual
make targets. 


I'd like to integrate tests more into `make check-*`. I'd also like to
add unit tests (that is, both statically and dynamically link with
libnftables and to be able to unit test code specifically). As a
developer, I'd like to type `make check` to run the the unit tests and
have a clear make target for the integration tests.

IMO "[PATCH nft 0/6] no recursive make" should be done to make that
nicer. But I guess that will be controversial. Recursive make seems
very popular, all around.



Thomas

Re: [PATCH nft] tests/shell: allow running tests as non-root users

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> On Thu, 2023-08-31 at 18:08 +0200, Florian Westphal wrote:
> > > For that to be useful, we must also unshare the PID and user
> > > namespace
> > > and map the root user inside that namespace.
> > 
> > Are you sure PIDNS unshare is needed for this?
> 
> Probably not, but does it hurt? I think it should be
> 
>   unshare -f -p -U -n --mount-proc --map-root-user \
>       bash -xc 'whoami; ip link; sleep 1 & ps aux'

Its an extra kernel option that needs to be enabled for this to work.
Probably on for all distro kernels by now but whats the added benefit?

> > > Test that don't work without real root should check for
> > > [ "$NFT_TEST_HAVE_REALROOT" != 1 ] and skip gracefully.
> > 
> > Thats fine, see my recent RFC to add such environment
> > variables to check if a particular feature is supported or not.
> > 
> > What I don't like here is the NFT_TEST_ROOTLESS environment
> > variable to alter behaviour of run-tests.sh behavior, but see below.
> 
> If you don't run with real-root, then you are maybe not testing the
> full thing and miss something. Hence, you have to opt-in to the new
> rootless functionality with NFT_TEST_ROOTLESS=1.

I disagree, I think a notice is fine, this disclaimer could be
repeated after test summary.

Especially if we start skipping tests we should also have
an indication that not all tests were run (ideally, we'd see which
ones and how many...).

> The check is to preserve the previous behavior, which failed without
> real-root.

If you absolutely insist then fine.

> > Why not get rid of the check?  Just auto-switch to unpriv userns and
> > error out if that fails.  You could just print a warning/notice here
> > and
> > then try userns mode.  And/or print a notice at the together with the
> > test summary.
> 
> Which check? About NFT_TEST_HAVE_REALROOT?

I meant $(id) -eq 0 || barf

I think the best is to:
./run-tests.sh called with uid 0   -> no changes
./run-tests.sh called with uid > 0 -> try unpriv netns and set
NFT_TEST_HAVE_USERNS=1 in the environment to allow test cases to adjust
if needed (e.g. bail out early).

>      podman run --privileged -ti fedora:latest
> 
> then `id -u` would wrongly indicate that the test has real-root. You
> can override that with `export  NFT_TEST_HAVE_REALROOT=0` to run in
> rootless mode.

Ah.  Well, with my proposal above you can still set
NFT_TEST_HAVE_USERNS=1 manually before ./run-tests.sh if uid 0 isn't
really uid 0.

> > > +if [ "$NFT_TEST_NO_UNSHARE" = 1 ]; then
> > > +       # The user opts-out from unshare. Proceed without.
> > 
> > Whats the use case?  If there is a good one, then i'd prefer a
> > command
> > line switch rather than environment.
> 
> I think command line switches are inferior. They are cumbersome to
> implement (in the script, the order of options surprisingly matters).
> They are also more limited in their usage. The script should integrate
> with a `make check-xyz` step or be called from others scripts, where
> the command line is not directly editable. Also, the name
> NFT_TEST_NO_UNSHARE is something unique that you can grep for (unlike
> the "-g" command line switch.

Yuck.  Do we need a totally different script then?

./run-tests.sh is for humans, not machines. I want sane defaults.

> also expected a failure to `unshare`, then printed a warning and
> proceeded without separate namespace. I think that fallback is
> undesirable, I would not want to run the test accidentally in the main
> namespace.

Then add a warning?  I very much dislike these environment variables,
developers will add them to their bash init scripts and then there is
big surprise why someone reports unexpected results/behaviours.

Command line options are typically known, environment not.
Don't get me wrong, environment variables are good, I have no objections
to setting NFT_TEST_HAVE_USERNS or the like for the individual tests to
consume.

> Now, by default it always tries to unshare, and aborts on failure. The
> variable is there to opt-out from that.

Can't we have a sane default without a need for new command line options
or enviroment variables?

> I'd like to integrate tests more into `make check-*`. I'd also like to
> add unit tests (that is, both statically and dynamically link with
> libnftables and to be able to unit test code specifically). As a
> developer, I'd like to type `make check` to run the the unit tests and
> have a clear make target for the integration tests.

Thats a good thing.  I do not care if I have to call ./run-tests.sh
or 'make tests', but please, sane defaults without code path explosion.

Re: [PATCH nft] tests/shell: allow running tests as non-root users

[Thomas Haller]

On Thu, 2023-08-31 at 20:18 +0200, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > On Thu, 2023-08-31 at 18:08 +0200, Florian Westphal wrote:
> > > > For that to be useful, we must also unshare the PID and user
> > > > namespace
> > > > and map the root user inside that namespace.
> > > 
> > > Are you sure PIDNS unshare is needed for this?
> > 
> > Probably not, but does it hurt? I think it should be
> > 
> >   unshare -f -p -U -n --mount-proc --map-root-user \
> >       bash -xc 'whoami; ip link; sleep 1 & ps aux'
> 
> Its an extra kernel option that needs to be enabled for this to work.
> Probably on for all distro kernels by now but whats the added
> benefit?

I think we should isolate more, if we can. In v2, there is a test an
fallback to without "-P". How about that?

> 
> > > > Test that don't work without real root should check for
> > > > [ "$NFT_TEST_HAVE_REALROOT" != 1 ] and skip gracefully.
> > > 
> > > Thats fine, see my recent RFC to add such environment
> > > variables to check if a particular feature is supported or not.
> > > 
> > > What I don't like here is the NFT_TEST_ROOTLESS environment
> > > variable to alter behaviour of run-tests.sh behavior, but see
> > > below.
> > 
> > If you don't run with real-root, then you are maybe not testing the
> > full thing and miss something. Hence, you have to opt-in to the new
> > rootless functionality with NFT_TEST_ROOTLESS=1.
> 
> I disagree, I think a notice is fine, this disclaimer could be
> repeated after test summary.

In v2, NFT_TEST_ROOTLESS is dropped and a notice added.

> 
> Especially if we start skipping tests we should also have
> an indication that not all tests were run (ideally, we'd see which
> ones and how many...).
> 
> > The check is to preserve the previous behavior, which failed
> > without
> > real-root.
> 
> If you absolutely insist then fine.

I don't. Dropped in v2.


> 
> > > Why not get rid of the check?  Just auto-switch to unpriv userns
> > > and
> > > error out if that fails.  You could just print a warning/notice
> > > here
> > > and
> > > then try userns mode.  And/or print a notice at the together with
> > > the
> > > test summary.
> > 
> > Which check? About NFT_TEST_HAVE_REALROOT?
> 
> I meant $(id) -eq 0 || barf
> 
> I think the best is to:
> ./run-tests.sh called with uid 0   -> no changes
> ./run-tests.sh called with uid > 0 -> try unpriv netns and set
> NFT_TEST_HAVE_USERNS=1 in the environment to allow test cases to
> adjust
> if needed (e.g. bail out early).

Addressed in v2 (with some variations).

> 
> >      podman run --privileged -ti fedora:latest
> > 
> > then `id -u` would wrongly indicate that the test has real-root.
> > You
> > can override that with `export  NFT_TEST_HAVE_REALROOT=0` to run in
> > rootless mode.
> 
> Ah.  Well, with my proposal above you can still set
> NFT_TEST_HAVE_USERNS=1 manually before ./run-tests.sh if uid 0 isn't
> really uid 0.
> 
> > > > +if [ "$NFT_TEST_NO_UNSHARE" = 1 ]; then
> > > > +       # The user opts-out from unshare. Proceed without.
> > > 
> > > Whats the use case?  If there is a good one, then i'd prefer a
> > > command
> > > line switch rather than environment.
> > 
> > I think command line switches are inferior. They are cumbersome to
> > implement (in the script, the order of options surprisingly
> > matters).
> > They are also more limited in their usage. The script should
> > integrate
> > with a `make check-xyz` step or be called from others scripts,
> > where
> > the command line is not directly editable. Also, the name
> > NFT_TEST_NO_UNSHARE is something unique that you can grep for
> > (unlike
> > the "-g" command line switch.
> 
> Yuck.  Do we need a totally different script then?

No. run-tests.sh is currently *the* runner script for running tests.
It must always be useful to call directly.

But it *might* be useful to call it from a wrapper script (like a make
target).

> 
> ./run-tests.sh is for humans, not machines. I want sane defaults.

> > also expected a failure to `unshare`, then printed a warning and
> > proceeded without separate namespace. I think that fallback is
> > undesirable, I would not want to run the test accidentally in the
> > main
> > namespace.
> 
> Then add a warning?  I very much dislike these environment variables,
> developers will add them to their bash init scripts and then there is
> big surprise why someone reports unexpected results/behaviours.
> 
> Command line options are typically known, environment not.
> Don't get me wrong, environment variables are good, I have no
> objections
> to setting NFT_TEST_HAVE_USERNS or the like for the individual tests
> to
> consume.

Already before, the script supports various command line options, but
those options were also settable via an environment variable. E.g.
"VERBOSE=y" vs. "-v".

I followed that style in v2. How about that?

> 
> > Now, by default it always tries to unshare, and aborts on failure.
> > The
> > variable is there to opt-out from that.
> 
> Can't we have a sane default without a need for new command line
> options
> or enviroment variables?

The only unsane default was NFT_TEST_ROOTLESS to opt-in to make
something work. It now works by default.


> 
> > I'd like to integrate tests more into `make check-*`. I'd also like
> > to
> > add unit tests (that is, both statically and dynamically link with
> > libnftables and to be able to unit test code specifically). As a
> > developer, I'd like to type `make check` to run the the unit tests
> > and
> > have a clear make target for the integration tests.
> 
> Thats a good thing.  I do not care if I have to call ./run-tests.sh
> or 'make tests', but please, sane defaults without code path
> explosion.
> 


Thomas