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

Support OpenSSL 3.0 #1596

Merged
merged 5 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 17 additions & 2 deletions .github/workflows/quick-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,27 @@ jobs:
run: apk add autoconf automake bash build-base cmake libtool libucontext-dev linux-headers openssl-dev
- name: super-test
run: ./super-test.sh quick
Linux:
Linux-old:
runs-on: ubuntu-20.04
strategy:
fail-fast: false
matrix:
compiler: [g++-7, g++-10, clang-6.0, clang-10]
compiler: [g++-7, clang-6.0]
steps:
- uses: actions/checkout@v2
- name: install dependencies
run: |
export DEBIAN_FRONTEND=noninteractive
sudo apt-get install -y build-essential git zlib1g-dev cmake libssl-dev ${{ matrix.compiler }}
- name: super-test
run: |
./super-test.sh quick ${{ matrix.compiler }}
Linux:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
compiler: [g++-12, clang-14]
steps:
- uses: actions/checkout@v2
- name: install dependencies
Expand Down
3 changes: 3 additions & 0 deletions c++/src/kj/compat/readiness-io.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ class ReadyInputStreamWrapper {
kj::Promise<void> whenReady();
// Returns a promise that resolves when read() will return non-null.

bool isAtEnd() { return eof; }
// Returns true if read() would return zero.

private:
AsyncInputStream& input;
kj::ForkedPromise<void> pumpTask = nullptr;
Expand Down
50 changes: 38 additions & 12 deletions c++/src/kj/compat/tls-test.c++
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,24 @@ KJ_TEST("TLS basics") {
auto server = serverPromise.wait(test.io.waitScope);

test.testConnection(*client, *server);

// Test clean shutdown.
{
auto eofPromise = server->readAllText();
KJ_EXPECT(!eofPromise.poll(test.io.waitScope));
client->shutdownWrite();
KJ_ASSERT(eofPromise.poll(test.io.waitScope));
KJ_EXPECT(eofPromise.wait(test.io.waitScope) == ""_kj);
}

// Test UNCLEAN shutdown in other direction.
{
auto eofPromise = client->readAllText();
KJ_EXPECT(!eofPromise.poll(test.io.waitScope));
{ auto drop = kj::mv(server); }
KJ_EXPECT(eofPromise.poll(test.io.waitScope));
KJ_EXPECT_THROW(DISCONNECTED, eofPromise.wait(test.io.waitScope));
}
}

KJ_TEST("TLS peer identity") {
Expand Down Expand Up @@ -682,22 +700,29 @@ void expectInvalidCert(kj::StringPtr hostname, TlsCertificate cert, kj::StringPt
KJ_EXPECT_THROW_MESSAGE(message, clientPromise.wait(test.io.waitScope));
}

// OpenSSL 3.0 changed error messages
#if OPENSSL_VERSION_NUMBER >= 0x30000000L && !defined(OPENSSL_IS_BORINGSSL)
#define SSL_MESSAGE_DIFFERENT_IN_V3(v11, v30) v30
#else
#define SSL_MESSAGE_DIFFERENT_IN_V3(v11, v30) v11
#endif

KJ_TEST("TLS certificate validation") {
expectInvalidCert("wrong.com", TlsCertificate(kj::str(VALID_CERT, INTERMEDIATE_CERT)),
"Hostname mismatch");
SSL_MESSAGE_DIFFERENT_IN_V3("Hostname mismatch", "hostname mismatch"));
expectInvalidCert("example.com", TlsCertificate(VALID_CERT),
"unable to get local issuer certificate");
expectInvalidCert("example.com", TlsCertificate(kj::str(EXPIRED_CERT, INTERMEDIATE_CERT)),
"certificate has expired");
expectInvalidCert("example.com", TlsCertificate(SELF_SIGNED_CERT),
"self signed certificate");
SSL_MESSAGE_DIFFERENT_IN_V3("self signed certificate", "self-signed certificate"));
}

// BoringSSL seems to print error messages differently.
#ifdef OPENSSL_IS_BORINGSSL
#define SSL_MESSAGE(interesting, boring) boring
#define SSL_MESSAGE_DIFFERENT_IN_BORINGSSL(interesting, boring) boring
#else
#define SSL_MESSAGE(interesting, boring) interesting
#define SSL_MESSAGE_DIFFERENT_IN_BORINGSSL(interesting, boring) interesting
#endif

KJ_TEST("TLS client certificate verification") {
Expand Down Expand Up @@ -740,14 +765,15 @@ KJ_TEST("TLS client certificate verification") {
auto serverPromise = test.tlsServer.wrapServer(kj::mv(pipe.ends[1]));

KJ_EXPECT_THROW_MESSAGE(
SSL_MESSAGE("peer did not return a certificate",
"PEER_DID_NOT_RETURN_A_CERTIFICATE"),
SSL_MESSAGE_DIFFERENT_IN_BORINGSSL("peer did not return a certificate",
"PEER_DID_NOT_RETURN_A_CERTIFICATE"),
serverPromise.wait(test.io.waitScope));
#if !KJ_NO_EXCEPTIONS // if exceptions are disabled, we're now in a bad state because
// KJ_EXPECT_THROW_MESSAGE() runs in a forked child process.
KJ_EXPECT_THROW_MESSAGE(
SSL_MESSAGE("alert", // "alert handshake failure" or "alert certificate required"
"ALERT"), // "ALERT_HANDSHAKE_FAILURE" or "ALERT_CERTIFICATE_REQUIRED"
SSL_MESSAGE_DIFFERENT_IN_BORINGSSL(
"alert", // "alert handshake failure" or "alert certificate required"
"ALERT"), // "ALERT_HANDSHAKE_FAILURE" or "ALERT_CERTIFICATE_REQUIRED"
clientPromise.wait(test.io.waitScope));
#endif
}
Expand All @@ -770,14 +796,14 @@ KJ_TEST("TLS client certificate verification") {
auto serverPromise = test.tlsServer.wrapServer(kj::mv(pipe.ends[1]));

KJ_EXPECT_THROW_MESSAGE(
SSL_MESSAGE("certificate verify failed",
"CERTIFICATE_VERIFY_FAILED"),
SSL_MESSAGE_DIFFERENT_IN_BORINGSSL("certificate verify failed",
"CERTIFICATE_VERIFY_FAILED"),
serverPromise.wait(test.io.waitScope));
#if !KJ_NO_EXCEPTIONS // if exceptions are disabled, we're now in a bad state because
// KJ_EXPECT_THROW_MESSAGE() runs in a forked child process.
KJ_EXPECT_THROW_MESSAGE(
SSL_MESSAGE("alert unknown ca",
"TLSV1_ALERT_UNKNOWN_CA"),
SSL_MESSAGE_DIFFERENT_IN_BORINGSSL("alert unknown ca",
"TLSV1_ALERT_UNKNOWN_CA"),
clientPromise.wait(test.io.waitScope));
#endif
}
Expand Down
22 changes: 21 additions & 1 deletion c++/src/kj/compat/tls.c++
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ void throwOpensslError() {

kj::Vector<kj::String> lines;
while (unsigned long long error = ERR_get_error()) {
#ifdef SSL_R_UNEXPECTED_EOF_WHILE_READING
// OpenSSL 3.0+ reports unexpected disconnects this way.
if (ERR_GET_REASON(error) == SSL_R_UNEXPECTED_EOF_WHILE_READING) {
kj::throwFatalException(KJ_EXCEPTION(DISCONNECTED,
"peer disconnected without gracefully ending TLS session"));
}
#endif

char message[1024];
ERR_error_string_n(error, message, sizeof(message));
lines.add(kj::heapString(message));
Expand Down Expand Up @@ -366,8 +374,12 @@ private:
throwOpensslError();
case SSL_ERROR_SYSCALL:
if (result == 0) {
// OpenSSL pre-3.0 reports unexpected disconnects this way. Note that 3.0+ report it
// as SSL_ERROR_SSL with the reason SSL_R_UNEXPECTED_EOF_WHILE_READING, which is
// handled in throwOpensslError().
disconnected = true;
return constPromise<size_t, 0>();
return KJ_EXCEPTION(DISCONNECTED,
"peer disconnected without gracefully ending TLS session");
} else {
// According to documentation we shouldn't get here, because our BIO never returns an
// "error". But in practice we do get here sometimes when the peer disconnects
Expand Down Expand Up @@ -404,12 +416,20 @@ private:

static long bioCtrl(BIO* b, int cmd, long num, void* ptr) {
switch (cmd) {
case BIO_CTRL_EOF:
return reinterpret_cast<TlsConnection*>(BIO_get_data(b))->readBuffer.isAtEnd();
case BIO_CTRL_FLUSH:
return 1;
case BIO_CTRL_PUSH:
case BIO_CTRL_POP:
// Informational?
return 0;
#ifdef BIO_CTRL_GET_KTLS_SEND
case BIO_CTRL_GET_KTLS_SEND:
case BIO_CTRL_GET_KTLS_RECV:
// TODO(someday): Support kTLS if the underlying stream is a raw socket.
return 0;
#endif
default:
KJ_LOG(WARNING, "unimplemented bio_ctrl", cmd);
return 0;
Expand Down
28 changes: 21 additions & 7 deletions super-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -403,13 +403,25 @@ if [ $IS_CLANG = yes ]; then
# Don't fail out on this ridiculous "argument unused during compilation" warning.
export CXXFLAGS="$CXXFLAGS -Wno-error=unused-command-line-argument"

# At the moment, only our clang-10 CI run seems to like -fcoroutines-ts. Earlier versions seem to
# have a library misconfiguration causing ./configure to result in the following error:
# conftest.cpp:12:12: fatal error: 'initializer_list' file not found
# #include <initializer_list>
# Let's use any clang version >= 10 so that if we move to a newer version, we'll get additional
# coverage by default.
if [ "${CXX#*-}" -ge 10 ] 2>/dev/null; then
# Enable coroutines if supported.
if [ "${CXX#*-}" -ge 14 ] 2>/dev/null; then
# Somewhere between version 10 and 14, Clang started supporting coroutines as a C++20 feature,
# and started issuing deprecating warnings for -fcoroutines-ts. (I'm not sure which version it
# was exactly since our CI jumped from 10 to 14, so I'm somewhat arbitrarily choosing 14 as the
# cutoff.)
export CXXFLAGS="$CXXFLAGS -std=c++20 -stdlib=libc++"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we lose any code coverage by using -std=c++20 instead of gnu++20? Maybe we don't actually use any GNU extensions that clang doesn't provide by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would expect that if we were using any "GNU extensions" that aren't enabled for -std=c++20, we'd see errors, because I'm not aware of us using #ifdefs that somehow distinguish whether GNU extensions are enabled. So since we see no errors, I suspect it doesn't really make a difference?

export LDFLAGS="-stdlib=libc++"

# TODO(someday): On Ubuntu 22.04, clang-14 with -stdlib=libc++ fails to link with libfuzzer,
# which looks like it might itself be linked against libstdc++? Need to investigate.
unset LIB_FUZZING_ENGINE
elif [ "${CXX#*-}" -ge 10 ] 2>/dev/null; then
# At the moment, only our clang-10 CI run seems to like -fcoroutines-ts. Earlier versions seem
# to have a library misconfiguration causing ./configure to result in the following error:
# conftest.cpp:12:12: fatal error: 'initializer_list' file not found
# #include <initializer_list>
# Let's use any clang version >= 10 so that if we move to a newer version, we'll get additional
# coverage by default.
export CXXFLAGS="$CXXFLAGS -std=gnu++17 -stdlib=libc++ -fcoroutines-ts"
export LDFLAGS="-fcoroutines-ts -stdlib=libc++"
fi
Expand All @@ -418,6 +430,8 @@ else
# uninitialized memory usage later on. GCC 4 also emits strange bogus warnings with
# -Wstrict-overflow, so we disable it.
CXXFLAGS="$CXXFLAGS -Wno-maybe-uninitialized -Wno-strict-overflow"

# TODO(someday): Enable coroutines in g++ if supported.
fi

cd c++
Expand Down