Skip to content

Commit

Permalink
tls: future-proof Utility::getErrorDescription (envoyproxy#16553)
Browse files Browse the repository at this point in the history 
As with any other dependency, BoringSSL is not a fixed thing.
envoyproxy#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 <davidben@google.com>
  • Loading branch information
@davidben
davidben authored and Le Yao committed Sep 30, 2021
1 parent 08f11cd commit dc63c45
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 45 deletions.
39 changes: 36 additions & 3 deletions source/extensions/transport_sockets/tls/utility.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -255,6 +280,9 @@ absl::optional<std::string> 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;
Expand Down Expand Up @@ -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
Expand Down
23 changes: 0 additions & 23 deletions source/extensions/transport_sockets/tls/utility.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
26 changes: 7 additions & 19 deletions test/extensions/transport_sockets/tls/utility_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include "absl/time/time.h"
#include "gtest/gtest.h"
#include "openssl/ssl.h"
#include "openssl/x509v3.h"

namespace Envoy {
Expand Down Expand Up @@ -148,31 +149,18 @@ TEST(UtilityTest, TestGetCertificationExtensionValue) {

TEST(UtilityTest, SslErrorDescriptionTest) {
const std::vector<std::pair<int, std::string>> 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");
}

Expand Down

0 comments on commit dc63c45

Please sign in to comment.