From 61d3eab28e6ead05a42ea43ab1250e26d5565aa0 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Mon, 9 Sep 2024 20:41:21 -0700 Subject: [PATCH] feature: pivot renegotiation bindings to managed by s2n-tls ... rather than managed by the application, like in the C library. This is required for use with s2n-tls-tokio and s2n-tls-hyper. --- .github/workflows/ci_rust.yml | 12 +- api/unstable/renegotiate.h | 31 +- bindings/rust/s2n-tls/src/connection.rs | 46 +- bindings/rust/s2n-tls/src/renegotiate.rs | 766 +++++++++++++++++++---- bindings/rust/s2n-tls/src/testing.rs | 12 +- 5 files changed, 740 insertions(+), 127 deletions(-) diff --git a/.github/workflows/ci_rust.yml b/.github/workflows/ci_rust.yml index 3c49b90c9cb..171e6001446 100644 --- a/.github/workflows/ci_rust.yml +++ b/.github/workflows/ci_rust.yml @@ -44,15 +44,19 @@ jobs: - name: Generate run: ${{env.ROOT_PATH}}/generate.sh - - name: Tests + # Ensure that all tests pass with the default feature set + - name: Default Tests + working-directory: ${{env.ROOT_PATH}} + run: cargo test + + - name: "Feature Tests: Fingerprint, kTLS, QUIC, and PQ" working-directory: ${{env.ROOT_PATH}} # Test all features except for FIPS, which is tested separately. run: cargo test --features unstable-fingerprint,unstable-ktls,quic,pq - # Ensure that all tests pass with the default feature set - - name: Default Tests + - name: "Feature Test: Renegotiate" working-directory: ${{env.ROOT_PATH}} - run: cargo test + run: cargo test --features unstable-renegotiate - name: Test external build # if this test is failing, make sure that api headers are appropriately diff --git a/api/unstable/renegotiate.h b/api/unstable/renegotiate.h index bf2c5412cda..a051443b8ae 100644 --- a/api/unstable/renegotiate.h +++ b/api/unstable/renegotiate.h @@ -29,6 +29,31 @@ * s2n-tls clients support secure (compliant with RFC5746) renegotiation for compatibility reasons, * but s2n-tls does NOT recommend its use. While s2n-tls addresses all currently known security concerns, * renegotiation has appeared in many CVEs and was completely removed from TLS1.3. + * + * A basic renegotiation integration with s2n-tls would look like: + * 1. The application calls s2n_recv as part of normal IO. + * 2. s2n_recv receives a request for renegotiation (a HelloRequest message) + * instead of application data. + * 3. s2n_recv calls the configured s2n_renegotiate_request_cb. + * 4. The application's implementation of the s2n_renegotiate_request_cb should: + * 1. Set the `response` parameter to S2N_RENEGOTIATE_ACCEPT + * 2. Set some application state to indicate that renegotiation is required. + * s2n_connection_set_ctx can be used to associate application state with + * a specific connection. + * 3. Return success. + * 5. s2n_recv returns as part of normal IO. + * 6. The application should check the application state set in 4.2 to determine + * whether or not renegotiation is required. + * 7. The application should complete any in-progress IO. Failing to do this will + * cause s2n_negotiate_wipe to fail. + * 1. For sending, the application must retry any blocked calls to s2n_send + * until they return success. + * 2. For receiving, the application must call s2n_recv to handle any buffered + * decrypted application data. s2n_peek indicates how much data is buffered. + * 8. The application should call s2n_renegotiate_wipe. + * 9. The application should reconfigure the connection, if necessary. + * 10. The application should call s2n_renegotiate until it indicates success, + * while handling any application data encountered. */ /** @@ -92,7 +117,7 @@ S2N_API int s2n_config_set_renegotiate_request_cb(struct s2n_config *config, s2n * * The application MUST handle any incomplete IO before calling this method. The last call to `s2n_send` must * have succeeded, and `s2n_peek` must return zero. If there is any data in the send or receive buffers, - * this method will fail. + * this method will fail. That means that this method cannot be called from inside s2n_renegotiate_request_cb. * * The application MUST repeat any connection-specific setup after calling this method. This method * cannot distinguish between internal connection state and configuration state set by the application, @@ -130,6 +155,10 @@ S2N_API int s2n_renegotiate_wipe(struct s2n_connection *conn); * copy the data to `app_data_buf`, and set `app_data_size` to the size of the data. * The application should handle the data in `app_data_buf` before calling s2n_renegotiate again. * + * This method cannot be called from inside s2n_renegotiate_request_cb. The receive + * call that triggered s2n_renegotiate_request_cb must complete before either + * s2n_renegotiate_wipe or s2n_renegotiate can be called. + * * @note s2n_renegotiate_wipe MUST be called before this method. * @note Calling this method on a server connection will fail. s2n-tls servers do not support renegotiation. * diff --git a/bindings/rust/s2n-tls/src/connection.rs b/bindings/rust/s2n-tls/src/connection.rs index 9e70bcd8fee..6f739e67dcd 100644 --- a/bindings/rust/s2n-tls/src/connection.rs +++ b/bindings/rust/s2n-tls/src/connection.rs @@ -3,6 +3,8 @@ #![allow(clippy::missing_safety_doc)] // TODO add safety docs +#[cfg(feature = "unstable-renegotiate")] +use crate::renegotiate::RenegotiateState; use crate::{ callbacks::*, cert_chain::CertificateChain, @@ -579,6 +581,7 @@ impl Connection { /// [negotiate](`Self::poll_negotiate`) has succeeded. /// /// Returns the number of bytes written, and may indicate a partial write. + #[cfg(not(feature = "unstable-renegotiate"))] pub fn poll_send(&mut self, buf: &[u8]) -> Poll> { let mut blocked = s2n_blocked_status::NOT_BLOCKED; let buf_len: isize = buf.len().try_into().map_err(|_| Error::INVALID_INPUT)?; @@ -586,16 +589,25 @@ impl Connection { unsafe { s2n_send(self.connection.as_ptr(), buf_ptr, buf_len, &mut blocked).into_poll() } } + #[cfg(not(feature = "unstable-renegotiate"))] + pub(crate) fn poll_recv_raw( + &mut self, + buf_ptr: *mut ::libc::c_void, + buf_len: isize, + ) -> Poll> { + let mut blocked = s2n_blocked_status::NOT_BLOCKED; + unsafe { s2n_recv(self.connection.as_ptr(), buf_ptr, buf_len, &mut blocked).into_poll() } + } + /// Reads and decrypts data from a connection where /// [negotiate](`Self::poll_negotiate`) has succeeded. /// /// Returns the number of bytes read, and may indicate a partial read. /// 0 bytes returned indicates EOF due to connection closure. pub fn poll_recv(&mut self, buf: &mut [u8]) -> Poll> { - let mut blocked = s2n_blocked_status::NOT_BLOCKED; let buf_len: isize = buf.len().try_into().map_err(|_| Error::INVALID_INPUT)?; let buf_ptr = buf.as_ptr() as *mut ::libc::c_void; - unsafe { s2n_recv(self.connection.as_ptr(), buf_ptr, buf_len, &mut blocked).into_poll() } + self.poll_recv_raw(buf_ptr, buf_len) } /// Reads and decrypts data from a connection where @@ -613,7 +625,6 @@ impl Connection { &mut self, buf: &mut [MaybeUninit], ) -> Poll> { - let mut blocked = s2n_blocked_status::NOT_BLOCKED; let buf_len: isize = buf.len().try_into().map_err(|_| Error::INVALID_INPUT)?; let buf_ptr = buf.as_ptr() as *mut ::libc::c_void; @@ -622,7 +633,7 @@ impl Connection { // 2. if s2n_recv returns `+n`, it guarantees that the first // `n` bytes of `buf` have been initialized, which allows this // function to return `Ok(n)` - unsafe { s2n_recv(self.connection.as_ptr(), buf_ptr, buf_len, &mut blocked).into_poll() } + self.poll_recv_raw(buf_ptr, buf_len) } /// Attempts to flush any data previously buffered by a call to [send](`Self::poll_send`). @@ -910,6 +921,19 @@ impl Connection { } } + pub fn message_type(&self) -> Result<&str, Error> { + let message = unsafe { + s2n_connection_get_last_message_name(self.connection.as_ptr()).into_result()? + }; + unsafe { + // SAFETY: Constructed strings have a null byte appended to them. + // SAFETY: The data has a 'static lifetime, because it resides in a + // static char array, and is never modified after its initial + // creation. + const_str!(message) + } + } + pub fn cipher_suite(&self) -> Result<&str, Error> { let cipher = unsafe { s2n_connection_get_cipher(self.connection.as_ptr()).into_result()? }; unsafe { @@ -1150,6 +1174,16 @@ impl Connection { Some(app_context) => app_context.downcast_mut::(), } } + + #[cfg(feature = "unstable-renegotiate")] + pub(crate) fn renegotiate_state_mut(&mut self) -> &mut RenegotiateState { + &mut self.context_mut().renegotiate_state + } + + #[cfg(feature = "unstable-renegotiate")] + pub(crate) fn renegotiate_state(&self) -> &RenegotiateState { + &self.context().renegotiate_state + } } struct Context { @@ -1159,6 +1193,8 @@ struct Context { verify_host_callback: Option>, connection_initialized: bool, app_context: Option>, + #[cfg(feature = "unstable-renegotiate")] + pub(crate) renegotiate_state: RenegotiateState, } impl Context { @@ -1170,6 +1206,8 @@ impl Context { verify_host_callback: None, connection_initialized: false, app_context: None, + #[cfg(feature = "unstable-renegotiate")] + renegotiate_state: RenegotiateState::default(), } } } diff --git a/bindings/rust/s2n-tls/src/renegotiate.rs b/bindings/rust/s2n-tls/src/renegotiate.rs index a1832618eeb..01a41395274 100644 --- a/bindings/rust/s2n-tls/src/renegotiate.rs +++ b/bindings/rust/s2n-tls/src/renegotiate.rs @@ -5,6 +5,74 @@ //! //! The use of renegotiation is strongly discouraged. //! See [the C API documentation](https://github.com/aws/s2n-tls/blob/main/api/unstable/renegotiate.h). +//! +//! Unlike the C API, the Rust bindings do not require the application to +//! integrate s2n_renegotiate_wipe or s2n_renegotiate into their existing code. +//! Instead, all that is required to enable renegotiation is setting the RenegotiateCallback. +//! +//! For example: +//! ``` +//! use s2n_tls::config::Builder; +//! use s2n_tls::connection::Connection; +//! use s2n_tls::error::Error; +//! use s2n_tls::renegotiate::{RenegotiateCallback, RenegotiateResponse}; +//! +//! #[derive(Default)] +//! struct Callback { }; +//! +//! impl RenegotiateCallback for Callback { +//! fn on_renegotiate_request( +//! &mut self, +//! conn: &mut Connection, +//! ) -> Option { +//! let response = match conn.server_name() { +//! Some("allowed_to_renegotiate") => RenegotiateResponse::Accept, +//! _ => RenegotiateResponse::Reject, +//! }; +//! Some(response) +//! } +//! +//! fn on_renegotiate_wipe(&mut self, conn: &mut Connection) -> Result<(), Error> { +//! conn.set_server_name("not_allowed_to_renegotiate")?; +//! Ok(()) +//! } +//! } +//! +//! let mut builder = Builder::new(); +//! builder.set_renegotiate_callback(Callback::default()); +//! ``` +//! +//! If all renegotiation requests will be accepted and no connection-level +//! configuration is required, then RenegotiateResponse can be used as the RenegotiateCallback. +//! However, be careful: using any async callback requires connection-level configuration +//! due to [Connection::set_waker()]. +//! +//! For example: +//! ``` +//! use s2n_tls::config::Builder; +//! use s2n_tls::renegotiate::RenegotiateResponse; +//! +//! let mut builder = Builder::new(); +//! builder.set_renegotiate_callback(RenegotiateResponse::Accept); +//! ``` +//! +//! When an s2n-tls client receives a renegotiation request, `on_renegotiate_request` +//! will be invoked. If `on_renegotiate_request` returns `RenegotiateResponse::Accept`, +//! then s2n-tls will automatically schedule renegotiation. The application will +//! be able to complete any in-progress writes and read any already decrypted +//! data. However, the next time that a read or write would trigger reading or +//! writing a new TLS record, s2n-tls will instead wipe the connection, block +//! all application IO requests, and negotiate a new handshake. Both `poll_recv` +//! and `poll_send` will return Pending until renegotiation is complete. +//! +//! Handling renegotiation this way allows it to be used with higher level abstractions +//! that are unaware of renegotiation, like s2n-tls-tokio or s2n-tls-hyper. +//! However, there are downsides. During renegotiation, `poll_recv` may write and +//! `poll_send` may read. This may pose a problem if we eventually implement a +//! proper "split" operation. It also makes waker contracts difficult to reason about, +//! so any integration should probably include as much testing and instrumentation +//! as possible. Please report any bugs encountered. +//! ``` use s2n_tls_sys::*; @@ -15,7 +83,7 @@ use crate::{ enums::CallbackResult, error::{Error, Fallible, Pollable}, }; -use std::task::Poll; +use std::task::Poll::{self, Pending, Ready}; /// How to handle a renegotiation request. /// @@ -37,20 +105,46 @@ impl From for s2n_renegotiate_response::Type { } } -/// A callback that triggers when the server requests renegotiation. -/// -/// Returning "None" will result in the C callback returning an error, -/// canceling the connection. -/// -/// See s2n_renegotiate_request_cb in [the C API documentation](https://github.com/aws/s2n-tls/blob/main/api/unstable/renegotiate.h). -// -// This method returns Option instead of Result because the callback has no mechanism -// for surfacing errors to the application, so Result would be somewhat deceptive. +/// Callbacks related to the renegotiation TLS feature. pub trait RenegotiateCallback: 'static + Send + Sync { + /// A callback that triggers when the client receives a renegotiation request + /// (a HelloRequest message) from the server. + /// + /// Returning `Some(RenegotiateResponse::Accept)` will trigger s2n-tls + /// to automatically wipe the connection and renegotiate. + /// + /// Returning "None" will result in the C callback returning an error, + /// canceling the connection. + /// + /// See s2n_renegotiate_request_cb in [the C API documentation](https://github.com/aws/s2n-tls/blob/main/api/unstable/renegotiate.h). + // + // This method returns Option instead of Result because the callback has no mechanism + // for surfacing errors to the application, so Result would be somewhat deceptive. fn on_renegotiate_request( &mut self, connection: &mut Connection, ) -> Option; + + /// A callback that triggers after the connection is wiped for renegotiation. + /// + /// Because renegotiation requires wiping the connection, connection-level + /// configuration like the server name will need to be set again via this callback. + /// + /// See s2n_renegotiate_wipe in [the C API documentation](https://github.com/aws/s2n-tls/blob/main/api/unstable/renegotiate.h). + /// The Rust equivalent of the listed connection-specific methods that are NOT wiped are: + /// - Methods to set the file descriptors: not currently supported by rust bindings + /// - Methods to set the send callback: + /// ([Connection::set_send_callback()], [Connection::set_send_context()]) + /// - Methods to set the recv callback: + /// ([Connection::set_receive_callback()], [Connection::set_receive_context()]) + /// + /// Wakers set via [Connection::set_waker()] count as connection-level configuration + /// and must be set again. + /// + /// If this callback returns `Err`, then renegotiation will fail with a fatal error. + fn on_renegotiate_wipe(&mut self, _connection: &mut Connection) -> Result<(), Error> { + Ok(()) + } } impl RenegotiateCallback for RenegotiateResponse { @@ -59,44 +153,211 @@ impl RenegotiateCallback for RenegotiateResponse { } } +#[derive(Debug, PartialEq, Copy, Clone, Default)] +pub(crate) struct RenegotiateState { + need_wipe: bool, + need_handshake: bool, + send_blocked: bool, +} + +impl RenegotiateState { + fn set_renegotiate(&mut self) { + // Requests for renegotiation should be ignored if a renegotiation is already in progress. + if !self.need_handshake { + self.need_wipe = true; + self.need_handshake = true; + } + } +} + impl Connection { + fn accept_renegotiate_request(&mut self) { + self.renegotiate_state_mut().set_renegotiate(); + } + + fn is_renegotiating(&self) -> bool { + self.renegotiate_state().need_handshake + } + /// Reset the connection so that it can be renegotiated. /// - /// Returning "None" will result in the C callback returning an error, - /// canceling the connection. - /// /// See s2n_renegotiate_wipe in [the C API documentation](https://github.com/aws/s2n-tls/blob/main/api/unstable/renegotiate.h). - /// The Rust equivalent of the connection-specific methods listed are: - /// - Methods to set the file descriptors: not currently supported by rust bindings - /// - Methods to set the send callback: - /// ([Connection::set_send_callback()], [Connection::set_send_context()]) - /// - Methods to set the recv callback: - /// ([Connection::set_receive_callback()], [Connection::set_receive_context()]) - pub fn wipe_for_renegotiate(&mut self) -> Result<(), Error> { - self.wipe_method(|conn| unsafe { s2n_renegotiate_wipe(conn.as_ptr()).into_result() }) + fn wipe_for_renegotiate(&mut self) -> Result<(), Error> { + let renegotiate_state = *self.renegotiate_state(); + self.wipe_method(|conn| unsafe { s2n_renegotiate_wipe(conn.as_ptr()).into_result() })?; + *self.renegotiate_state_mut() = renegotiate_state; + if let Some(mut config) = self.config() { + if let Some(callback) = config.context_mut().renegotiate.as_mut() { + callback.on_renegotiate_wipe(self)?; + } + } + Ok(()) } - /// Perform a new handshake on an already established connection. + /// Make progress on the renegotiation handshake. /// - /// The first element of the returned pair represents progress on the new - /// handshake, like [Connection::poll_negotiate()]. + /// This method matches the interface of `poll_recv`, and as such does not + /// actually indicate whether the handshake completes or not. It returns + /// `Ready` when application data is available, not when the handshake succeeds. /// - /// If any application data is received during the new handshake, the number - /// of bytes received is returned as the second element of the returned pair, - /// and the data is written to `buf`. + /// If the handshake succeeds, the renegotiation state stored on the connection + /// will be updated so that this method is not polled again. /// - /// See s2n_renegotiate in [the C API documentation](https://github.com/aws/s2n-tls/blob/main/api/unstable/renegotiate.h). - pub fn poll_renegotiate(&mut self, buf: &mut [u8]) -> (Poll>, usize) { + /// # Safety + /// We have to worry about interleaved `poll_recv` and `poll_send` calls + /// when managing state, but we do not have to worry about thread safety. + /// Both `poll_recv` and `poll_send` take mut references, and Connection does + /// not currently support a true "split" operation. + fn poll_renegotiate_raw( + &mut self, + buf_ptr: *mut libc::c_void, + buf_len: isize, + ) -> Poll> { + if self.renegotiate_state().need_wipe { + if self.renegotiate_state().send_blocked || self.peek_len() > 0 { + // It is safe to return Pending here because `poll_recv` and + // `poll_send` are already responsible for clearing the input + // and output buffers respectively. The first one to succeed + // will block, but the second will wipe and begin renegotiation. + return Pending; + } + self.wipe_for_renegotiate()?; + self.renegotiate_state_mut().need_wipe = false; + } + let mut blocked = s2n_blocked_status::NOT_BLOCKED; - let buf_len: isize = buf.len().try_into().unwrap_or(0); - let buf_ptr = buf.as_mut_ptr(); let mut read: isize = 0; - - let r = self.poll_negotiate_method(|conn| { - unsafe { s2n_renegotiate(conn.as_ptr(), buf_ptr, buf_len, &mut read, &mut blocked) } - .into_poll() + let result = self.poll_negotiate_method(|conn| { + unsafe { + s2n_renegotiate( + conn.as_ptr(), + buf_ptr as *mut u8, + buf_len, + &mut read, + &mut blocked, + ) + } + .into_poll() }); - (r, read.try_into().unwrap()) + + if result.is_ready() { + self.renegotiate_state_mut().need_handshake = false + } + if read > 0 { + return Ready(Ok(read.try_into().unwrap())); + } + match result { + Ready(Ok(_)) => Pending, + Ready(Err(err)) => Ready(Err(err)), + Pending => Pending, + } + } + + fn poll_renegotiate(&mut self, buf: &mut [u8]) -> Poll> { + let buf_len: isize = buf.len().try_into().map_err(|_| Error::INVALID_INPUT)?; + let buf_ptr = buf.as_mut_ptr() as *mut libc::c_void; + self.poll_renegotiate_raw(buf_ptr, buf_len) + } + + /// Encrypts and sends data on a connection where + /// [negotiate](`Self::poll_negotiate`) has succeeded. + /// + /// Returns the number of bytes written, and may indicate a partial write. + /// + /// Automatically handles renegotiation. + pub fn poll_send(&mut self, buf: &[u8]) -> Poll> { + let mut blocked = s2n_blocked_status::NOT_BLOCKED; + let buf_len: isize = buf.len().try_into().map_err(|_| Error::INVALID_INPUT)?; + let buf_ptr = buf.as_ptr() as *const libc::c_void; + + // If send is blocked, then we can't override poll_send to call + // poll_renegotiate until the application finishes retrying the send. + fn is_send_renegotiating(conn: &mut Connection) -> bool { + conn.is_renegotiating() && !conn.renegotiate_state().send_blocked + } + + let is_renegotiating = is_send_renegotiating(self); + let result = if is_renegotiating { + let mut empty = [0; 0]; + self.poll_renegotiate(&mut empty) + } else { + let result = + unsafe { s2n_send(self.as_ptr(), buf_ptr, buf_len, &mut blocked) }.into_poll(); + // s2n-tls can't automatically flush blocked sends. + // The application must call s2n_send again with the same data buffer + // in order to retry a send. + // Since we can't flush automatically, we need to track whether or + // not send has been flushed by the application. + self.renegotiate_state_mut().send_blocked = result.is_pending(); + result + }; + + // A call to poll_renegotiate can trigger the need to call s2n_send. + // If the handshake blocking sending application data completes, then we + // need to attempt to send the application data at least once before we + // return Pending. Otherwise, we aren't actually blocked on anything + // specific and could break an underlying IO waker contract. + // + // A call to s2n_send can not trigger the need to call poll_negotiate. + // Even if it clears the last of the buffered data blocking renegotiation, + // the result will always be `Ready(Ok(bytes_written))` rather than `Pending`. + // + // Despite only one case being possible, we follow the same pattern as + // we do for poll_recv for consistency and simplicity. + let is_next_renegotiating = is_send_renegotiating(self); + if result.is_pending() && is_renegotiating != is_next_renegotiating { + self.poll_send(buf) + } else { + result + } + } + + pub(crate) fn poll_recv_raw( + &mut self, + buf_ptr: *mut libc::c_void, + buf_len: isize, + ) -> Poll> { + let mut blocked = s2n_blocked_status::NOT_BLOCKED; + + // Let s2n_recv handle draining any buffered IO. + // We could let poll_negotiate handle it, but this way matches poll_send. + fn is_recv_renegotiating(conn: &mut Connection) -> bool { + conn.is_renegotiating() && conn.peek_len() == 0 + } + + // If we're just trying to drain the buffered IO, + // ensure that we don't read more records. + let buf_len = if self.is_renegotiating() && self.peek_len() > 0 { + std::cmp::min(buf_len, self.peek_len() as isize) + } else { + buf_len + }; + + let is_renegotiating = is_recv_renegotiating(self); + let result = if is_renegotiating { + self.poll_renegotiate_raw(buf_ptr, buf_len) + } else { + unsafe { s2n_recv(self.as_ptr(), buf_ptr, buf_len, &mut blocked).into_poll() } + }; + + // A call to s2n_recv can trigger the need to call poll_negotiate if it + // reads a HelloRequest but no ApplicationData. If we returned Pending in + // that case without attempting to progress the handshake, we could break + // an underlying IO waker contract; the operation wouldn't actually be blocked + // on anything specific. + // + // A call to poll_negotiate can trigger the need to call s2n_recv if it + // completes the handshake that is blocking receiving application data. + // The server does write the final message in some TLS1.2 handshakes. + // If we returned Pending in that case without attempting to read the + // application data requested by the application, we would again be + // at risk of breaking underlying IO waker contracts. + let is_renegotiating_next = is_recv_renegotiating(self); + if result.is_pending() && is_renegotiating != is_renegotiating_next { + self.poll_recv_raw(buf_ptr, buf_len) + } else { + result + } } } @@ -117,6 +378,13 @@ impl config::Builder { let callback = context.renegotiate.as_mut(); if let Some(callback) = callback { if let Some(result) = callback.on_renegotiate_request(conn) { + // If the callback indicates renegotiation, schedule it. + // This doesn't actually do any work related to renegotiation, + // It just indicates to `poll_recv` and `poll_send` + // that work needs to be done later. + if result == RenegotiateResponse::Accept { + conn.accept_renegotiate_request(); + } *response = result.into(); return CallbackResult::Success.into(); } @@ -160,21 +428,25 @@ mod tests { error::Error, io::{Read, Write}, pin::Pin, - task::Poll::{Pending, Ready}, + task::{ + Poll::{Pending, Ready}, + Waker, + }, }; // Currently renegotiation is not available from the openssl-sys bindings extern "C" { fn SSL_renegotiate(s: *mut openssl_sys::SSL) -> libc::size_t; fn SSL_renegotiate_pending(s: *mut openssl_sys::SSL) -> libc::size_t; + fn SSL_in_init(s: *mut openssl_sys::SSL) -> libc::size_t; } // std::task::ready is unstable fn unwrap_poll( poll: Poll>, - ) -> Result<(), crate::error::Error> { + ) -> Result { if let Ready(value) = poll { - return value.map(|_| ()); + return value; } panic!("Poll not Ready"); } @@ -290,9 +562,7 @@ mod tests { } } - // Send and receive the hello request message, triggering renegotiate. - // The result of s2n-tls receiving the request is returned. - fn trigger_renegotiate(&mut self) -> Result<(), crate::error::Error> { + fn send_renegotiate_request(&mut self) -> Result<(), crate::error::Error> { let openssl_ptr = self.server.ssl().as_ptr(); // Sanity check that renegotiation is not initially scheduled @@ -308,14 +578,11 @@ mod tests { // SSL_renegotiate doesn't actually send the message. // Like s2n-tls, a call to send / write is required. - let to_send = [0; 1]; self.server - .write(&to_send) + .write(&[0; 0]) .expect("Failed to write hello request"); - // s2n-tls needs to attempt to read data to receive the message - let mut recv_buffer = [0; 1]; - unwrap_poll(self.client.poll_recv(&mut recv_buffer)) + Ok(()) } // Send and receive application data. @@ -331,22 +598,68 @@ mod tests { Ok(()) } - fn assert_renegotiate(&mut self) { - let mut empty = [0; 0]; - let mut buf = [0; 1]; - let mut result = Pending; - while result.is_pending() { - // openssl can only make progress by sending and receiving a 0-length array. - // Both operations can fail for a number of irrelevant reasons while - // still making progress, so we just ignore the results. - _ = self.server.write(&empty); - _ = self.server.read(&mut empty); - - let (r, n) = self.client.poll_renegotiate(&mut buf); - assert_eq!(n, 0, "Unexpected application data"); - result = r; + // This indicates that openssl is performing a handshake, but not + // specifically a renegotiation handshake. Ensure that the initial + // handshake is complete before assuming that this indicates renegotiation. + fn openssl_is_handshaking(&self) -> bool { + (unsafe { SSL_in_init(self.server.ssl().as_ptr()) } == 1) + } + + // The client drives renegotiation via poll_recv in order to read + // application data written by the server after the new handshake. + fn assert_renegotiate(&mut self) -> Result<(), Box> { + const APP_DATA: &[u8] = "Renegotiation complete".as_bytes(); + let mut buffer = [0; APP_DATA.len()]; + + for _ in 0..20 { + let client_read_poll = self.client.poll_recv(&mut buffer); + println!( + "s2n result: {:?}, state: {:?}", + client_read_poll, + self.client.message_type()? + ); + match client_read_poll { + Pending => { + assert!(self.client.is_renegotiating(), "s2n-tls not renegotiating"); + } + Ready(Ok(bytes_read)) => { + assert_eq!(bytes_read, APP_DATA.len()); + assert_eq!(&buffer, APP_DATA); + break; + } + Ready(err) => err.map(|_| ())?, + }; + + // Openssl needs to read the new ClientHello in order to know + // that s2n-tls is actually renegotiating. + // But after initial read, reads and writes can both progress the handshake. + if !self.openssl_is_handshaking() { + let _ = self.server.read(&mut [0; 0]); + } else { + let server_write_result = self.server.write(APP_DATA); + println!( + "openssl result: {:?}, state: {:?}", + server_write_result, + self.server.ssl().state_string_long() + ); + match server_write_result { + Ok(bytes_written) => assert_eq!(bytes_written, APP_DATA.len()), + Err(_) => { + assert!(self.openssl_is_handshaking(), "openssl not renegotiating"); + } + } + } } - unwrap_poll(result).expect("Renegotiate"); + + assert!( + !self.client.is_renegotiating(), + "s2n-tls renegotiation not complete" + ); + assert!( + !self.openssl_is_handshaking(), + "openssl renegotiation not complete" + ); + Ok(()) } } @@ -359,8 +672,10 @@ mod tests { pair.handshake().expect("Initial handshake"); // Expect receiving the hello request to be successful - pair.trigger_renegotiate().expect("Trigger renegotiate"); + pair.send_renegotiate_request() + .expect("Server sends request"); pair.send_and_receive().expect("Application data"); + assert!(!pair.client.is_renegotiating(), "Unexpected renegotiation"); Ok(()) } @@ -382,8 +697,10 @@ mod tests { let mut pair = RenegotiateTestPair::from(builder)?; pair.handshake().expect("Initial handshake"); + pair.send_renegotiate_request() + .expect("Server sends request"); // Expect receiving the hello request to be an error - let error = pair.trigger_renegotiate().unwrap_err(); + let error = unwrap_poll(pair.client.poll_recv(&mut [0; 1])).unwrap_err(); assert_eq!(error.name(), "S2N_ERR_CANCELLED"); Ok(()) @@ -396,10 +713,10 @@ mod tests { let mut pair = RenegotiateTestPair::from(builder)?; pair.handshake().expect("Initial handshake"); - // Expect handling the hello request to succeed. + pair.send_renegotiate_request() + .expect("Server sends request"); // s2n-tls doesn't fail when it rejects renegotiatation, it just sends // a warning alert. The peer chooses how to handle that alert. - pair.trigger_renegotiate().expect("Trigger renegotiate"); // The openssl server receives the alert on its next read. // Openssl considers the alert an error. let openssl_error = pair.send_and_receive().unwrap_err(); @@ -409,23 +726,41 @@ mod tests { } #[test] - fn do_renegotiate() -> Result<(), Box> { + fn do_renegotiate_basic() -> Result<(), Box> { let mut builder = config::Builder::new(); builder.set_renegotiate_callback(RenegotiateResponse::Accept)?; let mut pair = RenegotiateTestPair::from(builder)?; pair.handshake().expect("Initial handshake"); - pair.trigger_renegotiate().expect("Trigger renegotiate"); pair.send_and_receive() .expect("Application data before renegotiate"); - pair.client - .wipe_for_renegotiate() - .expect("Wipe for renegotiate"); - - pair.assert_renegotiate(); - + pair.send_renegotiate_request() + .expect("Server sends request"); + pair.assert_renegotiate().expect("Renegotiate"); pair.send_and_receive() .expect("Application data after renegotiate"); + + Ok(()) + } + + #[test] + fn do_renegotiate_repeatedly() -> Result<(), Box> { + let mut builder = config::Builder::new(); + builder.set_renegotiate_callback(RenegotiateResponse::Accept)?; + let mut pair = RenegotiateTestPair::from(builder)?; + + pair.handshake().expect("Initial handshake"); + + for _ in 0..10 { + pair.send_and_receive() + .expect("Application data before renegotiate"); + pair.send_renegotiate_request() + .expect("Server sends request"); + pair.assert_renegotiate().expect("Renegotiate"); + pair.send_and_receive() + .expect("Application data after renegotiate"); + } + Ok(()) } @@ -434,28 +769,238 @@ mod tests { let mut builder = config::Builder::new(); builder.set_renegotiate_callback(RenegotiateResponse::Accept)?; let mut pair = RenegotiateTestPair::from(builder)?; + pair.handshake().expect("Initial handshake"); + + // The server can send: + // - APP_DATA + // - HELLO_REQUEST + // - APP_DATA + // - SERVER_HELLO + // No more application data is allowed until the handshake completes. + let server_data_before_request = "server_data_before_request".as_bytes(); + pair.server + .write(server_data_before_request) + .expect("server APP_DATA before HELLO_REQUEST"); + pair.send_renegotiate_request() + .expect("server HELLO_REQUEST"); + let server_data_before_hello = "server_data_before_hello".as_bytes(); + pair.server + .write(server_data_before_hello) + .expect("server APP_DATA before HELLO_REQUEST"); + let server_data = [server_data_before_request, server_data_before_hello]; + + // The client can send: + // - APP_DATA + // - CLIENT_HELLO + // No more application data is allowed until the handshake completes. + let client_data_before_hello = "client_data_before_hello".as_bytes(); + unwrap_poll(pair.client.poll_send(client_data_before_hello)) + .expect("client APP_DATA before CLIENT_HELLO"); + + // Client reads all server data + for data in server_data { + let mut buffer = [0; 100]; + let read = unwrap_poll(pair.client.poll_recv(&mut buffer))?; + assert_eq!(read, data.len()); + assert_eq!(&buffer[0..read], data); + } + + // Server reads all client data + let mut buffer = [0; 100]; + let read = pair.server.read(&mut buffer)?; + assert_eq!(read, client_data_before_hello.len()); + assert_eq!(&buffer[0..read], client_data_before_hello); + + // Assert that a renegotiation is in progress + assert!(pair.client.is_renegotiating()); + // Complete the renegotiation + pair.assert_renegotiate()?; + Ok(()) + } + + #[test] + fn do_renegotiate_with_buffered_read() -> Result<(), Box> { + let mut builder = config::Builder::new(); + builder.set_renegotiate_callback(RenegotiateResponse::Accept)?; + let mut pair = RenegotiateTestPair::from(builder)?; pair.handshake().expect("Initial handshake"); - pair.trigger_renegotiate().expect("Trigger renegotiate"); - pair.send_and_receive() - .expect("Application data before renegotiate"); - pair.client - .wipe_for_renegotiate() - .expect("Wipe for renegotiate"); - let to_write = "hello world"; - let mut buf = [0; 100]; + let server_data = "full server data".as_bytes(); + pair.send_renegotiate_request() + .expect("Server sends request"); + pair.server.write(&server_data)?; + + // Read the server data one byte at a time, slowly draining the buffered data. + for i in 0..server_data.len() { + // The renegotiation request is read with the first byte, + // but wiping is blocked until all the buffered data is drained. + assert_eq!(pair.client.is_renegotiating(), i > 0); + assert_eq!(pair.client.renegotiate_state().need_wipe, i > 0); + + let mut buffer = [0; 1]; + let read = unwrap_poll(pair.client.poll_recv(&mut buffer))?; + assert_eq!(read, 1); + assert_eq!(buffer[0], server_data[i]); + assert_eq!(pair.client.peek_len(), server_data.len() - i - 1); + } + + pair.assert_renegotiate().expect("Renegotiate"); + Ok(()) + } + + #[test] + fn do_renegotiate_with_buffered_write() -> Result<(), Box> { + unsafe extern "C" fn blocking_send_cb( + _: *mut libc::c_void, + _: *const u8, + _: u32, + ) -> libc::c_int { + errno::set_errno(errno::Errno(libc::EWOULDBLOCK)); + return -1; + } + + let mut builder = config::Builder::new(); + builder.set_renegotiate_callback(RenegotiateResponse::Accept)?; + let mut pair = RenegotiateTestPair::from(builder)?; + pair.handshake().expect("Initial handshake"); + + // The client needs to initially block on send. + let client_data = "client data".as_bytes(); + pair.client.set_send_callback(Some(blocking_send_cb))?; + assert!(pair.client.poll_send(&client_data).is_pending()); + assert!(pair.client.renegotiate_state_mut().send_blocked); + + // Renegotiation should also initially block on send. + pair.send_renegotiate_request() + .expect("Server sends request"); + assert!(pair.client.poll_recv(&mut [0; 1]).is_pending()); + assert!(pair.client.poll_send(&client_data).is_pending()); + assert!(pair.client.is_renegotiating()); + assert!(pair.client.renegotiate_state_mut().send_blocked); + + // Unblock sending by restoring the original callback + pair.client.set_send_callback(Some(TestPair::send_cb))?; + unwrap_poll(pair.client.poll_send(&client_data)).expect("Send unblocked"); + assert!(!pair.client.renegotiate_state_mut().send_blocked); + + // Server can now receive the data. + let mut buffer = [0; 100]; + let read = pair.server.read(&mut buffer).expect("Server read"); + assert_eq!(read, client_data.len()); + assert_eq!(&buffer[..read], client_data); + + pair.assert_renegotiate().expect("Renegotiate"); + Ok(()) + } + + #[test] + fn do_renegotiate_via_send() -> Result<(), Box> { + let mut builder = config::Builder::new(); + builder.set_renegotiate_callback(RenegotiateResponse::Accept)?; + let mut pair = RenegotiateTestPair::from(builder)?; + pair.handshake().expect("Initial handshake"); + + // Initially renegotiation can only be triggered by poll_recv. + // Setup the calls such that buffered data prevents renegotiation from + // making any progress on the initial poll_recv (the wipe is blocked). + let buffered_server_data = "buffered_server_data".as_bytes(); + pair.send_renegotiate_request() + .expect("server HELLO_REQUEST"); + pair.server.write(&buffered_server_data)?; + let read = unwrap_poll(pair.client.poll_recv(&mut [0; 1]))?; + assert_eq!(read, 1); + assert!(pair.client.is_renegotiating()); + // Buffered data blocks the wipe + assert!(pair.client.peek_len() > 0); + assert!(pair.client.renegotiate_state().need_wipe); + + // The server can continue to write application data. + // This application data is not buffered before renegotiation, + // and will need to be read during renegotiation. + let server_data = "server_data".as_bytes(); + pair.server + .write(server_data) + .expect("server writes app data"); + + // The server needs to call read in order to receive the ClientHello + // and start renegotiation. This should send the ServerHello. pair.server - .write(to_write.as_bytes()) - .expect("Application data during renegotiate"); - let (result, n) = pair.client.poll_renegotiate(&mut buf); - assert!(result.is_pending()); - assert_eq!(n, to_write.len(), "Incorrect application data"); - assert_eq!(&buf[..n], to_write.as_bytes()); + .read(&mut [0; 1]) + .expect_err("server blocks on reading app data"); + + // Assert that poll_send can't drain the buffered data so can't make + // progress on renegotiation. poll_send has no mechanism for returning + // the buffered data to the application. + let client_data = "client_data".as_bytes(); + assert!(pair.client.poll_send(&client_data).is_pending()); + assert!(pair.client.peek_len() > 0); + assert!(pair.client.renegotiate_state().need_wipe); + + // Drain the buffered data via poll_recv + let mut buffer = [0; 100]; + let expected = pair.client.peek_len(); + let read = unwrap_poll(pair.client.poll_recv(&mut buffer))?; + assert_eq!(read, expected); + assert_eq!(pair.client.peek_len(), 0); + assert!(pair.client.renegotiate_state().need_wipe); + + // Progress the handshake via poll_send. + // However, renegotiation is blocked on the next application data + // because poll_send has no mechansim for returning application data. + assert!(pair.client.poll_send(&client_data).is_pending()); + // Renegotiation at least progressed past the wipe + assert!(pair.client.is_renegotiating()); + assert!(!pair.client.renegotiate_state().need_wipe); + + // Let poll_recv handle the application data + let mut buffer = [0; 100]; + let read = unwrap_poll(pair.client.poll_recv(&mut buffer))?; + assert_eq!(read, server_data.len()); + assert_eq!(&buffer[..read], server_data); + + // Finish renegotiation with poll_send + loop { + // The s2n client should only send after completing the new handshake + match pair.client.poll_send(&client_data) { + Ready(Ok(sent)) => { + assert_eq!(sent, client_data.len()); + assert!(!pair.client.is_renegotiating()); + break; + } + Ready(err) => panic!("Renegotiate failed: {:?}", err), + Pending => assert!(pair.client.is_renegotiating()), + } + let mut buffer = [0; 100]; + // The openssl server should always block on reading + assert!(pair.server.read(&mut buffer).is_err()); + } + + // After renegotiation, the openssl server can read the sent data. + let mut buffer = [0; 100]; + let read = pair.server.read(&mut buffer)?; + assert_eq!(read, client_data.len()); + assert_eq!(&buffer[..read], client_data); Ok(()) } + #[derive(Debug, Clone)] + struct WakerRenegotiateCallback(Waker); + impl RenegotiateCallback for WakerRenegotiateCallback { + fn on_renegotiate_request(&mut self, conn: &mut Connection) -> Option { + RenegotiateResponse::Accept.on_renegotiate_request(conn) + } + + fn on_renegotiate_wipe( + &mut self, + conn: &mut Connection, + ) -> Result<(), crate::error::Error> { + conn.set_waker(Some(&self.0))?; + Ok(()) + } + } + #[test] fn do_renegotiate_with_async_callback() -> Result<(), Box> { // To test how renegotiate handles blocking on async callbacks, @@ -513,28 +1058,22 @@ mod tests { op: None, }; + let (waker, wake_count) = new_count_waker(); + let reneg_callback = WakerRenegotiateCallback(waker.clone()); + let mut builder = config::Builder::new(); - builder.set_renegotiate_callback(RenegotiateResponse::Accept)?; + builder.set_renegotiate_callback(reneg_callback)?; builder.set_private_key_callback(async_callback)?; let mut pair = RenegotiateTestPair::from(builder)?; - - let (waker, wake_count) = new_count_waker(); pair.client.set_waker(Some(&waker))?; pair.handshake().expect("Initial handshake"); assert_eq!(wake_count, count_per_handshake); - pair.trigger_renegotiate().expect("Trigger renegotiate"); - pair.send_and_receive() - .expect("Application data before renegotiate"); - pair.client - .wipe_for_renegotiate() - .expect("Wipe for renegotiate"); - // Reset the waker - pair.client.set_waker(Some(&waker))?; + pair.send_renegotiate_request() + .expect("Server sends request"); + pair.assert_renegotiate()?; - pair.assert_renegotiate(); assert_eq!(wake_count, count_per_handshake * 2); - Ok(()) } @@ -564,6 +1103,8 @@ mod tests { ) -> Poll> { ctx.waker().wake_by_ref(); let this = self.get_mut(); + // Assert that the server name is not already set + assert!(conn.server_name().is_none()); if this.count > 1 { // Repeatedly block the handshake in order to verify // that renegotiate can handle Pending callbacks. @@ -583,28 +1124,21 @@ mod tests { server_name: expected_server_name.to_owned(), }; + let (waker, wake_count) = new_count_waker(); + let reneg_callback = WakerRenegotiateCallback(waker.clone()); + let mut builder = config::Builder::new(); - builder.set_renegotiate_callback(RenegotiateResponse::Accept)?; + builder.set_renegotiate_callback(reneg_callback)?; builder.set_connection_initializer(initializer)?; - let mut pair = RenegotiateTestPair::from(builder)?; - let (waker, wake_count) = new_count_waker(); + let mut pair = RenegotiateTestPair::from(builder)?; pair.client.set_waker(Some(&waker))?; pair.handshake().expect("Initial handshake"); assert_eq!(wake_count, count_per_handshake); - pair.trigger_renegotiate().expect("Trigger renegotiate"); - pair.send_and_receive() - .expect("Application data before renegotiate"); - pair.client - .wipe_for_renegotiate() - .expect("Wipe for renegotiate"); - // Verify that the wipe cleared the server name - assert!(pair.client.server_name().is_none()); - // Reset the waker - pair.client.set_waker(Some(&waker))?; - - pair.assert_renegotiate(); + pair.send_renegotiate_request() + .expect("Server sends request"); + pair.assert_renegotiate()?; assert_eq!(wake_count, count_per_handshake * 2); // Both the client and server should have the correct server name diff --git a/bindings/rust/s2n-tls/src/testing.rs b/bindings/rust/s2n-tls/src/testing.rs index 1bf13d722db..feaab38acba 100644 --- a/bindings/rust/s2n-tls/src/testing.rs +++ b/bindings/rust/s2n-tls/src/testing.rs @@ -303,7 +303,11 @@ impl TestPair { } } - unsafe extern "C" fn send_cb(context: *mut c_void, data: *const u8, len: u32) -> c_int { + pub(crate) unsafe extern "C" fn send_cb( + context: *mut c_void, + data: *const u8, + len: u32, + ) -> c_int { let context = &*(context as *const LocalDataBuffer); let data = core::slice::from_raw_parts(data, len as _); let bytes_written = context.borrow_mut().write(data).unwrap(); @@ -312,7 +316,11 @@ impl TestPair { // Note: this callback will be invoked multiple times in the event that // the byte-slices of the VecDeque are not contiguous (wrap around). - unsafe extern "C" fn recv_cb(context: *mut c_void, data: *mut u8, len: u32) -> c_int { + pub(crate) unsafe extern "C" fn recv_cb( + context: *mut c_void, + data: *mut u8, + len: u32, + ) -> c_int { let context = &*(context as *const LocalDataBuffer); let data = core::slice::from_raw_parts_mut(data, len as _); match context.borrow_mut().read(data) {