Remove all connect and accept functions

Those functions were blurring the line between setup failures
(which are caused by the developer misusing the API) and actual
failures encountered on the stream while trying to achieve the
TLS handshake, especially on SslConnector and SslAcceptor.

Removing them allows for the removal of HandshakeError, as
the HandshakeError::SetupFailure variant becomes useless,
and there is no real need to distinguish in that error type
between Failure and WouldBlock when we can just check
the error stored in MidHandshakeSslStream.

This then allow us to simplify tokio_boring's own entry points,
also making them distinguish between setup failures and failures
on the stream.

Author: nox

boring/src/ssl/connector.rs

@@ -4,8 +4,8 @@ use std::ops::{Deref, DerefMut};
 use crate::dh::Dh;
 use crate::error::ErrorStack;
 use crate::ssl::{
-    HandshakeError, Ssl, SslContext, SslContextBuilder, SslContextRef, SslMethod, SslMode,
-    SslOptions, SslRef, SslStream, SslVerifyMode,
+    Ssl, SslContext, SslContextBuilder, SslContextRef, SslMethod, SslMode, SslOptions, SslRef,
+    SslVerifyMode,
 };
 use crate::version;
 use std::net::IpAddr;
@@ -112,21 +112,6 @@ impl SslConnector {
         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<S>(&self, domain: &str, stream: S) -> Result<SslStream<S>, HandshakeError<S>>
-    where
-        S: Read + Write,
-    {
-        self.setup_connect(domain, stream)
-            .map_err(HandshakeError::SetupFailure)?
-            .handshake()
-    }
-
     /// Returns a structure allowing for configuration of a single TLS session before connection.
     pub fn configure(&self) -> Result<ConnectConfiguration, ErrorStack> {
         Ssl::new(&self.0).map(|ssl| ConnectConfiguration {
@@ -253,21 +238,6 @@ impl ConnectConfiguration {
     {
         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<S>(self, domain: &str, stream: S) -> Result<SslStream<S>, HandshakeError<S>>
-    where
-        S: Read + Write,
-    {
-        self.setup_connect(domain, stream)
-            .map_err(HandshakeError::SetupFailure)?
-            .handshake()
-    }
 }
 
 impl Deref for ConnectConfiguration {
@@ -387,19 +357,6 @@ impl SslAcceptor {
         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<S>(&self, stream: S) -> Result<SslStream<S>, HandshakeError<S>>
-    where
-        S: Read + Write,
-    {
-        self.setup_accept(stream)
-            .map_err(HandshakeError::SetupFailure)?
-            .handshake()
-    }
-
     /// Consumes the `SslAcceptor`, returning the inner raw `SslContext`.
     #[must_use]
     pub fn into_context(self) -> SslContext {

boring/src/ssl/error.rs

@@ -1,15 +1,12 @@
 use crate::ffi;
-use crate::x509::X509VerifyError;
 use libc::c_int;
 use openssl_macros::corresponds;
 use std::error;
-use std::error::Error as StdError;
 use std::ffi::CStr;
 use std::fmt;
 use std::io;
 
 use crate::error::ErrorStack;
-use crate::ssl::MidHandshakeSslStream;
 
 /// `SSL_ERROR_*` error code returned from SSL functions.
 ///
@@ -206,67 +203,3 @@ impl error::Error for Error {
         }
     }
 }
-
-/// An error or intermediate state after a TLS handshake attempt.
-// FIXME overhaul
-#[derive(Debug)]
-pub enum HandshakeError<S> {
-    /// Setup failed.
-    SetupFailure(ErrorStack),
-    /// The handshake failed.
-    Failure(MidHandshakeSslStream<S>),
-    /// The handshake encountered a `WouldBlock` error midway through.
-    ///
-    /// This error will never be returned for blocking streams.
-    WouldBlock(MidHandshakeSslStream<S>),
-}
-
-impl<S: fmt::Debug> StdError for HandshakeError<S> {
-    fn source(&self) -> Option<&(dyn StdError + 'static)> {
-        match *self {
-            HandshakeError::SetupFailure(ref e) => Some(e),
-            HandshakeError::Failure(ref s) | HandshakeError::WouldBlock(ref s) => Some(s.error()),
-        }
-    }
-}
-
-impl<S> fmt::Display for HandshakeError<S> {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            HandshakeError::SetupFailure(ref e) => {
-                write!(f, "TLS stream setup failed {e}")
-            }
-            HandshakeError::Failure(ref s) => fmt_mid_handshake_error(s, f, "TLS handshake failed"),
-            HandshakeError::WouldBlock(ref s) => {
-                fmt_mid_handshake_error(s, f, "TLS handshake interrupted")
-            }
-        }
-    }
-}
-
-fn fmt_mid_handshake_error(
-    s: &MidHandshakeSslStream<impl Sized>,
-    f: &mut fmt::Formatter,
-    prefix: &str,
-) -> fmt::Result {
-    #[cfg(feature = "rpk")]
-    if s.ssl().ssl_context().is_rpk() {
-        write!(f, "{}", prefix)?;
-        return write!(f, " {}", s.error());
-    }
-
-    match s.ssl().verify_result() {
-        // INVALID_CALL is returned if no verification took place,
-        // such as before a cert is sent.
-        Ok(()) | Err(X509VerifyError::INVALID_CALL) => write!(f, "{prefix}")?,
-        Err(verify) => write!(f, "{prefix}: cert verification failed - {verify}")?,
-    }
-
-    write!(f, " {}", s.error())
-}
-
-impl<S> From<ErrorStack> for HandshakeError<S> {
-    fn from(e: ErrorStack) -> HandshakeError<S> {
-        HandshakeError::SetupFailure(e)
-    }
-}

boring/src/ssl/mod.rs

@@ -15,7 +15,11 @@
 //! let connector = SslConnector::builder(SslMethod::tls()).unwrap().build();
 //!
 //! let stream = TcpStream::connect("google.com:443").unwrap();
-//! let mut stream = connector.connect("google.com", stream).unwrap();
+//! let mut stream = connector
+//!     .setup_connect("google.com", stream)
+//!     .unwrap()
+//!     .handshake()
+//!     .unwrap();
 //!
 //! stream.write_all(b"GET / HTTP/1.0\r\n\r\n").unwrap();
 //! let mut res = vec![];
@@ -49,7 +53,12 @@
 //!         Ok(stream) => {
 //!             let acceptor = acceptor.clone();
 //!             thread::spawn(move || {
-//!                 let stream = acceptor.accept(stream).unwrap();
+//!                 let stream = acceptor
+//!                     .setup_accept(stream)
+//!                     .unwrap()
+//!                     .handshake()
+//!                     .unwrap();
+//!
 //!                 handle_client(stream);
 //!             });
 //!         }
@@ -107,7 +116,7 @@ pub use self::connector::{
     ConnectConfiguration, SslAcceptor, SslAcceptorBuilder, SslConnector, SslConnectorBuilder,
 };
 pub use self::ech::{SslEchKeys, SslEchKeysRef};
-pub use self::error::{Error, ErrorCode, HandshakeError};
+pub use self::error::{Error, ErrorCode};
 
 mod async_callbacks;
 mod bio;
@@ -2742,22 +2751,6 @@ impl Ssl {
         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.
-    pub fn connect<S>(self, stream: S) -> Result<SslStream<S>, HandshakeError<S>>
-    where
-        S: Read + Write,
-    {
-        self.setup_connect(stream).handshake()
-    }
-
     /// Initiates a server-side TLS handshake.
     ///
     /// This method is guaranteed to return without calling any callback defined
@@ -2790,24 +2783,6 @@ impl Ssl {
 
         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<S>(self, stream: S) -> Result<SslStream<S>, HandshakeError<S>>
-    where
-        S: Read + Write,
-    {
-        self.setup_accept(stream).handshake()
-    }
 }
 
 impl fmt::Debug for SslRef {
@@ -3903,17 +3878,14 @@ impl<S> MidHandshakeSslStream<S> {
 
     /// Restarts the handshake process.
     #[corresponds(SSL_do_handshake)]
-    pub fn handshake(mut self) -> Result<SslStream<S>, HandshakeError<S>> {
+    pub fn handshake(mut self) -> Result<SslStream<S>, Self> {
         let ret = unsafe { ffi::SSL_do_handshake(self.stream.ssl.as_ptr()) };
         if ret > 0 {
             Ok(self.stream)
         } else {
             self.error = self.stream.make_error(ret);
-            Err(if self.error.would_block() {
-                HandshakeError::WouldBlock(self)
-            } else {
-                HandshakeError::Failure(self)
-            })
+
+            Err(self)
         }
     }
 }
@@ -4249,22 +4221,20 @@ where
 
     /// Configure as an outgoing stream from a client.
     #[corresponds(SSL_set_connect_state)]
-    pub fn set_connect_state(&mut self) {
+    fn set_connect_state(&mut self) {
         unsafe { ffi::SSL_set_connect_state(self.inner.ssl.as_ptr()) }
     }
 
     /// Configure as an incoming stream to a server.
     #[corresponds(SSL_set_accept_state)]
-    pub fn set_accept_state(&mut self) {
+    fn set_accept_state(&mut self) {
         unsafe { ffi::SSL_set_accept_state(self.inner.ssl.as_ptr()) }
     }
 
     /// 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`].
-    #[must_use]
+    /// The caller is then free to call [`MidHandshakeSslStream::handshake`] and retry
+    /// on blocking errors.
     pub fn setup_connect(mut self) -> MidHandshakeSslStream<S> {
         self.set_connect_state();
 
@@ -4280,20 +4250,10 @@ where
         }
     }
 
-    /// Attempts a client-side TLS handshake.
-    ///
-    /// This is a convenience method which combines [`Self::setup_connect`] and
-    /// [`MidHandshakeSslStream::handshake`].
-    pub fn connect(self) -> Result<SslStream<S>, HandshakeError<S>> {
-        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`].
-    #[must_use]
+    /// The caller is then free to call [`MidHandshakeSslStream::handshake`] and retry
+    /// on blocking errors.
     pub fn setup_accept(mut self) -> MidHandshakeSslStream<S> {
         self.set_accept_state();
 
@@ -4308,33 +4268,6 @@ where
             },
         }
     }
-
-    /// Attempts a server-side TLS handshake.
-    ///
-    /// This is a convenience method which combines [`Self::setup_accept`] and
-    /// [`MidHandshakeSslStream::handshake`].
-    pub fn accept(self) -> Result<SslStream<S>, HandshakeError<S>> {
-        self.setup_accept().handshake()
-    }
-
-    /// Initiates the handshake.
-    ///
-    /// This will fail if `set_accept_state` or `set_connect_state` was not called first.
-    #[corresponds(SSL_do_handshake)]
-    pub fn handshake(self) -> Result<SslStream<S>, HandshakeError<S>> {
-        let mut stream = self.inner;
-        let ret = unsafe { ffi::SSL_do_handshake(stream.ssl.as_ptr()) };
-        if ret > 0 {
-            Ok(stream)
-        } else {
-            let error = stream.make_error(ret);
-            Err(if error.would_block() {
-                HandshakeError::WouldBlock(MidHandshakeSslStream { stream, error })
-            } else {
-                HandshakeError::Failure(MidHandshakeSslStream { stream, error })
-            })
-        }
-    }
 }
 
 impl<S> SslStreamBuilder<S> {

boring/src/ssl/test/custom_verify.rs

@@ -1,5 +1,5 @@
 use super::server::Server;
-use crate::ssl::{ErrorCode, HandshakeError, SslAlert, SslVerifyMode};
+use crate::ssl::{ErrorCode, SslAlert, SslVerifyMode};
 use crate::x509::X509StoreContext;
 use crate::{hash::MessageDigest, ssl::SslVerifyError};
 use hex;
@@ -9,11 +9,7 @@ use std::sync::atomic::{AtomicBool, Ordering};
 fn untrusted_callback_override_bad() {
     let mut server = Server::builder();
 
-    server.err_cb(|err| {
-        let HandshakeError::Failure(handshake) = err else {
-            panic!("expected failure error");
-        };
-
+    server.err_cb(|handshake| {
         assert_eq!(
             handshake.error().to_string(),
             "[SSLV3_ALERT_CERTIFICATE_REVOKED]"
@@ -242,11 +238,7 @@ fn both_callback() {
 fn retry() {
     let mut server = Server::builder();
 
-    server.err_cb(|err| {
-        let HandshakeError::Failure(handshake) = err else {
-            panic!("expected failure error");
-        };
-
+    server.err_cb(|handshake| {
         assert_eq!(
             handshake.error().to_string(),
             "[SSLV3_ALERT_CERTIFICATE_REVOKED]"
@@ -267,9 +259,7 @@ fn retry() {
             Err(SslVerifyError::Invalid(SslAlert::CERTIFICATE_REVOKED))
         });
 
-    let HandshakeError::WouldBlock(handshake) = client.connect_err() else {
-        panic!("should be WouldBlock");
-    };
+    let handshake = client.connect_err();
 
     assert!(CALLED_BACK.load(Ordering::SeqCst));
     assert!(handshake.error().would_block());