From c2bc98c166fd3b0076eb2b41052f9eaeaa3e312d Mon Sep 17 00:00:00 2001 From: Luwei Ge Date: Thu, 2 Nov 2023 21:38:57 +0000 Subject: [PATCH] address PR comments --- include/grpc/grpc_security.h | 6 ++-- .../grpcpp/security/tls_credentials_options.h | 6 ++-- .../credentials/tls/tls_credentials.cc | 14 ++++++++-- test/cpp/client/credentials_test.cc | 14 ++++++++++ test/cpp/server/credentials_test.cc | 28 +++++++++++++++++++ 5 files changed, 58 insertions(+), 10 deletions(-) diff --git a/include/grpc/grpc_security.h b/include/grpc/grpc_security.h index 60af79ab3f6..3eae29f5825 100644 --- a/include/grpc/grpc_security.h +++ b/include/grpc/grpc_security.h @@ -819,8 +819,7 @@ GRPCAPI grpc_tls_credentials_options* grpc_tls_credentials_options_create(void); * EXPERIMENTAL API - Subject to change * * Sets the minimum TLS version that will be negotiated during the TLS - * handshake. If not set, the underlying SSL library will pick the version - * automatically. + * handshake. If not set, the underlying SSL library will set it to TLS v1.2. */ GRPCAPI void grpc_tls_credentials_options_set_min_tls_version( grpc_tls_credentials_options* options, grpc_tls_version min_tls_version); @@ -829,8 +828,7 @@ GRPCAPI void grpc_tls_credentials_options_set_min_tls_version( * EXPERIMENTAL API - Subject to change * * Sets the maximum TLS version that will be negotiated during the TLS - * handshake. If not set, the underlying SSL library will pick the version - * automatically. + * handshake. If not set, the underlying SSL library will set it to TLS v1.3. */ GRPCAPI void grpc_tls_credentials_options_set_max_tls_version( grpc_tls_credentials_options* options, grpc_tls_version max_tls_version); diff --git a/include/grpcpp/security/tls_credentials_options.h b/include/grpcpp/security/tls_credentials_options.h index 659a51a1398..2209b7c6fb2 100644 --- a/include/grpcpp/security/tls_credentials_options.h +++ b/include/grpcpp/security/tls_credentials_options.h @@ -109,13 +109,11 @@ class TlsCredentialsOptions { void set_crl_provider(std::shared_ptr crl_provider); // Sets the minimum TLS version that will be negotiated during the TLS - // handshake. If not set, the underlying SSL library will pick the version - // automatically. + // handshake. If not set, the underlying SSL library will use TLS v1.2. // @param tls_version: The minimum TLS version. void set_min_tls_version(grpc_tls_version tls_version); // Sets the maximum TLS version that will be negotiated during the TLS - // handshake. If not set, the underlying SSL library will pick the version - // automatically. + // handshake. If not set, the underlying SSL library will use TLS v1.3. // @param tls_version: The maximum TLS version. void set_max_tls_version(grpc_tls_version tls_version); diff --git a/src/core/lib/security/credentials/tls/tls_credentials.cc b/src/core/lib/security/credentials/tls/tls_credentials.cc index 36f10ca1169..eac64307f8d 100644 --- a/src/core/lib/security/credentials/tls/tls_credentials.cc +++ b/src/core/lib/security/credentials/tls/tls_credentials.cc @@ -47,9 +47,19 @@ bool CredentialOptionSanityCheck(grpc_tls_credentials_options* options, return false; } // In this case, there will be non-retriable handshake errors. - if (options->min_tls_version() == grpc_tls_version::TLS1_3 && - options->max_tls_version() == grpc_tls_version::TLS1_2) { + if (options->min_tls_version() > options->max_tls_version()) { gpr_log(GPR_ERROR, "TLS min version must not be higher than max version."); + grpc_tls_credentials_options_destroy(options); + return false; + } + if (options->max_tls_version() > grpc_tls_version::TLS1_3) { + gpr_log(GPR_ERROR, "TLS max version must not be higher than v1.3."); + grpc_tls_credentials_options_destroy(options); + return false; + } + if (options->min_tls_version() < grpc_tls_version::TLS1_2) { + gpr_log(GPR_ERROR, "TLS min version must not be lower than v1.2."); + grpc_tls_credentials_options_destroy(options); return false; } if (!options->crl_directory().empty() && options->crl_provider() != nullptr) { diff --git a/test/cpp/client/credentials_test.cc b/test/cpp/client/credentials_test.cc index edf78b44f12..639076102d8 100644 --- a/test/cpp/client/credentials_test.cc +++ b/test/cpp/client/credentials_test.cc @@ -508,6 +508,20 @@ TEST(CredentialsTest, TlsChannelCredentialsWithGoodMinAndMaxTlsVersions) { EXPECT_NE(channel_credentials, nullptr); } +TEST(CredentialsTest, TlsChannelCredentialsWithMinTlsVersions) { + grpc::experimental::TlsChannelCredentialsOptions options; + options.set_min_tls_version(static_cast(-1)); + auto channel_credentials = grpc::experimental::TlsCredentials(options); + EXPECT_EQ(channel_credentials, nullptr); +} + +TEST(CredentialsTest, TlsChannelCredentialsWithMaxTlsVersions) { + grpc::experimental::TlsChannelCredentialsOptions options; + options.set_max_tls_version(static_cast(2)); + auto channel_credentials = grpc::experimental::TlsCredentials(options); + EXPECT_EQ(channel_credentials, nullptr); +} + TEST(CredentialsTest, TlsChannelCredentialsWithBadMinAndMaxTlsVersions) { grpc::experimental::TlsChannelCredentialsOptions options; options.set_min_tls_version(grpc_tls_version::TLS1_3); diff --git a/test/cpp/server/credentials_test.cc b/test/cpp/server/credentials_test.cc index 69a9a8f46dd..d5754634aa0 100644 --- a/test/cpp/server/credentials_test.cc +++ b/test/cpp/server/credentials_test.cc @@ -294,6 +294,34 @@ TEST( EXPECT_NE(server_credentials, nullptr); } +TEST(CredentialsTest, TlsServerCredentialsWithMinTlsVersions) { + experimental::IdentityKeyCertPair key_cert_pair; + key_cert_pair.private_key = kIdentityCertPrivateKey; + key_cert_pair.certificate_chain = kIdentityCertContents; + std::vector identity_key_cert_pairs; + identity_key_cert_pairs.emplace_back(key_cert_pair); + auto certificate_provider = std::make_shared( + kRootCertContents, identity_key_cert_pairs); + grpc::experimental::TlsServerCredentialsOptions options(certificate_provider); + options.set_min_tls_version(static_cast(-1)); + auto server_credentials = grpc::experimental::TlsServerCredentials(options); + EXPECT_EQ(server_credentials, nullptr); +} + +TEST(CredentialsTest, TlsServerCredentialsWithMaxTlsVersions) { + experimental::IdentityKeyCertPair key_cert_pair; + key_cert_pair.private_key = kIdentityCertPrivateKey; + key_cert_pair.certificate_chain = kIdentityCertContents; + std::vector identity_key_cert_pairs; + identity_key_cert_pairs.emplace_back(key_cert_pair); + auto certificate_provider = std::make_shared( + kRootCertContents, identity_key_cert_pairs); + grpc::experimental::TlsServerCredentialsOptions options(certificate_provider); + options.set_max_tls_version(static_cast(2)); + auto server_credentials = grpc::experimental::TlsServerCredentials(options); + EXPECT_EQ(server_credentials, nullptr); +} + TEST( CredentialsTest, TlsChannelCredentialsWithStaticDataCertificateProviderAndBadMinMaxTlsVersions) {