Re: [RFC PATCH v2 4/9] selftests/landlock: Test listening restriction

["Günther Noack" <gnoack@google.com>]

On Wed, Aug 14, 2024 at 11:01:46AM +0800, Mikhail Ivanov wrote:
> Add a test for listening restriction. It's similar to protocol.bind and
> protocol.connect tests.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  tools/testing/selftests/landlock/net_test.c | 44 +++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index 8126f5c0160f..b6fe9bde205f 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -689,6 +689,50 @@ TEST_F(protocol, connect)
>  				    restricted, restricted);
>  }
>  
> +TEST_F(protocol, listen)
> +{
> +	if (variant->sandbox == TCP_SANDBOX) {
> +		const struct landlock_ruleset_attr ruleset_attr = {
> +			.handled_access_net = ACCESS_ALL,
> +		};
> +		const struct landlock_net_port_attr tcp_not_restricted_p0 = {
> +			.allowed_access = ACCESS_ALL,
> +			.port = self->srv0.port,
> +		};
> +		const struct landlock_net_port_attr tcp_denied_listen_p1 = {
> +			.allowed_access = ACCESS_ALL &
> +					  ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
> +			.port = self->srv1.port,
> +		};
> +		int ruleset_fd;
> +
> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> +						     sizeof(ruleset_attr), 0);

Nit: The declaration and the assignment of ruleset_fd can be merged into one
line and made const.  (Not a big deal, but it was done a bit more consistently
in the rest of the code, I think.)

> +		ASSERT_LE(0, ruleset_fd);
> +
> +		/* Allows all actions for the first port. */
> +		ASSERT_EQ(0,
> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
> +					    &tcp_not_restricted_p0, 0));
> +
> +		/* Allows all actions despite listen. */
> +		ASSERT_EQ(0,
> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
> +					    &tcp_denied_listen_p1, 0));
> +
> +		enforce_ruleset(_metadata, ruleset_fd);
> +		EXPECT_EQ(0, close(ruleset_fd));
> +	}

This entire "if (variant->sandbox == TCP_SANDBOX)" conditional does the exact
same thing as the one from patch 5/9.  Should that (or parts of it) get
extracted into a suitable helper?

> +	bool restricted = is_restricted(&variant->prot, variant->sandbox);
> +
> +	test_restricted_net_fixture(_metadata, &self->srv0, false, false,
> +				    false);
> +	test_restricted_net_fixture(_metadata, &self->srv1, false, false,
> +				    restricted);
> +	test_restricted_net_fixture(_metadata, &self->srv2, restricted,
> +				    restricted, restricted);

If we start having logic and conditionals in the test implementation (in this
case, in test_restricted_test_fixture()), this might be a sign that that test
implementation should maybe be split apart?  Once the test is as complicated as
the code under test, it does not simplify our confidence in the code much any
more?

(It is often considered bad practice to put conditionals in tests, e.g. in
https://testing.googleblog.com/2014/07/testing-on-toilet-dont-put-logic-in.html)

Do you think we have a way to simplify that?


Readability remark: I am not that strongly invested in this idea, but in the
call to test_restricted_net_fixture(), it is difficult to understand "false,
false, false", without jumping around in the file.  Should we try to make this
more explicit?

I wonder whether we should just pass a struct, so that everything at least has a
name?

  test_restricted_net_fixture((struct expected_net_enforcement){
    .deny_bind = false,
    .deny_connect = false,
    .deny_listen = false,
  });

Then it would be clearer which boolean is which,
and you could use the fact that unspecified struct fields are zero-initialized?

(Alternatively, you could also spell out error codes here, instead of booleans.)

> +}
> +
>  TEST_F(protocol, bind_unspec)
>  {
>  	const struct landlock_ruleset_attr ruleset_attr = {
> -- 
> 2.34.1
> 

—Günther