-
Notifications
You must be signed in to change notification settings - Fork 713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ktls: self talk inet socket test #4075
Conversation
6ebe9e3
to
c884991
Compare
77fe2d6
to
dae8e67
Compare
dae8e67
to
28bd94c
Compare
28bd94c
to
275025d
Compare
tests/testlib/s2n_testlib.h
Outdated
S2N_RESULT (*server_post_handshake_cb)(struct s2n_connection *conn); | ||
S2N_RESULT (*client_post_handshake_cb)(struct s2n_connection *conn); | ||
}; | ||
S2N_RESULT s2n_noop_inet_socket_cb(struct s2n_connection *conn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These definitions seem specific to s2n_self_talk_inet_socket_test
. Should they just be defined in this test? Or are you planning on using this stuff in other tests too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are building blocks for later PRs where we enable ktls selectively on send/recv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right ok - the callback struct itself makes sense. But even in that example, it seems to only be used in s2n_self_talk_inet_socket_test
. If the callback struct isn't needed outside of this test, couldn't it just be defined in the test itself rather than testlib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yea the missing piece is that there might be more than 1 inet_test file.
This one tests happy path and there are other that test alerts, or not happy path. But I guess for the purposes of this PR we can move it to the same test file. Also putting impls of the callback s2n_noop_inet_socket_cb
in a common place makes the tests less cluttered.
char sync = 0; | ||
char recv_buffer[1] = { 0 }; | ||
|
||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in its own scope? Is the plan to add more send/receive tests here?
Would you not want to be able to send and receive specific data for some kTLS tests? Like if there's a specific test that you want to fail after sending/receiving something? Or is it enough to just run all of your send/receive tests for all kTLS tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The { }
is possibly bad practice of scoping 'important' stuff from me. It might be good to see how this is going to be used later. I have a link in the description.
So while we only have client/server_post_handshake_cb
hooks its possible to add more later on. The idea of this testing is to run some set of tests for userspace tls and then run the same set of tests for when kTLS is enabled. Behavior should be exactly the same to the application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra scoping seems a bit odd, but could just be me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no its definitely odd and i have gotten comments on other PRs since this one. Ill remove it: 42582bd
EXPECT_SUCCESS(s2n_connection_set_blinding(server_conn, S2N_SELF_SERVICE_BLINDING)); | ||
EXPECT_EQUAL(s2n_connection_get_delay(server_conn), 0); | ||
EXPECT_SUCCESS(s2n_connection_set_fd(server_conn, fd)); | ||
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will none of this setup stuff change? For example, what if you wanted to test kTLS with TLS 1.2 vs 1.3? It seems like the start_server/start_client functions will make it hard to customize each kTLS test. Did you consider something more flexible like how the self talk tests work in s2n_self_talk_alerts_test? In this file, it looks like each test sets up their own s2n_connection and forks separately, so less reused code but more flexible maybe. Not sure, though, maybe you won't really need to change the setup between tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its going to be possible to add more complexity by adding more hooks to struct self_talk_inet_socket_callbacks
.
Since both start_server/client take the callback struct, its going to be pretty easy to customize behavior later on. But maybe I am mis-understanding your concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope - just wasn't sure how it was going to be configured. Adding to the callback struct makes sense. To me it seems a bit more confusing this way than just writing each test by itself exactly how you want it, but I can see why you probably want to reuse stuff with all the setup needed for forking and stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the only configuration that needs to happen is the config, a better solution will probably be to pass the config into these methods rather than creating it here.
b808cda
to
42582bd
Compare
07e08b3
to
bfdd5c2
Compare
EXPECT_SUCCESS(s2n_connection_set_blinding(server_conn, S2N_SELF_SERVICE_BLINDING)); | ||
EXPECT_EQUAL(s2n_connection_get_delay(server_conn), 0); | ||
EXPECT_SUCCESS(s2n_connection_set_fd(server_conn, fd)); | ||
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "default")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the only configuration that needs to happen is the config, a better solution will probably be to pass the config into these methods rather than creating it here.
bfdd5c2
to
7bb4d8b
Compare
7bb4d8b
to
4b68a8d
Compare
Description of changes:
Our self talk test use
AF_UNIX
, which is a local link. This PR adds a self_talk test which usesAF_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.Here is an example of what this will eventually be used like: https://github.com/toidiu/s2n-tls/blob/b7aa689c11898df970f670c851e4b8e0a262991e/tests/unit/s2n_self_talk_inet_socket_test.c#L233-L289
Additionally we can add other callbacks to make it more extensible
Call-outs:
Is it ok to add a unit which relies on AF_INET socket since unit tests are supposed to be independent of platform/networking?
The current test test a normal TLS connection. But it uses
self_talk_inet_socket_callbacks
to call methods post handshake. These hooks can be used to enable kTLS. Additionally more hooks can be created to customize sending/validating the connection.Previously attempted to add this in #3807
Testing:
Adds a self talk test
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.