Address most PR comments

BUILD

@@ -3548,6 +3548,7 @@ grpc_cc_library(
     external_deps = [
         "absl/base:core_headers",
         "absl/status",
+        "absl/status:statusor",
         "absl/strings",
         "libcrypto",
         "libssl",

src/core/lib/security/credentials/tls/grpc_tls_crl_provider.cc

@@ -53,16 +53,20 @@ namespace grpc_core {
 namespace experimental {
 
 namespace {
-std::string IssuerFromCrl(X509_CRL* crl) {
+absl::StatusOr<std::string> IssuerFromCrl(X509_CRL* crl) {
   if (crl == nullptr) {
     return "";
   }
   X509_NAME* issuer = X509_CRL_get_issuer(crl);
-  int len;
+  if (issuer == nullptr) {
+    return absl::InvalidArgumentError("crl cannot have null issuer");
+  }
   unsigned char* buf;
   buf = nullptr;
-  len = i2d_X509_NAME(issuer, &buf);
-  if (len < 0 || buf == nullptr) return "";
+  int len = i2d_X509_NAME(issuer, &buf);
+  if (len < 0 || buf == nullptr) {
+    return absl::InvalidArgumentError("crl cannot have null issuer");
+  }
   std::string ret(reinterpret_cast<char const*>(buf), len);
   OPENSSL_free(buf);
   return ret;
@@ -105,11 +109,11 @@ absl::StatusOr<std::unique_ptr<Crl>> Crl::Parse(absl::string_view crl_string) {
 }
 
 absl::StatusOr<std::unique_ptr<CrlImpl>> CrlImpl::Create(X509_CRL* crl) {
-  std::string issuer = IssuerFromCrl(crl);
-  if (issuer.empty()) {
-    return absl::InvalidArgumentError("Issuer of crl cannot be empty");
+  absl::StatusOr<std::string> issuer = IssuerFromCrl(crl);
+  if (!issuer.ok()) {
+    return issuer.status();
   }
-  return std::make_unique<CrlImpl>(crl, issuer);
+  return std::make_unique<CrlImpl>(crl, *issuer);
 }
 
 CrlImpl::~CrlImpl() { X509_CRL_free(crl_); }

src/core/tsi/ssl_transport_security.cc

@@ -1004,12 +1004,12 @@ static int GetCrlFromProvider(X509_STORE_CTX* ctx, X509_CRL** crl_out,
   auto* provider = static_cast<grpc_core::experimental::CrlProvider*>(
       SSL_CTX_get_ex_data(ssl_ctx, g_ssl_ctx_ex_crl_provider_index));
 
-  std::string issuer_name = grpc_core::IssuerFromCert(cert);
-  if (issuer_name == "") {
-    gpr_log(GPR_ERROR, "Certificate has empty issuer, cannot do CRL lookup");
+  absl::StatusOr<std::string> issuer_name = grpc_core::IssuerFromCert(cert);
+  if (!issuer_name.ok()) {
+    gpr_log(GPR_ERROR, "Could not get certificate issuer name");
     return 0;
   }
-  grpc_core::experimental::CertificateInfoImpl cert_impl(issuer_name);
+  grpc_core::experimental::CertificateInfoImpl cert_impl(*issuer_name);
   std::shared_ptr<grpc_core::experimental::Crl> internal_crl =
       provider->GetCrl(cert_impl);
   // There wasn't a CRL found in the provider. Returning 0 will end up causing

src/core/tsi/ssl_transport_security_utils.cc

@@ -25,6 +25,9 @@
 #include <openssl/ssl.h>
 #include <openssl/x509v3.h>
 
+#include "absl/status/status.h"
+#include "absl/status/statusor.h"
+
 #include "src/core/tsi/transport_security_interface.h"
 
 namespace grpc_core {
@@ -248,7 +251,10 @@ tsi_result SslProtectorUnprotect(const unsigned char* protected_frames_bytes,
   return result;
 }
 
-int verify_crl_signature(X509_CRL* crl, X509* issuer) {
+int VerifyCrlSignature(X509_CRL* crl, X509* issuer) {
+  if (issuer == nullptr || crl == nullptr) {
+    return 1;
+  }
   EVP_PKEY* ikey = X509_get_pubkey(issuer);
   if (ikey == nullptr) {
     // Can't verify signature because we couldn't get the pubkey, fail the
@@ -261,23 +267,36 @@ int verify_crl_signature(X509_CRL* crl, X509* issuer) {
   return ret;
 }
 
-int verify_crl_cert_issuer_names_match(X509_CRL* crl, X509* issuer) {
-  return X509_NAME_cmp(X509_get_issuer_name(issuer), X509_CRL_get_issuer(crl));
+int VerifyCrlCertIssuerNamesMatch(X509_CRL* crl, X509* issuer) {
+  if (issuer == nullptr || crl == nullptr) {
+    return 1;
+  }
+  X509_NAME* issuer_name = X509_get_issuer_name(issuer);
+  if (issuer == nullptr) {
+    return 1;
+  }
+  X509_NAME* crl_issuer_name = X509_CRL_get_issuer(crl);
+  if (crl_issuer_name == nullptr) {
+    return 1;
+  }
+  return X509_NAME_cmp(issuer_name, crl_issuer_name);
 }
 
-bool verify_crl_sign_bit(X509* issuer) {
+bool VerifyCrlSignBit(X509* issuer) {
+  if (issuer == nullptr) {
+    return false;
+  }
   return (X509_get_key_usage(issuer) & KU_CRL_SIGN) != 0;
 }
 
-std::string IssuerFromCert(X509* cert) {
+absl::StatusOr<std::string> IssuerFromCert(X509* cert) {
   if (cert == nullptr) {
-    return "";
+    return absl::InvalidArgumentError("cert cannot be null");
   }
   X509_NAME* issuer = X509_get_issuer_name(cert);
-  int len;
   unsigned char* buf;
   buf = nullptr;
-  len = i2d_X509_NAME(issuer, &buf);
+  int len = i2d_X509_NAME(issuer, &buf);
   if (len < 0 || buf == nullptr) return "";
   std::string ret(reinterpret_cast<char const*>(buf), len);
   OPENSSL_free(buf);

src/core/tsi/ssl_transport_security_utils.h

@@ -23,6 +23,8 @@
 
 #include <openssl/x509.h>
 
+#include "absl/status/status.h"
+#include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
 
 #include <grpc/grpc_security_constants.h>
@@ -145,19 +147,19 @@ tsi_result SslProtectorUnprotect(const unsigned char* protected_frames_bytes,
 // Verifies the CRL signature using X509_CRL_verify
 // https://www.openssl.org/docs/man3.0/man3/X509_CRL_verify.html
 // return: 1 if valid, 0 if invalid, and -1 is there was a problem
-int verify_crl_signature(X509_CRL* crl, X509* issuer);
+int VerifyCrlSignature(X509_CRL* crl, X509* issuer);
 
 // Verifies the CRL issuer and certificate issuer name match
 // return: 0 if equal, otherwise a value per X509_NAME_cmp
-int verify_crl_cert_issuer_names_match(X509_CRL* crl, X509* issuer);
+int VerifyCrlCertIssuerNamesMatch(X509_CRL* crl, X509* issuer);
 
 // Verifies the certificate in question has the cRLSign bit present
 // return: true if cRLSign bit is present, 0 otherwise
-bool verify_crl_sign_bit(X509* issuer);
+bool VerifyCrlSignBit(X509* issuer);
 
 // Gets a stable representation of the issuer name from an X509 certificate
 // return: a std::string of the DER encoding of the X509_NAME issuer name
-std::string IssuerFromCert(X509* cert);
+absl::StatusOr<std::string> IssuerFromCert(X509* cert);
 
 }  // namespace grpc_core
 

test/core/security/grpc_tls_crl_provider_test.cc

@@ -89,21 +89,23 @@ class FakeDirectoryReader : public DirectoryReader {
 class CrlProviderTest : public ::testing::Test {
  public:
   void SetUp() override {
-    std::string cert_string = GetFileContents(kRootCert.data());
-    X509* issuer = read_cert(cert_string);
-    crl_issuer = IssuerFromCert(issuer);
+    std::string pem_cert = GetFileContents(kRootCert.data());
+    X509* issuer = ReadPemCert(pem_cert);
+    auto base_crl_issuer = IssuerFromCert(issuer);
+    ASSERT_TRUE(base_crl_issuer.ok());
+    base_crl_issuer_ = *base_crl_issuer;
     std::string intermediate_string =
         GetFileContents(kCrlIntermediateIssuerPath.data());
-    X509* intermediate_issuer = read_cert(intermediate_string);
-    intermediate_crl_issuer = IssuerFromCert(intermediate_issuer);
+    X509* intermediate_issuer = ReadPemCert(intermediate_string);
+    auto intermediate_crl_issuer = IssuerFromCert(intermediate_issuer);
+    ASSERT_TRUE(intermediate_crl_issuer.ok());
+    intermediate_crl_issuer_ = *intermediate_crl_issuer;
   }
 
  protected:
-  std::string crl_issuer;
-  std::string intermediate_crl_issuer;
-  X509* read_cert(absl::string_view cert_string) {
-    BIO* cert_bio = BIO_new_mem_buf(cert_string.data(),
-                                    static_cast<int>(cert_string.size()));
+  X509* ReadPemCert(absl::string_view pem_cert) {
+    BIO* cert_bio =
+        BIO_new_mem_buf(pem_cert.data(), static_cast<int>(pem_cert.size()));
     // Errors on BIO
     if (cert_bio == nullptr) {
       return nullptr;
@@ -112,6 +114,9 @@ class CrlProviderTest : public ::testing::Test {
     BIO_free(cert_bio);
     return cert;
   }
+
+  std::string base_crl_issuer_;
+  std::string intermediate_crl_issuer_;
 };
 
 class DirectoryReloaderCrlProviderTest : public CrlProviderTest {
@@ -172,7 +177,7 @@ TEST_F(CrlProviderTest, CanParseCrl) {
   absl::StatusOr<std::shared_ptr<Crl>> crl = Crl::Parse(crl_string);
   ASSERT_TRUE(crl.ok()) << crl.status();
   ASSERT_NE(*crl, nullptr);
-  EXPECT_EQ((*crl)->Issuer(), crl_issuer);
+  EXPECT_EQ((*crl)->Issuer(), base_crl_issuer_);
 }
 
 TEST_F(CrlProviderTest, InvalidFile) {
@@ -188,10 +193,10 @@ TEST_F(CrlProviderTest, StaticCrlProviderLookup) {
   absl::StatusOr<std::shared_ptr<CrlProvider>> provider =
       experimental::CreateStaticCrlProvider(crl_strings);
   ASSERT_TRUE(provider.ok()) << provider.status();
-  CertificateInfoImpl cert(crl_issuer);
+  CertificateInfoImpl cert(base_crl_issuer_);
   auto crl = (*provider)->GetCrl(cert);
   ASSERT_NE(crl, nullptr);
-  EXPECT_EQ(crl->Issuer(), crl_issuer);
+  EXPECT_EQ(crl->Issuer(), base_crl_issuer_);
 }
 
 TEST_F(CrlProviderTest, StaticCrlProviderLookupIssuerNotFound) {
@@ -208,14 +213,14 @@ TEST_F(DirectoryReloaderCrlProviderTest, CrlLookupGood) {
   auto provider =
       CreateCrlProvider(kCrlDirectory, std::chrono::seconds(60), nullptr);
   ASSERT_TRUE(provider.ok()) << provider.status();
-  CertificateInfoImpl cert(crl_issuer);
+  CertificateInfoImpl cert(base_crl_issuer_);
   auto crl = (*provider)->GetCrl(cert);
   ASSERT_NE(crl, nullptr);
-  EXPECT_EQ(crl->Issuer(), crl_issuer);
-  CertificateInfoImpl intermediate(intermediate_crl_issuer);
+  EXPECT_EQ(crl->Issuer(), base_crl_issuer_);
+  CertificateInfoImpl intermediate(intermediate_crl_issuer_);
   auto intermediate_crl = (*provider)->GetCrl(intermediate);
   ASSERT_NE(intermediate_crl, nullptr);
-  EXPECT_EQ(intermediate_crl->Issuer(), intermediate_crl_issuer);
+  EXPECT_EQ(intermediate_crl->Issuer(), intermediate_crl_issuer_);
 }
 
 TEST_F(DirectoryReloaderCrlProviderTest, CrlLookupMissingIssuer) {
@@ -231,7 +236,7 @@ TEST_F(DirectoryReloaderCrlProviderTest, ReloadsAndDeletes) {
   const std::chrono::seconds kRefreshDuration(60);
   auto provider = CreateCrlProvider(kRefreshDuration, nullptr);
   ASSERT_TRUE(provider.ok()) << provider.status();
-  CertificateInfoImpl cert(crl_issuer);
+  CertificateInfoImpl cert(base_crl_issuer_);
   auto should_be_no_crl = (*provider)->GetCrl(cert);
   ASSERT_EQ(should_be_no_crl, nullptr);
   // Give the provider files to find in the directory
@@ -239,7 +244,7 @@ TEST_F(DirectoryReloaderCrlProviderTest, ReloadsAndDeletes) {
   event_engine_->TickForDuration(kRefreshDuration);
   auto crl = (*provider)->GetCrl(cert);
   ASSERT_NE(crl, nullptr);
-  EXPECT_EQ(crl->Issuer(), crl_issuer);
+  EXPECT_EQ(crl->Issuer(), base_crl_issuer_);
   // Now we won't see any files in our directory
   directory_reader_->SetFilesInDirectory({});
   event_engine_->TickForDuration(kRefreshDuration);
@@ -256,10 +261,10 @@ TEST_F(DirectoryReloaderCrlProviderTest, WithCorruption) {
   auto provider =
       CreateCrlProvider(kRefreshDuration, std::move(reload_error_callback));
   ASSERT_TRUE(provider.ok()) << provider.status();
-  CertificateInfoImpl cert(crl_issuer);
+  CertificateInfoImpl cert(base_crl_issuer_);
   auto crl = (*provider)->GetCrl(cert);
   ASSERT_NE(crl, nullptr);
-  EXPECT_EQ(crl->Issuer(), crl_issuer);
+  EXPECT_EQ(crl->Issuer(), base_crl_issuer_);
   EXPECT_EQ(reload_errors.size(), 0);
   // Point the provider at a non-crl file so loading fails
   // Should result in the CRL Reloader keeping the old CRL data
@@ -267,7 +272,7 @@ TEST_F(DirectoryReloaderCrlProviderTest, WithCorruption) {
   event_engine_->TickForDuration(kRefreshDuration);
   auto crl_post_update = (*provider)->GetCrl(cert);
   ASSERT_NE(crl_post_update, nullptr);
-  EXPECT_EQ(crl_post_update->Issuer(), crl_issuer);
+  EXPECT_EQ(crl_post_update->Issuer(), base_crl_issuer_);
   EXPECT_EQ(reload_errors.size(), 1);
 }
 

test/core/tsi/ssl_transport_security_utils_test.cc

@@ -471,107 +471,107 @@ X509* read_cert(absl::string_view cert_string) {
 
 TEST(CrlUtils, VerifySignatureValid) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kValidCrl, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kCrlIssuer, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_EQ(verify_crl_signature(crl, issuer), 1);
+  EXPECT_EQ(VerifyCrlSignature(crl, issuer), 1);
 }
 
 TEST(CrlUtils, VerifySignatureIntermediateValid) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kIntermediateCrl, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kIntermediateCrlIssuer, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_EQ(verify_crl_signature(crl, issuer), 1);
+  EXPECT_EQ(VerifyCrlSignature(crl, issuer), 1);
 }
 
 TEST(CrlUtils, VerifySignatureModifiedSignature) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kModifiedSignature, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kCrlIssuer, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_EQ(verify_crl_signature(crl, issuer), 0);
+  EXPECT_EQ(VerifyCrlSignature(crl, issuer), 0);
 }
 
 TEST(CrlUtils, VerifySignatureModifiedContent) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kModifiedContent, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kCrlIssuer, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   EXPECT_EQ(crl, nullptr);
 }
 
 TEST(CrlUtils, VerifySignatureWrongIssuer) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kValidCrl, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kIntermediateCrlIssuer, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_EQ(verify_crl_signature(crl, issuer), 0);
+  EXPECT_EQ(VerifyCrlSignature(crl, issuer), 0);
 }
 
 TEST(CrlUtils, VerifySignatureWrongIssuer2) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kIntermediateCrl, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kCrlIssuer, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_EQ(verify_crl_signature(crl, issuer), 0);
+  EXPECT_EQ(VerifyCrlSignature(crl, issuer), 0);
 }
 
 TEST(CrlUtils, VerifyIssuerNamesMatch) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kValidCrl, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kCrlIssuer, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_EQ(verify_crl_cert_issuer_names_match(crl, issuer), 0);
+  EXPECT_EQ(VerifyCrlCertIssuerNamesMatch(crl, issuer), 0);
 }
 
 TEST(CrlUtils, VerifyIssuerNamesDontMatch) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kValidCrl, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kLeafCert, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_NE(verify_crl_cert_issuer_names_match(crl, issuer), 0);
+  EXPECT_NE(VerifyCrlCertIssuerNamesMatch(crl, issuer), 0);
 }
 
 TEST(CrlUtils, DuplicatedIssuerNamePassesButSignatureCheckFails) {
   absl::StatusOr<Slice> crl_slice = LoadFile(kValidCrl, false);
-  ASSERT_TRUE(crl_slice.ok()) << crl_slice.status();
+  ASSERT_EQ(crl_slice.status(), absl::OkStatus()) << crl_slice.status();
   absl::StatusOr<Slice> issuer_slice = LoadFile(kEvilCa, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509_CRL* crl = read_crl(crl_slice->as_string_view());
   X509* issuer = read_cert(issuer_slice->as_string_view());
   // The issuer names will match, but it should fail a signature check
-  EXPECT_EQ(verify_crl_cert_issuer_names_match(crl, issuer), 0);
-  EXPECT_EQ(verify_crl_signature(crl, issuer), 0);
+  EXPECT_EQ(VerifyCrlCertIssuerNamesMatch(crl, issuer), 0);
+  EXPECT_EQ(VerifyCrlSignature(crl, issuer), 0);
 }
 
 TEST(CrlUtils, VerifyCrlSignBitExists) {
   absl::StatusOr<Slice> issuer_slice = LoadFile(kCrlIssuer, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_TRUE(verify_crl_sign_bit(issuer));
+  EXPECT_TRUE(VerifyCrlSignBit(issuer));
 }
 
 TEST(CrlUtils, VerifyCrlSignBitMissing) {
   absl::StatusOr<Slice> issuer_slice = LoadFile(kLeafCert, false);
-  ASSERT_TRUE(issuer_slice.ok());
+  ASSERT_EQ(issuer_slice.status(), absl::OkStatus());
   X509* issuer = read_cert(issuer_slice->as_string_view());
-  EXPECT_FALSE(verify_crl_sign_bit(issuer));
+  EXPECT_FALSE(VerifyCrlSignBit(issuer));
 }
 
 }  // namespace testing