Skip to content

Commit

Permalink
Remove all connect and accept functions
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@nox
nox committed Aug 4, 2023
1 parent bc4dd56 commit 756f807
Show file tree
Hide file tree
Showing 11 changed files with 137 additions and 283 deletions.
47 changes: 2 additions & 45 deletions boring/src/ssl/connector.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -111,21 +111,6 @@ impl SslConnector {
self.configure()?.setup_connect(domain, stream)
}

/// Attempts a client-side TLS session on a stream.
///
/// The domain is used for SNI and hostname verification.
///
/// 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 {
Expand Down Expand Up @@ -239,21 +224,6 @@ impl ConnectConfiguration {

Ok(self.ssl.setup_connect(stream))
}

/// Attempts a client-side TLS session on a stream.
///
/// The domain is used for SNI 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 {
Expand Down Expand Up @@ -373,19 +343,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`.
pub fn into_context(self) -> SslContext {
self.0
Expand Down
65 changes: 0 additions & 65 deletions boring/src/ssl/error.rs
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
use crate::ffi;
use libc::c_int;
use std::error;
use std::error::Error as StdError;
use std::fmt;
use std::io;

use crate::error::ErrorStack;
use crate::ssl::MidHandshakeSslStream;
use crate::x509::X509VerifyResult;

/// An error code returned from SSL functions.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -130,65 +127,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() {
X509VerifyResult::OK => write!(f, "{}", prefix)?,
verify => write!(f, "{}: cert verification failed - {}", prefix, verify)?,
}

write!(f, " {}", s.error())
}

impl<S> From<ErrorStack> for HandshakeError<S> {
fn from(e: ErrorStack) -> HandshakeError<S> {
HandshakeError::SetupFailure(e)
}
}
114 changes: 21 additions & 93 deletions boring/src/ssl/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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![];
Expand Down Expand Up @@ -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);
//! });
//! }
Expand Down Expand Up @@ -98,7 +107,7 @@ use crate::{cvt, cvt_0i, cvt_n, cvt_p, init};
pub use crate::ssl::connector::{
ConnectConfiguration, SslAcceptor, SslAcceptorBuilder, SslConnector, SslConnectorBuilder,
};
pub use crate::ssl::error::{Error, ErrorCode, HandshakeError};
pub use crate::ssl::error::{Error, ErrorCode};

mod bio;
mod callbacks;
Expand Down Expand Up @@ -2324,22 +2333,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
Expand Down Expand Up @@ -2372,24 +2365,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 {
Expand Down Expand Up @@ -3192,16 +3167,14 @@ impl<S> MidHandshakeSslStream<S> {
/// This corresponds to [`SSL_do_handshake`].
///
/// [`SSL_do_handshake`]: https://www.openssl.org/docs/manmaster/man3/SSL_do_handshake.html
pub fn handshake(mut self) -> Result<SslStream<S>, HandshakeError<S>> {
pub fn handshake(mut self) -> Result<SslStream<S>, MidHandshakeSslStream<S>> {
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);
match self.error.would_block() {
true => Err(HandshakeError::WouldBlock(self)),
false => Err(HandshakeError::Failure(self)),
}

Err(self)
}
}
}
Expand Down Expand Up @@ -3484,7 +3457,7 @@ where
/// This corresponds to [`SSL_set_connect_state`].
///
/// [`SSL_set_connect_state`]: https://www.openssl.org/docs/manmaster/man3/SSL_set_connect_state.html
pub fn set_connect_state(&mut self) {
fn set_connect_state(&mut self) {
unsafe { ffi::SSL_set_connect_state(self.inner.ssl.as_ptr()) }
}

Expand All @@ -3493,15 +3466,14 @@ where
/// This corresponds to [`SSL_set_accept_state`].
///
/// [`SSL_set_accept_state`]: https://www.openssl.org/docs/manmaster/man3/SSL_set_accept_state.html
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`].
/// 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();

Expand All @@ -3517,19 +3489,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`].
/// 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();

Expand All @@ -3544,41 +3507,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.
///
/// This corresponds to [`SSL_do_handshake`].
///
/// [`SSL_do_handshake`]: https://www.openssl.org/docs/manmaster/man3/SSL_do_handshake.html
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);
match error.would_block() {
true => Err(HandshakeError::WouldBlock(MidHandshakeSslStream {
stream,
error,
})),
false => Err(HandshakeError::Failure(MidHandshakeSslStream {
stream,
error,
})),
}
}
}
}

impl<S> SslStreamBuilder<S> {
Expand Down
Loading

0 comments on commit 756f807

Please sign in to comment.