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

Changes from 1 commit
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
60 changes: 15 additions & 45 deletions net/SslSocket.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ class SslStreamSocket final : public StreamSocket
// Not an error; should be handled elsewhere.
case SSL_ERROR_NONE: // 0
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
errno = last_errno; // Restore errno.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to do this with some RAII guard class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we could. What complicates it a little is that we override it in a couple of cases.

return rc;

// Peer stopped writing. We have nothing more to read, but can write.
Expand All @@ -372,20 +373,8 @@ class SslStreamSocket final : public StreamSocket
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)
Expand All @@ -394,31 +383,29 @@ class SslStreamSocket final : public StreamSocket
#else
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
#endif
_sslWantsTo = SslWantsTo::Write;
_sslWantsTo =
sslError == SSL_ERROR_WANT_READ ? SslWantsTo::Read : SslWantsTo::Write;
errno = last_errno; // Restore errno.
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
case SSL_ERROR_WANT_CLIENT_HELLO_CB: // 11
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_CLIENT_HELLO_CB: // 11
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
case SSL_ERROR_WANT_ASYNC: // 9
case SSL_ERROR_WANT_ASYNC_JOB: // 10
LOG_WRN(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);
LOG_ASSERT(!"SSL_MODE_ASYNC is unsupported");
errno = last_errno; // Restore errno.
return rc;

Expand All @@ -434,8 +421,14 @@ class SslStreamSocket final : public StreamSocket
}

[[fallthrough]];

// Non-recoverable, fatal I/O error occurred.
// SSL_shutdown() must not be called.
case SSL_ERROR_SSL: // 1
default:
{
LOG_TRC(SSL_LOG_PREFIX(context, sslError, rc, last_errno) << ": " << bioErrStr);

// Effectively an EAGAIN error at the BIO layer
if (BIO_should_retry(_bio))
{
Expand All @@ -452,29 +445,6 @@ class SslStreamSocket final : public StreamSocket
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();

Expand Down