-
Notifications
You must be signed in to change notification settings - Fork 724
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 Rust bindings for certificate chains #4398
Conversation
// | ||
// s2n_cert_chain_and_key objects can be shared across threads. | ||
// | ||
// (TODO: Verify? Connection isn't for some reason.) |
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.
Yeah we removed Sync from the Connection a bit ago because it technically isn't Sync right now.
The interesting thing here is that calling s2n_connection_get_peer_cert_chain copies the certificate chain
from the s2n_connection to the s2n_cert_chain_and_key. Which I think is important, the memory for this cert chain isn't associated with the s2n_connection at all (and therefore doesn't need to be either Sync nor Send?) However, if you get the chain from the function s2n_connection_get_selected_cert(currently not implemented in your PR), it is a pointer to memory inside of an s2n_connection struct.
🤔 Maybe the safest thing is to have this only impl Send, like the Connection? I actually don't think it's needed with your current PR, but if we ever added s2n_connection_get_selected_cert to the rust bindings I think we would need it.
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.
Seems OK. Even if this is a pointer into the Connection it's not obvious that prevents us from implementing Sync (FWIW, I didn't really find rationale for why not in the other thread, maybe we should document that somewhere) -- if the certificate itself is mostly just a bag of bytes, then it seems OK for it to be Sync even if the wider connection isn't.
However, if you get the chain from the function s2n_connection_get_selected_cert(currently not implemented in your PR), it is a pointer to memory inside of an s2n_connection struct.
Given this might be the case, I'll go ahead and add a lifetime to CertificateChain so that we can implement that API later on.
2315aac
to
9e2b9c0
Compare
} | ||
} | ||
|
||
pub fn len(&self) -> Result<u32, Error> { |
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.
This should potentially be a usize to be more consistent with Rust idioms. But the length returned by s2n is a u32. Not sure what our preference is in such a case.
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.
I think I would prefer a usize
cast here. In hindsight, the C code should have returned a size_t
.
I'm also wondering if we really want to return a Result
here... The things that can go wrong is either the pointers are null (which isn't possible with this abstraction) or there aren't any certs in the chain, in which case we should just unwrap_or(0)
.
s2n-tls/crypto/s2n_certificate.c
Lines 642 to 648 in 20010e6
int s2n_cert_chain_get_length(const struct s2n_cert_chain_and_key *chain_and_key, uint32_t *cert_length) | |
{ | |
POSIX_ENSURE_REF(chain_and_key); | |
POSIX_ENSURE_REF(cert_length); | |
struct s2n_cert *head_cert = chain_and_key->cert_chain->head; | |
POSIX_ENSURE_REF(head_cert); |
9e2b9c0
to
a1e8661
Compare
Fixed clippy/rustfmt. |
} | ||
} | ||
|
||
pub fn len(&self) -> Result<u32, Error> { |
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.
I think I would prefer a usize
cast here. In hindsight, the C code should have returned a size_t
.
I'm also wondering if we really want to return a Result
here... The things that can go wrong is either the pointers are null (which isn't possible with this abstraction) or there aren't any certs in the chain, in which case we should just unwrap_or(0)
.
s2n-tls/crypto/s2n_certificate.c
Lines 642 to 648 in 20010e6
int s2n_cert_chain_get_length(const struct s2n_cert_chain_and_key *chain_and_key, uint32_t *cert_length) | |
{ | |
POSIX_ENSURE_REF(chain_and_key); | |
POSIX_ENSURE_REF(cert_length); | |
struct s2n_cert *head_cert = chain_and_key->cert_chain->head; | |
POSIX_ENSURE_REF(head_cert); |
This exposes existing functionality to retrieve peer & self certificate chains after handshakes from the Connection object.
a1e8661
to
a0a62f0
Compare
Filed #4428 to check license headers for Rust code in the future. |
This exposes existing functionality to retrieve peer certificate chains and selected certificates after handshakes from the Connection object.
Description of changes:
Adding Rust bindings for existing APIs. No new real functionality.
Call-outs:
Several of the C APIs take non-const pointers to memory (see comments in code). The current Rust APIs assume that's just a poorly "documented" API and pass pointers to &[u8] slices in Rust. It would be UB for the C code to write through those pointers. I spot-checked the APIs a little and it looks like this comes down to the stuffer APIs lacking a const-safe version; I don't think there are any actual writes but it's hard to be certain since the memory is passed through quite a few functions. I'm open to making the Rust APIs take &mut [u8], but it would be unfortunate to do so.
Please also take a look at the manual Send/Sync impls which are mostly guesses, I don't know the internals of these APIs enough to say whether those are accurate.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
Tests added using existing framework. This only checks a single cert chain, we'd need to check-in certificates with longer chains and make use of them to validate more. However we do check that we're not looping on that single cert in the iterator, so it's probably OK.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.