Make X509Store shareable between contexts

#362

boring/src/ssl/mod.rs

@@ -88,7 +88,7 @@ use crate::ssl::bio::BioMethod;
 use crate::ssl::callbacks::*;
 use crate::ssl::error::InnerError;
 use crate::stack::{Stack, StackRef, Stackable};
-use crate::x509::store::{X509Store, X509StoreBuilderRef, X509StoreRef};
+use crate::x509::store::{X509Store, X509StoreBuilder, X509StoreBuilderRef, X509StoreRef};
 use crate::x509::verify::X509VerifyParamRef;
 use crate::x509::{
     X509Name, X509Ref, X509StoreContextRef, X509VerifyError, X509VerifyResult, X509,
@@ -933,6 +933,8 @@ extern "C" fn rpk_verify_failure_callback(
 /// A builder for `SslContext`s.
 pub struct SslContextBuilder {
     ctx: SslContext,
+    /// If it's not shared, it can be exposed as mutable
+    has_shared_cert_store: bool,
     #[cfg(feature = "rpk")]
     is_rpk: bool,
 }
@@ -1006,7 +1008,11 @@ impl SslContextBuilder {
     #[cfg(feature = "rpk")]
     pub unsafe fn from_ptr(ctx: *mut ffi::SSL_CTX, is_rpk: bool) -> SslContextBuilder {
         let ctx = SslContext::from_ptr(ctx);
-        let mut builder = SslContextBuilder { ctx, is_rpk };
+        let mut builder = SslContextBuilder {
+            ctx,
+            is_rpk,
+            has_shared_cert_store: false,
+        };
 
         builder.set_ex_data(*RPK_FLAG_INDEX, is_rpk);
 
@@ -1022,6 +1028,7 @@ impl SslContextBuilder {
     pub unsafe fn from_ptr(ctx: *mut ffi::SSL_CTX) -> SslContextBuilder {
         SslContextBuilder {
             ctx: SslContext::from_ptr(ctx),
+            has_shared_cert_store: false,
         }
     }
 
@@ -1206,17 +1213,48 @@ impl SslContextBuilder {
         }
     }
 
+    /// Use [`set_cert_store_builder`] or [`set_cert_store_ref`] instead.
+    ///
     /// Replaces the context's certificate store.
     #[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;
         unsafe {
             ffi::SSL_CTX_set_cert_store(self.as_ptr(), cert_store.into_ptr());
         }
     }
 
+    /// Replaces the context's certificate store, and allows mutating the store afterwards.
+    #[corresponds(SSL_CTX_set_cert_store)]
+    pub fn set_cert_store_builder(&mut self, cert_store: X509StoreBuilder) {
+        #[cfg(feature = "rpk")]
+        assert!(!self.is_rpk, "This API is not supported for RPK");
+
+        self.has_shared_cert_store = false;
+        unsafe {
+            ffi::SSL_CTX_set_cert_store(self.as_ptr(), cert_store.into_ptr());
+        }
+    }
+
+    /// Replaces the context's certificate store, and keeps it immutable.
+    ///
+    /// 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());
+        }
+    }
+
     /// Controls read ahead behavior.
     ///
     /// If enabled, OpenSSL will read as much data as is available from the underlying stream,
@@ -1701,11 +1739,25 @@ impl SslContextBuilder {
     }
 
     /// Returns a mutable reference to the context's certificate store.
+    ///
+    /// Newly-created `SslContextBuilder` will have its own default mutable store.
+    ///
+    /// ## Panics
+    ///
+    /// * If a shared store has been set via [`set_cert_store_ref`]
+    /// * If context has been created for Raw Public Key verification (requires `rpk` Cargo feature)
+    ///
     #[corresponds(SSL_CTX_get_cert_store)]
     pub fn cert_store_mut(&mut self) -> &mut X509StoreBuilderRef {
         #[cfg(feature = "rpk")]
         assert!(!self.is_rpk, "This API is not supported for RPK");
 
+        assert!(
+            !self.has_shared_cert_store,
+            "Shared X509Store can't be mutated. Make a new store"
+        );
+        // OTOH, it's not safe to return a shared &X509Store when the builder owns it exclusively
+
         unsafe { X509StoreBuilderRef::from_ptr_mut(ffi::SSL_CTX_get_cert_store(self.as_ptr())) }
     }
 

boring/src/ssl/test/mod.rs

@@ -1,4 +1,4 @@
-use hex;
+use foreign_types::{ForeignType, ForeignTypeRef};
 use std::io;
 use std::io::prelude::*;
 use std::mem;
@@ -18,6 +18,7 @@ use crate::ssl::{
     ExtensionType, ShutdownResult, ShutdownState, Ssl, SslAcceptor, SslAcceptorBuilder,
     SslConnector, SslContext, SslFiletype, SslMethod, SslOptions, SslStream, SslVerifyMode,
 };
+use crate::x509::store::X509StoreBuilder;
 use crate::x509::verify::X509CheckFlags;
 use crate::x509::{X509Name, X509};
 
@@ -308,6 +309,52 @@ fn test_select_cert_ok() {
     client.connect();
 }
 
+#[test]
+fn test_mutable_store() {
+    #![allow(deprecated)]
+
+    let cert = include_bytes!("../../../test/cert.pem");
+    let cert = X509::from_pem(cert).unwrap();
+    let cert2 = include_bytes!("../../../test/root-ca.pem");
+    let cert2 = X509::from_pem(cert2).unwrap();
+
+    let mut ctx = SslContext::builder(SslMethod::tls()).unwrap();
+    ctx.cert_store_mut().add_cert(cert.clone()).unwrap();
+    assert_eq!(1, ctx.cert_store().objects_len());
+
+    ctx.set_cert_store_builder(X509StoreBuilder::new().unwrap());
+    assert_eq!(0, ctx.cert_store().objects_len());
+
+    ctx.cert_store_mut().add_cert(cert.clone()).unwrap();
+    assert_eq!(1, ctx.cert_store().objects_len());
+
+    let mut new_store = X509StoreBuilder::new().unwrap();
+    new_store.add_cert(cert).unwrap();
+    new_store.add_cert(cert2).unwrap();
+    let new_store = new_store.build();
+    assert_eq!(2, new_store.objects_len());
+
+    ctx.set_cert_store_ref(&new_store);
+    assert_eq!(2, ctx.cert_store().objects_len());
+    assert!(std::ptr::eq(new_store.as_ptr(), ctx.cert_store().as_ptr()));
+
+    let ctx = ctx.build();
+    assert!(std::ptr::eq(new_store.as_ptr(), ctx.cert_store().as_ptr()));
+
+    drop(new_store);
+    assert_eq!(2, ctx.cert_store().objects_len());
+}
+
+#[test]
+#[should_panic(expected = "mutated")]
+fn shared_store_must_not_be_mutated() {
+    let mut ctx = SslContext::builder(SslMethod::tls()).unwrap();
+
+    let shared = X509StoreBuilder::new().unwrap().build();
+    ctx.set_cert_store_ref(&shared);
+    ctx.cert_store_mut();
+}
+
 #[test]
 fn test_select_cert_error() {
     let mut server = Server::builder();

boring/src/x509/store.rs

@@ -115,6 +115,14 @@ impl X509StoreBuilderRef {
     pub fn set_param(&mut self, param: &X509VerifyParamRef) -> Result<(), ErrorStack> {
         unsafe { cvt(ffi::X509_STORE_set1_param(self.as_ptr(), param.as_ptr())).map(|_| ()) }
     }
+
+    /// For testing only
+    #[cfg(test)]
+    pub fn objects_len(&self) -> usize {
+        unsafe {
+            StackRef::<X509Object>::from_ptr(ffi::X509_STORE_get0_objects(self.as_ptr())).len()
+        }
+    }
 }
 
 foreign_type_and_impl_send_sync! {