diff --git a/boring/src/ssl/mod.rs b/boring/src/ssl/mod.rs index 980026a1..85028833 100644 --- a/boring/src/ssl/mod.rs +++ b/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())) } } diff --git a/boring/src/ssl/test/mod.rs b/boring/src/ssl/test/mod.rs index fdcb7096..40806b5f 100644 --- a/boring/src/ssl/test/mod.rs +++ b/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(); diff --git a/boring/src/x509/store.rs b/boring/src/x509/store.rs index d5669920..d4bcd045 100644 --- a/boring/src/x509/store.rs +++ b/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::::from_ptr(ffi::X509_STORE_get0_objects(self.as_ptr())).len() + } + } } foreign_type_and_impl_send_sync! {