Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revamp handshake entrypoints #142

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
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.
nox committed Aug 4, 2023
commit 7942e71ae8b7882d96236092b629fabc38423935
47 changes: 2 additions & 45 deletions boring/src/ssl/connector.rs
Original file line number Diff line number Diff line change
@@ -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;

@@ -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 {
@@ -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 {
@@ -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
65 changes: 0 additions & 65 deletions boring/src/ssl/error.rs
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)]
@@ -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
Original file line number Diff line number Diff line change
@@ -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);
//! });
//! }
@@ -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;
@@ -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
@@ -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 {
@@ -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)
}
}
}
@@ -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()) }
}

@@ -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();

@@ -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();

@@ -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> {
Loading