On 16/05/2022 17:20, Konstantin Meskhidze wrote:

Please create a commit which moves all the needed code for all network 
tests. I think there is only this helper though.

You add whitespace changes.

"Number of pending connection queues to be hold." maybe? This is not use 
in this patch so it shouldn't be added by this patch.

+
+static int create_socket(struct __test_metadata *const _metadata,
+			bool ip6, bool reuse_addr)

This helper is good and I think you can improve it by leveraging test 
variants. You could even factor out all the ipv4/ipv6 tests thanks to 
new helpers such as bind_variant() and connect_variant(). No need to add 
_metadata to those though. This would avoid duplicating all ipv4/ipv6 
tests and even simplifying bind() and connect() calls. Something like this:

// rename "socket_test" to "socket" (no need to duplicate "test")
FIXTURE_VARIANT(socket)
{
	const bool is_ipv4;
};

/* clang-format off */
FIXTURE_VARIANT_ADD(socket, ipv4) {
	/* clang-format on */
	.is_ipv4 = true,
};

/* clang-format off */
FIXTURE_VARIANT_ADD(socket, ipv6) {
	/* clang-format on */
	.is_ipv4 = false,
};

static int socket_variant(const FIXTURE_VARIANT(socket) *const variant, 
const int type)
{
	if (variant->is_ipv4)
		return socket(AF_INET, type | SOCK_CLOEXEC, 0);
	else
		return socket(AF_INET6, type | SOCK_CLOEXEC, 0);
}

socket_variant(variant, SOCK_STREAM);
// this could be used to create UDP sockets too


static int bind_variant(const FIXTURE_VARIANT(socket) *const variant, 
const int sockfd, const FIXTURE_DATA(socket) *const self, const size_t 
index)
{
	if (variant->is_ipv4)
		return bind(sockfd, &self->addr4[index], sizeof(self->addr4[index]));
	else
		return bind(sockfd, &self->addr6[index], sizeof(self->addr6[index]));
}

bind_variant(variant, sockfd, self, 0);

+{
+		int sockfd;
+		int one = 1;
+
+		if (ip6)
+			sockfd = socket(AF_INET6, SOCK_STREAM | SOCK_CLOEXEC, 0);
+		else
+			sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
+
+		ASSERT_LE(0, sockfd);
+		/* Allows to reuse of local address */
+		if (reuse_addr)
+			ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET,
+					SO_REUSEADDR, &one, sizeof(one)));

This reuse_addr part is not used in this patch and I think it would 
simplify this helper to not add reuse_addr but to explicitely call 
setsockopt() when required. This also enables to get rid of _metadata in 
this helper.

+		return sockfd;
+}
+
+FIXTURE(socket_test) {
+	uint port[MAX_SOCKET_NUM];
+	struct sockaddr_in addr4[MAX_SOCKET_NUM];
+	struct sockaddr_in6 addr6[MAX_SOCKET_NUM];
+};
+
+FIXTURE_SETUP(socket_test)
+{
+	int i;
+	/* Creates IP4 socket addresses */
+	for (i = 0; i < MAX_SOCKET_NUM; i++) {

Nice!

+		self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i;
+		self->addr4[i].sin_family = AF_INET;
+		self->addr4[i].sin_port = htons(self->port[i]);
+		self->addr4[i].sin_addr.s_addr = htonl(INADDR_ANY);

Could you use the local addr (127.0.0.1) instead?

+		memset(&(self->addr4[i].sin_zero), '\0', 8);
+	}
+
+	/* Creates IP6 socket addresses */
+	for (i = 0; i < MAX_SOCKET_NUM; i++) {
+		self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i;
+		self->addr6[i].sin6_family = AF_INET6;
+		self->addr6[i].sin6_port = htons(self->port[i]);
+		self->addr6[i].sin6_addr = in6addr_any;

ditto

+	}
+
+	set_cap(_metadata, CAP_SYS_ADMIN);
+	ASSERT_EQ(0, unshare(CLONE_NEWNET));
+	ASSERT_EQ(0, system("ip link set dev lo up"));

If this is really required, could you avoid calling system() but set up 
the network in C? You can strace it to see what is going on underneath.

+	clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+FIXTURE_TEARDOWN(socket_test)
+{ }
+
+TEST_F_FORK(socket_test, bind_no_restrictions_ip4) {
+
+	int sockfd;
+
+	sockfd = create_socket(_metadata, false, false);
+	ASSERT_LE(0, sockfd);
+
+	/* Binds a socket to port[0] */

This comment is not very useful in this context considering the below 
line. It will be even more clear with the bind_variant() call.

+	ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], sizeof(self->addr4[0])));
+
+	ASSERT_EQ(0, close(sockfd));
+}
+
+TEST_F_FORK(socket_test, bind_no_restrictions_ip6) {
+
+	int sockfd;
+
+	sockfd = create_socket(_metadata, true, false);
+	ASSERT_LE(0, sockfd);
+
+	/* Binds a socket to port[0] */
+	ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], sizeof(self->addr6[0])));
+
+	ASSERT_EQ(0, close(sockfd));
+}
+
+TEST_F_FORK(socket_test, bind_with_restrictions_ip4) {
+
+	int sockfd;
+
+	struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
+				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
+	};
+	struct landlock_net_service_attr net_service_1 = {
+		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
+				  LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		.port = self->port[0],
+	};
+	struct landlock_net_service_attr net_service_2 = {
+		.allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		.port = self->port[1],
+	};
+	struct landlock_net_service_attr net_service_3 = {
+		.allowed_access = 0,
+		.port = self->port[2],
+	};
+
+	const int ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+			sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	/* Allows connect and bind operations to the port[0] socket. */

This comment is useful though because the below call is more complex.

+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
+				&net_service_1, 0));
+	/* Allows connect and deny bind operations to the port[1] socket. */
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
+				&net_service_2, 0));
+	/* Empty allowed_access (i.e. deny rules) are ignored in network actions
+	 * for port[2] socket.
+	 */
+	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
+				&net_service_3, 0));
+	ASSERT_EQ(ENOMSG, errno);
+
+	/* Enforces the ruleset. */
+	enforce_ruleset(_metadata, ruleset_fd);
+
+	sockfd = create_socket(_metadata, false, false);
+	ASSERT_LE(0, sockfd);
+	/* Binds a socket to port[0] */
+	ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], sizeof(self->addr4[0])));
+
+	/* Close bounded socket*/
+	ASSERT_EQ(0, close(sockfd));
+
+	sockfd = create_socket(_metadata, false, false);
+	ASSERT_LE(0, sockfd);
+	/* Binds a socket to port[1] */
+	ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr4[1], sizeof(self->addr4[1])));
+	ASSERT_EQ(EACCES, errno);
+
+	sockfd = create_socket(_metadata, false, false);
+	ASSERT_LE(0, sockfd);
+	/* Binds a socket to port[2] */
+	ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr4[2], sizeof(self->addr4[2])));
+	ASSERT_EQ(EACCES, errno);
+}
+
+TEST_F_FORK(socket_test, bind_with_restrictions_ip6) {
+
+	int sockfd;
+
+	struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
+				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
+	};
+	struct landlock_net_service_attr net_service_1 = {
+		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
+				  LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		.port = self->port[0],
+	};
+	struct landlock_net_service_attr net_service_2 = {
+		.allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		.port = self->port[1],
+	};
+	struct landlock_net_service_attr net_service_3 = {
+		.allowed_access = 0,
+		.port = self->port[2],
+	};
+
+	const int ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+			sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	/* Allows connect and bind operations to the port[0] socket. */
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
+				&net_service_1, 0));
+	/* Allows connect and deny bind operations to the port[1] socket. */
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
+				&net_service_2, 0));
+	/* Empty allowed_access (i.e. deny rules) are ignored in network actions
+	 * for port[2] socket.
+	 */
+	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
+				&net_service_3, 0));
+	ASSERT_EQ(ENOMSG, errno);
+
+	/* Enforces the ruleset. */
+	enforce_ruleset(_metadata, ruleset_fd);
+
+	sockfd = create_socket(_metadata, true, false);
+	ASSERT_LE(0, sockfd);
+	/* Binds a socket to port[0] */
+	ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], sizeof(self->addr6[0])));
+
+	/* Close bounded socket*/
+	ASSERT_EQ(0, close(sockfd));
+
+	sockfd = create_socket(_metadata, false, false);
+	ASSERT_LE(0, sockfd);
+	/* Binds a socket to port[1] */
+	ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr6[1], sizeof(self->addr6[1])));
+	ASSERT_EQ(EACCES, errno);
+
+	sockfd = create_socket(_metadata, false, false);
+	ASSERT_LE(0, sockfd);
+	/* Binds a socket to port[2] */
+	ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr6[2], sizeof(self->addr6[2])));
+	ASSERT_EQ(EACCES, errno);
+}
+TEST_HARNESS_MAIN
--
2.25.1