Skip to content
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

test(s2n-quic): show how to block clients on handshake confirmation #1766

Merged
merged 1 commit into from
May 19, 2023

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented May 16, 2023

Description of changes:

This change adds a test that shows an example of an event provider that blocks the client from opening any streams until the handshake is completely confirmed by the server.

When using mTLS, the client is technically allowed to start sending application data before the server has actually authenticated the client. For clients that want to wait before getting a confirmation of the server accepting the client's certificate, they can use the ClientConfirm struct as an event provider on the Client.

Note that waiting for handshake confirmation will add another round trip to the handshake so keep that in mind when deciding if this functionality is useful.

This module includes 3 distinct tests to show the behavior:

  • The mtls_happy_case creates a successful connection and sends some data
  • The mtls_failure_no_wait shows the case when the wait_ready functionality is not used. The client opens a connection and stream, sends some data, only to find that the server rejected the client's certificate when trying to receive the response.
  • The mtls_failure_with_wait shows that when using the wait_ready functionality the client is unable to open a stream, as the task was blocked until the server either confirmed or rejected the connection attempt.

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

@camshaft camshaft force-pushed the camshaft/client-confirm-test branch from 3306d58 to 89bc81e Compare May 16, 2023 21:38
@camshaft camshaft marked this pull request as ready for review May 16, 2023 21:39
@WesleyRosenblum
Copy link
Contributor

Any reason this can't go in examples?

Comment on lines +183 to +189
stream.send(vec![42; LEN].into()).await.unwrap();
stream.finish().unwrap();

let mut recv_len = 0;
while let Some(chunk) = stream.receive().await.unwrap() {
recv_len += chunk.len();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i get the sending data but how are we receiving data? I dont see how the client is receiving data (where is the server sending data?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default server implementation just echos back everything from the client.

let mut stream = conn.open_bidirectional_stream().await.unwrap();

stream.send(vec![42; LEN].into()).await.unwrap();
stream.finish().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want flush here instead of finish for the happy case? or this does seem to be acting like echo so i guess receiving confirms we sent stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't really need to flush; that'll block the task until the server ACKs the whole thing. But we do need to call finish, which will tell the server that we're done sending. If we don't call finish here, the connection will hang /deadlock cause the client is waiting on receiving data from the server, which is still waiting receiving more data from the client.

@camshaft
Copy link
Contributor Author

Any reason this can't go in examples?

It can; it just makes it a lot harder to test. I don't know if I anticipate this being used that much so IMO it's fine to stay as a test that we can point people to if needed. If it starts getting more demand, then we can move it. Thoughts?

@WesleyRosenblum
Copy link
Contributor

Any reason this can't go in examples?

It can; it just makes it a lot harder to test. I don't know if I anticipate this being used that much so IMO it's fine to stay as a test that we can point people to if needed. If it starts getting more demand, then we can move it. Thoughts?

That's fine, but what makes it harder to test as an example?

@toidiu
Copy link
Contributor

toidiu commented May 19, 2023

Any reason this can't go in examples?

It can; it just makes it a lot harder to test. I don't know if I anticipate this being used that much so IMO it's fine to stay as a test that we can point people to if needed. If it starts getting more demand, then we can move it. Thoughts?

That's fine, but what makes it harder to test as an example?

I believe the reason is that we cant really run an example in CI (launch a server and client and check for correct behavior). However, we can do that as part of the integration test.

@camshaft
Copy link
Contributor Author

Yeah we don't currently have a good way to run examples as part of CI. We check that they build but I definitely wanted so automation around this test to show it working.

@camshaft camshaft merged commit c1a0e5e into main May 19, 2023
@camshaft camshaft deleted the camshaft/client-confirm-test branch May 19, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants