Revisit Clone impl for X509Store

[nox]

Implementing Clone for X509Store has been attempted twice in #108 and #339.

The latest revert in #353 documented this shouldn't be done because of SslContextBuilder::set_cert_store and SslContextBuilder::cert_store_mut.

let mut ctx = SslContext::builder().unwrap();
let store = X509Store::new().unwrap();

ctx.set_cert_store(store.clone());

// This is bad.
let aliased_store = ctx.cert_store_mut();

Maybe we should just remove cert_store_mut and make people use set_cert_store only?

[nox]

Another solution would be to somehow make cert_store_mut return something only if the store's refcount is 1, but there is no method to check the refcount of any crypto object in boringssl, you can only increment it with a _up_ref function or decrement it through a _free function.

[0x676e67]

The benefit of implementing cloning is to reduce the time spent repeatedly adding certificates, especially when repeatedly creating connectors.

As an aside, when creating the SslConnector, calling set_default_verify_paths is relatively time-consuming—in my local tests, it took 22ms. Is this worth optimizing? It might be possible to skip this option during builder creation and manually set it later using cert_store_mut or set_cert_store.

boring/boring/src/ssl/connector.rs

Line 81 in 15975dd

ctx.set_default_verify_paths()?;

[kornelski]

X509Store itself is immutable. Everywhere else in the library there's only a one-way path from a mutable builder to an immutable object, like X509StoreBuilder → X509Store.

The problem here is that set_cert_store takes X509Store, and casts it back to a X509StoreBuilder, undoing the "freezing" of the object.

It seems to me that the correct design would be for set_cert_store to take X509StoreBuilder instead. It would then have a guarantee it's a mutable non-copyable object, and there would be no issue exposing it as the same X509StoreBuilder again.

This could be added in a backwards-compatible way:

fn set_cert_store_mut(&mut self, store: X509StoreBuilder)

[kornelski]

Maybe the current API can be kept if exclusive vs shared status of X509Store could be saved. Then the cert_store_mut could check the state at run time, and panic if necessary.

Unfortunately, X509_STORE_set_ex_data is not in boringssl, so the state needs to be stored in some other way:

• Add an AtomicBool field to X509StoreBuilder and X509Store. This is least hacky.
• Abuse X509_STORE_set_cleanup and X509_STORE_get_cleanup. Set a no-op cleanup function on it to mark it as cloned. The setter is not thread-safe, so it would have to use a custom fn clone(&mut self) rather than the Clone trait.
• Maintain a global list of cloned X509Store objects. Multi-threading bottleneck.

[kornelski]

Another option is not to make X509Store cloneable by itself, but only via SslContextBuilder:

fn set_cert_store_ref(&mut self, store: &X509Store)

And use of that method would always block use of cert_store_mut. This way only SslContextBuilder would have to carry a flag whether cert_store_mut is allowed.

[nox]

It seems to me that the correct design would be for set_cert_store to take X509StoreBuilder instead. It would then have a guarantee it's a mutable non-copyable object, and there would be no issue exposing it as the same X509StoreBuilder again.

The problem is not set_cert_store, it's cert_store_mut. We want set_cert_store to take a X509Store because we want to be able to clone large cert sets by just incrementing its refcount to use in new TLS contexts.

[nox]

We can also continue to provide some mutable access to the default X509Store by storing in SslContextBuilder whether we overrode it.

struct SslContextBuilder {
    ctx: SslContext,
    is_default_store: bool,
    #[cfg(feature = "rpk")]
    is_rpk: bool,
}

impl X509ContextBuilder {
    pub fn new(method: SslMethod) -> Result<(), ErrorStack> {
        unsafe {
            init();
            let ctx = cvt_p(ffi::SSL_CTX_new(method.as_ptr()))?;

            #[cfg(feature = "rpk")]
            let ctx = {
                SslContextBuilder::from_ptr(ctx, false);
            };

            #[cfg(not(feature = "rpk"))]
            let ctx = {
                SslContextBuilder::from_ptr(ctx);
            };

            ctx.is_default_store = true;

            Ok(ctx)
        }
    }

    pub fn default_cert_store_mut(&mut self) -> Option<&mut X509StoreBuilderRef> {
        #[cfg(feature = "rpk")]
        assert!(!self.is_rpk, "This API is not supported for RPK");

        unsafe { 
            self.is_default_store.then(|| {
                X509StoreBuilderRef::from_ptr_mut(SSL_CTX_get_cert_store(self.as_ptr()))
            })
        }
    }

    pub fn set_cert_store(&mut self, cert_store: X509Store) {
        #[cfg(feature = "rpk")]
        assert!(!self.is_rpk, "This API is not supported for RPK");

        unsafe {
            self.is_default_store = false;

            ffi::SSL_CTX_set_cert_store(self.as_ptr(), cert_store.into_ptr());
        }
    }
}

This means we can't have a SslContextBuilder::cert_store(&self) -> &X509StoreRef method though.

[nox]

Edited comment above as we don't need to store a pointer to the default store, we can just store a boolean.

[kornelski]

Yeah, boolean in the SslContextBuilder makes it much simpler. I've made a PR.