Address code review comments

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; }
 };
 
 }