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

Add client authentication support for rustls #2129

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thynson
Copy link

@thynson thynson commented Feb 16, 2024

Description of changes:

This PR adds support of client authentication to s2n-quic-rustls

Call-outs:

Testing:

I can write an example for it, but I'm not sure how it's tested automatically yet.

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

@maddeleine
Copy link
Contributor

maddeleine commented Feb 19, 2024

Note that it is already possible to do client authentication in s2n-quic using rustls. Here's an example of how it's done: https://github.com/aws/s2n-quic/tree/main/examples/rustls-mtls. It involves implementing a TLS provider in order to enable client auth in rustls. Is there some reason why that example doesn't work for you/why you need to add this feature to the s2n-quic-rustls crate?

Edit: Actually, we should probably enable client auth in rustls without having to impl the TLS provider. The rustls client auth example we have is a bit heavy handed.

@thynson
Copy link
Author

thynson commented Feb 20, 2024

Of course anyone can use their own Provider to implement mTLS, but why not to make it into the s2n-quic-rustls, so that it can be as easy as using s2n-quic-tls?

@camshaft
Copy link
Contributor

camshaft commented Mar 7, 2024

We're wanting to update the rustls dependency first (see #2143) and then we can get this change in

@thynson
Copy link
Author

thynson commented Mar 8, 2024

I'll continue this PR after #2143 merged, and I want to resolve #1957 in this PR too.

@SergioBenitez SergioBenitez mentioned this pull request Mar 19, 2024
2 tasks
@thynson thynson marked this pull request as ready for review March 24, 2024 14:31
@thynson
Copy link
Author

thynson commented Mar 24, 2024

The rustls-mtls example was updated to demonstrate the stuffes introduced in this PR.

Some off-topic thoughts: IntoCertificate and IntoPrivateKey make it easier to deal with certificates and keys. However, their behavior depends on the filename extension and performs sync filesystem I/O. Probably it'd be better to expose some low-level API that accepts CertificateDer and PrivateKeyDer instead.

@thynson
Copy link
Author

thynson commented Mar 24, 2024

I'll continue this PR after #2143 merged, and I want to resolve #1957 in this PR too.

Regarding #1957, correct me if I'm wrong, but it seems not only an issue for rustls, but also applies to s2n-tls. Some change to Connection needs to be made before it can be implemented, which is out of my ability and interest currently.

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.

None yet

3 participants