test: add self talk test using AF_INET socket #3807

toidiu · 2023-02-03T22:47:49Z

Description of changes:

socket man page: https://man7.org/linux/man-pages/man2/socket.2.html

Our self talk test use AF_UNIX, which is a local link. This PR adds a self_talk test which uses AF_INET (ipv4 protocols). Using AF_INET is going to be necessary to test kTLS functionality since its not possible to enable kTLS for a AF_UNIX socket.

Testing:

self talk test

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

toidiu · 2023-02-09T05:18:43Z

OpenBSD fails with. Thinking that this is because we run the test in a OpenBSD VM rather than an actual instance. Planning to ignore running the test on that platform to get around this.

 154/233 Test #154: s2n_self_talk_inet_socket_test ...................***Timeout 1500.14 sec
  FAILED test 8
  !(((connect(fd, (struct sockaddr *) &saddr, addrlen))) == (-1)) is not true  (/Users/runner/work/s2n-tls/s2n-tls/tests/unit/s2n_self_talk_inet_socket_test.c:167)
  Error Message: 'Unimplemented feature'
   Debug String: 'Error encountered in /Users/runner/work/s2n-tls/s2n-tls/error/s2n_errno.c:465'
   System Error: Invalid argument (22)

toidiu · 2023-02-09T21:22:20Z

fix applied in ffc9b52

OpenBSD fails with. Thinking that this is because we run the test in a OpenBSD VM rather than an actual instance. Planning to ignore running the test on that platform to get around this.

 154/233 Test #154: s2n_self_talk_inet_socket_test ...................***Timeout 1500.14 sec
  FAILED test 8
  !(((connect(fd, (struct sockaddr *) &saddr, addrlen))) == (-1)) is not true  (/Users/runner/work/s2n-tls/s2n-tls/tests/unit/s2n_self_talk_inet_socket_test.c:167)
  Error Message: 'Unimplemented feature'
   Debug String: 'Error encountered in /Users/runner/work/s2n-tls/s2n-tls/error/s2n_errno.c:465'
   System Error: Invalid argument (22)

OpenBSD was failing due the use of INADDR_ANY (network ip). Using INADDR_LOOPBACK 4007dbc fixes the issue but it causes MacOS/FreeBSD to fail (which is why I was using INADDR_ANY).

https://stackoverflow.com/a/34042435 describers the issue in MacOS. To get around this I am going to define the value myself. This is safe because the hex value 0x7f000001 translates to 127.0.0.1 so its a literal conversion and there is no magic translation going on.

#define S2N_TEST_INADDR_LOOPBACK 0x7f000001 /* 127.0.0.1 */

tests/unit/s2n_self_talk_inet_socket_test.c

maddeleine · 2023-02-10T22:22:51Z

tests/unit/s2n_self_talk_inet_socket_test.c

+
+    /* Setup config */
+    EXPECT_SUCCESS(s2n_connection_set_blinding(server_conn, S2N_SELF_SERVICE_BLINDING));
+    EXPECT_EQUAL(s2n_connection_get_delay(server_conn), 0);


Isn't this function only used to see if blinding was invoked? I'm not sure why you'd check it before actually attempting a connection.

it was a sanity check for myself when writing the test. The previous version with NON_BLOCKING sockets and sleeps required this. I can remove it.

maddeleine · 2023-02-10T22:25:42Z

tests/unit/s2n_self_talk_inet_socket_test.c

+    EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS12);
+
+    char sync = 0;
+    char send_buffer[1] = { 0 };


Suggested change

char send_buffer[1] = { 0 };

char send_buffer[] = { 0 };

Nit: In C you don't need to specify how large the buffer is if you're initializing it with elements.

maddeleine · 2023-02-10T22:29:15Z

tests/unit/s2n_self_talk_inet_socket_test.c

+    return S2N_RESULT_OK;
+}
+
+int main(int argc, char **argv)


I'm not exactly sure what the point of this test is. You seem to be testing that you can negotiate and send/recv data on a special socket? But I'm not sure why that needs to be tested. Maybe you could add a comment explaining the purpose of this test, as it's not obvious to me.
Are we planning on adding more to this test in the future?

I mention the reason for this in the PR description. I dont want to add that in the code because this test really has nothing to do with kTLS (it just established the base case).

Adding comments about kTLS is confusing because we could add tests similar to this that dont deal with kTLS. The only special thing about this test is that it uses a AF_INET socket. Thats actually huge because thats what our customers use.

If the test has nothing to do with kTLS, then we should have a comment describing what it's testing without mentioning kTLS.

Yeah, to me it doesn't really look like you're testing s2n here. It looks like you're testing the INET domain, which isn't really related to s2n. Is the point of this test to get a technique/method in place so that you can test KTLS in the future?

tests/unit/s2n_self_talk_inet_socket_test.c

harrisonkaiser · 2023-02-10T22:32:47Z

tests/unit/s2n_self_talk_inet_socket_test.c

+    int sync_pipe[2] = { 0 };
+    EXPECT_SUCCESS(pipe(sync_pipe));
+
+    pid_t child = fork();


I'm uncertain, but I thought we moved away from using explicit forks in our tests. -- Although I'm not sure what they got replaced with.

They were replaced with s2n_negotiate_test_server_and_client + non-blocking io sockets or stuffers. You use forks if both sides need to be talking at the same time. If they're going to alternate talking, you don't need forks.

Talked to Apoorv offline: he DOES need both sides talking at the same time for the socket listen/connect part. But I wonder if we can isolate just that part with threads or something so that we can otherwise keep everything in the same process?

harrisonkaiser · 2023-02-10T22:34:33Z

tests/unit/s2n_self_talk_inet_socket_test.c

+
+        RESULT_GUARD_POSIX(read(read_pipe, &sync, 1));
+        RESULT_GUARD_POSIX(s2n_recv(client_conn, recv_buffer, 1, &blocked));
+        RESULT_ENSURE_EQ(memcmp(&CHAR_B, &recv_buffer[0], 1), 0);


Can't these just use EXPECT_EQ? Why call memcmp to compare a single byte?

lrstewart

If this test isn't explicitly for ktls, I'm not sure I understand its purpose. If it is for ktls, then you're likely going to want multiple self-talk tests, and you're likely going to want non-blocking IO. This test won't work for either of those cases.

Maybe what you want is a version of s2n_test_io_pair that uses an AF_INET socket.

lrstewart · 2023-02-10T22:56:09Z

tests/unit/s2n_self_talk_inet_socket_test.c

+
+static S2N_RESULT start_client(int fd, int read_pipe)


This model is fine for a one-off test, but it makes writing self-talk tests extremely difficult. You've got a single self-talk test in this whole file :) I would strongly suggest looking into getting this working with s2n_negotiate_test_server_and_client.

lrstewart · 2023-02-10T22:58:45Z

tests/unit/s2n_self_talk_inet_socket_test.c

+    s2n_blocked_status blocked = S2N_NOT_BLOCKED;
+    RESULT_GUARD_POSIX(s2n_negotiate(client_conn, &blocked));


This works because you're using blocking sockets. If you were using non-blocking sockets, it would be possible for s2n_negotiate to fail with S2N_ERR_IO_BLOCKING and you'd need to handle that.

toidiu · 2023-02-13T21:18:17Z

spoke offline and while we will eventually want a INET enabled socket, we will only want it to test kTLS. So I am closing this PR until kTLS is complete and testable

toidiu · 2023-02-13T21:21:31Z

To keep the context and conclusion in one place: #3807 (comment)

Talked to Apoorv offline: he DOES need both sides talking at the same time for the socket listen/connect part. But I wonder if we can isolate just that part with threads or something so that we can otherwise keep everything in the same process?

The current testing strategy(fork) is too verbose and the reason why we moved away from it. Condensing the setup into a setup function and then only including the testing functionality in the test is what we want ideally.

toidiu · 2023-06-27T06:10:17Z

Continuing this in #4075

github-actions bot added the s2n-core team label Feb 3, 2023

toidiu force-pushed the ak-ktlsx_test_selftalk_keyupdate branch 2 times, most recently from 49ccf81 to c260588 Compare February 3, 2023 22:53

toidiu marked this pull request as ready for review February 3, 2023 22:55

toidiu requested a review from camshaft February 3, 2023 22:56

toidiu mentioned this pull request Feb 3, 2023

Tracking issue: kTLS MVP #3711

Open

32 tasks

toidiu marked this pull request as draft February 6, 2023 06:05

toidiu force-pushed the ak-ktlsx_test_selftalk_keyupdate branch 5 times, most recently from 1565f98 to 708a342 Compare February 9, 2023 02:00

toidiu marked this pull request as ready for review February 9, 2023 05:21

camshaft reviewed Feb 10, 2023

View reviewed changes

toidiu added 4 commits February 10, 2023 12:38

test: add self talk test using real AF_INET socket

7e4e61c

attempt INADDR_LOOPBACK

c0a13e6

define S2N_TEST_INADDR_LOOPBACK

be90766

init, cleanup and blocking socket mode

ad22c2e

toidiu force-pushed the ak-ktlsx_test_selftalk_keyupdate branch from ffc9b52 to ad22c2e Compare February 10, 2023 20:38

camshaft approved these changes Feb 10, 2023

View reviewed changes

toidiu requested review from maddeleine and lrstewart February 10, 2023 21:58

maddeleine reviewed Feb 10, 2023

View reviewed changes

harrisonkaiser reviewed Feb 10, 2023

View reviewed changes

lrstewart reviewed Feb 10, 2023

View reviewed changes

toidiu closed this Feb 13, 2023

toidiu mentioned this pull request Jun 27, 2023

ktls: self talk inet socket test #4075

Merged

toidiu deleted the ak-ktlsx_test_selftalk_keyupdate branch July 11, 2024 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add self talk test using AF_INET socket #3807

test: add self talk test using AF_INET socket #3807

toidiu commented Feb 3, 2023

toidiu commented Feb 9, 2023

toidiu commented Feb 9, 2023 •

edited

Loading

maddeleine Feb 10, 2023

toidiu Feb 10, 2023 •

edited

Loading

maddeleine Feb 10, 2023

maddeleine Feb 10, 2023 •

edited

Loading

toidiu Feb 10, 2023

lrstewart Feb 10, 2023 •

edited

Loading

maddeleine Feb 10, 2023

harrisonkaiser Feb 10, 2023

lrstewart Feb 10, 2023

lrstewart Feb 11, 2023 •

edited

Loading

harrisonkaiser Feb 10, 2023

lrstewart left a comment

lrstewart Feb 10, 2023

lrstewart Feb 10, 2023

toidiu commented Feb 13, 2023

toidiu commented Feb 13, 2023

toidiu commented Jun 27, 2023

		s2n_blocked_status blocked = S2N_NOT_BLOCKED;
		RESULT_GUARD_POSIX(s2n_negotiate(client_conn, &blocked));

test: add self talk test using AF_INET socket #3807

test: add self talk test using AF_INET socket #3807

Conversation

toidiu commented Feb 3, 2023

Description of changes:

Testing:

toidiu commented Feb 9, 2023

toidiu commented Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

toidiu Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddeleine Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrstewart Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrstewart Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrstewart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toidiu commented Feb 13, 2023

toidiu commented Feb 13, 2023

toidiu commented Jun 27, 2023

toidiu commented Feb 9, 2023 •

edited

Loading

toidiu Feb 10, 2023 •

edited

Loading

maddeleine Feb 10, 2023 •

edited

Loading

lrstewart Feb 10, 2023 •

edited

Loading

lrstewart Feb 11, 2023 •

edited

Loading