Improve OpenSSL codec tests and error detection for X509 PEM parsing

Also support creating non-authenticated clients in case the codec will
be used for non-RPC purposes at some point.

4 files changed, 301 insertions, 79 deletions

diff --git a/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp b/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
index 4e8bf31e75e..e19d99ee305 100644
--- a/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
+++ b/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
@@ -1,8 +1,11 @@
 // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #include <vespa/vespalib/testkit/test_kit.h>
+#include <vespa/vespalib/data/smart_buffer.h>
 #include <vespa/vespalib/net/tls/tls_context.h>
 #include <vespa/vespalib/net/tls/transport_security_options.h>
 #include <vespa/vespalib/net/tls/crypto_codec.h>
+#include <vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.h>
+#include <vespa/vespalib/net/tls/impl/openssl_tls_context_impl.h>
 #include <vespa/vespalib/test/make_tls_options_for_testing.h>
 #include <iostream>
 #include <stdlib.h>
@@ -30,104 +33,283 @@ const char* hs_state_to_str(HandshakeResult::State state) noexcept {
     }
 }
 
-void log_handshake_result(const char* mode, const HandshakeResult& res) {
+void print_handshake_result(const char* mode, const HandshakeResult& res) {
     fprintf(stderr, "(handshake) %s consumed %zu peer bytes, wrote %zu peer bytes. State: %s\n",
             mode, res.bytes_consumed, res.bytes_produced,
             hs_state_to_str(res.state));
 }
 
-void log_encode_result(const char* mode, const EncodeResult& res) {
+void print_encode_result(const char* mode, const EncodeResult& res) {
     fprintf(stderr, "(encode) %s read %zu plaintext, wrote %zu cipher. State: %s\n",
             mode, res.bytes_consumed, res.bytes_produced,
             res.failed ? "Broken! D:" : "OK");
 }
 
-void log_decode_result(const char* mode, const DecodeResult& res) {
+void print_decode_result(const char* mode, const DecodeResult& res) {
     fprintf(stderr, "(decode) %s read %zu cipher, wrote %zu plaintext. State: %s\n",
             mode, res.bytes_consumed, res.bytes_produced,
             decode_state_to_str(res.state));
 }
 
-bool complete_handshake(CryptoCodec& client, CryptoCodec& server) {
-    // Not using vespalib::string here since it doesn't have erase(iter, length) implemented.
-    std::string client_to_server_buf;
-    std::string server_to_client_buf;
-
-    HandshakeResult cli_res;
-    HandshakeResult serv_res;
-    while (!(cli_res.done() && serv_res.done())) {
-        client_to_server_buf.resize(client.min_encode_buffer_size());
-        server_to_client_buf.resize(server.min_encode_buffer_size());
-
-        cli_res = client.handshake(server_to_client_buf.data(), serv_res.bytes_produced,
-                                   client_to_server_buf.data(), client_to_server_buf.size());
-        log_handshake_result("client", cli_res);
-        server_to_client_buf.erase(server_to_client_buf.begin(), server_to_client_buf.begin() + cli_res.bytes_consumed);
-
-        serv_res = server.handshake(client_to_server_buf.data(), cli_res.bytes_produced,
-                                    server_to_client_buf.data(), server_to_client_buf.size());
-        log_handshake_result("server", serv_res);
-        client_to_server_buf.erase(client_to_server_buf.begin(), client_to_server_buf.begin() + serv_res.bytes_consumed);
-
-        if (cli_res.failed() || serv_res.failed()) {
-            return false;
+struct Fixture {
+    TransportSecurityOptions tls_opts;
+    std::unique_ptr<TlsContext> tls_ctx;
+    std::unique_ptr<CryptoCodec> client;
+    std::unique_ptr<CryptoCodec> server;
+    SmartBuffer client_to_server;
+    SmartBuffer server_to_client;
+
+    Fixture()
+        : tls_opts(vespalib::test::make_tls_options_for_testing()),
+          tls_ctx(TlsContext::create_default_context(tls_opts)),
+          client(create_openssl_codec(*tls_ctx, CryptoCodec::Mode::Client)),
+          server(create_openssl_codec(*tls_ctx, CryptoCodec::Mode::Server)),
+          client_to_server(64 * 1024),
+          server_to_client(64 * 1024)
+    {}
+
+    static TransportSecurityOptions create_options_without_own_peer_cert() {
+        auto source_opts = vespalib::test::make_tls_options_for_testing();
+        return TransportSecurityOptions(source_opts.ca_certs_pem(), "", "");
+    }
+
+    static std::unique_ptr<CryptoCodec> create_openssl_codec(
+            const TransportSecurityOptions& opts, CryptoCodec::Mode mode) {
+        auto ctx = TlsContext::create_default_context(opts);
+        return create_openssl_codec(*ctx, mode);
+    }
+
+    static std::unique_ptr<CryptoCodec> create_openssl_codec(
+            const TlsContext& ctx, CryptoCodec::Mode mode) {
+        return std::make_unique<impl::OpenSslCryptoCodecImpl>(
+                *dynamic_cast<const impl::OpenSslTlsContextImpl&>(ctx).native_context(), mode);
+    }
+
+    EncodeResult do_encode(CryptoCodec& codec, Output& buffer, vespalib::stringref plaintext) {
+        auto out = buffer.reserve(codec.min_encode_buffer_size());
+        auto enc_res = codec.encode(plaintext.data(), plaintext.size(), out.data, out.size);
+        buffer.commit(enc_res.bytes_produced);
+        return enc_res;
+    }
+
+    DecodeResult do_decode(CryptoCodec& codec, Input& buffer, vespalib::string& out,
+                           size_t max_bytes_produced, size_t max_bytes_consumed) {
+        auto in = buffer.obtain();
+        out.resize(max_bytes_produced);
+        auto to_consume = std::min(in.size, max_bytes_consumed);
+        auto enc_res = codec.decode(in.data, to_consume, &out[0], out.size());
+        buffer.evict(enc_res.bytes_consumed);
+        out.resize(enc_res.bytes_produced);
+        return enc_res;
+    }
+
+    EncodeResult client_encode(vespalib::stringref plaintext) {
+        auto res = do_encode(*client, client_to_server, plaintext);
+        print_encode_result("client", res);
+        return res;
+    }
+
+    EncodeResult server_encode(vespalib::stringref plaintext) {
+        auto res = do_encode(*server, server_to_client, plaintext);
+        print_encode_result("server", res);
+        return res;
+    }
+
+    DecodeResult client_decode(vespalib::string& out, size_t max_bytes_produced,
+                               size_t max_bytes_consumed = UINT64_MAX) {
+        auto res = do_decode(*client, server_to_client, out, max_bytes_produced, max_bytes_consumed);
+        print_decode_result("client", res);
+        return res;
+    }
+
+    DecodeResult server_decode(vespalib::string& out, size_t max_bytes_produced,
+                               size_t max_bytes_consumed = UINT64_MAX) {
+        auto res = do_decode(*server, client_to_server, out, max_bytes_produced, max_bytes_consumed);
+        print_decode_result("server", res);
+        return res;
+    }
+
+    HandshakeResult do_handshake(CryptoCodec& codec, Input& input, Output& output) {
+        auto in = input.obtain();
+        auto out = output.reserve(codec.min_encode_buffer_size());
+        auto hs_result = codec.handshake(in.data, in.size, out.data, out.size);
+        input.evict(hs_result.bytes_consumed);
+        output.commit(hs_result.bytes_produced);
+        return hs_result;
+    }
+
+    bool handshake() {
+        HandshakeResult cli_res;
+        HandshakeResult serv_res;
+        while (!(cli_res.done() && serv_res.done())) {
+            cli_res  = do_handshake(*client, server_to_client, client_to_server);
+            serv_res = do_handshake(*server, client_to_server, server_to_client);
+            print_handshake_result("client", cli_res);
+            print_handshake_result("server", serv_res);
+
+            if (cli_res.failed() || serv_res.failed()) {
+                return false;
+            }
         }
+        return true;
     }
-    return true;
+};
+
+TEST_F("client and server can complete handshake", Fixture) {
+    EXPECT_TRUE(f.handshake());
+}
+
+TEST_F("clients and servers can send single data frame after handshake (not full duplex)", Fixture) {
+    ASSERT_TRUE(f.handshake());
+
+    vespalib::string client_plaintext = "Hellooo world! :D";
+    vespalib::string server_plaintext = "Goodbye moon~ :3";
+
+    ASSERT_FALSE(f.client_encode(client_plaintext).failed);
+    vespalib::string server_plaintext_out;
+    ASSERT_TRUE(f.server_decode(server_plaintext_out, 256).frame_decoded_ok());
+    EXPECT_EQUAL(client_plaintext, server_plaintext_out);
+
+    ASSERT_FALSE(f.server_encode(server_plaintext).failed);
+    vespalib::string client_plaintext_out;
+    ASSERT_TRUE(f.client_decode(client_plaintext_out, 256).frame_decoded_ok());
+    EXPECT_EQUAL(server_plaintext, client_plaintext_out);
+}
+
+TEST_F("clients and servers can send single data frame after handshake (full duplex)", Fixture) {
+    ASSERT_TRUE(f.handshake());
+
+    vespalib::string client_plaintext = "Greetings globe! :D";
+    vespalib::string server_plaintext = "Sayonara luna~ :3";
+
+    ASSERT_FALSE(f.client_encode(client_plaintext).failed);
+    ASSERT_FALSE(f.server_encode(server_plaintext).failed);
+
+    vespalib::string client_plaintext_out;
+    vespalib::string server_plaintext_out;
+    ASSERT_TRUE(f.server_decode(server_plaintext_out, 256).frame_decoded_ok());
+    EXPECT_EQUAL(client_plaintext, server_plaintext_out);
+    ASSERT_TRUE(f.client_decode(client_plaintext_out, 256).frame_decoded_ok());
+    EXPECT_EQUAL(server_plaintext, client_plaintext_out);
 }
 
-TEST("client and server can complete handshake") {
-    // TODO move to fixture
-    auto tls_opts = vespalib::test::make_tls_options_for_testing();
-    auto tls_ctx = TlsContext::create_default_context(tls_opts);
-    auto client = CryptoCodec::create_default_codec(*tls_ctx, CryptoCodec::Mode::Client);
-    auto server = CryptoCodec::create_default_codec(*tls_ctx, CryptoCodec::Mode::Server);
+TEST_F("short ciphertext read on decode() returns NeedsMorePeerData", Fixture) {
+    ASSERT_TRUE(f.handshake());
 
-    EXPECT_TRUE(complete_handshake(*client, *server));
+    vespalib::string client_plaintext = "very secret foo";
+    ASSERT_FALSE(f.client_encode(client_plaintext).failed);
+
+    vespalib::string server_plaintext_out;
+    auto dec_res = f.server_decode(server_plaintext_out, 256, 10);
+    EXPECT_FALSE(dec_res.failed()); // Short read is not a failure mode
+    EXPECT_TRUE(dec_res.state == DecodeResult::State::NeedsMorePeerData);
 }
 
-TEST("client can send single data frame to server after handshake") {
-    // TODO move to fixture
-    auto tls_opts = vespalib::test::make_tls_options_for_testing();
-    auto tls_ctx = TlsContext::create_default_context(tls_opts);
-    auto client = CryptoCodec::create_default_codec(*tls_ctx, CryptoCodec::Mode::Client);
-    auto server = CryptoCodec::create_default_codec(*tls_ctx, CryptoCodec::Mode::Server);
+TEST_F("Encodes larger than max frame size are split up", Fixture) {
+    ASSERT_TRUE(f.handshake());
+    constexpr auto frame_size = impl::OpenSslCryptoCodecImpl::MaximumFramePlaintextSize;
+    vespalib::string client_plaintext(frame_size + 50, 'X');
+
+    auto enc_res = f.client_encode(client_plaintext);
+    ASSERT_FALSE(enc_res.failed);
+    ASSERT_EQUAL(frame_size, enc_res.bytes_consumed);
+    auto remainder = client_plaintext.substr(frame_size);
+
+    enc_res = f.client_encode(remainder);
+    ASSERT_FALSE(enc_res.failed);
+    ASSERT_EQUAL(50u, enc_res.bytes_consumed);
 
-    ASSERT_TRUE(complete_handshake(*client, *server));
+    // Over on the server side, we expect to decode 2 matching frames
+    vespalib::string server_plaintext_out;
+    auto dec_res = f.server_decode(server_plaintext_out, frame_size);
+    ASSERT_TRUE(dec_res.frame_decoded_ok());
+    EXPECT_EQUAL(frame_size, dec_res.bytes_produced);
 
-    std::string client_to_server_buf;
-    client_to_server_buf.resize(client->min_encode_buffer_size());
+    vespalib::string remainder_out;
+    dec_res = f.server_decode(remainder_out, frame_size);
+    ASSERT_TRUE(dec_res.frame_decoded_ok());
+    EXPECT_EQUAL(50u, dec_res.bytes_produced);
 
-    std::string client_plaintext = "Hellooo world! :D";
-    auto cli_res = client->encode(client_plaintext.data(), client_plaintext.size(),
-                                  client_to_server_buf.data(), client_to_server_buf.size());
-    log_encode_result("client", cli_res);
+    EXPECT_EQUAL(client_plaintext, server_plaintext_out + remainder);
+}
+
+TEST_F("client without a certificate is rejected by server", Fixture) {
+    f.client = f.create_openssl_codec(f.create_options_without_own_peer_cert(), CryptoCodec::Mode::Client);
+    EXPECT_FALSE(f.handshake());
+}
 
-    std::string server_plaintext_out;
-    server_plaintext_out.resize(server->min_decode_buffer_size());
-    auto serv_res = server->decode(client_to_server_buf.data(), cli_res.bytes_produced,
-                                   server_plaintext_out.data(), server_plaintext_out.size());
-    log_decode_result("server", serv_res);
+// Certificate note: public keys must be of the same type as those
+// used by vespalib::test::make_tls_options_for_testing(). In this case,
+// it's P-256 EC keys.
+// Also note: the Subject of this CA is different from the baseline
+// test CA to avoid validation errors. This also means the Issuer is
+// different for the below host certificate.
+constexpr const char* unknown_ca_pem = R"(-----BEGIN CERTIFICATE-----
+MIIBvzCCAWYCCQDEtg8a8Y5bBzAKBggqhkjOPQQDAjBoMQswCQYDVQQGEwJVUzEU
+MBIGA1UEBwwLTG9vbmV5VmlsbGUxDjAMBgNVBAoMBUFDTUUyMRcwFQYDVQQLDA5B
+Q01FIHRlc3QgQ0EgMjEaMBgGA1UEAwwRYWNtZTIuZXhhbXBsZS5jb20wHhcNMTgw
+OTI3MTM0NjA3WhcNNDYwMjEyMTM0NjA3WjBoMQswCQYDVQQGEwJVUzEUMBIGA1UE
+BwwLTG9vbmV5VmlsbGUxDjAMBgNVBAoMBUFDTUUyMRcwFQYDVQQLDA5BQ01FIHRl
+c3QgQ0EgMjEaMBgGA1UEAwwRYWNtZTIuZXhhbXBsZS5jb20wWTATBgcqhkjOPQIB
+BggqhkjOPQMBBwNCAAT88ScwGmpJ4NycxZBaqgSpw+IXfeIFR1oCGpxlLaKyrdpw
+Sl9SeuAyJfW4yNinzUeiuX+5hSrzly4yFrOIW/n6MAoGCCqGSM49BAMCA0cAMEQC
+IGNCYvQyZm/7GgTCi55y3RWK0tEE73ivEut9V0qvlqarAiBj8IDxv+Dm0ZFlB/8E
+EYn91JZORccsNSJkfIWqrGEjBA==
+-----END CERTIFICATE-----)";
 
-    ASSERT_FALSE(cli_res.failed);
-    ASSERT_FALSE(serv_res.failed());
+// Signed by unknown CA above
+constexpr const char* untrusted_host_cert_pem = R"(-----BEGIN CERTIFICATE-----
+MIIBrzCCAVYCCQDAZrFWZPw7djAKBggqhkjOPQQDAjBoMQswCQYDVQQGEwJVUzEU
+MBIGA1UEBwwLTG9vbmV5VmlsbGUxDjAMBgNVBAoMBUFDTUUyMRcwFQYDVQQLDA5B
+Q01FIHRlc3QgQ0EgMjEaMBgGA1UEAwwRYWNtZTIuZXhhbXBsZS5jb20wHhcNMTgw
+OTI3MTM0NjA3WhcNNDYwMjEyMTM0NjA3WjBYMQswCQYDVQQGEwJVUzEUMBIGA1UE
+BwwLTG9vbmV5VmlsbGUxGjAYBgNVBAoMEVJvYWQgUnVubmVyLCBJbmMuMRcwFQYD
+VQQDDA5yci5leGFtcGxlLmNvbTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABMrp
+YgaA3CbDCaHa5CC6Vr7TLHEPNMkLNGnr2692a57ExWk1FMzNlZfaS79b67o6zxAu
+/HMiEHtseecH96UaGg4wCgYIKoZIzj0EAwIDRwAwRAIgOjiCql8VIe0/Ihyymr0a
+IforjEAMmPffLdHnMJzbya8CIBKWeTzSnG7/0PE0K73vqr+OrE5V31FjvzvYpvdT
+tSe+
+-----END CERTIFICATE-----)";
+
+constexpr const char* untrusted_host_key_pem = R"(-----BEGIN EC PARAMETERS-----
+BggqhkjOPQMBBw==
+-----END EC PARAMETERS-----
+-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEIHwh0Is5sf4emYv0UBsHSCCMI0XCV2RzhafIQ3j1BTK0oAoGCCqGSM49
+AwEHoUQDQgAEyuliBoDcJsMJodrkILpWvtMscQ80yQs0aevbr3ZrnsTFaTUUzM2V
+l9pLv1vrujrPEC78cyIQe2x55wf3pRoaDg==
+-----END EC PRIVATE KEY-----)";
+
+TEST_F("client with certificate signed by untrusted CA is rejected by server", Fixture) {
+    TransportSecurityOptions client_opts(unknown_ca_pem, untrusted_host_cert_pem, untrusted_host_key_pem);
+    f.client = f.create_openssl_codec(client_opts, CryptoCodec::Mode::Client);
+    EXPECT_FALSE(f.handshake());
+}
+
+TEST_F("server with certificate signed by untrusted CA is rejected by client", Fixture) {
+    TransportSecurityOptions server_opts(unknown_ca_pem, untrusted_host_cert_pem, untrusted_host_key_pem);
+    f.server = f.create_openssl_codec(server_opts, CryptoCodec::Mode::Server);
+    EXPECT_FALSE(f.handshake());
+}
 
-    ASSERT_TRUE(serv_res.state == DecodeResult::State::OK);
-    std::string data_received(server_plaintext_out.data(), serv_res.bytes_produced);
-    EXPECT_EQUAL(client_plaintext, data_received);
+TEST_F("Can specify multiple trusted CA certs in transport options", Fixture) {
+    auto& base_opts = f.tls_opts;
+    auto multi_ca_pem = base_opts.ca_certs_pem() + "\n" + unknown_ca_pem;
+    TransportSecurityOptions multi_ca_using_ca_1(multi_ca_pem, untrusted_host_cert_pem, untrusted_host_key_pem);
+    TransportSecurityOptions multi_ca_using_ca_2(multi_ca_pem, base_opts.cert_chain_pem(), base_opts.private_key_pem());
+    // Let client be signed by CA 1, server by CA 2. Both have the two CAs in their trust store
+    // so this should allow for a successful handshake.
+    f.client = f.create_openssl_codec(multi_ca_using_ca_1, CryptoCodec::Mode::Client);
+    f.server = f.create_openssl_codec(multi_ca_using_ca_2, CryptoCodec::Mode::Server);
+    EXPECT_TRUE(f.handshake());
 }
 
 /*
  * TODO tests:
- *  - full duplex read/write
- *  - read and write of > frame size data
  *  - handshakes with multi frame writes
  *  - completed handshake with pipelined data frame
- *  - short ciphertext reads on decode
  *  - short plaintext writes on decode (.. if we even want to support this..)
- *  - short ciphertext write on encode
- *  - peer certificate validation on server
- *  - peer certificate validation on client
+ *  - short ciphertext write on encode (.. if we even want to support this..)
  *  - detection of peer shutdown session
  */
 
diff --git a/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h b/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h
index 6e690c809a5..2df2c9f4741 100644
--- a/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h
+++ b/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h
@@ -43,6 +43,7 @@ struct DecodeResult {
     State state = State::Failed;
 
     bool failed() const noexcept { return (state == State::Failed); }
+    bool frame_decoded_ok() const noexcept { return (state == State::OK); }
 };
 
 class TlsContext;
diff --git a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.cpp b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.cpp
index d1cddcfaf8c..15db0128f1e 100644
--- a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.cpp
@@ -137,6 +137,12 @@ BioPtr new_tls_frame_const_memory_bio() {
     return bio;
 }
 
+vespalib::string ssl_error_from_stack() {
+    char buf[256];
+    ERR_error_string_n(ERR_get_error(), buf, sizeof(buf));
+    return vespalib::string(buf);
+}
+
 } // anon ns
 
 OpenSslCryptoCodecImpl::OpenSslCryptoCodecImpl(::SSL_CTX& ctx, Mode mode)
@@ -236,10 +242,8 @@ HandshakeResult OpenSslCryptoCodecImpl::do_handshake_and_consume_peer_input_byte
         LOG(debug, "SSL_do_handshake() is complete, using protocol %s", SSL_get_version(_ssl.get()));
         return handshake_consumed_bytes_and_is_complete(static_cast<size_t>(consumed));
     } else {
-        LOG(error, "SSL_do_handshake() returned unexpected error: %s", ssl_error_to_str(ssl_result));
-        char buf[256];
-        ERR_error_string_n(ERR_get_error(), buf, sizeof(buf));
-        LOG(error, "%s", buf);
+        LOG(error, "SSL_do_handshake() returned unexpected error: %s (%s)",
+            ssl_error_to_str(ssl_result), ssl_error_from_stack().c_str());
         return handshake_failed();
     }
 }
@@ -258,13 +262,15 @@ EncodeResult OpenSslCryptoCodecImpl::encode(const char* plaintext, size_t plaint
 
     size_t bytes_consumed = 0;
     if (plaintext_size != 0) {
+        ::ERR_clear_error();
         int to_consume = static_cast<int>(std::min(plaintext_size, MaximumFramePlaintextSize));
         // SSL_write encodes plaintext to ciphertext and writes to _output_bio
         int consumed = ::SSL_write(_ssl.get(), plaintext, to_consume);
         LOG(spam, "After SSL_write() -> %d _output_bio pending=%d", consumed, BIO_pending(_output_bio));
         if (consumed < 0) {
             int ssl_error = ::SSL_get_error(_ssl.get(), consumed);
-            LOG(error, "SSL_write() failed to write frame, got error %s", ssl_error_to_str(ssl_error));
+            LOG(error, "SSL_write() failed to write frame, got error %s (%s)",
+                ssl_error_to_str(ssl_error), ssl_error_from_stack().c_str());
             // TODO explicitly detect and log TLS renegotiation error (SSL_ERROR_WANT_READ)?
             return encode_failed();
         } else if (consumed != to_consume) {
@@ -301,6 +307,7 @@ DecodeResult OpenSslCryptoCodecImpl::decode(const char* ciphertext, size_t ciphe
 
 DecodeResult OpenSslCryptoCodecImpl::drain_and_produce_plaintext_from_ssl(
         char* plaintext, size_t plaintext_size) noexcept {
+    ::ERR_clear_error();
     // SSL_read() is named a bit confusingly. We read _from_ the SSL-internal state
     // via the input BIO _into_ to the receiving plaintext buffer.
     // This may consume the entire, parts of, or none of the input BIO's data,
@@ -319,7 +326,8 @@ DecodeResult OpenSslCryptoCodecImpl::drain_and_produce_plaintext_from_ssl(
             LOG(spam, "SSL_read() returned SSL_ERROR_WANT_READ, must get more ciphertext");
             return decode_needs_more_peer_data();
         default:
-            LOG(error, "SSL_read() returned unexpected error: %s", ssl_error_to_str(ssl_error));
+            LOG(error, "SSL_read() returned unexpected error: %s (%s)",
+                ssl_error_to_str(ssl_error), ssl_error_from_stack().c_str());
             return decode_failed();
         }
     }
diff --git a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.cpp b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.cpp
index 27250dd43fc..050fd2be7e5 100644
--- a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.cpp
@@ -3,6 +3,7 @@
 #include "openssl_tls_context_impl.h"
 #include <vespa/vespalib/net/tls/crypto_exception.h>
 #include <vespa/vespalib/net/tls/transport_security_options.h>
+#include <vespa/vespalib/util/stringfmt.h>
 #include <mutex>
 #include <vector>
 #include <memory>
@@ -99,6 +100,21 @@ BioPtr bio_from_string(vespalib::stringref str) {
     return bio;
 }
 
+bool has_pem_eof_on_stack() {
+    const auto err = ERR_peek_last_error();
+    if (!err) {
+        return false;
+    }
+    return ((ERR_GET_LIB(err) == ERR_LIB_PEM)
+            && (ERR_GET_REASON(err) == PEM_R_NO_START_LINE));
+}
+
+vespalib::string ssl_error_from_stack() {
+    char buf[256];
+    ERR_error_string_n(ERR_get_error(), buf, sizeof(buf));
+    return vespalib::string(buf);
+}
+
 // Several OpenSSL functions take a magical user passphrase argument with
 // potentially horrible default behavior for password protected input.
 //
@@ -117,14 +133,28 @@ constexpr inline void *empty_passphrase() {
     return const_cast<void *>(static_cast<const void *>(""));
 }
 
+void verify_pem_ok_or_eof(::X509* x509) {
+    // It's OK if we don't have an X509 cert returned iff we failed to find
+    // something that looks like the start of a PEM entry. This is to catch
+    // cases where the PEM itself is malformed, since the X509 read routines
+    // just return either nullptr or a cert object, making it hard to debug.
+    if (!x509 && !has_pem_eof_on_stack()) {
+        throw CryptoException(make_string("Failed to add X509 certificate from PEM: %s",
+                                          ssl_error_from_stack().c_str()));
+    }
+}
+
 // Attempt to read a PEM encoded (trusted) certificate from the given BIO.
 // BIO might contain further certificates if function returns non-nullptr.
 // Returns nullptr if no certificate could be loaded. This is usually an error,
 // as this should be the first certificate in the chain.
 X509Ptr read_trusted_x509_from_bio(::BIO& bio) {
+    ::ERR_clear_error();
     // "_AUX" means the certificate is trusted. Why they couldn't name this function
     // something with "trusted" instead is left as an exercise to the reader.
-    return X509Ptr(::PEM_read_bio_X509_AUX(&bio, nullptr, nullptr, empty_passphrase()));
+    X509Ptr x509(::PEM_read_bio_X509_AUX(&bio, nullptr, nullptr, empty_passphrase()));
+    verify_pem_ok_or_eof(x509.get());
+    return x509;
 }
 
 // Attempt to read a PEM encoded certificate from the given BIO.
@@ -132,7 +162,10 @@ X509Ptr read_trusted_x509_from_bio(::BIO& bio) {
 // Returns nullptr if no certificate could be loaded. This usually implies
 // that there are no more certificates left in the chain.
 X509Ptr read_untrusted_x509_from_bio(::BIO& bio) {
-    return X509Ptr(::PEM_read_bio_X509(&bio, nullptr, nullptr, empty_passphrase()));
+    ::ERR_clear_error();
+    X509Ptr x509(::PEM_read_bio_X509(&bio, nullptr, nullptr, empty_passphrase()));
+    verify_pem_ok_or_eof(x509.get());
+    return x509;
 }
 
 SslCtxPtr new_tls_ctx_with_auto_init() {
@@ -157,9 +190,11 @@ OpenSslTlsContextImpl::OpenSslTlsContextImpl(const TransportSecurityOptions& ts_
         throw CryptoException("Failed to create new TLS context");
     }
     add_certificate_authorities(ts_opts.ca_certs_pem());
-    add_certificate_chain(ts_opts.cert_chain_pem());
-    use_private_key(ts_opts.private_key_pem());
-    verify_private_key();
+    if (!ts_opts.cert_chain_pem().empty() && !ts_opts.private_key_pem().empty()) {
+        add_certificate_chain(ts_opts.cert_chain_pem());
+        use_private_key(ts_opts.private_key_pem());
+        verify_private_key();
+    }
     enable_ephemeral_key_exchange();
     disable_compression();
     enforce_peer_certificate_verification();
@@ -170,7 +205,6 @@ OpenSslTlsContextImpl::OpenSslTlsContextImpl(const TransportSecurityOptions& ts_
 OpenSslTlsContextImpl::~OpenSslTlsContextImpl() = default;
 
 void OpenSslTlsContextImpl::add_certificate_authorities(vespalib::stringref ca_pem) {
-    // TODO support empty CA set...? Ever useful?
     auto bio = bio_from_string(ca_pem);
     ::X509_STORE* cert_store = ::SSL_CTX_get_cert_store(_ctx.get()); // Internal pointer, not owned by us.
     while (true) {
@@ -185,7 +219,6 @@ void OpenSslTlsContextImpl::add_certificate_authorities(vespalib::stringref ca_p
 }
 
 void OpenSslTlsContextImpl::add_certificate_chain(vespalib::stringref chain_pem) {
-    ::ERR_clear_error();
     auto bio = bio_from_string(chain_pem);
     // First certificate in the chain is the node's own (trusted) certificate.
     auto own_cert = read_trusted_x509_from_bio(*bio);
@@ -201,9 +234,7 @@ void OpenSslTlsContextImpl::add_certificate_chain(vespalib::stringref chain_pem)
     while (true) {
         auto ca_cert = read_untrusted_x509_from_bio(*bio);
         if (!ca_cert) {
-            // No more certificates in chain, hooray!
-            ::ERR_clear_error();
-            break;
+            break; // No more certificates in chain, hooray!
         }
         // Ownership of certificate _is_ transferred here!
         if (!::SSL_CTX_add_extra_chain_cert(_ctx.get(), ca_cert.release())) {