diff options
author | Tor Brede Vekterli <vekterli@oath.com> | 2018-09-28 14:16:04 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-09-28 14:16:04 +0200 |
commit | c43e8989b5ba0056e0031d045d740cd6a92a7303 (patch) | |
tree | 7759a7f11fda24edba4b2ef61ff7b2b4fffd26f1 | |
parent | 8d80010a385f40d4bb852e6b11810692a67e90ed (diff) | |
parent | 601ebbccf0ab83d385b06cfc92373a2fc4da761c (diff) |
Merge pull request #7132 from vespa-engine/vekterli/more-openssl-testing-and-improved-pem-error-reporting
Improve OpenSSL codec tests and error detection for X509 PEM parsing
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())) { |