From 3d7ff0a5c08bdece7f66cad6c974b8f488ade4fe Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Thu, 3 Aug 2023 18:35:55 +0200 Subject: [PATCH] Introduce setup_accept and setup_connect These two new kinds of methods immediately return a MidHandshakeSslStream instead of actually initiating a handshake. This greatly simplifies loops around MidHandshakeSslStream::WouldBlock. --- boring/src/ssl/connector.rs | 71 ++++++++++++++-- boring/src/ssl/mod.rs | 157 +++++++++++++++++++++++------------- tokio-boring/src/lib.rs | 83 +++++-------------- 3 files changed, 186 insertions(+), 125 deletions(-) diff --git a/boring/src/ssl/connector.rs b/boring/src/ssl/connector.rs index 6bb58dab..e910a324 100644 --- a/boring/src/ssl/connector.rs +++ b/boring/src/ssl/connector.rs @@ -10,6 +10,8 @@ use crate::ssl::{ use crate::version; use std::net::IpAddr; +use super::MidHandshakeSslStream; + const FFDHE_2048: &str = " -----BEGIN DH PARAMETERS----- MIIBCAKCAQEA//////////+t+FRYortKmq/cViAnPTzx2LnFg84tNpWp4TZBFGQz @@ -99,11 +101,30 @@ impl SslConnector { /// Initiates a client-side TLS session on a stream. /// /// The domain is used for SNI and hostname verification. + pub fn setup_connect( + &self, + domain: &str, + stream: S, + ) -> Result, ErrorStack> + where + S: Read + Write, + { + self.configure()?.setup_connect(domain, stream) + } + + /// Attempts a client-side TLS session on a stream. + /// + /// The domain is used for SNI (if it is not an IP address) and hostname verification if enabled. + /// + /// This is a convenience method which combines [`Self::setup_connect`] and + /// [`MidHandshakeSslStream::handshake`]. pub fn connect(&self, domain: &str, stream: S) -> Result, HandshakeError> where S: Read + Write, { - self.configure()?.connect(domain, stream) + self.setup_connect(domain, stream) + .map_err(HandshakeError::SetupFailure)? + .handshake() } /// Returns a structure allowing for configuration of a single TLS session before connection. @@ -190,7 +211,7 @@ impl ConnectConfiguration { self.verify_hostname = verify_hostname; } - /// Returns an `Ssl` configured to connect to the provided domain. + /// Returns an [`Ssl`] configured to connect to the provided domain. /// /// The domain is used for SNI (if it is not an IP address) and hostname verification if enabled. pub fn into_ssl(mut self, domain: &str) -> Result { @@ -214,11 +235,33 @@ impl ConnectConfiguration { /// Initiates a client-side TLS session on a stream. /// /// The domain is used for SNI (if it is not an IP address) and hostname verification if enabled. + /// + /// This is a convenience method which combines [`Self::into_ssl`] and + /// [`Ssl::setup_connect`]. + pub fn setup_connect( + self, + domain: &str, + stream: S, + ) -> Result, ErrorStack> + where + S: Read + Write, + { + Ok(self.into_ssl(domain)?.setup_connect(stream)) + } + + /// Attempts a client-side TLS session on a stream. + /// + /// The domain is used for SNI (if it is not an IP address) and hostname verification if enabled. + /// + /// This is a convenience method which combines [`Self::setup_connect`] and + /// [`MidHandshakeSslStream::handshake`]. pub fn connect(self, domain: &str, stream: S) -> Result, HandshakeError> where S: Read + Write, { - self.into_ssl(domain)?.connect(stream) + self.setup_connect(domain, stream) + .map_err(HandshakeError::SetupFailure)? + .handshake() } } @@ -327,13 +370,29 @@ impl SslAcceptor { Ok(SslAcceptorBuilder(ctx)) } - /// Initiates a server-side TLS session on a stream. - pub fn accept(&self, stream: S) -> Result, HandshakeError> + /// Initiates a server-side TLS handshake on a stream. + /// + /// See [`Ssl::setup_accept`] for more details. + pub fn setup_accept(&self, stream: S) -> Result, ErrorStack> where S: Read + Write, { let ssl = Ssl::new(&self.0)?; - ssl.accept(stream) + + Ok(ssl.setup_accept(stream)) + } + + /// Attempts a server-side TLS handshake on a stream. + /// + /// This is a convenience method which combines [`Self::setup_accept`] and + /// [`MidHandshakeSslStream::handshake`]. + pub fn accept(&self, stream: S) -> Result, HandshakeError> + where + S: Read + Write, + { + self.setup_accept(stream) + .map_err(HandshakeError::SetupFailure)? + .handshake() } /// Consumes the `SslAcceptor`, returning the inner raw `SslContext`. diff --git a/boring/src/ssl/mod.rs b/boring/src/ssl/mod.rs index 3f8bb3dc..c9cd6f6e 100644 --- a/boring/src/ssl/mod.rs +++ b/boring/src/ssl/mod.rs @@ -2320,10 +2320,11 @@ impl Ssl { } } - /// Creates a new `Ssl`. + /// Creates a new [`Ssl`]. /// - /// This corresponds to [`SSL_new`]. - /// This function does the same as [`Self:new()`] except that it takes &[SslContextRef]. + /// This corresponds to [`SSL_new`](`ffi::SSL_new`). + /// + /// This function does the same as [`Self:new`] except that it takes &[SslContextRef]. // Both functions exist for backward compatibility (no breaking API). pub fn new_from_ref(ctx: &SslContextRef) -> Result { unsafe { @@ -2337,34 +2338,52 @@ impl Ssl { } } - /// Initiates a client-side TLS handshake. + /// Initiates a client-side TLS handshake, returning a [`MidHandshakeSslStream`]. /// - /// This corresponds to [`SSL_connect`]. + /// This method is guaranteed to return without calling any callback defined + /// in the internal [`Ssl`] or [`SslContext`]. + /// + /// See [`SslStreamBuilder::setup_connect`] for more details. + /// + /// # Warning + /// + /// BoringSSL's default configuration is insecure. It is highly recommended to use + /// [`SslConnector`] rather than [`Ssl`] directly, as it manages that configuration. + pub fn setup_connect(self, stream: S) -> MidHandshakeSslStream + where + S: Read + Write, + { + SslStreamBuilder::new(self, stream).setup_connect() + } + + /// Attempts a client-side TLS handshake. + /// + /// This is a convenience method which combines [`Self::setup_connect`] and + /// [`MidHandshakeSslStream::handshake`]. /// /// # Warning /// /// OpenSSL's default configuration is insecure. It is highly recommended to use - /// `SslConnector` rather than `Ssl` directly, as it manages that configuration. - /// - /// [`SSL_connect`]: https://www.openssl.org/docs/manmaster/man3/SSL_connect.html + /// [`SslConnector`] rather than `Ssl` directly, as it manages that configuration. pub fn connect(self, stream: S) -> Result, HandshakeError> where S: Read + Write, { - SslStreamBuilder::new(self, stream).connect() + self.setup_connect(stream).handshake() } /// Initiates a server-side TLS handshake. /// - /// This corresponds to [`SSL_accept`]. + /// This method is guaranteed to return without calling any callback defined + /// in the internal [`Ssl`] or [`SslContext`]. + /// + /// See [`SslStreamBuilder::setup_accept`] for more details. /// /// # Warning /// - /// OpenSSL's default configuration is insecure. It is highly recommended to use - /// `SslAcceptor` rather than `Ssl` directly, as it manages that configuration. - /// - /// [`SSL_accept`]: https://www.openssl.org/docs/manmaster/man3/SSL_accept.html - pub fn accept(self, stream: S) -> Result, HandshakeError> + /// BoringSSL's default configuration is insecure. It is highly recommended to use + /// [`SslAcceptor`] rather than [`Ssl`] directly, as it manages that configuration. + pub fn setup_accept(self, stream: S) -> MidHandshakeSslStream where S: Read + Write, { @@ -2383,7 +2402,25 @@ impl Ssl { } } - SslStreamBuilder::new(self, stream).accept() + SslStreamBuilder::new(self, stream).setup_accept() + } + + /// Attempts a server-side TLS handshake. + /// + /// This is a convenience method which combines [`Self::setup_accept`] and + /// [`MidHandshakeSslStream::handshake`]. + /// + /// # Warning + /// + /// OpenSSL's default configuration is insecure. It is highly recommended to use + /// `SslAcceptor` rather than `Ssl` directly, as it manages that configuration. + /// + /// [`SSL_accept`]: https://www.openssl.org/docs/manmaster/man3/SSL_accept.html + pub fn accept(self, stream: S) -> Result, HandshakeError> + where + S: Read + Write, + { + self.setup_accept(stream).handshake() } } @@ -3594,56 +3631,68 @@ where unsafe { ffi::SSL_set_accept_state(self.inner.ssl.as_ptr()) } } - /// See `Ssl::connect` - pub fn connect(self) -> Result, HandshakeError> { - let mut stream = self.inner; + /// Initiates a client-side TLS handshake, returning a [`MidHandshakeSslStream`]. + /// + /// This method calls [`Self::set_connect_state`] and returns without actually + /// initiating the handshake. The caller is then free to call + /// [`MidHandshakeSslStream`] and loop on [`HandshakeError::WouldBlock`]. + pub fn setup_connect(mut self) -> MidHandshakeSslStream { + self.set_connect_state(); #[cfg(feature = "kx-safe-default")] - stream.ssl.client_set_default_curves_list(); + self.inner.ssl.client_set_default_curves_list(); - let ret = unsafe { ffi::SSL_connect(stream.ssl.as_ptr()) }; - if ret > 0 { - Ok(stream) - } else { - let error = stream.make_error(ret); - match error.would_block() { - true => Err(HandshakeError::WouldBlock(MidHandshakeSslStream { - stream, - error, - })), - false => Err(HandshakeError::Failure(MidHandshakeSslStream { - stream, - error, - })), - } + MidHandshakeSslStream { + stream: self.inner, + error: Error { + code: ErrorCode::WANT_WRITE, + cause: Some(InnerError::Io(io::Error::new( + io::ErrorKind::WouldBlock, + "connect handshake has not started yet", + ))), + }, } } - /// See `Ssl::accept` - pub fn accept(self) -> Result, HandshakeError> { - let mut stream = self.inner; + /// Attempts a client-side TLS handshake. + /// + /// This is a convenience method which combines [`Self::setup_connect`] and + /// [`MidHandshakeSslStream::handshake`]. + pub fn connect(self) -> Result, HandshakeError> { + self.setup_connect().handshake() + } + + /// Initiates a server-side TLS handshake, returning a [`MidHandshakeSslStream`]. + /// + /// This method calls [`Self::set_accept_state`] and returns without actually + /// initiating the handshake. The caller is then free to call + /// [`MidHandshakeSslStream`] and loop on [`HandshakeError::WouldBlock`]. + pub fn setup_accept(mut self) -> MidHandshakeSslStream { + self.set_accept_state(); #[cfg(feature = "kx-safe-default")] - stream.ssl.server_set_default_curves_list(); + self.inner.ssl.server_set_default_curves_list(); - let ret = unsafe { ffi::SSL_accept(stream.ssl.as_ptr()) }; - if ret > 0 { - Ok(stream) - } else { - let error = stream.make_error(ret); - match error.would_block() { - true => Err(HandshakeError::WouldBlock(MidHandshakeSslStream { - stream, - error, - })), - false => Err(HandshakeError::Failure(MidHandshakeSslStream { - stream, - error, - })), - } + MidHandshakeSslStream { + stream: self.inner, + error: Error { + code: ErrorCode::WANT_READ, + cause: Some(InnerError::Io(io::Error::new( + io::ErrorKind::WouldBlock, + "accept handshake has not started yet", + ))), + }, } } + /// Attempts a server-side TLS handshake. + /// + /// This is a convenience method which combines [`Self::setup_accept`] and + /// [`MidHandshakeSslStream::handshake`]. + pub fn accept(self) -> Result, HandshakeError> { + self.setup_accept().handshake() + } + /// Initiates the handshake. /// /// This will fail if `set_accept_state` or `set_connect_state` was not called first. diff --git a/tokio-boring/src/lib.rs b/tokio-boring/src/lib.rs index a0dd58c5..f594231d 100644 --- a/tokio-boring/src/lib.rs +++ b/tokio-boring/src/lib.rs @@ -13,6 +13,7 @@ #![warn(missing_docs)] #![cfg_attr(docsrs, feature(doc_auto_cfg))] +use boring::error::ErrorStack; use boring::ssl::{ self, ConnectConfiguration, ErrorCode, MidHandshakeSslStream, ShutdownResult, SslAcceptor, SslRef, @@ -35,7 +36,7 @@ pub async fn connect( where S: AsyncRead + AsyncWrite + Unpin, { - handshake(|s| config.connect(domain, s), stream).await + handshake(|s| config.setup_connect(domain, s), stream).await } /// Asynchronously performs a server-side TLS handshake over the provided stream. @@ -43,24 +44,22 @@ pub async fn accept(acceptor: &SslAcceptor, stream: S) -> Result where S: AsyncRead + AsyncWrite + Unpin, { - handshake(|s| acceptor.accept(s), stream).await + handshake(|s| acceptor.setup_accept(s), stream).await } -async fn handshake(f: F, stream: S) -> Result, HandshakeError> +async fn handshake( + f: impl FnOnce(StreamWrapper) -> Result>, ErrorStack>, + stream: S, +) -> Result, HandshakeError> where - F: FnOnce( - StreamWrapper, - ) - -> Result>, ssl::HandshakeError>> - + Unpin, S: AsyncRead + AsyncWrite + Unpin, { - let start = StartHandshakeFuture(Some(StartHandshakeFutureInner { f, stream })); + let ongoing_handshake = Some( + f(StreamWrapper { stream, context: 0 }) + .map_err(|err| HandshakeError(ssl::HandshakeError::SetupFailure(err)))?, + ); - match start.await? { - StartedHandshake::Done(s) => Ok(s), - StartedHandshake::Mid(s) => HandshakeFuture(Some(s)).await, - } + HandshakeFuture(ongoing_handshake).await } struct StreamWrapper { @@ -334,53 +333,6 @@ where } } -enum StartedHandshake { - Done(SslStream), - Mid(MidHandshakeSslStream>), -} - -struct StartHandshakeFuture(Option>); - -struct StartHandshakeFutureInner { - f: F, - stream: S, -} - -impl Future for StartHandshakeFuture -where - F: FnOnce( - StreamWrapper, - ) - -> Result>, ssl::HandshakeError>> - + Unpin, - S: Unpin, -{ - type Output = Result, HandshakeError>; - - fn poll( - mut self: Pin<&mut Self>, - ctx: &mut Context<'_>, - ) -> Poll, HandshakeError>> { - let inner = self.0.take().expect("future polled after completion"); - - let stream = StreamWrapper { - stream: inner.stream, - context: ctx as *mut _ as usize, - }; - match (inner.f)(stream) { - Ok(mut s) => { - s.get_mut().context = 0; - Poll::Ready(Ok(StartedHandshake::Done(SslStream(s)))) - } - Err(ssl::HandshakeError::WouldBlock(mut s)) => { - s.get_mut().context = 0; - Poll::Ready(Ok(StartedHandshake::Mid(s))) - } - Err(e) => Poll::Ready(Err(HandshakeError(e))), - } - } -} - struct HandshakeFuture(Option>>); impl Future for HandshakeFuture @@ -389,21 +341,22 @@ where { type Output = Result, HandshakeError>; - fn poll( - mut self: Pin<&mut Self>, - ctx: &mut Context<'_>, - ) -> Poll, HandshakeError>> { + fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let mut s = self.0.take().expect("future polled after completion"); - s.get_mut().context = ctx as *mut _ as usize; + s.get_mut().context = cx as *mut _ as usize; + match s.handshake() { Ok(mut s) => { s.get_mut().context = 0; + Poll::Ready(Ok(SslStream(s))) } Err(ssl::HandshakeError::WouldBlock(mut s)) => { s.get_mut().context = 0; + self.0 = Some(s); + Poll::Pending } Err(e) => Poll::Ready(Err(HandshakeError(e))),