Safe clone for X509Store

boring/src/ssl/mod.rs

@@ -1203,16 +1203,18 @@ impl SslContextBuilder {
         }
     }
 
-    /// Use [`set_cert_store_builder`] or [`set_cert_store_ref`] instead.
+    /// Replaces the context's certificate store, and keeps it immutable.
+    ///
+    /// This method allows sharing the `X509Store`, but calls to `cert_store_mut` will panic.
     ///
-    /// Replaces the context's certificate store.
+    /// Use [`set_cert_store_builder`] to set a mutable cert store
+    /// (there's no way to have both sharing and mutability).
     #[corresponds(SSL_CTX_set_cert_store)]
-    #[deprecated(note = "Use set_cert_store_builder or set_cert_store_ref instead")]
     pub fn set_cert_store(&mut self, cert_store: X509Store) {
         #[cfg(feature = "rpk")]
         assert!(!self.is_rpk, "This API is not supported for RPK");
 
-        self.has_shared_cert_store = false;
+        self.has_shared_cert_store = true;
         unsafe {
             ffi::SSL_CTX_set_cert_store(self.as_ptr(), cert_store.into_ptr());
         }
@@ -1235,14 +1237,7 @@ impl SslContextBuilder {
     /// This method allows sharing the `X509Store`, but calls to `cert_store_mut` will panic.
     #[corresponds(SSL_CTX_set_cert_store)]
     pub fn set_cert_store_ref(&mut self, cert_store: &X509Store) {
-        #[cfg(feature = "rpk")]
-        assert!(!self.is_rpk, "This API is not supported for RPK");
-
-        self.has_shared_cert_store = true;
-        unsafe {
-            ffi::X509_STORE_up_ref(cert_store.as_ptr());
-            ffi::SSL_CTX_set_cert_store(self.as_ptr(), cert_store.as_ptr());
-        }
+        self.set_cert_store(cert_store.to_owned());
     }
 
     /// Controls read ahead behavior.
@@ -1771,7 +1766,8 @@ impl SslContextBuilder {
 
         assert!(
             !self.has_shared_cert_store,
-            "Shared X509Store can't be mutated. Make a new store"
+            "Shared X509Store can't be mutated. Use set_cert_store_builder() instead of set_cert_store()
+                or completely finish building the cert store setting it."
         );
         // OTOH, it's not safe to return a shared &X509Store when the builder owns it exclusively
 

boring/src/x509/store.rs

@@ -134,6 +134,23 @@ foreign_type_and_impl_send_sync! {
     pub struct X509Store;
 }
 
+impl ToOwned for X509StoreRef {
+    type Owned = X509Store;
+
+    fn to_owned(&self) -> X509Store {
+        unsafe {
+            ffi::X509_STORE_up_ref(self.as_ptr());
+            X509Store::from_ptr(self.as_ptr())
+        }
+    }
+}
+
+impl Clone for X509Store {
+    fn clone(&self) -> X509Store {
+        (**self).to_owned()
+    }
+}
+
 impl X509StoreRef {
     /// **Warning: this method is unsound**
     ///
@@ -160,12 +177,15 @@ impl X509StoreRef {
 }
 
 #[test]
-#[allow(dead_code)]
-// X509Store must not implement Clone because `SslContextBuilder::cert_store_mut` lets
-// you get a mutable reference to a store that could have been cloned before being
-// passed to `SslContextBuilder::set_cert_store`.
-fn no_clone_for_x509store() {
-    trait MustNotImplementClone {}
-    impl<T: Clone> MustNotImplementClone for T {}
-    impl MustNotImplementClone for X509Store {}
+#[should_panic = "Shared X509Store can't be mutated"]
+fn set_cert_store_pevents_mutability() {
+    use crate::ssl::*;
+
+    let mut ctx = SslContext::builder(SslMethod::tls()).unwrap();
+    let store = X509StoreBuilder::new().unwrap().build();
+
+    ctx.set_cert_store(store.clone());
+
+    // This is bad.
+    let _aliased_store = ctx.cert_store_mut();
 }