| Message ID | 20240814030151.2380280-5-ivanov.mikhail1@huawei-partners.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Support TCP listen access-control | expand |
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
8/20/2024 3:31 PM, Günther Noack wrote: > 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.) Current variant is performed in every TEST_F() method. I assume that this is required in order to not make a mess by combining the ruleset_attr and several rule structures with the operation of creating ruleset. WDYT? > >> + 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? I don't think replacing if (variant->sandbox == TCP_SANDBOX) with if (is_tcp_sandbox(variant)) will change anything, this condition is already quite simple. If you think that such helper is more convenient, I can add it. > >> + 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? I agree.. using 3 external booleans to control behavior of the test is really messy. I believe the best we can do to avoid this is to split "test_restricted_net_fixture()" into few independent tests. For example we can turn this call: test_restricted_net_fixture(_metadata, &self->srv0, false, false, false); into multiple smaller tests: /* Tries to bind with invalid and minimal addrlen. */ EXPECT_EQ(0, TEST_BIND(&self->srv0)); /* Tries to connect with invalid and minimal addrlen. */ EXPECT_EQ(0, TEST_CONNECT(&self->srv0)); /* Tries to listen. */ EXPECT_EQ(0, TEST_LISTEN(&self->srv0)); /* Connection tests. */ EXPECT_EQ(0, TEST_CLIENT_SERVER(&self->srv0)); Each test is wrapped in a macro that implicitly passes _metadata argument. This case in which every access is allowed can be wrapped in a macro: TEST_UNRESTRICTED_NET_FIXTURE(&self->srv0); Such approach has following cons though: * A lot of duplicated code. These small helpers should be added to every test that uses "test_restricted_net_fixture()". Currently there are 16 calls of this helper. * There is wouldn't be a single entity that is used to test a network under different sandbox scenarios. If we split the helper each test should care about (1) sandboxing, (2) running all required tests. For example TEST_LISTEN() and TEST_CLIENT_SERVER() could not be called if bind is restricted. For example protocol.bind test would have following lines after "test_restricted_net_fixture()" is removed: TEST_UNRESTRICTED_NET_FIXTURE(&self->srv0); if (is_restricted(&variant->prot, variant->sandbox)) { EXPECT_EQ(-EACCES, TEST_BIND(&self->srv1)); EXPECT_EQ(0, TEST_CONNECT(&self->srv1)); EXPECT_EQ(-EACCES, TEST_BIND(&self->srv2)); EXPECT_EQ(-EACCES, TEST_CONNECT(&self->srv2)); } else { TEST_UNRESTRICTED_NET_FIXTURE(&self->srv1); TEST_UNRESTRICTED_NET_FIXTURE(&self->srv2); } I suggest leaving "test_restricted_net_fixture()" and refactor this booleans (in the way you suggested) in order not to lose simplicity in the testing: bool restricted = is_restricted(&variant->prot, variant->sandbox); test_restricted_net_fixture(_metadata, &self->srv0, (struct expected_net_enforcement){ .deny_bind = false, .deny_connect = false, .deny_listen = false }); test_restricted_net_fixture(_metadata, &self->srv1, (struct expected_net_enforcement){ .deny_bind = false, .deny_connect = restricted, .deny_listen = false }); test_restricted_net_fixture(_metadata, &self->srv2, (struct expected_net_enforcement){ .deny_bind = restricted, .deny_connect = restricted, .deny_listen = restricted }); But it's really not obvious design issue and splitting helper can really be a better solution. WDYT? > > > 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.) Agreed, this is a best option for refactoring. I've also tried adding access_mask field to the service_fixture struct with all accesses allowed by default. In a test, then you just need to remove the necessary accesses after sandboxing: if (is_restricted(&variant->prot, variant->sandbox)) clear_access(&self->srv2, LANDLOCK_ACCESS_NET_BIND_TCP | LANDLOCK_ACCESS_NET_CONNECT_TCP); test_restricted_net_fixture(_metadata, &self->srv2); But this solution is too implicit for the helper. Passing struct would be better. > >> +} >> + >> TEST_F(protocol, bind_unspec) >> { >> const struct landlock_ruleset_attr ruleset_attr = { >> -- >> 2.34.1 >> > > —Günther
On Tue, Aug 20, 2024 at 09:46:56PM +0300, Mikhail Ivanov wrote: > 8/20/2024 3:31 PM, Günther Noack wrote: > > 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.) > > Current variant is performed in every TEST_F() method. I assume that > this is required in order to not make a mess by combining the > ruleset_attr and several rule structures with the operation of creating > ruleset. WDYT? Using variant->sandbox helps identify test scenarios. > > > > > > + 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? > > I don't think replacing > if (variant->sandbox == TCP_SANDBOX) > with > if (is_tcp_sandbox(variant)) > will change anything, this condition is already quite simple. If > you think that such helper is more convenient, I can add it. The variant->sandbox check is OK, but the following code block should not be duplicated because it makes more code to review and we may wonder if it does the same thing. Intead we can have something like this: if (variant->sandbox == TCP_SANDBOX) restrict_tcp_listen(_metadata, self); > > > > > > + 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? > > I agree.. using 3 external booleans to control behavior of the > test is really messy. I believe the best we can do to avoid this is to > split "test_restricted_net_fixture()" into few independent tests. For > example we can turn this call: > > test_restricted_net_fixture(_metadata, &self->srv0, false, > false, false); > > into multiple smaller tests: > > /* Tries to bind with invalid and minimal addrlen. */ > EXPECT_EQ(0, TEST_BIND(&self->srv0)); > > /* Tries to connect with invalid and minimal addrlen. */ > EXPECT_EQ(0, TEST_CONNECT(&self->srv0)); > > /* Tries to listen. */ > EXPECT_EQ(0, TEST_LISTEN(&self->srv0)); > > /* Connection tests. */ > EXPECT_EQ(0, TEST_CLIENT_SERVER(&self->srv0)); These standalone bind/connect/listen/client_server looks good. > > Each test is wrapped in a macro that implicitly passes _metadata argument. I'd prefer to not use macros to pass argument because it makes it more difficult to understand what is going on. Just create a test_*(_metadata, ...) helper. > > This case in which every access is allowed can be wrapped in a macro: > > TEST_UNRESTRICTED_NET_FIXTURE(&self->srv0); Let's try to avoid macros as much as possible. > > Such approach has following cons though: > * A lot of duplicated code. These small helpers should be added to every > test that uses "test_restricted_net_fixture()". Currently there > are 16 calls of this helper. We can start by calling these test_listen()-like helpers in test_bind_and_connect(). We should be careful to not change too much the existing test code to be able to run them against older kernels without too much changes. > > * There is wouldn't be a single entity that is used to test a network > under different sandbox scenarios. If we split the helper each test > should care about (1) sandboxing, (2) running all required tests. For > example TEST_LISTEN() and TEST_CLIENT_SERVER() could not be called if > bind is restricted. Yes, this might be an issue, but for this specific case we may write a dedicated test if it helps. > > For example protocol.bind test would have following lines after > "test_restricted_net_fixture()" is removed: > > TEST_UNRESTRICTED_NET_FIXTURE(&self->srv0); > > if (is_restricted(&variant->prot, variant->sandbox)) { > EXPECT_EQ(-EACCES, TEST_BIND(&self->srv1)); > EXPECT_EQ(0, TEST_CONNECT(&self->srv1)); > > EXPECT_EQ(-EACCES, TEST_BIND(&self->srv2)); > EXPECT_EQ(-EACCES, TEST_CONNECT(&self->srv2)); > } else { > TEST_UNRESTRICTED_NET_FIXTURE(&self->srv1); > TEST_UNRESTRICTED_NET_FIXTURE(&self->srv2); > } > > I suggest leaving "test_restricted_net_fixture()" and refactor this > booleans (in the way you suggested) in order not to lose simplicity in > the testing: > > bool restricted = is_restricted(&variant->prot, > variant->sandbox); > > test_restricted_net_fixture(_metadata, &self->srv0, > (struct expected_net_enforcement){ > .deny_bind = false, > .deny_connect = false, > .deny_listen = false > }); > test_restricted_net_fixture(_metadata, &self->srv1, > (struct expected_net_enforcement){ > .deny_bind = false, > .deny_connect = restricted, > .deny_listen = false > }); > test_restricted_net_fixture(_metadata, &self->srv2, > (struct expected_net_enforcement){ > .deny_bind = restricted, > .deny_connect = restricted, > .deny_listen = restricted > }); > > But it's really not obvious design issue and splitting helper can really > be a better solution. WDYT? > > > > > > > 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.) > > Agreed, this is a best option for refactoring. > > I've also tried adding access_mask field to the service_fixture struct > with all accesses allowed by default. In a test, then you just need to > remove the necessary accesses after sandboxing: > > if (is_restricted(&variant->prot, variant->sandbox)) > clear_access(&self->srv2, > LANDLOCK_ACCESS_NET_BIND_TCP | > LANDLOCK_ACCESS_NET_CONNECT_TCP); > > test_restricted_net_fixture(_metadata, &self->srv2); > > But this solution is too implicit for the helper. Passing struct would > be better. What about passing the variant to these tests and creating more fine-grained is_restricted_*() helpers? > > > > > > +} > > > + > > > TEST_F(protocol, bind_unspec) > > > { > > > const struct landlock_ruleset_attr ruleset_attr = { > > > -- > > > 2.34.1 > > > > > > > —Günther >
On 9/25/2024 9:31 PM, Mickaël Salaün wrote: > On Tue, Aug 20, 2024 at 09:46:56PM +0300, Mikhail Ivanov wrote: >> 8/20/2024 3:31 PM, Günther Noack wrote: >>> 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.) >> >> Current variant is performed in every TEST_F() method. I assume that >> this is required in order to not make a mess by combining the >> ruleset_attr and several rule structures with the operation of creating >> ruleset. WDYT? > > Using variant->sandbox helps identify test scenarios. Sorry, I'm not sure I understand if this advice can be applied to the discussed nit. > >> >>> >>>> + 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? >> >> I don't think replacing >> if (variant->sandbox == TCP_SANDBOX) >> with >> if (is_tcp_sandbox(variant)) >> will change anything, this condition is already quite simple. If >> you think that such helper is more convenient, I can add it. > > The variant->sandbox check is OK, but the following code block should > not be duplicated because it makes more code to review and we may wonder > if it does the same thing. Intead we can have something like this: > > if (variant->sandbox == TCP_SANDBOX) > restrict_tcp_listen(_metadata, self); Good suggestion, thank you! Probably it would be more simple to make a single restrict_tcp() helper and pass appropriate access right to it: if (variant->sandbox == TCP_SANDBOX) restrict_tcp(_metadata, self, LANDLOCK_ACCESS_NET_{LISTEN|BIND|CONNECT}_TCP); > >> >>> >>>> + 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? >> >> I agree.. using 3 external booleans to control behavior of the >> test is really messy. I believe the best we can do to avoid this is to >> split "test_restricted_net_fixture()" into few independent tests. For >> example we can turn this call: >> >> test_restricted_net_fixture(_metadata, &self->srv0, false, >> false, false); >> >> into multiple smaller tests: >> >> /* Tries to bind with invalid and minimal addrlen. */ >> EXPECT_EQ(0, TEST_BIND(&self->srv0)); >> >> /* Tries to connect with invalid and minimal addrlen. */ >> EXPECT_EQ(0, TEST_CONNECT(&self->srv0)); >> >> /* Tries to listen. */ >> EXPECT_EQ(0, TEST_LISTEN(&self->srv0)); >> >> /* Connection tests. */ >> EXPECT_EQ(0, TEST_CLIENT_SERVER(&self->srv0)); > > These standalone bind/connect/listen/client_server looks good. > >> >> Each test is wrapped in a macro that implicitly passes _metadata argument. > > I'd prefer to not use macros to pass argument because it makes it more > difficult to understand what is going on. Just create a > test_*(_metadata, ...) helper. Ok, agreed > >> >> This case in which every access is allowed can be wrapped in a macro: >> >> TEST_UNRESTRICTED_NET_FIXTURE(&self->srv0); > > Let's try to avoid macros as much as possible. Ok, using such macros in tests might be really confusing. > >> >> Such approach has following cons though: >> * A lot of duplicated code. These small helpers should be added to every >> test that uses "test_restricted_net_fixture()". Currently there >> are 16 calls of this helper. > > We can start by calling these test_listen()-like helpers in > test_bind_and_connect(). We should be careful to not change too much > the existing test code to be able to run them against older kernels > without too much changes. Yeah, ofc we can do this if you think we need a smoother refactoring process. We can discuss initial changes in dedicated topic [1]. [1] https://github.com/landlock-lsm/linux/issues/34 > >> >> * There is wouldn't be a single entity that is used to test a network >> under different sandbox scenarios. If we split the helper each test >> should care about (1) sandboxing, (2) running all required tests. For >> example TEST_LISTEN() and TEST_CLIENT_SERVER() could not be called if >> bind is restricted. > > Yes, this might be an issue, but for this specific case we may write a > dedicated test if it helps. Agreed > >> >> For example protocol.bind test would have following lines after >> "test_restricted_net_fixture()" is removed: >> >> TEST_UNRESTRICTED_NET_FIXTURE(&self->srv0); >> >> if (is_restricted(&variant->prot, variant->sandbox)) { >> EXPECT_EQ(-EACCES, TEST_BIND(&self->srv1)); >> EXPECT_EQ(0, TEST_CONNECT(&self->srv1)); >> >> EXPECT_EQ(-EACCES, TEST_BIND(&self->srv2)); >> EXPECT_EQ(-EACCES, TEST_CONNECT(&self->srv2)); >> } else { >> TEST_UNRESTRICTED_NET_FIXTURE(&self->srv1); >> TEST_UNRESTRICTED_NET_FIXTURE(&self->srv2); >> } >> >> I suggest leaving "test_restricted_net_fixture()" and refactor this >> booleans (in the way you suggested) in order not to lose simplicity in >> the testing: >> >> bool restricted = is_restricted(&variant->prot, >> variant->sandbox); >> >> test_restricted_net_fixture(_metadata, &self->srv0, >> (struct expected_net_enforcement){ >> .deny_bind = false, >> .deny_connect = false, >> .deny_listen = false >> }); >> test_restricted_net_fixture(_metadata, &self->srv1, >> (struct expected_net_enforcement){ >> .deny_bind = false, >> .deny_connect = restricted, >> .deny_listen = false >> }); >> test_restricted_net_fixture(_metadata, &self->srv2, >> (struct expected_net_enforcement){ >> .deny_bind = restricted, >> .deny_connect = restricted, >> .deny_listen = restricted >> }); >> >> But it's really not obvious design issue and splitting helper can really >> be a better solution. WDYT? >> >>> >>> >>> 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.) >> >> Agreed, this is a best option for refactoring. >> >> I've also tried adding access_mask field to the service_fixture struct >> with all accesses allowed by default. In a test, then you just need to >> remove the necessary accesses after sandboxing: >> >> if (is_restricted(&variant->prot, variant->sandbox)) >> clear_access(&self->srv2, >> LANDLOCK_ACCESS_NET_BIND_TCP | >> LANDLOCK_ACCESS_NET_CONNECT_TCP); >> >> test_restricted_net_fixture(_metadata, &self->srv2); >> >> But this solution is too implicit for the helper. Passing struct would >> be better. > > What about passing the variant to these tests and creating more > fine-grained is_restricted_*() helpers? Do you mean making is_restricted_{bind|connect|listen}()? We can't identify which access rights are restricted for the port if we pass only `variant` to test_bind_and_connect(). > >> >>> >>>> +} >>>> + >>>> TEST_F(protocol, bind_unspec) >>>> { >>>> const struct landlock_ruleset_attr ruleset_attr = { >>>> -- >>>> 2.34.1 >>>> >>> >>> —Günther >>
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); + 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)); + } + 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); +} + TEST_F(protocol, bind_unspec) { const struct landlock_ruleset_attr ruleset_attr = {
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(+)