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

feat(s2n-quic): Add the certificate chain to TlsSession #2349

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

Mark-Simulacrum
Copy link
Collaborator

Description of changes:

This allows users to query the certificate chain when in the TlsSession event callbacks (for dc or for on_tls_exporter_ready).

Call-outs:

The new API is ~immediately stable, which feels unfortunate -- the Vec<Vec> is clunky and not really what we'd ideally have. However improving on that is also tricky, so maybe it's not worth optimizing much here. s2n-tls under the hood has something closer to a linked list of buffers, with that represented by multiple layers of indirection to Rust as a Iterator<Item = &[u8]>. I'm pretty sure we can't directly expose a -> impl Iterator though, since the lifetimes don't end up working out.

Testing:

Added a test showing the newly added functionality works. Note that it's mTLS, the peer_cert_chain_der method will return an Err if the other side didn't present any certs.

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

@Mark-Simulacrum Mark-Simulacrum changed the title feat(s2n-quic): Add the certififcate chain to TlsSession feat(s2n-quic): Add the certificate chain to TlsSession Oct 11, 2024
@Mark-Simulacrum Mark-Simulacrum force-pushed the expose-cert-chain branch 3 times, most recently from 9a3b5b7 to 34a12dd Compare October 14, 2024 14:53
This allows users to query the certificate chain when in the TlsSession
event callbacks (for dc or for on_tls_exporter_ready). The API added
here allocates a Vec<Vec<u8>> which is plausibly quite wasteful, so the
API is for now doc(hidden) to avoid stabilizing it.
@@ -45,6 +60,9 @@ pub trait TlsSession: Send {
) -> Result<(), TlsExportError>;

fn cipher_suite(&self) -> CipherSuite;

#[cfg(feature = "alloc")]
fn peer_cert_chain_der(&self) -> Result<Vec<Vec<u8>>, ChainError>;
Copy link
Contributor

@camshaft camshaft Oct 14, 2024

Choose a reason for hiding this comment

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

Would it be worth making the returned value an associated type+trait instead? Like you said, the Vecs are a bit clunky and it would be nice to avoid if we can.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It requires lots of layers -- at least with the s2n-tls API currently -- since there's lots of layers of indirection involved. I started out writing that up but didn't manage to make it super nice.

I think we could try to "hide" the Vec here behind a trait (e.g., Iterator<Item = &[u8]> I think could work) but it seems likely that is not going to work for some future case (at least without allocating here).

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can always add a more complicated interface later, if needed.

@camshaft camshaft enabled auto-merge (squash) October 14, 2024 17:29
@camshaft camshaft merged commit fe96450 into aws:main Oct 14, 2024
119 checks passed
@Mark-Simulacrum Mark-Simulacrum deleted the expose-cert-chain branch October 14, 2024 19:55
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