From 5bd68b3f80cc6b7f2deb33220c7ee72ad75481f4 Mon Sep 17 00:00:00 2001 From: Tor Brede Vekterli Date: Thu, 18 Oct 2018 14:25:18 +0000 Subject: Add support for half-close to `CryptoCodec` and OpenSSL implementation --- .../net/tls/openssl_impl/openssl_impl_test.cpp | 71 +++++++++-- vespalib/src/vespa/vespalib/net/tls/crypto_codec.h | 19 ++- .../net/tls/impl/openssl_crypto_codec_impl.cpp | 139 ++++++++++++--------- .../net/tls/impl/openssl_crypto_codec_impl.h | 3 + 4 files changed, 163 insertions(+), 69 deletions(-) (limited to 'vespalib') 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 423ea6222a2..844d9591a45 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 @@ -18,12 +18,12 @@ using namespace vespalib::net::tls::impl; const char* decode_state_to_str(DecodeResult::State state) noexcept { switch (state) { - case DecodeResult::State::Failed: return "Broken"; - case DecodeResult::State::OK: return "OK"; - case DecodeResult::State::NeedsMorePeerData: return "NeedsMorePeerData"; - default: - abort(); + case DecodeResult::State::Failed: return "Broken"; + case DecodeResult::State::OK: return "OK"; + case DecodeResult::State::NeedsMorePeerData: return "NeedsMorePeerData"; + case DecodeResult::State::Closed: return "Closed"; } + abort(); } const char* hs_state_to_str(HandshakeResult::State state) noexcept { @@ -31,9 +31,8 @@ const char* hs_state_to_str(HandshakeResult::State state) noexcept { case HandshakeResult::State::Failed: return "Broken"; case HandshakeResult::State::Done: return "Done"; case HandshakeResult::State::NeedsMorePeerData: return "NeedsMorePeerData"; - default: - abort(); } + abort(); } void print_handshake_result(const char* mode, const HandshakeResult& res) { @@ -133,6 +132,37 @@ struct Fixture { return res; } + DecodeResult client_decode_ignore_plaintext_output() { + vespalib::string dummy_decoded; + constexpr size_t dummy_max_decoded = 100; + return client_decode(dummy_decoded, dummy_max_decoded); + } + + DecodeResult server_decode_ignore_plaintext_output() { + vespalib::string dummy_decoded; + constexpr size_t dummy_max_decoded = 100; + return server_decode(dummy_decoded, dummy_max_decoded); + } + + EncodeResult do_half_close(CryptoCodec& codec, Output& buffer) { + auto out = buffer.reserve(codec.min_encode_buffer_size()); + auto enc_res = codec.half_close(out.data, out.size); + buffer.commit(enc_res.bytes_produced); + return enc_res; + } + + EncodeResult client_half_close() { + auto res = do_half_close(*client, client_to_server); + print_encode_result("client", res); + return res; + } + + EncodeResult server_half_close() { + auto res = do_half_close(*server, server_to_client); + print_encode_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()); @@ -245,6 +275,33 @@ TEST_F("client without a certificate is rejected by server", Fixture) { EXPECT_FALSE(f.handshake()); } +void check_half_close_encoded_ok(const EncodeResult& close_res) { + EXPECT_FALSE(close_res.failed); + EXPECT_GREATER(close_res.bytes_produced, 0u); + EXPECT_EQUAL(close_res.bytes_consumed, 0u); +} + +void check_decode_peer_is_reported_closed(const DecodeResult& decoded) { + EXPECT_TRUE(decoded.closed()); + EXPECT_GREATER(decoded.bytes_consumed, 0u); + EXPECT_EQUAL(decoded.bytes_produced, 0u); +} + +TEST_F("Both peers can half-close their connections", Fixture) { + ASSERT_TRUE(f.handshake()); + auto close_res = f.client_half_close(); + check_half_close_encoded_ok(close_res); + + auto decoded = f.server_decode_ignore_plaintext_output(); + check_decode_peer_is_reported_closed(decoded); + + close_res = f.server_half_close(); + check_half_close_encoded_ok(close_res); + + decoded = f.client_decode_ignore_plaintext_output(); + check_decode_peer_is_reported_closed(decoded); +} + // 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. diff --git a/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h b/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h index 3ec05e207d3..246fd5cfc66 100644 --- a/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h +++ b/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h @@ -37,11 +37,12 @@ struct DecodeResult { enum class State { Failed, OK, - NeedsMorePeerData - // TODO add Closed/Shutdown as own state? + NeedsMorePeerData, + Closed }; State state = State::Failed; + bool closed() const noexcept { return (state == State::Closed); } bool failed() const noexcept { return (state == State::Failed); } bool frame_decoded_ok() const noexcept { return (state == State::OK); } }; @@ -105,6 +106,9 @@ public: * complete frame is not present in `ciphertext`. In this case, decode() * must be called again once more data is available. * + * If result.closed() == true, the peer has half-closed their connection + * and no more data may be decoded. + * * Precondition: handshake must be completed * Precondition: plaintext_size >= min_decode_buffer_size() * Postcondition: if result.state == DecodeResult::State::OK, at least 1 @@ -113,6 +117,17 @@ public: virtual DecodeResult decode(const char* ciphertext, size_t ciphertext_size, char* plaintext, size_t plaintext_size) noexcept = 0; + /** + * Encodes a frame into `ciphertext` which signals to the peer that all writes + * are complete. The peer may still send data to be decoded. + * + * After calling this method, encode() must not be called on the same codec instance. + * + * Precondition: ciphertext_size >= min_encode_buffer_size(), i.e. it must be + * possible to encode at least 1 frame. + */ + virtual EncodeResult half_close(char* ciphertext, size_t ciphertext_size) noexcept = 0; + /* * Creates an implementation defined CryptoCodec that provides at least TLSv1.2 * compliant handshaking and full duplex data transfer. 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 25eb12e25bd..fee2b19f911 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 @@ -44,36 +44,36 @@ const char* ssl_error_to_str(int ssl_error) noexcept { // From https://www.openssl.org/docs/manmaster/man3/SSL_get_error.html // Our code paths shouldn't trigger most of these, but included for completeness switch (ssl_error) { - case SSL_ERROR_NONE: - return "SSL_ERROR_NONE"; - case SSL_ERROR_ZERO_RETURN: - return "SSL_ERROR_ZERO_RETURN"; - case SSL_ERROR_WANT_READ: - return "SSL_ERROR_WANT_READ"; - case SSL_ERROR_WANT_WRITE: - return "SSL_ERROR_WANT_WRITE"; - case SSL_ERROR_WANT_CONNECT: - return "SSL_ERROR_WANT_CONNECT"; - case SSL_ERROR_WANT_ACCEPT: - return "SSL_ERROR_WANT_ACCEPT"; - case SSL_ERROR_WANT_X509_LOOKUP: - return "SSL_ERROR_WANT_X509_LOOKUP"; + case SSL_ERROR_NONE: + return "SSL_ERROR_NONE"; + case SSL_ERROR_ZERO_RETURN: + return "SSL_ERROR_ZERO_RETURN"; + case SSL_ERROR_WANT_READ: + return "SSL_ERROR_WANT_READ"; + case SSL_ERROR_WANT_WRITE: + return "SSL_ERROR_WANT_WRITE"; + case SSL_ERROR_WANT_CONNECT: + return "SSL_ERROR_WANT_CONNECT"; + case SSL_ERROR_WANT_ACCEPT: + return "SSL_ERROR_WANT_ACCEPT"; + case SSL_ERROR_WANT_X509_LOOKUP: + return "SSL_ERROR_WANT_X509_LOOKUP"; #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) - case SSL_ERROR_WANT_ASYNC: - return "SSL_ERROR_WANT_ASYNC"; - case SSL_ERROR_WANT_ASYNC_JOB: - return "SSL_ERROR_WANT_ASYNC_JOB"; + case SSL_ERROR_WANT_ASYNC: + return "SSL_ERROR_WANT_ASYNC"; + case SSL_ERROR_WANT_ASYNC_JOB: + return "SSL_ERROR_WANT_ASYNC_JOB"; #endif #if (OPENSSL_VERSION_NUMBER >= 0x10101000L) - case SSL_ERROR_WANT_CLIENT_HELLO_CB: - return "SSL_ERROR_WANT_CLIENT_HELLO_CB"; + case SSL_ERROR_WANT_CLIENT_HELLO_CB: + return "SSL_ERROR_WANT_CLIENT_HELLO_CB"; #endif - case SSL_ERROR_SYSCALL: - return "SSL_ERROR_SYSCALL"; - case SSL_ERROR_SSL: - return "SSL_ERROR_SSL"; - default: - return "Unknown SSL error code"; + case SSL_ERROR_SYSCALL: + return "SSL_ERROR_SYSCALL"; + case SSL_ERROR_SSL: + return "SSL_ERROR_SSL"; + default: + return "Unknown SSL error code"; } } @@ -109,6 +109,10 @@ DecodeResult decode_failed() noexcept { return {0, 0, DecodeResult::State::Failed}; } +DecodeResult decode_peer_has_closed() noexcept { + return {0, 0, DecodeResult::State::Closed}; +} + DecodeResult decoded_frames_with_plaintext_bytes(size_t produced_bytes) noexcept { return {0, produced_bytes, DecodeResult::State::OK}; } @@ -143,6 +147,11 @@ vespalib::string ssl_error_from_stack() { return vespalib::string(buf); } +void log_ssl_error(const char* source, int ssl_error) { + LOG(error, "%s returned unexpected error: %s (%s)", + source, ssl_error_to_str(ssl_error), ssl_error_from_stack().c_str()); +} + } // anon ns OpenSslCryptoCodecImpl::OpenSslCryptoCodecImpl(std::shared_ptr ctx, Mode mode) @@ -154,7 +163,7 @@ OpenSslCryptoCodecImpl::OpenSslCryptoCodecImpl(std::shared_ptr BIO_read --> [peer] - * : SSL_do_handshake() <--(_input_bio ciphertext)-- BIO_write <-- [peer] + * handshake() : SSL_do_handshake() --(_output_bio ciphertext)--> [peer] + * : SSL_do_handshake() <--(_input_bio ciphertext)-- [peer] * * Once handshaking is complete, the input BIO is only used for decodes and the output * BIO is only used for encodes. We explicitly disallow TLS renegotiation, both for * the sake of simplicity and for added security (renegotiation is a bit of a rat's nest). * - * encode() : SSL_write(plaintext) --(_output_bio ciphertext)--> BIO_read --> [peer] - * decode() : SSL_read(plaintext) <--(_input_bio ciphertext)-- BIO_write <-- [peer] + * encode() : SSL_write(plaintext) --(_output_bio ciphertext)--> [peer] + * decode() : SSL_read(plaintext) <--(_input_bio ciphertext)-- [peer] * */ BioPtr tmp_input_bio = new_tls_frame_const_memory_bio(); @@ -221,7 +230,6 @@ HandshakeResult OpenSslCryptoCodecImpl::handshake(const char* from_peer, size_t HandshakeResult OpenSslCryptoCodecImpl::do_handshake_and_consume_peer_input_bytes() noexcept { // Assumption: SSL_do_handshake will place all required outgoing handshake // data in the output memory BIO without requiring WANT_WRITE. - // TODO test multi-frame sized handshake const long pending_read_before = BIO_pending(_input_bio); ::ERR_clear_error(); @@ -233,7 +241,6 @@ HandshakeResult OpenSslCryptoCodecImpl::do_handshake_and_consume_peer_input_byte if (ssl_result == SSL_ERROR_WANT_READ) { LOG(spam, "SSL_do_handshake() returned SSL_ERROR_WANT_READ"); - return handshake_consumed_bytes_and_needs_more_peer_data(static_cast(consumed)); } else if (ssl_result == SSL_ERROR_NONE) { // At this point SSL_do_handshake has stated it does not need any more peer data, i.e. @@ -245,8 +252,7 @@ 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(consumed)); } else { - LOG(error, "SSL_do_handshake() returned unexpected error: %s (%s)", - ssl_error_to_str(ssl_result), ssl_error_from_stack().c_str()); + log_ssl_error("SSL_do_handshake()", ssl_result); return handshake_failed(); } } @@ -266,23 +272,19 @@ 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(std::min(plaintext_size, MaximumFramePlaintextSize)); + const int to_consume = static_cast(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)); + const int consumed = ::SSL_write(_ssl.get(), plaintext, to_consume); if (consumed < 0) { - int ssl_error = ::SSL_get_error(_ssl.get(), consumed); - 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(); + log_ssl_error("SSL_write()", ::SSL_get_error(_ssl.get(), consumed)); + return encode_failed(); // TODO explicitly detect and log TLS renegotiation error (SSL_ERROR_WANT_READ)? } else if (consumed != to_consume) { LOG(error, "SSL_write() returned OK but did not consume all requested plaintext"); return encode_failed(); } bytes_consumed = static_cast(consumed); } - int produced = BIO_pending(_output_bio); + const int produced = BIO_pending(_output_bio); return encoded_bytes(bytes_consumed, static_cast(produced)); } DecodeResult OpenSslCryptoCodecImpl::decode(const char* ciphertext, size_t ciphertext_size, @@ -316,27 +318,44 @@ DecodeResult OpenSslCryptoCodecImpl::drain_and_produce_plaintext_from_ssl( // This may consume the entire, parts of, or none of the input BIO's data, // depending on how much TLS frame data is available and its size relative // to the receiving plaintext buffer. - int produced = ::SSL_read(_ssl.get(), plaintext, static_cast(plaintext_size)); + const int produced = ::SSL_read(_ssl.get(), plaintext, static_cast(plaintext_size)); if (produced > 0) { // At least 1 frame decoded successfully. return decoded_frames_with_plaintext_bytes(static_cast(produced)); } else { - int ssl_error = ::SSL_get_error(_ssl.get(), produced); - switch (ssl_error) { - case SSL_ERROR_WANT_READ: - // SSL_read() was not able to decode a full frame with the ciphertext that - // we've fed it thus far; caller must feed it some and then try again. - LOG(spam, "SSL_read() returned SSL_ERROR_WANT_READ, must get more ciphertext"); - return decode_needs_more_peer_data(); - case SSL_ERROR_ZERO_RETURN: - LOG(debug, "SSL_read() returned SSL_ERROR_ZERO_RETURN; connection has been shut down normally by the peer"); - return decode_failed(); // We'll just break the connection as per usual. - default: - LOG(error, "SSL_read() returned unexpected error: %s (%s)", - ssl_error_to_str(ssl_error), ssl_error_from_stack().c_str()); - return decode_failed(); - } + return remap_ssl_read_failure_to_decode_result(produced); + } +} + +DecodeResult OpenSslCryptoCodecImpl::remap_ssl_read_failure_to_decode_result(int read_result) noexcept { + const int ssl_error = ::SSL_get_error(_ssl.get(), read_result); + switch (ssl_error) { + case SSL_ERROR_WANT_READ: + // SSL_read() was not able to decode a full frame with the ciphertext that + // we've fed it thus far; caller must feed it some and then try again. + LOG(spam, "SSL_read() returned SSL_ERROR_WANT_READ, must get more ciphertext"); + return decode_needs_more_peer_data(); + case SSL_ERROR_ZERO_RETURN: + LOG(debug, "SSL_read() returned SSL_ERROR_ZERO_RETURN; connection has been shut down normally by the peer"); + return decode_peer_has_closed(); + default: + log_ssl_error("SSL_read()", ssl_error); + return decode_failed(); + } +} + +EncodeResult OpenSslCryptoCodecImpl::half_close(char* ciphertext, size_t ciphertext_size) noexcept { + LOG_ASSERT(verify_buf(ciphertext, ciphertext_size)); + MutableBufferViewGuard mut_view_guard(*_output_bio, ciphertext, ciphertext_size); + const int pending_before = BIO_pending(_output_bio); + int ssl_result = ::SSL_shutdown(_ssl.get()); + if (ssl_result < 0) { + log_ssl_error("SSL_shutdown()", ::SSL_get_error(_ssl.get(), ssl_result)); + return encode_failed(); } + const int pending_after = BIO_pending(_output_bio); + LOG_ASSERT(pending_after >= pending_before); + return encoded_bytes(0, static_cast(pending_after - pending_before)); } } diff --git a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.h b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.h index 572377827d6..21581256ce6 100644 --- a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.h +++ b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_crypto_codec_impl.h @@ -58,9 +58,12 @@ public: char* ciphertext, size_t ciphertext_size) noexcept override; DecodeResult decode(const char* ciphertext, size_t ciphertext_size, char* plaintext, size_t plaintext_size) noexcept override; + EncodeResult half_close(char* ciphertext, size_t ciphertext_size) noexcept override; private: HandshakeResult do_handshake_and_consume_peer_input_bytes() noexcept; DecodeResult drain_and_produce_plaintext_from_ssl(char* plaintext, size_t plaintext_size) noexcept; + // Precondition: read_result < 0 + DecodeResult remap_ssl_read_failure_to_decode_result(int read_result) noexcept; }; } -- cgit v1.2.3