Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add set_cert_verify_callback (SSL_CTX_set_cert_verify) #287

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions boring/src/ssl/callbacks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,34 @@ where
unsafe { raw_custom_verify_callback(ssl, out_alert, callback) }
}

pub(super) unsafe extern "C" fn raw_cert_verify<F>(
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::<F>();

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<F>(
ssl: *mut ffi::SSL,
out_alert: *mut u8,
Expand Down
43 changes: 43 additions & 0 deletions boring/src/ssl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<F>(&mut self, callback: F)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can unset by passing NULL, but that's not possible with this binding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems we either change this to Option or add another function that unsets this callback. Changing this to Option seems to make Rust unable to reason about arguments to this function when it is called, requiring type annotations (which are onerous when you just want to pass None).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a TODO

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::<F>(), callback);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_verify_callback uses

            // this needs to be in an Arc since the callback can register a new callback!
            self.replace_ex_data(Ssl::cached_ex_index(), Arc::new(callback)); 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my digging, this is copy-pasta. see this comment by @nox https://github.com/cloudflare/boring/blob/master/boring/src/ssl/callbacks.rs#L200-L202

I agree that it is impossible to get a mutable reference to an SslRef or SslContextRef, so I don't see why this needs to be an Arc. I am happy to make it one just to be safe, though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment with the reference.

unsafe {
ffi::SSL_CTX_set_cert_verify_callback(
self.as_ptr(),
Some(raw_cert_verify::<F>),
ptr::null_mut(),
);
}
}

/// Configures the certificate verification method for new connections.
#[corresponds(SSL_CTX_set_verify)]
pub fn set_verify(&mut self, mode: SslVerifyMode) {
Expand Down
99 changes: 99 additions & 0 deletions boring/src/ssl/test/cert_verify.rs
Original file line number Diff line number Diff line change
@@ -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();
}
1 change: 1 addition & 0 deletions boring/src/ssl/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading