envoyproxy · zuercher · May 12, 2022 · Mar 30, 2022 · Mar 30, 2022 · Apr 7, 2022
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -66,6 +66,9 @@ removed_config_or_runtime:
 - area: tcp_proxy
   change: |
     removed ``envoy.reloadable_features.new_tcp_connection_pool`` and legacy code paths.
+- area: tls
+  change: |
+    fixed a bug when a certificate is invalid, ``days_until_expiration`` reports a big number. After this fix, when a certificate expires, it reports as ``0``.
 - area: conn pool
   change: |
     removed ``envoy.reloadable_features.conn_pool_delete_when_idle`` and legacy code paths.

diff --git a/envoy/ssl/context.h b/envoy/ssl/context.h
@@ -21,9 +21,10 @@ class Context {
   virtual ~Context() = default;
 
   /**
-   * @return the number of days in this context until the next certificate will expire
+   * @return the number of days in this context until the next certificate will expire, the value is
+   * set when not expired.
    */
-  virtual size_t daysUntilFirstCertExpires() const PURE;
+  virtual absl::optional<uint32_t> daysUntilFirstCertExpires() const PURE;
 
   /**
    * @return certificate details conforming to proto admin.v2alpha.certs.

diff --git a/envoy/ssl/context_manager.h b/envoy/ssl/context_manager.h
@@ -33,9 +33,10 @@ class ContextManager {
                          const std::vector<std::string>& server_names) PURE;
 
   /**
-   * @return the number of days until the next certificate being managed will expire.
+   * @return the number of days until the next certificate being managed will expire, the value is
+   * set when not expired.
    */
-  virtual size_t daysUntilFirstCertExpires() const PURE;
+  virtual absl::optional<uint32_t> daysUntilFirstCertExpires() const PURE;
 
   /**
    * Iterates through the contexts currently attached to a listener.

@@ -75,7 +75,7 @@ class CertValidator {
                                         uint8_t hash_buffer[EVP_MAX_MD_SIZE],
                                         unsigned hash_length) PURE;
 
-  virtual size_t daysUntilFirstCertExpires() const PURE;
+  virtual absl::optional<uint32_t> daysUntilFirstCertExpires() const PURE;
   virtual std::string getCaFileName() const PURE;
   virtual Envoy::Ssl::CertificateDetailsPtr getCaCertInformation() const PURE;
 };

@@ -473,7 +473,7 @@ Envoy::Ssl::CertificateDetailsPtr DefaultCertValidator::getCaCertInformation() c
   return Utility::certificateDetails(ca_cert_.get(), getCaFileName(), time_source_);
 }
 
-size_t DefaultCertValidator::daysUntilFirstCertExpires() const {
+absl::optional<uint32_t> DefaultCertValidator::daysUntilFirstCertExpires() const {
   return Utility::getDaysUntilExpiration(ca_cert_.get(), time_source_);
 }
 

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <array>
+#include <cstdint>
 #include <deque>
 #include <functional>
 #include <string>
@@ -50,7 +51,7 @@ class DefaultCertValidator : public CertValidator, Logger::Loggable<Logger::Id::
   void updateDigestForSessionId(bssl::ScopedEVP_MD_CTX& md, uint8_t hash_buffer[EVP_MAX_MD_SIZE],
                                 unsigned hash_length) override;
 
-  size_t daysUntilFirstCertExpires() const override;
+  absl::optional<uint32_t> daysUntilFirstCertExpires() const override;
   std::string getCaFileName() const override { return ca_file_path_; };
   Envoy::Ssl::CertificateDetailsPtr getCaCertInformation() const override;
 

@@ -1,5 +1,7 @@
 #include "source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.h"
 
+#include <cstdint>
+
 #include "envoy/extensions/transport_sockets/tls/v3/common.pb.h"
 #include "envoy/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.pb.h"
 #include "envoy/network/transport_socket.h"
@@ -263,14 +265,16 @@ std::string SPIFFEValidator::extractTrustDomain(const std::string& san) {
   return "";
 }
 
-size_t SPIFFEValidator::daysUntilFirstCertExpires() const {
+absl::optional<uint32_t> SPIFFEValidator::daysUntilFirstCertExpires() const {
   if (ca_certs_.empty()) {
-    return 0;
+    return absl::make_optional(std::numeric_limits<uint32_t>::max());
   }
-  size_t ret = SIZE_MAX;
+  absl::optional<uint32_t> ret = absl::make_optional(std::numeric_limits<uint32_t>::max());
   for (auto& cert : ca_certs_) {
-    size_t tmp = Utility::getDaysUntilExpiration(cert.get(), time_source_);
-    if (tmp < ret) {
+    const absl::optional<uint32_t> tmp = Utility::getDaysUntilExpiration(cert.get(), time_source_);
+    if (!tmp.has_value()) {
+      return absl::nullopt;
+    } else if (tmp.value() < ret.value()) {
       ret = tmp;
     }
   }

@@ -50,7 +50,7 @@ class SPIFFEValidator : public CertValidator {
   void updateDigestForSessionId(bssl::ScopedEVP_MD_CTX& md, uint8_t hash_buffer[EVP_MAX_MD_SIZE],
                                 unsigned hash_length) override;
 
-  size_t daysUntilFirstCertExpires() const override;
+  absl::optional<uint32_t> daysUntilFirstCertExpires() const override;
   std::string getCaFileName() const override { return ca_file_name_; }
   Envoy::Ssl::CertificateDetailsPtr getCaCertInformation() const override;
 

@@ -1,6 +1,7 @@
 #include "source/extensions/transport_sockets/tls/context_impl.h"
 
 #include <algorithm>
+#include <cstdint>
 #include <memory>
 #include <string>
 #include <vector>
@@ -484,14 +485,18 @@ std::vector<Ssl::PrivateKeyMethodProviderSharedPtr> ContextImpl::getPrivateKeyMe
   return providers;
 }
 
-size_t ContextImpl::daysUntilFirstCertExpires() const {
-  int daysUntilExpiration = cert_validator_->daysUntilFirstCertExpires();
-  for (auto& ctx : tls_contexts_) {
-    daysUntilExpiration = std::min<int>(
-        Utility::getDaysUntilExpiration(ctx.cert_chain_.get(), time_source_), daysUntilExpiration);
+absl::optional<uint32_t> ContextImpl::daysUntilFirstCertExpires() const {
+  absl::optional<uint32_t> daysUntilExpiration = cert_validator_->daysUntilFirstCertExpires();
+  if (!daysUntilExpiration.has_value()) {
+    return absl::nullopt;
   }
-  if (daysUntilExpiration < 0) { // Ensure that the return value is unsigned
-    return 0;
+  for (auto& ctx : tls_contexts_) {
+    const absl::optional<uint32_t> tmp =
+        Utility::getDaysUntilExpiration(ctx.cert_chain_.get(), time_source_);
+    if (!tmp.has_value()) {
+      return absl::nullopt;
+    }
+    daysUntilExpiration = std::min<uint32_t>(tmp.value(), daysUntilExpiration.value());
   }
   return daysUntilExpiration;
 }

@@ -81,7 +81,7 @@ class ContextImpl : public virtual Envoy::Ssl::Context,
 
   static int sslSocketIndex();
   // Ssl::Context
-  size_t daysUntilFirstCertExpires() const override;
+  absl::optional<uint32_t> daysUntilFirstCertExpires() const override;
   Envoy::Ssl::CertificateDetailsPtr getCaCertInformation() const override;
   std::vector<Envoy::Ssl::CertificateDetailsPtr> getCertChainInformation() const override;
   absl::optional<uint64_t> secondsUntilFirstOcspResponseExpires() const override;

@@ -1,6 +1,7 @@
 #include "source/extensions/transport_sockets/tls/context_manager_impl.h"
 
 #include <algorithm>
+#include <cstddef>
 #include <functional>
 #include <limits>
 
@@ -47,11 +48,15 @@ ContextManagerImpl::createSslServerContext(Stats::Scope& scope,
   return context;
 }
 
-size_t ContextManagerImpl::daysUntilFirstCertExpires() const {
-  size_t ret = std::numeric_limits<int>::max();
+absl::optional<uint32_t> ContextManagerImpl::daysUntilFirstCertExpires() const {
+  absl::optional<uint32_t> ret = absl::make_optional(std::numeric_limits<uint32_t>::max());
   for (const auto& context : contexts_) {
     if (context) {
-      ret = std::min<size_t>(context->daysUntilFirstCertExpires(), ret);
+      const absl::optional<uint32_t> tmp = context->daysUntilFirstCertExpires();
+      if (!tmp.has_value()) {
+        return absl::nullopt;
+      }
+      ret = std::min<uint32_t>(tmp.value(), ret.value());
     }
   }
   return ret;

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <cstdint>
 #include <functional>
 #include <list>
 
@@ -34,7 +35,7 @@ class ContextManagerImpl final : public Envoy::Ssl::ContextManager {
   Ssl::ServerContextSharedPtr
   createSslServerContext(Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config,
                          const std::vector<std::string>& server_names) override;
-  size_t daysUntilFirstCertExpires() const override;
+  absl::optional<uint32_t> daysUntilFirstCertExpires() const override;
   absl::optional<uint64_t> secondsUntilFirstOcspResponseExpires() const override;
   void iterateContexts(std::function<void(const Envoy::Ssl::Context&)> callback) override;
   Ssl::PrivateKeyMethodManager& privateKeyMethodManager() override {

@@ -1,5 +1,7 @@
 #include "source/extensions/transport_sockets/tls/utility.h"
 
+#include <cstdint>
+
 #include "source/common/common/assert.h"
 #include "source/common/common/empty_string.h"
 #include "source/common/common/safe_memcpy.h"
@@ -45,8 +47,8 @@ Envoy::Ssl::CertificateDetailsPtr Utility::certificateDetails(X509* cert, const
       std::make_unique<envoy::admin::v3::CertificateDetails>();
   certificate_details->set_path(path);
   certificate_details->set_serial_number(Utility::getSerialNumberFromCertificate(*cert));
-  certificate_details->set_days_until_expiration(
-      Utility::getDaysUntilExpiration(cert, time_source));
+  const auto days_until_expiry = Utility::getDaysUntilExpiration(cert, time_source).value_or(0);
+  certificate_details->set_days_until_expiration(days_until_expiry);
 
   ProtobufWkt::Timestamp* valid_from = certificate_details->mutable_valid_from();
   TimestampUtil::systemClockToTimestamp(Utility::getValidFrom(*cert), *valid_from);
@@ -253,16 +255,19 @@ std::string Utility::getSubjectFromCertificate(X509& cert) {
   return getRFC2253NameFromCertificate(cert, CertName::Subject);
 }
 
-int32_t Utility::getDaysUntilExpiration(const X509* cert, TimeSource& time_source) {
+absl::optional<uint32_t> Utility::getDaysUntilExpiration(const X509* cert,
+                                                         TimeSource& time_source) {
   if (cert == nullptr) {
-    return std::numeric_limits<int>::max();
+    return absl::make_optional(std::numeric_limits<uint32_t>::max());
   }
   int days, seconds;
   if (ASN1_TIME_diff(&days, &seconds, currentASN1_Time(time_source).get(),
                      X509_get0_notAfter(cert))) {
-    return days;
+    if (days >= 0 && seconds >= 0) {
+      return absl::make_optional(days);
+    }
   }
-  return 0;
+  return absl::nullopt;
 }
 
 absl::string_view Utility::getCertificateExtensionValue(X509& cert,

@@ -89,9 +89,9 @@ absl::string_view getCertificateExtensionValue(X509& cert, absl::string_view ext
  * Returns the days until this certificate is valid.
  * @param cert the certificate
  * @param time_source the time source to use for current time calculation.
- * @return the number of days till this certificate is valid.
+ * @return the number of days till this certificate is valid, the value is set when not expired.
  */
-int32_t getDaysUntilExpiration(const X509* cert, TimeSource& time_source);
+absl::optional<uint32_t> getDaysUntilExpiration(const X509* cert, TimeSource& time_source);
 
 /**
  * Returns the time from when this certificate is valid.

diff --git a/source/server/server.cc b/source/server/server.cc
@@ -276,7 +276,7 @@ void InstanceImpl::updateServerStats() {
   server_stats_->total_connections_.set(listener_manager_->numConnections() +
                                         parent_stats.parent_connections_);
   server_stats_->days_until_first_cert_expiring_.set(
-      sslContextManager().daysUntilFirstCertExpires());
+      sslContextManager().daysUntilFirstCertExpires().value());
 
   auto secs_until_ocsp_response_expires =
       sslContextManager().secondsUntilFirstOcspResponseExpires();

diff --git a/source/server/ssl_context_manager.cc b/source/server/ssl_context_manager.cc
@@ -1,5 +1,7 @@
 #include "source/server/ssl_context_manager.h"
 
+#include <cstddef>
+
 #include "envoy/common/exception.h"
 #include "envoy/registry/registry.h"
 
@@ -25,7 +27,9 @@ class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager {
     throwException();
   }
 
-  size_t daysUntilFirstCertExpires() const override { return std::numeric_limits<int>::max(); }
+  absl::optional<uint32_t> daysUntilFirstCertExpires() const override {
+    return absl::make_optional(std::numeric_limits<uint32_t>::max());
+  }
   absl::optional<uint64_t> secondsUntilFirstOcspResponseExpires() const override {
     return absl::nullopt;
   }

@@ -1,3 +1,4 @@
+#include <cstdint>
 #include <memory>
 #include <regex>
 #include <string>
@@ -539,7 +540,7 @@ name: envoy.tls.cert_validator.spiffe
 
 TEST_F(TestSPIFFEValidator, TestDaysUntilFirstCertExpires) {
   initialize();
-  EXPECT_EQ(0, validator().daysUntilFirstCertExpires());
+  EXPECT_EQ(std::numeric_limits<uint32_t>::max(), validator().daysUntilFirstCertExpires().value());
 
   Event::SimulatedTimeSystem time_system;
   time_system.setSystemTime(std::chrono::milliseconds(0));
@@ -557,9 +558,29 @@ name: envoy.tls.cert_validator.spiffe
         filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/intermediate_ca_cert.pem"
   )EOF"),
              time_system);
-  EXPECT_EQ(19231, validator().daysUntilFirstCertExpires());
+  EXPECT_EQ(19231, validator().daysUntilFirstCertExpires().value());
   time_system.setSystemTime(std::chrono::milliseconds(864000000));
-  EXPECT_EQ(19221, validator().daysUntilFirstCertExpires());
+  EXPECT_EQ(19221, validator().daysUntilFirstCertExpires().value());
+}
+
+TEST_F(TestSPIFFEValidator, TestDaysUntilFirstCertExpiresExpired) {
+  Event::SimulatedTimeSystem time_system;
+  // 2033-05-18 03:33:20 UTC
+  const time_t known_date_time = 2000000000;
+  time_system.setSystemTime(std::chrono::system_clock::from_time_t(known_date_time));
+
+  initialize(TestEnvironment::substitute(R"EOF(
+name: envoy.tls.cert_validator.spiffe
+typed_config:
+  "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.SPIFFECertValidatorConfig
+  trust_domains:
+    - name: example.com
+      trust_bundle:
+        filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/spiffe_san_cert.pem"
+  )EOF"),
+             time_system);
+
+  EXPECT_EQ(absl::nullopt, validator().daysUntilFirstCertExpires());
 }
 
 TEST_F(TestSPIFFEValidator, TestAddClientValidationContext) {

@@ -153,7 +153,7 @@ TEST_F(SslContextImplTest, TestExpiringCert) {
   // Calculate the days until test cert expires
   auto cert_expiry = TestUtility::parseTime(TEST_UNITTEST_CERT_NOT_AFTER, "%b %d %H:%M:%S %Y GMT");
   int64_t days_until_expiry = absl::ToInt64Hours(cert_expiry - absl::Now()) / 24;
-  EXPECT_EQ(context->daysUntilFirstCertExpires(), days_until_expiry);
+  EXPECT_EQ(context->daysUntilFirstCertExpires().value(), days_until_expiry);
 }
 
 TEST_F(SslContextImplTest, TestExpiredCert) {
@@ -171,7 +171,7 @@ TEST_F(SslContextImplTest, TestExpiredCert) {
   ClientContextConfigImpl cfg(tls_context, factory_context_);
   Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg));
   auto cleanup = cleanUpHelper(context);
-  EXPECT_EQ(0U, context->daysUntilFirstCertExpires());
+  EXPECT_EQ(absl::nullopt, context->daysUntilFirstCertExpires());
 }
 
 // Validate that when the context is updated, the daysUntilFirstCertExpires returns the current
@@ -191,7 +191,7 @@ TEST_F(SslContextImplTest, TestContextUpdate) {
   TestUtility::loadFromYaml(TestEnvironment::substitute(expired_yaml), tls_context);
   ClientContextConfigImpl cfg(tls_context, factory_context_);
   Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg));
-  EXPECT_EQ(manager_.daysUntilFirstCertExpires(), 0U);
+  EXPECT_EQ(manager_.daysUntilFirstCertExpires(), absl::nullopt);
 
   const std::string expiring_yaml = R"EOF(
   common_tls_context:
@@ -215,17 +215,17 @@ TEST_F(SslContextImplTest, TestContextUpdate) {
   // context expiry.
   auto cert_expiry = TestUtility::parseTime(TEST_UNITTEST_CERT_NOT_AFTER, "%b %d %H:%M:%S %Y GMT");
   int64_t days_until_expiry = absl::ToInt64Hours(cert_expiry - absl::Now()) / 24;
-  EXPECT_EQ(new_context->daysUntilFirstCertExpires(), days_until_expiry);
-  EXPECT_EQ(manager_.daysUntilFirstCertExpires(), days_until_expiry);
+  EXPECT_EQ(new_context->daysUntilFirstCertExpires().value(), days_until_expiry);
+  EXPECT_EQ(manager_.daysUntilFirstCertExpires().value(), days_until_expiry);
 
   // Update the context again and validate daysUntilFirstCertExpires still reflects the current
   // expiry.
   Envoy::Ssl::ClientContextSharedPtr updated_context(manager_.createSslClientContext(store_, cfg));
   manager_.removeContext(new_context);
   auto cleanup = cleanUpHelper(updated_context);
 
-  EXPECT_EQ(updated_context->daysUntilFirstCertExpires(), 0U);
-  EXPECT_EQ(manager_.daysUntilFirstCertExpires(), 0U);
+  EXPECT_EQ(updated_context->daysUntilFirstCertExpires(), absl::nullopt);
+  EXPECT_EQ(manager_.daysUntilFirstCertExpires(), absl::nullopt);
 }
 
 TEST_F(SslContextImplTest, TestGetCertInformation) {

@@ -1,3 +1,4 @@
+#include <cstdint>
 #include <string>
 #include <vector>
 
@@ -122,17 +123,13 @@ TEST(UtilityTest, TestDaysUntilExpiration) {
   Event::SimulatedTimeSystem time_source;
   time_source.setSystemTime(std::chrono::system_clock::from_time_t(known_date_time));
 
-  // Get expiration time from the certificate info.
-  const absl::Time expiration =
-      TestUtility::parseTime(TEST_SAN_DNS_CERT_NOT_AFTER, "%b %e %H:%M:%S %Y GMT");
-
-  int days = std::difftime(absl::ToTimeT(expiration), known_date_time) / (60 * 60 * 24);
-  EXPECT_EQ(days, Utility::getDaysUntilExpiration(cert.get(), time_source));
+  EXPECT_EQ(absl::nullopt, Utility::getDaysUntilExpiration(cert.get(), time_source));
 }
 
 TEST(UtilityTest, TestDaysUntilExpirationWithNull) {
   Event::SimulatedTimeSystem time_source;
-  EXPECT_EQ(std::numeric_limits<int>::max(), Utility::getDaysUntilExpiration(nullptr, time_source));
+  EXPECT_EQ(std::numeric_limits<uint32_t>::max(),
+            Utility::getDaysUntilExpiration(nullptr, time_source).value());
 }
 
 TEST(UtilityTest, TestValidFrom) {