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

track upstream Rustls 0.22.x alpha changes. #341

Merged
merged 19 commits into from
Nov 20, 2023
Merged

Conversation

cpu
Copy link
Member

@cpu cpu commented Aug 10, 2023

Description

This branch tracks the breaking API changes that have accrued in the upstream Rustls, Webpki and Rustls-pemfile repos for the pending 0.22.0 release.

I've chosen to try and break categories of changes into unique commits, but this means that intermediate commits won't build: they're all-or-nothing with the updated dependency versions.

TODO:

  • removing cargo patch once rustls 0.22.0-alpha.0 published on crates.io
  • rustdoc comments
  • better error handling for verifier builder
  • debug and fix crash for server w/ AUTH_CERT
  • rebase for rework CastPtr, CastConstPtr, BoxCastPtr, ArcCastPtr #353
  • rebase for Improving webpki verifier CRL support ergonomics rustls#1552
  • update for rustls alpha.6
    • webpki client verifier updates
      • revocation depth
      • unknown status policy
      • root hints
    • support new server cert verifier builder
      • roots
      • CRLs
      • revocation depth
      • unknown status policy
  • plan for custom crypto providers? - deferred for now
  • plan for aws-lc-rs provider? - deferred for now
  • ????
  • profit

src/cipher.rs Outdated Show resolved Hide resolved
@cpu cpu requested review from jsha and ctz August 10, 2023 19:48
@cpu
Copy link
Member Author

cpu commented Aug 10, 2023

cpu requested review from jsha and ctz now

Tagged for early feedback, but there's no rush to review this. The upstream 0.22.0 release is a little bit out yet.

@jsha
Copy link
Collaborator

jsha commented Aug 10, 2023 via email

@cpu
Copy link
Member Author

cpu commented Aug 10, 2023

Yep, the pattern I've used is "rustls_result always gets the return position if there is a possible error." Functions that can't error can return a pointer for convenience.

☑️ Thanks! I will fix that up.

@cpu cpu force-pushed the cpu-rustls-0.22-alpha branch 2 times, most recently from 6e988af to 0753268 Compare August 11, 2023 14:32
@cpu
Copy link
Member Author

cpu commented Aug 11, 2023

☑️ Thanks! I will fix that up.

Reworked the error handling, added a first pass at updated rustdoc comments.

src/cipher.rs Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Oct 2, 2023

🚧 Work in progress 🚧

  • Updated this branch for the latest available alphas
  • Rolled in the pki-types changes from upstream
  • Broke the client/server tests - see this comment

@cpu cpu force-pushed the cpu-rustls-0.22-alpha branch 3 times, most recently from adc4ebb to ce31cd5 Compare October 3, 2023 18:11
@jsha
Copy link
Collaborator

jsha commented Oct 3, 2023

I've paged back in some more of this context and I'm realizing there's a challenge / mis-design about how the CastConstPtr / ArcCastPtr traits work.

CastConstPtr means "on the Rust side, we can turn const *rustls_foo (C) into *const rustls::Foo (Rust) with a simple as cast."

ArcCastPtr means "on the Rust side, const *rustls_foo is the output of Arc::into_raw called on a rustls::Foo." And because CastConstPtr is a trait bound, it also carries the same meaning.

Those definitions seem potentially conflicting because the layout of an Arc<rustls::Foo> is different from the layout of a *const rustls::Foo. The layout of an Arc is defined here and here on ArcInner.

The tricky thing is that Arc::into_raw returns a pointer to the data field of ArcInner - that's past the beginning of the struct by two usizes (the strong and weak counts). That's okay, because Arc::from_raw knows how to then adjust backwards in memory by two usizes to reconstruct the original Arc. And of course it's more useful for into_raw to return a pointer to T than a pointer to an private ArcInner struct that happens to contain your T at some offset.

So, given a const * rustls_foo there are two casts we can perform:

  1. A simple as cast to *const rustls_foo, pointing to the inner data
  2. Arc::from_raw, which gets us a real Arc, including the strong and weak counts. But this runs into some subtle problems when we drop that Arc. Dropping decrements the strong count, and will very often result in freeing the pointed-to memory. That's why we have ArcCastPtr::to_arc.

So far, we've been allowing both, but I think in practice we should forbid (1) and always reconstruct the "real" type, to avoid confusion. In other words, ArcCastPtr should be disjoint from CastConstPtr rather than a subset of it.

The other thing to mention here is that the RustType associated type in CastPtr / CastConstPtr / BoxCastPtr / ArcCastPtr is supposed to be a pointee type. That is, it should not include any *, &, Box, or Arc (unless it happens to be doubly-indirect). For instance, here we have the definition for rustls_certified_key / rustls::CertifiedKey:

https://github.com/rustls/rustls/blob/ce31cd5559fcf1bc938c11c0363a4ef9f0c129f1/src/cipher.rs#L264-L269

impl CastPtr for rustls_certified_key {
    type RustType = CertifiedKey;
}

impl ArcCastPtr for rustls_certified_key {}

That represents a C-side type of const *rustls_certified_key and a Rust-side type of Arc<rustls::CertifiedKey>. Note that the associated type is type RustType = CertifiedKey, not type RustType Arc<CertifiedKey>. That's because in the helper functions (e.g. cast_const_ptr), we need to be able to tack on the correct punctuation (e.g. to say the return type is *const Self::RustType). It's not the clearest way to express things, but it's the way to make the type system do what we want from it.

In this branch, the definition for rustls_client_cert_verifier is:

impl CastPtr for rustls_client_cert_verifier {
    type RustType = Arc<dyn ClientCertVerifier>;
}

impl ArcCastPtr for rustls_client_cert_verifier {}

But ideally that should look like:

impl CastPtr for rustls_client_cert_verifier {
    type RustType = dyn ClientCertVerifier;
}

impl ArcCastPtr for rustls_client_cert_verifier {}

The problem is, as you ran into: dyn ClientCertVerifier is unsized (!Sized). The rule with unsized types is: you can only store them in pointers / smart pointers. For instance Box<dyn ClientCertVerifier> is fine; so is Arc<dyn ClientCertVerifier> and &dyn ClientCertVerifier. That should work fine for us, since we're really only dealing in pointers.

However, our implementations of ArcCastPtr / ConstCastPtr have Sized bounds. Why? I think this is honestly a mistake I made when implementing. I saw an error message saying I couldn't do something because xyz didn't have a size known at compile time, so I added a + Sized bound and that fixed it. Really, the fix should have been something like this:

 pub(crate) trait CastPtr {
-    type RustType;
+    type RustType: ?Sized;

The deal is that type parameters and associated types are Sized by default. In other words that type RustType was really acting like type RustType: Sized. But we can explicitly disavow the default Sized bound using the special ?Sized bound.

Adding the ?Sized bound to RustType in both CastPtr and CastConstPtr allows us to remove the + Sized bound for ArcCastPtr and BoxCastPtr, which in turn allows us to say RustType = dyn ClientCertVerifier.

However, then we run into some other problems: Our set_mut_ptr methods take an owned T as a parameter. That's fine when T is sized, but when T is dyn ClientCertVerifier, that doesn't work; function parameters must be sized, and there's no way around that requirement. You can't disavow the Sized requirement for function parameters.

We also run into this delightfully esoteric error message, which I vaguely see the direction of but don't really know how to fix:

error[E0606]: casting `*const <Self as CastConstPtr>::RustType` as `*const Self` is invalid
   --> src/lib.rs:402:9  
    |
402 |         Arc::into_raw(Arc::new(src)) as *const _
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: vtable kinds may not match

I think the net result of all of this is that our best bet might be to double-indirect things: Give the C side a Box<Arc<dyn ClientCertVerifier>>. That way we don't need to worry about the complexity of dyn types.

Below is a patch I was noodling on where I added the ?Sized bound. It doesn't work and is probably a dead end, but might be interesting:

diff --git a/src/cipher.rs b/src/cipher.rs
index dce36138..43cdaed2 100644
--- a/src/cipher.rs
+++ b/src/cipher.rs
@@ -582,7 +582,7 @@ pub struct rustls_client_cert_verifier {
 }
 
 impl CastPtr for rustls_client_cert_verifier {
-    type RustType = Arc<dyn ClientCertVerifier>;
+    type RustType = dyn ClientCertVerifier;
 }
 
 impl ArcCastPtr for rustls_client_cert_verifier {}
@@ -738,7 +738,7 @@ impl rustls_web_pki_client_cert_verifier_builder {
                 Err(e) => return error::map_verifier_builder_error(e),
             };
 
-            ArcCastPtr::set_mut_ptr(verifier_out, verifier);
+            ArcCastPtr::set_mut_ptr(verifier_out, verifier.as_ref());
 
             rustls_result::Ok
         }
diff --git a/src/lib.rs b/src/lib.rs
index 2c31dd10..75232a54 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -301,7 +301,7 @@ mod tests {
 /// Implementing this is required in order to use `try_ref_from_ptr!` or
 /// `try_mut_from_ptr!`.
 pub(crate) trait CastPtr {
-    type RustType;
+    type RustType: ?Sized;
 
     fn cast_mut_ptr(ptr: *mut Self) -> *mut Self::RustType {
         ptr as *mut _
@@ -311,7 +311,7 @@ pub(crate) trait CastPtr {
 /// CastConstPtr represents a subset of CastPtr, for when we can only treat
 /// something as a const (for instance when dealing with Arc).
 pub(crate) trait CastConstPtr {
-    type RustType;
+    type RustType: ?Sized;
 
     fn cast_const_ptr(ptr: *const Self) -> *const Self::RustType {
         ptr as *const _
@@ -324,6 +324,8 @@ pub(crate) trait CastConstPtr {
 impl<T, R> CastConstPtr for T
 where
     T: CastPtr<RustType = R>,
+    T: ?Sized,
+    R: ?Sized,
 {
     type RustType = R;
 }
@@ -331,7 +333,7 @@ where
 // An implementation of BoxCastPtr means that when we give C code a pointer to the relevant type,
 // it is actually a Box. At most one of BoxCastPtr or ArcCastPtr should be implemented for a given
 // type.
-pub(crate) trait BoxCastPtr: CastPtr + Sized {
+pub(crate) trait BoxCastPtr: CastPtr {
     fn to_box(ptr: *mut Self) -> Option<Box<Self::RustType>> {
         if ptr.is_null() {
             return None;
@@ -354,7 +356,7 @@ pub(crate) trait BoxCastPtr: CastPtr + Sized {
 // An implementation of ArcCastPtr means that when we give C code a pointer to the relevant type,
 // it is actually a Arc. At most one of BoxCastPtr or ArcCastPtr should be implemented for a given
 // // type.
-pub(crate) trait ArcCastPtr: CastConstPtr + Sized {
+pub(crate) trait ArcCastPtr: CastConstPtr {
     /// Sometimes we create an Arc, then call `into_raw` and return the resulting raw pointer
     /// to C. C can then call rustls_server_session_new multiple times using that
     /// same raw pointer. On each call, we need to reconstruct the Arc. But once we reconstruct the Arc,
diff --git a/src/server.rs b/src/server.rs
index bd0c1064..979f2699 100644
--- a/src/server.rs
+++ b/src/server.rs
@@ -166,7 +166,7 @@ impl rustls_server_config_builder {
     ) {
         ffi_panic_boundary! {
             let builder: &mut ServerConfigBuilder = try_mut_from_ptr!(builder);
-            let verifier = try_ref_from_ptr!(verifier);
+            let verifier = try_arc_from_ptr!(verifier);
             builder.verifier = verifier.clone();
         }
     }

@cpu
Copy link
Member Author

cpu commented Oct 4, 2023

I've paged back in some more of this context and I'm realizing there's a challenge / mis-design about how the CastConstPtr / ArcCastPtr traits work.

Thanks for the detailed reply. Complications abound 😵 It will take me a little bit to digest everything here.

It's worrying that the mis-designed implementation "works". E.g. rustc, clippy, our integration tests, and valgrind all fail to detect anything amiss. Do you have any intuition about why that might be? Are the tests too simple? Did we get "lucky" and not step on the landmine?

@cpu
Copy link
Member Author

cpu commented Oct 4, 2023

I think the net result of all of this is that our best bet might be to double-indirect things: Give the C side a Box<Arc>. That way we don't need to worry about the complexity of dyn types.

I took a crack at this. Here's what [the diff]f12aa12) entailed (writing this out to check my own understanding):

  • rustls_client_cert_verifier is changed to implement BoxCastPtr instead of ArcCastPtr.
  • The RustType for the CastPtr impl for rustls_client_cert_verifier remains Arc<dyn ClientCertVerifier> - we're OK with it not being a "pointee" type, because it's double-indirect. That is, a Box over the Arc<dyn ClientCertVerifier>.
  • The rustls_client_cert_verifier_free fn impl changes to operate on a *mut input pointer argument, which we free by using BoxCastPtr::to_box to convert from *mut rustls_client_cert_verifier to Box<Arc<dyn ClientCertVerifier>>. Internally this ends up being an as *mut conversion and the Box<Arc<dyn ClientCertVerifier>> is dropped with the stack frame, freeing the Box and appropriately decrementing the Arc reference counters.
  • The rustls_web_pki_client_cert_verifier_builder_build fn is changed to accept a *mut out-pointer. We build a Arc<dyn ClientCertVerifier> with the Rustls rustls::webpki::ClientCertVerifierBuilder's build fn, and then assign it to the out pointer with BoxCastPtr::set_mut_ptr. Internally this is constructing a Box around the Arc<dyn ClientCertVerifier>, and calling Box::into_raw to get the *mut pointer to the Box for the out pointer assignment.

Separately, I had the same issue with the new rustls_root_cert_store adjustment: it was implementing CastPtr with RustType = Arc<RootCertStore>. I think we don't need the double indirection here since RootCertStore isn't a dyn trait. So for this instance I tacked on a diff that:

  • Changes the CastPtr impl to use RustType = RootCertStore - no issues here, RootCertStore is Sized.
  • The rustls_root_cert_store_builder_build fn that creates a rustls_root_cert_store uses ArcCastPtr::set_mut_ptr to wrap the RootCertStore in an Arc<RootCertStore>. Internally it converts the Arc<RootCertStore> to a *const written to the out pointer.
  • The rustls_web_pki_client_cert_verifier_builder_new fn that takes a *const rustls_root_cert_store argument now uses try_arc_from_ptr! instead of try_ref_from_ptr! to reconstitute a Arc<RootCertStore> for the ClientCertVerifierBuilder being constructed.

I think this avoids the issues you were highlighting 🤞

@jsha
Copy link
Collaborator

jsha commented Oct 4, 2023

Yep, your summary all looks good. A couple of super pedantic points:

remains Arc - we're OK with it not being a "pointee" type, because it's double-indirect. That is, a Box over the Arc.

Really Arc<dyn ClientCertVerifier> is a pointee type in this situation because the Box points to it; but yeah this all makes sense.

The rustls_client_cert_verifier_free fn impl changes to operate on a *mut input pointer argument, which we free by using BoxCastPtr::to_box to convert from *mut rustls_client_cert_verifier to Box<Arc>. Internally this ends up being an as *mut conversion

to_box does do an as *mut conversion; it follows that with Box::from_raw, which is how we get back to a state of "here's a Rust object with ownership, which will cause memory to be freed when it is dropped at the end of the function."

It's worrying that the mis-designed implementation "works".

I agree; I need to poke at it a bit more and see what the deal is, and if there's anything to improve in terms of detection. It's possible that we weren't exercising "interesting" code paths for Arc because we only created one of a thing and didn't clone it?

@cpu
Copy link
Member Author

cpu commented Oct 4, 2023

Yep, your summary all looks good.

Great, thanks for taking a look. I can work on tidying up the commit history now that it seems like we're stabilizing on an overall approach.

I think there might also be one work item left to consider (maybe separate from this branch?): making ArcCastPtr disjoint from CastConstPtr based on your earlier comment:

So far, we've been allowing both, but I think in practice we should forbid (1) and always reconstruct the "real" type, to avoid confusion. In other words, ArcCastPtr should be disjoint from CastConstPtr rather than a subset of it.

Should I give that a go?

Really Arc is a pointee type in this situation because the Box points to it

Ah ok, good point (pun not intended 😆)

to_box does do an as *mut conversion; it follows that with Box::from_raw, which is how we get back to a state of "here's a Rust object with ownership, which will cause memory to be freed when it is dropped at the end of the function."

ACK ☑️

It's possible that we weren't exercising "interesting" code paths for Arc because we only created one of a thing and didn't clone it?

That would make sense 🤔

@jsha
Copy link
Collaborator

jsha commented Oct 5, 2023

It's worrying that the mis-designed implementation "works"... Do you have any intuition about why that might be?

I looked back at one of the earlier revisions here: 6e988af. And it turns out you had already implemented the doubly-indirect solution we eventually landed on: allocate a Box<Arc<dyn ClientCertVerifier>>, turn that outer Box into a raw *mut pointer and hand it over to C:

impl CastPtr for rustls_client_cert_verifier {
    type RustType = Arc<dyn ClientCertVerifier>;
}

impl BoxCastPtr for rustls_client_cert_verifier {}

6e988af#diff-48d27698cc708c7efe770fd897e4885efaa0a15cae30d54936068603ba03bbd9R170

let verifier: Arc<dyn ClientCertVerifier> = try_ref_from_ptr!(verifier).clone();

For more detail on what's happening in that line of code, it could be:

let verifier_ref: &Arc<dyn ClientCertVerifier> = try_ref_from_ptr!(verifier);
let verifier = verifier_ref.clone();

That's taking advantage of the fact that we can get infinitely many read-only references to the contents of a Box, even though we don't own the box. It's also taking advantage of the fact that an Arc can be cloned from an &Arc.

This could also have been:

let verifier_box: Box<Arc<dyn ClientCertVerifier>> = try_box_from_ptr!(verifier); // would also need verifier to be a `*mut`, not a *const
let verifier_ref: &Arc<dyn ClientCertVerifier> = verifier_box.as_ref();
let verifier: Arc<dyn ClientCertVerifier> = verifier.clone();
...
Box::leak(verifier_box); // make sure we don't free the box on drop, since the C side still owns it

In practice we only use try_box_from_ptr! when we want to really take ownership of the pointer, because it's too easy to leave off the Box::leak.

Anyhow, long story short: the mis-design was that it was (and is) possible to implement BoxCastPtr and ArcCastPtr for the same type, which enables conversions of a pointer to either Box<T> or Arc<T> even though it's truly only one of those. In your earlier revision, the pointer was truly a Box and we didn't get any valgrind or other errors because you never actually invoked the conversion to Arc<T>. The pointer was created with:

BoxCastPtr::set_mut_ptr(verifier_out, verifier);

Freed with:

BoxCastPtr::to_box(verifier);

And utilized with:

let verifier: Arc<dyn ClientCertVerifier> = try_ref_from_ptr!(verifier).clone();

(that last one would have worked fine for Box or Arc).

So I think we're all good here, with the main action item figuring out if we can use the type system or a lint to prevent implementing both conversion traits on a single type.

@cpu
Copy link
Member Author

cpu commented Oct 6, 2023

I can work on tidying up the commit history

Rebased on main and tidied up. As called out in the PR desc the intermediate commits don't build/test cleanly, but it felt like it would be easier to review this way. I suggest we squash merge instead of the usual rebase merge once this branch is ready to go.

I'll try to keep it up to date with the 0.22.x progress in the main Rustls repo.

This commit removes the error handling related to certificate SCTs. The
upstream Rustls project removed embedded SCT support in 0.22.x.
The upstream traits no longer have any default fn implementations,
because they relied on webpki/*ring* and Rustls is making that optional.

In this branch we're continuing to keep a webpki/*ring* dep. and so can
reconstitute the default fns by deferring to the webpki impls as
appropriate.
This commit updates several imports that were once provided when the
`dangerous_configuration` feature was enabled to use their new homes in
specific `danger` modules. The upstream feature flag was removed and
these new `danger` modules are always available.
Both the `ALL_CIPHER_SUITES` and `DEFAULT_CIPHER_SUITES` symbols are now
specific to a crypto provider. Since for the time being rustls-ffi will
stick with using *ring* for the crypto provider this commit updates the
imports to use the symbols provided by `rustls::crypto::ring` instead of
the crate root.
This commit updates rustls-ffi to use the shared pki-types crate,
similar to the upstream rustls projects.
@cpu
Copy link
Member Author

cpu commented Nov 17, 2023

cpu force-pushed the cpu-rustls-0.22-alpha branch from 0b7c359 to 49b4eb9

Addressed review feedback, updated with the try_arc_from_ptr -> try_clone_arc change.

Copy link
Collaborator

@jsha jsha left a comment

Choose a reason for hiding this comment

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

This looks great. I'm inclined to go ahead and merge this to main, so we can iterate further (e.g. adding crypto providers) without you needing to constantly rebase. 0.22 is coming up soon, and if we really need to do another rustls-ffi release targeting 0.21, we can do it off of a branch. What do you think?

Copy link
Member

@ctz ctz left a comment

Choose a reason for hiding this comment

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

i agree on merging this.

src/cipher.rs Outdated
/// A root certificate store.
/// <https://docs.rs/rustls/latest/rustls/struct.RootCertStore.html>
pub struct rustls_root_cert_store {
/// A `rustls_root_cert_store` being constructed. A builder can be modified by
Copy link
Member

Choose a reason for hiding this comment

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

paragraph break after headline?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

src/cipher.rs Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Nov 20, 2023

Thanks for the reviews. Merging it sounds good to me. I'll address the two paragraph break items and then do that 👍

This commit implements a builder pattern for `root_cert_store` so that
we can have a path to both a mutable root cert store while trust anchors
are being added, and a const root cert store suitable for an `Arc` once
completed.
This commit reworks the rustls-ffi API for client certificate validation
to track the new builder based API that landed in Rustls
rustls/rustls#1368
The upstream Rustls project has added `Debug` bounds to many traits. This
commit updates rustls-ffi implementations to derive `Debug`, or
implement it by hand, as required.
The upstream rustls crate moved the `cipher_suite` module and
defines into provider specific packages.

Since rustls-ffi is presently hardcoded to use the *ring*-based crypto
provider this commit updates the cipher suite references to use
`rustls::crypto::ring::cipher_suite` in place of `rustls::cipher_suite`.
This commit updates references to `ClientCertVerifierBuilderError` to
track the upstream rename to `VerifierBuilderError`.
This re-export was removed and instead we need to use
`rustls::crypto::ring::sign::any_supported_type` since this is
a property of the *ring* specific crypto provider.
* Implement a builder pattern and built representation for the webpki
  server cert verifier.
* Update the client config builder to consume a built server cert
  verifier.
* Update the roots builder to support loading roots from a file in
  addition to pem buffer.
@cpu
Copy link
Member Author

cpu commented Nov 20, 2023

When CI finishes I'll squash merge this. Since the individual commits don't build/test cleanly this feels better than rebase merging for this situation.

@cpu cpu marked this pull request as ready for review November 20, 2023 15:21
@cpu cpu changed the title WIP: track upstream Rustls 0.22.x alpha changes. track upstream Rustls 0.22.x alpha changes. Nov 20, 2023
@cpu cpu merged commit 58e2b58 into rustls:main Nov 20, 2023
21 checks passed
@cpu cpu deleted the cpu-rustls-0.22-alpha branch November 20, 2023 15:22
@jsha jsha mentioned this pull request Dec 3, 2023
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