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): verify connection success/failure when mTLS is required #1725

Merged
merged 7 commits into from
May 2, 2023

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Apr 24, 2023

Description of changes:

This PR adds some integration testing around connection establishment when mTLS negotiation is required. There are two test:

  • happy-path: test when mTLS is required and successful. We expect the handshake to complete and data to be sent/received
  • failure-case: test when handshake fails specifically due to failed client-certficat-auth (mTLS). We expect client TLS handshake to succeed but the Quic handshake to fail. We also expect the server handshake to fail. Finally we expect that no data is sent by the server.

Call-outs:

  • I include some new certs for testing mTLS. These certs were generated using the script in examples/rustls-mtls. I considered modifying the rustls-mtls example to use the new certs in s2n-quic-core but the changes were getting large for minimal gain (since its testing certs)
  • An example already exists for using mTLS with rustls.
  • I created an issue to add testing for rustls since we might want to make it easier to enable mTLS for rustls also. Add mTLS testing for rustls #1726

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

@toidiu toidiu marked this pull request as ready for review April 24, 2023 17:51
@camshaft camshaft changed the title test[s2n-quic]: verify connection success when mTLS is required test(s2n-quic): verify connection success when mTLS is required Apr 24, 2023
@toidiu toidiu marked this pull request as draft April 24, 2023 21:59
@toidiu toidiu marked this pull request as ready for review April 25, 2023 18:12
@toidiu toidiu requested a review from camshaft April 25, 2023 18:13
quic/s2n-quic/src/tests.rs Outdated Show resolved Hide resolved
quic/s2n-quic/src/tests.rs Outdated Show resolved Hide resolved
quic/s2n-quic/src/tests.rs Outdated Show resolved Hide resolved
quic/s2n-quic/src/tests.rs Outdated Show resolved Hide resolved
quic/s2n-quic/src/tests.rs Outdated Show resolved Hide resolved
quic/s2n-quic/src/tests.rs Outdated Show resolved Hide resolved
quic/s2n-quic/src/tests.rs Outdated Show resolved Hide resolved
quic/s2n-quic/src/tests.rs Outdated Show resolved Hide resolved
Comment on lines 741 to 744
// restore the network after approximately half the max handshake
// timeout to allow the handshake to succeed
delay(max_handshake_duration / 2).await;
model.set_drop_rate(0.0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I choose the max_handshake / 2 after some experimentation. Its hard to pinpoint the sleep duration since there seems to be some interplay between the network delay, sleep and max handshake duration.

However, since we are testing all the various combinations I think it sets a good baseline for us to work from. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spoke offline. The delay and loss wasnt providing much value and I decided to remove it. Instead I added a success and failure case for mTLS testing.

@toidiu toidiu changed the title test(s2n-quic): verify connection success when mTLS is required test(s2n-quic): verify connection success/failure when mTLS is required May 2, 2023
@camshaft
Copy link
Contributor

camshaft commented May 2, 2023

I was really hoping to have the failure test to show that the server doesn't even accept the connection and yield to the application at all. Here's my recommended changes:
mtls.patch

@toidiu toidiu merged commit 8681fed into main May 2, 2023
@toidiu toidiu deleted the ak-mtlsTesting branch May 2, 2023 22:09
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.

2 participants