search

rustls / rustls-ffi

Use Rustls from any language
Other
124 stars 31 forks source link

track upstream Rustls 0.22.x alpha changes. #341

Closed cpu closed 7 months ago

cpu commented 11 months ago

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:

cpu commented 11 months ago

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 commented 11 months ago

Is the right way to handle the fallibility of the build() operation to rework this function to return a rustls_result and use an "out pointer arg" for the builder?

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.

And to refine on that a bit further: functions that can't error for reasons other than null inputs or internal panics can return a pointer for convenience.

cpu commented 11 months ago

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.

:ballot_box_with_check: Thanks! I will fix that up.

cpu commented 11 months ago

:ballot_box_with_check: Thanks! I will fix that up.

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

cpu commented 9 months ago

:construction: Work in progress :construction:

jsha commented 9 months ago

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 commented 9 months ago

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 :dizzy_face: 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 commented 9 months ago

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]https://github.com/rustls/rustls-ffi/pull/341/commits/f12aa122840eac719c948ce7dea49b003f3cff0e) entailed (writing this out to check my own understanding):

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:

I think this avoids the issues you were highlighting :crossed_fingers:

jsha commented 9 months ago

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 commented 9 months ago

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 :laughing:)

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 :ballot_box_with_check:

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 :thinking:

jsha commented 9 months ago

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: 6e988afb55cc8763d52b66eb9f06f191ac4335bf. 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 {}

https://github.com/rustls/rustls-ffi/commit/6e988afb55cc8763d52b66eb9f06f191ac4335bf#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 commented 9 months ago

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.

cpu commented 8 months ago

cpu force-pushed the cpu-rustls-0.22-alpha branch from 48d9e81 to 363c445

Rebased to rework w/ the new Castable trait.

cpu commented 8 months ago

@jsha @ctz Since we're getting close to a 0.22 Rustls release I've updated this branch to use the latest alpha and caught up on the various API changes. I think it's ~reviewable in the current state if you have time for feedback.

Right now the rustls-ffi side is written to assume ring as the crypto provider. I think worst case we could release in that state and maintain status-quo, but to get the most out of the new 0.22 release we might want to come up with a plan for how to expose the choice of using aws-lc-rs, or a custom crypto provider through this API. Have either of you given this any thought? The aws-lc-rs support is probably not too much of a lift. Getting the crypto provider API right might be trickier.

jsha commented 7 months ago

Excellent, thanks! Will take a look.

And yeah, it would be good to figure out how to expose the crypto provider choice. I'll do some thinking about that.

cpu commented 7 months ago

cpu force-pushed the cpu-rustls-0.22-alpha branch from 05b1789 to e6b1953

Resolved conflicts, tidied the commit history a little bit (e.g. using the latest alphas from the start), and adjusted to use try_take! where applicable.

cpu commented 7 months ago

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.

cpu commented 7 months ago

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

cpu commented 7 months ago

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.