From dc63c453094932dadaa3c1355c72b123b69783bc Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 27 May 2021 12:28:08 -0400 Subject: [PATCH] tls: future-proof Utility::getErrorDescription (#16553) As with any other dependency, BoringSSL is not a fixed thing. https://github.com/envoyproxy/envoy/pull/14600 added an enumeration over all BoringSSL errors. This incorrectly assumes we'd never add more errors, and unnecessarily adds an dependency on errors (e.g. SSL_ERROR_WANT_CHANNEL_ID_LOOKUP) that Envoy will never encounter and may be removed in the future. Instead, the correct function is SSL_error_description. The original code enumerated errors because Envoy tries to support an old version of BoringSSL, but in that case the future-proof scheme would be to use a BORINGSSL_API_VERSION ifdef. Next, this rewrites the test. The tests assume SSL_ERROR_* constants are stable, which is invalid, and they assume that 19 will never be allocated when it has been and, in fact, we allocate them consecutively. Instead, use the constants, test a few error codes that Envoy already depends on, and use -1 as the sample unknown error. This ensures Envoy's logging reflect future values BoringSSL may add and avoids this code breaking Envoy in a future version of BoringSSL. Signed-off-by: David Benjamin --- .../transport_sockets/tls/utility.cc | 39 +++++++++++++++++-- .../transport_sockets/tls/utility.h | 23 ----------- .../transport_sockets/tls/utility_test.cc | 26 ++++--------- 3 files changed, 43 insertions(+), 45 deletions(-) diff --git a/source/extensions/transport_sockets/tls/utility.cc b/source/extensions/transport_sockets/tls/utility.cc index 10688dd6052b..1dfa5df48b69 100644 --- a/source/extensions/transport_sockets/tls/utility.cc +++ b/source/extensions/transport_sockets/tls/utility.cc @@ -14,6 +14,31 @@ namespace Extensions { namespace TransportSockets { namespace Tls { +#if BORINGSSL_API_VERSION < 10 +static constexpr absl::string_view SSL_ERROR_NONE_MESSAGE = "NONE"; +static constexpr absl::string_view SSL_ERROR_SSL_MESSAGE = "SSL"; +static constexpr absl::string_view SSL_ERROR_WANT_READ_MESSAGE = "WANT_READ"; +static constexpr absl::string_view SSL_ERROR_WANT_WRITE_MESSAGE = "WANT_WRITE"; +static constexpr absl::string_view SSL_ERROR_WANT_X509_LOOPUP_MESSAGE = "WANT_X509_LOOKUP"; +static constexpr absl::string_view SSL_ERROR_SYSCALL_MESSAGE = "SYSCALL"; +static constexpr absl::string_view SSL_ERROR_ZERO_RETURN_MESSAGE = "ZERO_RETURN"; +static constexpr absl::string_view SSL_ERROR_WANT_CONNECT_MESSAGE = "WANT_CONNECT"; +static constexpr absl::string_view SSL_ERROR_WANT_ACCEPT_MESSAGE = "WANT_ACCEPT"; +static constexpr absl::string_view SSL_ERROR_WANT_CHANNEL_ID_LOOKUP_MESSAGE = + "WANT_CHANNEL_ID_LOOKUP"; +static constexpr absl::string_view SSL_ERROR_PENDING_SESSION_MESSAGE = "PENDING_SESSION"; +static constexpr absl::string_view SSL_ERROR_PENDING_CERTIFICATE_MESSAGE = "PENDING_CERTIFICATE"; +static constexpr absl::string_view SSL_ERROR_WANT_PRIVATE_KEY_OPERATION_MESSAGE = + "WANT_PRIVATE_KEY_OPERATION"; +static constexpr absl::string_view SSL_ERROR_PENDING_TICKET_MESSAGE = "PENDING_TICKET"; +static constexpr absl::string_view SSL_ERROR_EARLY_DATA_REJECTED_MESSAGE = "EARLY_DATA_REJECTED"; +static constexpr absl::string_view SSL_ERROR_WANT_CERTIFICATE_VERIFY_MESSAGE = + "WANT_CERTIFICATE_VERIFY"; +static constexpr absl::string_view SSL_ERROR_HANDOFF_MESSAGE = "HANDOFF"; +static constexpr absl::string_view SSL_ERROR_HANDBACK_MESSAGE = "HANDBACK"; +#endif +static constexpr absl::string_view SSL_ERROR_UNKNOWN_ERROR_MESSAGE = "UNKNOWN_ERROR"; + Envoy::Ssl::CertificateDetailsPtr Utility::certificateDetails(X509* cert, const std::string& path, TimeSource& time_source) { Envoy::Ssl::CertificateDetailsPtr certificate_details = @@ -255,6 +280,9 @@ absl::optional Utility::getLastCryptoError() { } absl::string_view Utility::getErrorDescription(int err) { +#if BORINGSSL_API_VERSION < 10 + // TODO(davidben): Remove this and the corresponding SSL_ERROR_*_MESSAGE constants when the FIPS + // build is updated to a later version. switch (err) { case SSL_ERROR_NONE: return SSL_ERROR_NONE_MESSAGE; @@ -292,10 +320,15 @@ absl::string_view Utility::getErrorDescription(int err) { return SSL_ERROR_HANDOFF_MESSAGE; case SSL_ERROR_HANDBACK: return SSL_ERROR_HANDBACK_MESSAGE; - default: - ENVOY_BUG(false, "Unknown BoringSSL error had occurred"); - return SSL_ERROR_UNKNOWN_ERROR_MESSAGE; } +#else + const char* description = SSL_error_description(err); + if (description) { + return description; + } +#endif + ENVOY_BUG(false, "Unknown BoringSSL error had occurred"); + return SSL_ERROR_UNKNOWN_ERROR_MESSAGE; } } // namespace Tls diff --git a/source/extensions/transport_sockets/tls/utility.h b/source/extensions/transport_sockets/tls/utility.h index 2b4c2ab2dff2..3bb48b2aa148 100644 --- a/source/extensions/transport_sockets/tls/utility.h +++ b/source/extensions/transport_sockets/tls/utility.h @@ -17,29 +17,6 @@ namespace TransportSockets { namespace Tls { namespace Utility { -static constexpr absl::string_view SSL_ERROR_NONE_MESSAGE = "NONE"; -static constexpr absl::string_view SSL_ERROR_SSL_MESSAGE = "SSL"; -static constexpr absl::string_view SSL_ERROR_WANT_READ_MESSAGE = "WANT_READ"; -static constexpr absl::string_view SSL_ERROR_WANT_WRITE_MESSAGE = "WANT_WRITE"; -static constexpr absl::string_view SSL_ERROR_WANT_X509_LOOPUP_MESSAGE = "WANT_X509_LOOKUP"; -static constexpr absl::string_view SSL_ERROR_SYSCALL_MESSAGE = "SYSCALL"; -static constexpr absl::string_view SSL_ERROR_ZERO_RETURN_MESSAGE = "ZERO_RETURN"; -static constexpr absl::string_view SSL_ERROR_WANT_CONNECT_MESSAGE = "WANT_CONNECT"; -static constexpr absl::string_view SSL_ERROR_WANT_ACCEPT_MESSAGE = "WANT_ACCEPT"; -static constexpr absl::string_view SSL_ERROR_WANT_CHANNEL_ID_LOOKUP_MESSAGE = - "WANT_CHANNEL_ID_LOOKUP"; -static constexpr absl::string_view SSL_ERROR_PENDING_SESSION_MESSAGE = "PENDING_SESSION"; -static constexpr absl::string_view SSL_ERROR_PENDING_CERTIFICATE_MESSAGE = "PENDING_CERTIFICATE"; -static constexpr absl::string_view SSL_ERROR_WANT_PRIVATE_KEY_OPERATION_MESSAGE = - "WANT_PRIVATE_KEY_OPERATION"; -static constexpr absl::string_view SSL_ERROR_PENDING_TICKET_MESSAGE = "PENDING_TICKET"; -static constexpr absl::string_view SSL_ERROR_EARLY_DATA_REJECTED_MESSAGE = "EARLY_DATA_REJECTED"; -static constexpr absl::string_view SSL_ERROR_WANT_CERTIFICATE_VERIFY_MESSAGE = - "WANT_CERTIFICATE_VERIFY"; -static constexpr absl::string_view SSL_ERROR_HANDOFF_MESSAGE = "HANDOFF"; -static constexpr absl::string_view SSL_ERROR_HANDBACK_MESSAGE = "HANDBACK"; -static constexpr absl::string_view SSL_ERROR_UNKNOWN_ERROR_MESSAGE = "UNKNOWN_ERROR"; - Envoy::Ssl::CertificateDetailsPtr certificateDetails(X509* cert, const std::string& path, TimeSource& time_source); diff --git a/test/extensions/transport_sockets/tls/utility_test.cc b/test/extensions/transport_sockets/tls/utility_test.cc index 5b416aaa369a..32077060c04f 100644 --- a/test/extensions/transport_sockets/tls/utility_test.cc +++ b/test/extensions/transport_sockets/tls/utility_test.cc @@ -12,6 +12,7 @@ #include "absl/time/time.h" #include "gtest/gtest.h" +#include "openssl/ssl.h" #include "openssl/x509v3.h" namespace Envoy { @@ -148,31 +149,18 @@ TEST(UtilityTest, TestGetCertificationExtensionValue) { TEST(UtilityTest, SslErrorDescriptionTest) { const std::vector> test_set = { - {0, "NONE"}, - {1, "SSL"}, - {2, "WANT_READ"}, - {3, "WANT_WRITE"}, - {4, "WANT_X509_LOOKUP"}, - {5, "SYSCALL"}, - {6, "ZERO_RETURN"}, - {7, "WANT_CONNECT"}, - {8, "WANT_ACCEPT"}, - {9, "WANT_CHANNEL_ID_LOOKUP"}, - {11, "PENDING_SESSION"}, - {12, "PENDING_CERTIFICATE"}, - {13, "WANT_PRIVATE_KEY_OPERATION"}, - {14, "PENDING_TICKET"}, - {15, "EARLY_DATA_REJECTED"}, - {16, "WANT_CERTIFICATE_VERIFY"}, - {17, "HANDOFF"}, - {18, "HANDBACK"}, + {SSL_ERROR_NONE, "NONE"}, + {SSL_ERROR_SSL, "SSL"}, + {SSL_ERROR_WANT_READ, "WANT_READ"}, + {SSL_ERROR_WANT_WRITE, "WANT_WRITE"}, + {SSL_ERROR_WANT_PRIVATE_KEY_OPERATION, "WANT_PRIVATE_KEY_OPERATION"}, }; for (const auto& test_data : test_set) { EXPECT_EQ(test_data.second, Utility::getErrorDescription(test_data.first)); } - EXPECT_ENVOY_BUG(EXPECT_EQ(Utility::getErrorDescription(19), "UNKNOWN_ERROR"), + EXPECT_ENVOY_BUG(EXPECT_EQ(Utility::getErrorDescription(-1), "UNKNOWN_ERROR"), "Unknown BoringSSL error had occurred"); }