diff options
author | Tor Brede Vekterli <vekterli@oath.com> | 2018-09-03 13:16:08 +0000 |
---|---|---|
committer | Tor Brede Vekterli <vekterli@oath.com> | 2018-09-03 13:16:08 +0000 |
commit | 179ae7f8965beb80b687c1d365b5fa54c7c27890 (patch) | |
tree | c6fe312534e7eeada827d542eb841bfb2aa70493 /vespalib | |
parent | aa114684417f9d0852e479e46d13a1c713412241 (diff) |
Address code review comments
Diffstat (limited to 'vespalib')
8 files changed, 50 insertions, 33 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 eda1b4b6879..cba88f2ba56 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 @@ -88,7 +88,7 @@ const char* decode_state_to_str(DecodeResult::State state) noexcept { const char* hs_state_to_str(HandshakeResult::State state) noexcept { switch (state) { case HandshakeResult::State::Failed: return "Broken"; - case HandshakeResult::State::Done: return "Completed"; + case HandshakeResult::State::Done: return "Done"; case HandshakeResult::State::NeedsMorePeerData: return "NeedsMorePeerData"; default: abort(); @@ -114,13 +114,15 @@ void log_decode_result(const char* mode, const DecodeResult& res) { } 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, serv_res; + 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(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()); @@ -159,10 +161,7 @@ TEST("client can send single data frame to server after handshake") { ASSERT_TRUE(complete_handshake(*client, *server)); std::string client_to_server_buf; - std::string server_to_client_buf; - client_to_server_buf.resize(client->min_encode_buffer_size()); - server_to_client_buf.resize(client->min_encode_buffer_size()); std::string client_plaintext = "Hellooo world! :D"; auto cli_res = client->encode(client_plaintext.data(), client_plaintext.size(), @@ -170,7 +169,7 @@ TEST("client can send single data frame to server after handshake") { log_encode_result("client", cli_res); std::string server_plaintext_out; - server_plaintext_out.resize(client->min_encode_buffer_size()); + 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); diff --git a/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h b/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h index 569b51aaaac..6e690c809a5 100644 --- a/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h +++ b/vespalib/src/vespa/vespalib/net/tls/crypto_codec.h @@ -14,7 +14,6 @@ struct HandshakeResult { Failed, Done, NeedsMorePeerData - // TODO add Closed/Shutdown as own state? }; State state = State::Failed; @@ -63,9 +62,19 @@ public: virtual ~CryptoCodec() = default; + /* + * Minimum buffer size required to represent one wire format frame + * of encrypted (ciphertext) data, including frame overhead. + */ virtual size_t min_encode_buffer_size() const noexcept = 0; + /* + * Minimum buffer size required to represent the decoded (plaintext) + * output of a single frame of encrypted data. + */ + virtual size_t min_decode_buffer_size() const noexcept = 0; /* + * Precondition: to_peer_buf_size >= min_encode_buffer_size() * Postcondition: if result.done(), the handshake process has completed * and data may be passed through encode()/decode(). */ @@ -78,9 +87,13 @@ public: * field will be < plaintext_size. The number of actual ciphertext bytes produced * is available in the returned result's produced_bytes field. * - * Precondition: handshake must be completed - * Precondition: ciphertext_size >= min_encode_buffer_size(), i.e. it must be - * possible to write at least 1 frame. + * Precondition: handshake must be completed + * Precondition: ciphertext_size >= min_encode_buffer_size(), i.e. it must be + * possible to encode at least 1 frame. + * Postcondition: if plaintext_size > 0 and result.failed == false, a single + * frame of ciphertext has been written into the to_peer buffer. + * Size of written frame is given by result.bytes_produced. This + * includes all protocol-specific frame overhead. */ virtual EncodeResult encode(const char* plaintext, size_t plaintext_size, char* ciphertext, size_t ciphertext_size) noexcept = 0; @@ -91,7 +104,8 @@ public: * complete frame is not present in `ciphertext`. In this case, decode() * must be called again once more data is available. * - * Precondition: handshake must be completed + * Precondition: handshake must be completed + * Precondition: plaintext_size >= min_decode_buffer_size() * Postcondition: if result.state == DecodeResult::State::OK, at least 1 * complete frame has been written to the `plaintext` buffer */ 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 db0d84cf434..ad9e8575b97 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 @@ -37,6 +37,11 @@ public: size_t min_encode_buffer_size() const noexcept override { return MaximumTlsFrameSize; } + size_t min_decode_buffer_size() const noexcept override { + // Technically this would be MaximumFramePayloadSize, but we like power + // of two numbers for buffer sizes, yes we do. + return MaximumTlsFrameSize; + } HandshakeResult handshake(const char* from_peer, size_t from_peer_buf_size, char* to_peer, size_t to_peer_buf_size) noexcept override; 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 89ac68fa059..207180f3c96 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 @@ -7,8 +7,6 @@ #include <vector> #include <memory> #include <stdexcept> -#include <string> -#include <string_view> #include <openssl/ssl.h> #include <openssl/crypto.h> #include <openssl/err.h> @@ -88,7 +86,7 @@ void ensure_openssl_initialized_once() { (void) openssl_resources; } -BioPtr bio_from_string(std::string_view str) { +BioPtr bio_from_string(stringref str) { LOG_ASSERT(str.size() <= INT_MAX); BioPtr bio(::BIO_new_mem_buf(str.data(), static_cast<int>(str.size()))); if (!bio) { @@ -161,7 +159,7 @@ OpenSslTlsContextImpl::~OpenSslTlsContextImpl() { ::SSL_CTX_free(_ctx); } -void OpenSslTlsContextImpl::add_certificate_authorities(std::string_view ca_pem) { +void OpenSslTlsContextImpl::add_certificate_authorities(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); // Internal pointer, not owned by us. @@ -176,7 +174,7 @@ void OpenSslTlsContextImpl::add_certificate_authorities(std::string_view ca_pem) } } -void OpenSslTlsContextImpl::add_certificate_chain(std::string_view chain_pem) { +void OpenSslTlsContextImpl::add_certificate_chain(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. @@ -204,7 +202,7 @@ void OpenSslTlsContextImpl::add_certificate_chain(std::string_view chain_pem) { } } -void OpenSslTlsContextImpl::use_private_key(std::string_view key_pem) { +void OpenSslTlsContextImpl::use_private_key(stringref key_pem) { auto bio = bio_from_string(key_pem); EvpPkeyPtr key(::PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, empty_passphrase())); if (!key) { diff --git a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.h b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.h index 26e5c38233e..5fa982ee7ad 100644 --- a/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.h +++ b/vespalib/src/vespa/vespalib/net/tls/impl/openssl_tls_context_impl.h @@ -3,6 +3,7 @@ #include "openssl_typedefs.h" #include <vespa/vespalib/net/tls/tls_context.h> +#include <vespa/vespalib/stllike/string.h> namespace vespalib::net::tls::impl { @@ -15,9 +16,9 @@ public: ::SSL_CTX* native_context() const noexcept { return _ctx; } private: // Note: single use per instance; does _not_ clear existing chain! - void add_certificate_authorities(std::string_view ca_pem); - void add_certificate_chain(std::string_view chain_pem); - void use_private_key(std::string_view key_pem); + void add_certificate_authorities(stringref ca_pem); + void add_certificate_chain(stringref chain_pem); + void use_private_key(stringref key_pem); void verify_private_key(); // Enable use of ephemeral key exchange (ECDHE), allowing forward secrecy. void enable_ephemeral_key_exchange(); diff --git a/vespalib/src/vespa/vespalib/net/tls/tls_context.cpp b/vespalib/src/vespa/vespalib/net/tls/tls_context.cpp index 4a5240136e4..467838975e7 100644 --- a/vespalib/src/vespa/vespalib/net/tls/tls_context.cpp +++ b/vespalib/src/vespa/vespalib/net/tls/tls_context.cpp @@ -8,4 +8,4 @@ std::unique_ptr<TlsContext> TlsContext::create_default_context(const TransportSe return std::make_unique<impl::OpenSslTlsContextImpl>(opts); } -}
\ No newline at end of file +} diff --git a/vespalib/src/vespa/vespalib/net/tls/transport_security_options.cpp b/vespalib/src/vespa/vespalib/net/tls/transport_security_options.cpp index 02d3375554f..4e39fe4d7fa 100644 --- a/vespalib/src/vespa/vespalib/net/tls/transport_security_options.cpp +++ b/vespalib/src/vespa/vespalib/net/tls/transport_security_options.cpp @@ -6,7 +6,7 @@ namespace vespalib::net::tls { TransportSecurityOptions::~TransportSecurityOptions() { - OPENSSL_cleanse(_private_key_pem.data(), _private_key_pem.size()); + OPENSSL_cleanse(&_private_key_pem[0], _private_key_pem.size()); } } diff --git a/vespalib/src/vespa/vespalib/net/tls/transport_security_options.h b/vespalib/src/vespa/vespalib/net/tls/transport_security_options.h index 62f3d053578..12fb34196b1 100644 --- a/vespalib/src/vespa/vespalib/net/tls/transport_security_options.h +++ b/vespalib/src/vespa/vespalib/net/tls/transport_security_options.h @@ -2,29 +2,29 @@ #pragma once -#include <string> +#include <vespa/vespalib/stllike/string.h> namespace vespalib::net::tls { class TransportSecurityOptions { - std::string _ca_certs_pem; - std::string _cert_chain_pem; - std::string _private_key_pem; + string _ca_certs_pem; + string _cert_chain_pem; + string _private_key_pem; public: TransportSecurityOptions() = default; - TransportSecurityOptions(std::string ca_certs_pem, - std::string cert_chain_pem, - std::string private_key_pem) + TransportSecurityOptions(string ca_certs_pem, + string cert_chain_pem, + string private_key_pem) : _ca_certs_pem(std::move(ca_certs_pem)), _cert_chain_pem(std::move(cert_chain_pem)), _private_key_pem(std::move(private_key_pem)) {} ~TransportSecurityOptions(); - const std::string& ca_certs_pem() const noexcept { return _ca_certs_pem; } - const std::string& cert_chain_pem() const noexcept { return _cert_chain_pem; } - const std::string& private_key_pem() const noexcept { return _private_key_pem; } + const string& ca_certs_pem() const noexcept { return _ca_certs_pem; } + const string& cert_chain_pem() const noexcept { return _cert_chain_pem; } + const string& private_key_pem() const noexcept { return _private_key_pem; } }; } |