From 61f838240e6cb4863a74664ae3748d071a20b009 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 23 May 2023 19:43:05 +0000 Subject: [PATCH 1/4] bindings: add set_check_stapled_ocsp_response --- bindings/rust/s2n-tls/src/config.rs | 17 ++++++ bindings/rust/s2n-tls/src/testing/s2n_tls.rs | 56 ++++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/bindings/rust/s2n-tls/src/config.rs b/bindings/rust/s2n-tls/src/config.rs index 7f6340c0ea7..6657dfecb6b 100644 --- a/bindings/rust/s2n-tls/src/config.rs +++ b/bindings/rust/s2n-tls/src/config.rs @@ -380,6 +380,23 @@ impl Builder { self.enable_ocsp() } + /// Toggles whether or not to validate stapled OCSP responses. + /// + /// Setting `check_ocsp` to `true` means OCSP responses will be validated when they are encountered, + /// while `false` means this step will be skipped. + /// + /// The default value is `true` if the underlying libCrypto implementation supports OCSP. + pub fn set_check_stapled_ocsp_response( + &mut self, + check_ocsp: bool, + ) -> Result<&mut Self, Error> { + unsafe { + s2n_config_set_check_stapled_ocsp_response(self.as_mut_ptr(), check_ocsp as u8) + .into_result() + }?; + Ok(self) + } + /// Sets the callback to use for verifying that a hostname from an X.509 certificate is /// trusted. /// diff --git a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs index 8af6b802dd1..04438486245 100644 --- a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs +++ b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs @@ -706,4 +706,60 @@ mod tests { establish_connection(config_with_system_certs); }); } + + #[test] + fn set_check_stapled_ocsp_response_true() { + let keypair = CertKeyPair::default(); + + temp_env::with_var("SSL_CERT_FILE", Some(keypair.cert_path()), || { + let mut builder = Builder::new(); + builder + .load_pem(keypair.cert(), keypair.key()) + .unwrap() + .set_security_policy(&security::DEFAULT_TLS13) + .unwrap() + .set_verify_host_callback(InsecureAcceptAllCertificatesHandler {}) + .unwrap(); + + // Enable OCSP and some OCSP data that will fail validation + builder.enable_ocsp().unwrap(); + builder.set_ocsp_data(&[1, 2, 3]).unwrap(); + // Enable OCSP validation (the default) + builder.set_check_stapled_ocsp_response(true).unwrap(); + + let config = builder.build().unwrap(); + + let mut pair = tls_pair(config); + + // The handshake should fail + assert!(poll_tls_pair_result(&mut pair).is_err()); + }) + } + + #[test] + fn set_check_stapled_ocsp_response_false() { + let keypair = CertKeyPair::default(); + + temp_env::with_var("SSL_CERT_FILE", Some(keypair.cert_path()), || { + let mut builder = Builder::new(); + builder + .load_pem(keypair.cert(), keypair.key()) + .unwrap() + .set_security_policy(&security::DEFAULT_TLS13) + .unwrap() + .set_verify_host_callback(InsecureAcceptAllCertificatesHandler {}) + .unwrap(); + + // Enable OCSP and some OCSP data that will fail validation + builder.enable_ocsp().unwrap(); + builder.set_ocsp_data(&[1, 2, 3]).unwrap(); + // Disable OCSP validation + builder.set_check_stapled_ocsp_response(false).unwrap(); + + let config = builder.build().unwrap(); + + // The handshake should succeed since OCSP validation is disabled + establish_connection(config); + }) + } } From 783c3fecb07cfa31f63dd213f27ff50c13ed4dd0 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Sat, 27 May 2023 00:09:22 +0000 Subject: [PATCH 2/4] Have trust_location not enable OCSP --- bindings/rust/s2n-tls/src/config.rs | 33 +++-- .../rust/s2n-tls/src/testing/client_hello.rs | 37 ++++++ bindings/rust/s2n-tls/src/testing/s2n_tls.rs | 114 +++++++++--------- tls/s2n_connection.c | 2 + 4 files changed, 112 insertions(+), 74 deletions(-) diff --git a/bindings/rust/s2n-tls/src/config.rs b/bindings/rust/s2n-tls/src/config.rs index 6657dfecb6b..de9b530aaee 100644 --- a/bindings/rust/s2n-tls/src/config.rs +++ b/bindings/rust/s2n-tls/src/config.rs @@ -152,6 +152,7 @@ impl Drop for Config { pub struct Builder { config: Config, load_system_certs: bool, + enable_ocsp: bool, } impl Builder { @@ -181,6 +182,7 @@ impl Builder { Self { config: Config(config), load_system_certs: true, + enable_ocsp: false, } } @@ -288,6 +290,10 @@ impl Builder { Ok(self) } + /// Adds to the trust store from a CA file or directory containing trusted certificates. + /// + /// NOTE: This function is equivalent to `s2n_config_set_verification_ca_location` except it does + /// not automatically enable the client to request OCSP stapling from the server. pub fn trust_location( &mut self, file: Option<&Path>, @@ -353,6 +359,7 @@ impl Builder { s2n_config_set_status_request_type(self.as_mut_ptr(), s2n_status_request_type::OCSP) .into_result() }?; + self.enable_ocsp = true; Ok(self) } @@ -380,23 +387,6 @@ impl Builder { self.enable_ocsp() } - /// Toggles whether or not to validate stapled OCSP responses. - /// - /// Setting `check_ocsp` to `true` means OCSP responses will be validated when they are encountered, - /// while `false` means this step will be skipped. - /// - /// The default value is `true` if the underlying libCrypto implementation supports OCSP. - pub fn set_check_stapled_ocsp_response( - &mut self, - check_ocsp: bool, - ) -> Result<&mut Self, Error> { - unsafe { - s2n_config_set_check_stapled_ocsp_response(self.as_mut_ptr(), check_ocsp as u8) - .into_result() - }?; - Ok(self) - } - /// Sets the callback to use for verifying that a hostname from an X.509 certificate is /// trusted. /// @@ -602,6 +592,15 @@ impl Builder { } } + // If OCSP has not been explicitly requested, turn off OCSP. This is to prevent the `trust_location()` function + // from automatically enabling `OCSP` due to the legacy behavior of `s2n_config_set_verification_ca_location` + if !self.enable_ocsp { + unsafe { + s2n_config_set_status_request_type(self.as_mut_ptr(), s2n_status_request_type::NONE) + .into_result()? + }; + } + Ok(self.config) } diff --git a/bindings/rust/s2n-tls/src/testing/client_hello.rs b/bindings/rust/s2n-tls/src/testing/client_hello.rs index 5b9b577f01f..49a3ef795e7 100644 --- a/bindings/rust/s2n-tls/src/testing/client_hello.rs +++ b/bindings/rust/s2n-tls/src/testing/client_hello.rs @@ -7,6 +7,7 @@ use crate::{ }; use alloc::sync::Arc; use core::{sync::atomic::Ordering, task::Poll}; +use s2n_tls_sys::{s2n_client_hello_has_extension, s2n_connection_get_client_hello}; use std::{fmt, io, pin::Pin, sync::atomic::AtomicUsize}; // The Future returned by MockClientHelloHandler. @@ -132,6 +133,42 @@ impl Drop for FailingCHFuture { assert!(self.invoked.load(Ordering::SeqCst) >= 1); } } +/// A client hello handler that asserts that the extension with the given +/// IANA code is either present or not present in the client hello +pub struct HasExtensionClientHelloHandler { + pub extension_iana: u16, + pub extension_expected: bool, +} + +impl ClientHelloCallback for HasExtensionClientHelloHandler { + fn on_client_hello( + &self, + connection: &mut crate::connection::Connection, + ) -> Result>>, error::Error> { + let mut exists = false; + + unsafe { + let client_hello = s2n_connection_get_client_hello(connection.as_ptr()); + s2n_client_hello_has_extension(client_hello, self.extension_iana, &mut exists as _); + } + + if self.extension_expected { + assert!( + exists, + "Extension {} was not found in the client hello", + self.extension_iana + ); + } else { + assert!( + !exists, + "Unexpected extension {} found in the client hello", + self.extension_iana + ) + } + + Ok(None) + } +} #[derive(Debug)] pub struct CustomError; diff --git a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs index 04438486245..17d6c6f6bb4 100644 --- a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs +++ b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs @@ -204,7 +204,7 @@ mod tests { }; use alloc::sync::Arc; use core::sync::atomic::Ordering; - use futures_test::task::new_count_waker; + use futures_test::task::{new_count_waker, noop_waker}; use std::{fs, path::Path, pin::Pin, sync::atomic::AtomicUsize}; #[test] @@ -532,6 +532,62 @@ mod tests { Ok(()) } + /// `trust_location()` calls `s2n_config_set_verification_ca_location()`, which has the legacy behavior + /// of enabling OCSP on clients. Since we do not want to replicate that behavior in the Rust bindings, + /// this test verifies that `trust_location()` does not turn on OCSP. It also verifies that turning + /// on OCSP explicitly still works when `trust_location()` is called. + #[test] + fn trust_location_does_not_change_ocsp_status() -> Result<(), Error> { + let pem_dir = Path::new(concat!(env!("CARGO_MANIFEST_DIR"), "/../../../tests/pems")); + let mut cert = pem_dir.to_path_buf(); + cert.push("rsa_4096_sha512_client_cert.pem"); + let mut key = pem_dir.to_path_buf(); + key.push("rsa_4096_sha512_client_key.pem"); + + const OCSP_IANA_EXTENSION_ID: u16 = 5; + + for enable_ocsp in [true, false] { + let config = { + let mut config = crate::config::Builder::new(); + + if enable_ocsp { + config.enable_ocsp()?; + } + + config.set_security_policy(&security::DEFAULT_TLS13)?; + config.set_verify_host_callback(InsecureAcceptAllCertificatesHandler {})?; + config.set_client_hello_callback(HasExtensionClientHelloHandler { + // This client hello handler will check for the OCSP extension + extension_iana: OCSP_IANA_EXTENSION_ID, + extension_expected: enable_ocsp, + })?; + config.load_pem(&fs::read(&cert)?, &fs::read(&key)?)?; + config.trust_location(Some(&cert), None)?; + config.build()? + }; + + let server = { + // create and configure a server connection + let mut server = crate::connection::Connection::new_server(); + server.set_config(config.clone())?; + server.set_waker(Some(&noop_waker()))?; + Harness::new(server) + }; + + let client = { + // create a client connection + let mut client = crate::connection::Connection::new_client(); + client.set_config(config)?; + Harness::new(client) + }; + + let pair = Pair::new(server, client); + + poll_tls_pair(pair); + } + Ok(()) + } + #[test] fn connection_level_verify_host_callback() -> Result<(), Error> { let reject_config = { @@ -706,60 +762,4 @@ mod tests { establish_connection(config_with_system_certs); }); } - - #[test] - fn set_check_stapled_ocsp_response_true() { - let keypair = CertKeyPair::default(); - - temp_env::with_var("SSL_CERT_FILE", Some(keypair.cert_path()), || { - let mut builder = Builder::new(); - builder - .load_pem(keypair.cert(), keypair.key()) - .unwrap() - .set_security_policy(&security::DEFAULT_TLS13) - .unwrap() - .set_verify_host_callback(InsecureAcceptAllCertificatesHandler {}) - .unwrap(); - - // Enable OCSP and some OCSP data that will fail validation - builder.enable_ocsp().unwrap(); - builder.set_ocsp_data(&[1, 2, 3]).unwrap(); - // Enable OCSP validation (the default) - builder.set_check_stapled_ocsp_response(true).unwrap(); - - let config = builder.build().unwrap(); - - let mut pair = tls_pair(config); - - // The handshake should fail - assert!(poll_tls_pair_result(&mut pair).is_err()); - }) - } - - #[test] - fn set_check_stapled_ocsp_response_false() { - let keypair = CertKeyPair::default(); - - temp_env::with_var("SSL_CERT_FILE", Some(keypair.cert_path()), || { - let mut builder = Builder::new(); - builder - .load_pem(keypair.cert(), keypair.key()) - .unwrap() - .set_security_policy(&security::DEFAULT_TLS13) - .unwrap() - .set_verify_host_callback(InsecureAcceptAllCertificatesHandler {}) - .unwrap(); - - // Enable OCSP and some OCSP data that will fail validation - builder.enable_ocsp().unwrap(); - builder.set_ocsp_data(&[1, 2, 3]).unwrap(); - // Disable OCSP validation - builder.set_check_stapled_ocsp_response(false).unwrap(); - - let config = builder.build().unwrap(); - - // The handshake should succeed since OCSP validation is disabled - establish_connection(config); - }) - } } diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index 07768e20750..b1aa1e2b4e9 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -359,6 +359,8 @@ int s2n_connection_set_config(struct s2n_connection *conn, struct s2n_config *co * However, the s2n_config_set_verification_ca_location behavior predates client authentication * support for OCSP stapling, so could only affect whether clients requested OCSP stapling. We * therefore only have to maintain the legacy behavior for clients, not servers. + * + * Note: The Rust bindings do not maintain the legacy behavior. */ conn->request_ocsp_status = config->ocsp_status_requested_by_user; if (config->ocsp_status_requested_by_s2n && conn->mode == S2N_CLIENT) { From c296579bd82b90089103f33d0929aa6f3092e094 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Mon, 19 Jun 2023 22:19:23 +0000 Subject: [PATCH 3/4] Use tls_pair --- bindings/rust/s2n-tls/src/testing/s2n_tls.rs | 25 +++++++------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs index 8a620a5775c..e21fed0d0bb 100644 --- a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs +++ b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs @@ -32,6 +32,10 @@ impl Harness { pub fn connection(&self) -> &Connection { &self.connection } + + pub fn connection_mut(&mut self) -> &mut Connection { + &mut self.connection + } } impl super::Connection for Harness { @@ -570,22 +574,11 @@ mod tests { config.build()? }; - let server = { - // create and configure a server connection - let mut server = crate::connection::Connection::new_server(); - server.set_config(config.clone())?; - server.set_waker(Some(&noop_waker()))?; - Harness::new(server) - }; - - let client = { - // create a client connection - let mut client = crate::connection::Connection::new_client(); - client.set_config(config)?; - Harness::new(client) - }; - - let pair = Pair::new(server, client); + let mut pair = tls_pair(config); + pair.server + .0 + .connection_mut() + .set_waker(Some(&noop_waker()))?; poll_tls_pair(pair); } From 9d81b5b1bec1566828aa1a71db55a3f0a28f45c0 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Mon, 19 Jun 2023 22:23:34 +0000 Subject: [PATCH 4/4] move ocsp check into trust_location --- bindings/rust/s2n-tls/src/config.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/bindings/rust/s2n-tls/src/config.rs b/bindings/rust/s2n-tls/src/config.rs index de9b530aaee..f9bb7cfaaae 100644 --- a/bindings/rust/s2n-tls/src/config.rs +++ b/bindings/rust/s2n-tls/src/config.rs @@ -326,6 +326,16 @@ impl Builder { s2n_config_set_verification_ca_location(self.as_mut_ptr(), file_ptr, dir_ptr) .into_result() }?; + + // If OCSP has not been explicitly requested, turn off OCSP. This is to prevent this function from + // automatically enabling `OCSP` due to the legacy behavior of `s2n_config_set_verification_ca_location` + if !self.enable_ocsp { + unsafe { + s2n_config_set_status_request_type(self.as_mut_ptr(), s2n_status_request_type::NONE) + .into_result()? + }; + } + Ok(self) } @@ -592,15 +602,6 @@ impl Builder { } } - // If OCSP has not been explicitly requested, turn off OCSP. This is to prevent the `trust_location()` function - // from automatically enabling `OCSP` due to the legacy behavior of `s2n_config_set_verification_ca_location` - if !self.enable_ocsp { - unsafe { - s2n_config_set_status_request_type(self.as_mut_ptr(), s2n_status_request_type::NONE) - .into_result()? - }; - } - Ok(self.config) }