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.

Sign up for GitHub

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

SSL Improvements (2) #10866

Merged
merged 9 commits into from
Jan 7, 2025
Merged

SSL Improvements (2) #10866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
710d5f7
wsd: net: simplify ssl error handling
Ashod Jan 6, 2025
04eae54
wsd: net: unify SSL error logging
Ashod Jan 6, 2025
c08aed4
wsd: net: remove single-use macro
Ashod Jan 6, 2025
6553d7a
wsd: net: conditional use of SSL_ERROR_WANT_RETRY_VERIFY
Ashod Jan 7, 2025
222f20f
wsd: include missing atomic header
Ashod Jan 7, 2025
77bbe6a
wsd: net: assert catch-all SSL errors
Ashod Jan 7, 2025
8faa1c4
wsd: net: peek the error only once
Ashod Jan 7, 2025
f0204fa
wsd: net: assert on unexpected SSL errors
Ashod Jan 7, 2025
71f4ba3
wsd: net: hoist restoring errno
Ashod Jan 7, 2025
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
1 change: 1 addition & 0 deletions common/ConfigUtil.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include <Util.hpp>

#include <atomic>
#include <string>
#include <map>

Expand Down
147 changes: 58 additions & 89 deletions net/SslSocket.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,23 @@

#pragma once

#include <common/Log.hpp>
#include <net/Ssl.hpp>
#include <net/Socket.hpp>

#include <openssl/ssl.h>

#include <cerrno>
#include <sstream>
#include <string>

/// Create an OpenSSL version number from its components
/// that is comparable to OPENSSL_VERSION_NUMBER.
/// The layout is 0xMNN00PPS where M is major, n is minor,
/// P is patch-level, and S is 1 for pre-release.
#define MAKE_OPENSSL_VERSION_NUMBER(MAJOR, MINOR, PATCH) \
((MAJOR << 28) | (MINOR << 20) | (PATCH << 4))

/// An SSL/TSL, non-blocking, data streaming socket.
class SslStreamSocket final : public StreamSocket
{
Expand Down Expand Up @@ -288,7 +298,7 @@ class SslStreamSocket final : public StreamSocket
/// Handles the state of SSL after read or write.
int handleSslState(const int rc, const char* context)
{
const auto last_errno = errno;
int last_errno = errno; // Capture first thing.

ASSERT_CORRECT_SOCKET_THREAD(this);

Expand All @@ -301,6 +311,7 @@ class SslStreamSocket final : public StreamSocket

// Handle errors in the error-queue.
const int ret = handleSslError(rc, last_errno, context);
errno = last_errno; // Restore errno.

return ret;
}
Expand Down Expand Up @@ -334,15 +345,17 @@ class SslStreamSocket final : public StreamSocket
return "WANT_ASYNC_JOB";
case SSL_ERROR_WANT_CLIENT_HELLO_CB:
return "WANT_CLIENT_HELLO_CB";
#if OPENSSL_VERSION_NUMBER > MAKE_OPENSSL_VERSION_NUMBER(3, 0, 0)
case SSL_ERROR_WANT_RETRY_VERIFY:
return "WANT_RETRY_VERIFY";
#endif
}

return "UNKNOWN";
}

/// Handle SSL errors after read or write. Called from handleSslState().
int handleSslError(const int rc, const int last_errno, const char* context)
int handleSslError(const int rc, int& last_errno, const char* context)
{
assert(rc <= 0 && "Expected SSL failure to handle but have success");

Expand All @@ -351,75 +364,57 @@ class SslStreamSocket final : public StreamSocket
const int sslError = SSL_get_error(_ssl, rc);
LOG_ASSERT_MSG(sslError != SSL_ERROR_NONE, "Expected an SSL error to handle but have none");

#define SSL_LOG_PREFIX(CONTEXT, SSL_ERR, RC, ERRNO) \
"SSL error (" << CONTEXT << "): " << sslErrorToName(SSL_ERR) << " (" << SSL_ERR \
<< "), rc: " << RC << ", errno: " << ERRNO << " (" \
<< Util::symbolicErrno(last_errno) << ": " << std::strerror(last_errno) << ")"
// If the error is coming from BIO, get the code and text.
const unsigned long bioError = ERR_peek_error();
const std::string bioErrStr = getBioError(bioError);

LOG_TRC("SSL error (" << context << "): " << sslErrorToName(sslError) << " (" << sslError
<< "), rc: " << rc << ", errno: " << last_errno << " ("
<< Util::symbolicErrno(last_errno) << ": "
<< std::strerror(last_errno) << ")" << ": " << bioErrStr);

// Handle non-fatal cases first.
const std::string bioErrStr = getBioError();
switch (sslError)
{
// Not an error; should be handled elsewhere.
case SSL_ERROR_NONE: // 0
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
return rc;

// Peer stopped writing. We have nothing more to read, but can write.
// This doesn't necessarily signify that we are disconnected.
case SSL_ERROR_ZERO_RETURN: // 6
// Shutdown complete, we're disconnected.
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
errno = last_errno; // Restore errno.
return 0;

// Retry: Need to read data.
// Retry: Need to read/write data. Effectively, EAGAIN but for a specific operation.
case SSL_ERROR_WANT_READ: // 2
#if OPENSSL_VERSION_NUMBER > 0x10100000L
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno)
<< " has " << (SSL_has_pending(_ssl) ? "" : "no")
<< " pending data to read: " << SSL_pending(_ssl) << ". " << bioErrStr);
#else
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
#endif
_sslWantsTo = SslWantsTo::Read;
errno = last_errno; // Restore errno.
return rc;

// Retry: Need to write data.
case SSL_ERROR_WANT_WRITE: // 3
#if OPENSSL_VERSION_NUMBER > 0x10100000L
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno)
<< " has " << (SSL_has_pending(_ssl) ? "" : "no")
<< " pending data to read: " << SSL_pending(_ssl) << ". " << bioErrStr);
#else
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
static_assert(MAKE_OPENSSL_VERSION_NUMBER(1, 1, 0) == 0x10100000L);
#if OPENSSL_VERSION_NUMBER > MAKE_OPENSSL_VERSION_NUMBER(1, 1, 0)
LOG_TRC(sslErrorToName(sslError)
<< " with " << (SSL_has_pending(_ssl) ? "(" : "no(") << SSL_pending(_ssl)
<< ") pending data to read");
#endif
_sslWantsTo = SslWantsTo::Write;
errno = last_errno; // Restore errno.
_sslWantsTo =
sslError == SSL_ERROR_WANT_READ ? SslWantsTo::Read : SslWantsTo::Write;
return rc;

// Retry.
case SSL_ERROR_WANT_CONNECT: // 7
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
errno = last_errno; // Restore errno.
return rc;

// Retry.
case SSL_ERROR_WANT_ACCEPT: // 8
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
errno = last_errno; // Restore errno.
return rc;

// Unexpected: happens only with SSL_CTX_set_client_cert_cb().
case SSL_ERROR_WANT_X509_LOOKUP: // 4
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
errno = last_errno; // Restore errno.
case SSL_ERROR_WANT_CLIENT_HELLO_CB: // 11
LOG_ASSERT_MSG(!"Unhandled use of SSL_CTX_set_client_cert_cb()",
"Unhandled " << sslErrorToName(sslError)
<< " with SSL_CTX_set_client_cert_cb()");
return rc;

// Unexpected: happens only with SSL_CTX_set_client_cert_cb().
case SSL_ERROR_WANT_CLIENT_HELLO_CB: // 11
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
errno = last_errno; // Restore errno.
case SSL_ERROR_WANT_ASYNC: // 9
case SSL_ERROR_WANT_ASYNC_JOB: // 10
LOG_ASSERT_MSG(!"Unhandled use of SSL_MODE_ASYNC",
"Unhandled " << sslErrorToName(sslError) << " with SSL_MODE_ASYNC");
return rc;

// Non-recoverable, fatal I/O error occurred.
Expand All @@ -428,56 +423,35 @@ class SslStreamSocket final : public StreamSocket
if (last_errno != 0)
{
// Posix API error, let the caller handle.
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
errno = last_errno; // Restore errno.
return rc;
}

[[fallthrough]];

// Non-recoverable, fatal I/O error occurred.
// SSL_shutdown() must not be called.
case SSL_ERROR_SSL: // 1
default:
{
// We should know what errors we handle here.
LOG_ASSERT_MSG(sslError == SSL_ERROR_SYSCALL || sslError == SSL_ERROR_SSL,
"Unexpected SSL error " << sslErrorToName(sslError));

// Effectively an EAGAIN error at the BIO layer
if (BIO_should_retry(_bio))
{
#if OPENSSL_VERSION_NUMBER > 0x10100000L
LOG_TRC("BIO asks for retry - underlying EAGAIN? ("
<< context << "): " << SSL_get_error(_ssl, rc) << " has_pending "
<< SSL_has_pending(_ssl) << " bytes: " << SSL_pending(_ssl) << ". "
<< bioErrStr);
static_assert(MAKE_OPENSSL_VERSION_NUMBER(1, 1, 0) == 0x10100000L);
#if OPENSSL_VERSION_NUMBER > MAKE_OPENSSL_VERSION_NUMBER(1, 1, 0)
LOG_TRC("BIO asks for retry - underlying EAGAIN? with "
<< (SSL_has_pending(_ssl) ? "(" : "no(") << SSL_pending(_ssl)
<< ") pending data to read");
#else
LOG_TRC("BIO asks for retry - underlying EAGAIN? " << SSL_get_error(_ssl, rc)
<< ". " << bioErrStr);
LOG_TRC("BIO asks for retry - underlying EAGAIN?");
#endif
errno = last_errno ? last_errno : EAGAIN; // Restore errno.
last_errno = last_errno ? last_errno : EAGAIN; // Set errno if unset.
return -1; // poll is used to detect real errors.
}

// Non-recoverable, fatal I/O error occurred.
// SSL_shutdown() must not be called.
if (sslError == SSL_ERROR_SSL) // 1
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
else if (sslError == SSL_ERROR_SYSCALL) // 5
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
#if OPENSSL_VERSION_NUMBER > 0x10100000L
else if (sslError == SSL_ERROR_WANT_ASYNC) // 9
{
LOG_WRN(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
LOG_ASSERT_MSG(sslError != SSL_ERROR_WANT_ASYNC,
"Unexpected WANT_ASYNC; SSL_MODE_ASYNC is unsupported");
}
else if (sslError == SSL_ERROR_WANT_ASYNC_JOB) // 10
{
LOG_WRN(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
LOG_ASSERT_MSG(sslError != SSL_ERROR_WANT_ASYNC_JOB,
"Unexpected WANT_ASYNC_JOB; SSL_MODE_ASYNC is unsupported");
}
#endif
else
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);

// The error is coming from BIO. Find out what happened.
const long bioError = ERR_peek_error();

std::ostringstream oss;
oss << '#' << getFD();

Expand All @@ -488,7 +462,6 @@ class SslStreamSocket final : public StreamSocket
// Socket closed. Not an error.
oss << " (" << context << "): closed. " << bioErrStr;
LOG_INF(oss.str());
errno = last_errno; // Restore errno.
return 0;
}
else if (rc == -1)
Expand All @@ -509,7 +482,6 @@ class SslStreamSocket final : public StreamSocket
const std::string msg = oss.str();
LOG_TRC("Throwing SSL Error ("
<< context << "): " << msg); // Locate the source of the exception.
errno = last_errno; // Restore errno.

handshakeFail();

Expand All @@ -518,22 +490,19 @@ class SslStreamSocket final : public StreamSocket
if (!sslVerifyResult.empty())
LOG_ERR("SSL verification warning (" << context << "): " << sslVerifyResult);

errno = last_errno; // Restore errno before throwing.
throw std::runtime_error(msg);
}
break;
}

#undef SSL_LOG_PREFIX

errno = last_errno; // Restore errno.
return rc;
}

std::string getBioError() const
/// Get the error string for the given error code from OpenSSL.
std::string getBioError(const unsigned long bioError) const
{
// The error is coming from BIO. Find out what happened.
const long bioError = ERR_peek_error();

std::ostringstream oss;
oss << "BIO error: " << bioError;

Expand Down
Loading