diff --git a/boring/src/ssl/callbacks.rs b/boring/src/ssl/callbacks.rs index f592d9d2..f41108d5 100644 --- a/boring/src/ssl/callbacks.rs +++ b/boring/src/ssl/callbacks.rs @@ -64,6 +64,34 @@ where unsafe { raw_custom_verify_callback(ssl, out_alert, callback) } } +pub(super) unsafe extern "C" fn raw_cert_verify( + x509_ctx: *mut ffi::X509_STORE_CTX, + _arg: *mut c_void, +) -> c_int +where + F: Fn(&mut X509StoreContextRef) -> bool + 'static + Sync + Send, +{ + // SAFETY: boring provides valid inputs. + let ctx = unsafe { X509StoreContextRef::from_ptr_mut(x509_ctx) }; + + let ssl_idx = X509StoreContext::ssl_idx().expect("BUG: store context ssl index missing"); + let verify_idx = SslContext::cached_ex_index::(); + + let verify = ctx + .ex_data(ssl_idx) + .expect("BUG: store context missing ssl") + .ssl_context() + .ex_data(verify_idx) + .expect("BUG: verify callback missing"); + + // SAFETY: The callback won't outlive the context it's associated with + // because there is no way to get a mutable reference to the `SslContext`, + // so the callback can't replace itself. + let verify = unsafe { &*(verify as *const F) }; + + verify(ctx) as c_int +} + pub(super) unsafe extern "C" fn ssl_raw_custom_verify( ssl: *mut ffi::SSL, out_alert: *mut u8, diff --git a/boring/src/ssl/mod.rs b/boring/src/ssl/mod.rs index 64fcf8bb..8ea6ede9 100644 --- a/boring/src/ssl/mod.rs +++ b/boring/src/ssl/mod.rs @@ -1000,6 +1000,49 @@ impl SslContextBuilder { self.ctx.as_ptr() } + /// Registers a certificate verification callback that replaces the default verification + /// process. + /// + /// The callback returns true if the certificate chain is valid, and false if not. + /// A viable verification result value (either `Ok(())` or an `Err(X509VerifyError)`) must be + /// reflected in the error member of `X509StoreContextRef`, which can be done by calling + /// `X509StoreContextRef::set_error`. However, the callback's return value determines + /// whether the chain is accepted or not. + /// + /// *Warning*: Providing a complete verification procedure is a complex task. See + /// https://docs.openssl.org/master/man3/SSL_CTX_set_cert_verify_callback/#notes for more + /// information. + /// + /// TODO: Add the ability to unset the callback by either adding a new function or wrapping the + /// callback in an `Option`. + /// + /// # Panics + /// + /// This method panics if this `SslContext` is associated with a RPK context. + #[corresponds(SSL_CTX_set_cert_verify_callback)] + pub fn set_cert_verify_callback(&mut self, callback: F) + where + F: Fn(&mut X509StoreContextRef) -> bool + 'static + Sync + Send, + { + #[cfg(feature = "rpk")] + assert!(!self.is_rpk, "This API is not supported for RPK"); + + // NOTE(jlarisch): Q: Why don't we wrap the callback in an Arc, since + // `set_verify_callback` does? + // A: I don't think that Arc is necessary, and I don't think one is necessary here. + // There's no way to get a mutable reference to the `Ssl` or `SslContext`, which + // is what you need to register a new callback. + // See the NOTE in `ssl_raw_verify` for confirmation. + self.replace_ex_data(SslContext::cached_ex_index::(), callback); + unsafe { + ffi::SSL_CTX_set_cert_verify_callback( + self.as_ptr(), + Some(raw_cert_verify::), + ptr::null_mut(), + ); + } + } + /// Configures the certificate verification method for new connections. #[corresponds(SSL_CTX_set_verify)] pub fn set_verify(&mut self, mode: SslVerifyMode) { diff --git a/boring/src/ssl/test/cert_verify.rs b/boring/src/ssl/test/cert_verify.rs new file mode 100644 index 00000000..929db48c --- /dev/null +++ b/boring/src/ssl/test/cert_verify.rs @@ -0,0 +1,99 @@ +use crate::hash::MessageDigest; +use crate::ssl::test::Server; +use crate::ssl::SslVerifyMode; + +#[test] +fn error_when_trusted_but_callback_returns_false() { + let mut server = Server::builder(); + server.should_error(); + let server = server.build(); + let mut client = server.client_with_root_ca(); + client.ctx().set_verify(SslVerifyMode::PEER); + client.ctx().set_cert_verify_callback(|x509| { + // The cert is OK + assert!(x509.verify_cert().unwrap()); + assert!(x509.current_cert().is_some()); + assert!(x509.verify_result().is_ok()); + // But we return false + false + }); + + client.connect_err(); +} + +#[test] +fn no_error_when_untrusted_but_callback_returns_true() { + let server = Server::builder().build(); + let mut client = server.client(); + client.ctx().set_verify(SslVerifyMode::PEER); + client.ctx().set_cert_verify_callback(|x509| { + // The cert is not OK + assert!(!x509.verify_cert().unwrap()); + assert!(x509.current_cert().is_some()); + assert!(x509.verify_result().is_err()); + // But we return true + true + }); + + client.connect(); +} + +#[test] +fn no_error_when_trusted_and_callback_returns_true() { + let server = Server::builder().build(); + let mut client = server.client_with_root_ca(); + client.ctx().set_verify(SslVerifyMode::PEER); + client.ctx().set_cert_verify_callback(|x509| { + // The cert is OK + assert!(x509.verify_cert().unwrap()); + assert!(x509.current_cert().is_some()); + assert!(x509.verify_result().is_ok()); + // And we return true + true + }); + client.connect(); +} + +#[test] +fn callback_receives_correct_certificate() { + let server = Server::builder().build(); + let mut client = server.client(); + let expected = "59172d9313e84459bcff27f967e79e6e9217e584"; + client.ctx().set_verify(SslVerifyMode::PEER); + client.ctx().set_cert_verify_callback(move |x509| { + assert!(!x509.verify_cert().unwrap()); + assert!(x509.current_cert().is_some()); + assert!(x509.verify_result().is_err()); + let cert = x509.current_cert().unwrap(); + let digest = cert.digest(MessageDigest::sha1()).unwrap(); + assert_eq!(hex::encode(digest), expected); + true + }); + + client.connect(); +} + +#[test] +fn callback_receives_correct_chain() { + let server = Server::builder().build(); + let mut client = server.client_with_root_ca(); + let leaf_sha1 = "59172d9313e84459bcff27f967e79e6e9217e584"; + let root_sha1 = "c0cbdf7cdd03c9773e5468e1f6d2da7d5cbb1875"; + client.ctx().set_verify(SslVerifyMode::PEER); + client.ctx().set_cert_verify_callback(move |x509| { + assert!(x509.verify_cert().unwrap()); + assert!(x509.current_cert().is_some()); + assert!(x509.verify_result().is_ok()); + let chain = x509.chain().unwrap(); + assert!(chain.len() == 2); + let leaf_cert = chain.get(0).unwrap(); + let leaf_digest = leaf_cert.digest(MessageDigest::sha1()).unwrap(); + assert_eq!(hex::encode(leaf_digest), leaf_sha1); + let root_cert = chain.get(1).unwrap(); + let root_digest = root_cert.digest(MessageDigest::sha1()).unwrap(); + assert_eq!(hex::encode(root_digest), root_sha1); + true + }); + + client.connect(); +} diff --git a/boring/src/ssl/test/mod.rs b/boring/src/ssl/test/mod.rs index f3b0fd29..6010cf98 100644 --- a/boring/src/ssl/test/mod.rs +++ b/boring/src/ssl/test/mod.rs @@ -24,6 +24,7 @@ use crate::x509::{X509Name, X509}; #[cfg(not(feature = "fips"))] use super::CompliancePolicy; +mod cert_verify; mod custom_verify; mod private_key_method; mod server;