TLS splicing/bumping

[LuyaoZhong]

Copy changes of envoyproxy/envoy#23192.

This PR is to merge following parts, and do integration test.

Certificate Provider framework envoyproxy/envoy#19582
SNI-based cert selection in tls transport socket envoyproxy/envoy#22036
A new network filter - BumpingFilter envoyproxy/envoy#22582
Certificate Provider instance - LocalMimicCertProvider envoyproxy/envoy#23063

Signed-off-by: Luyao Zhong [email protected]
Signed-off-by: LeiZhang [email protected]

[ipuustin]

Could you see if the commits could be done in a way that would make the review easier? I think the cert validator could be a separate commit (since that might be upstreamed separately) and the config files could be in one commit. There seems to be also two commits about expiration time, could they be just one commit?

Do the tests compile now correctly (if you say bazel build //test/... or do you need to take in the mocks change commit too?

[ipuustin]

Maybe use intel.com.key instead of baidu.com.key?

[LuyaoZhong]

will remove this file, this will not be used any more

[ipuustin]

These could be updated to point to this repo when we merge the PR. Maybe after image release so we can skip the building part?

[LuyaoZhong]

yes

[ipuustin]

Just remove code, no need to comment. Version control will track the changes.

[LuyaoZhong]

Could you see if the commits could be done in a way that would make the review easier? I think the cert validator could be a separate commit (since that might be upstreamed separately) and the config files could be in one commit. There seems to be also two commits about expiration time, could they be just one commit?

I think you mean "cert provider framework". I have talked with Matt, this will not be upstreamed separately until bumping is ready.
"sni-based cert selection" will be upstreamed separately, but before it get merged we don't want to modify current PoC. For convenient and to skip SDL, we are developing on one single PR envoyproxy/envoy#23192 and we just copy changes to intel repo.

Do the tests compile now correctly (if you say bazel build //test/... or do you need to take in the mocks change commit too?
we need to take in the mocks commit, will add.

@ipuustin Thanks. We have more changes on the way, would like you to help review when it is ready.

[ipuustin]

Thanks!

[poussa]

@LuyaoZhong can you update this PR so that it matches the upstream envoy PR which contains the crash fix. After that we can merge.

[LuyaoZhong]

@LuyaoZhong can you update this PR so that it matches the upstream envoy PR which contains the crash fix. After that we can merge.

done

[ipuustin]

Please run fix_format and also clang_tidy. The target is to have code quality on the same level as upstream Envoy process to allow for easy transfer of code.

diff --git a/envoy/certificate_provider/certificate_provider.h b/envoy/certificate_provider/certificate_provider.h
index 4af5ca40ff..e243b38f14 100644
--- a/envoy/certificate_provider/certificate_provider.h
+++ b/envoy/certificate_provider/certificate_provider.h
@@ -86,7 +86,8 @@ public:
    * @return OnDemandUpdateHandle the handle which can remove that update callback.
    */
   virtual OnDemandUpdateHandlePtr addOnDemandUpdateCallback(
-      const std::string cert_name, absl::optional<Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
+      const std::string cert_name,
+      absl::optional<Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
       Event::Dispatcher& thread_local_dispatcher, OnDemandUpdateCallbacks& callback) PURE;

   /**
diff --git a/source/extensions/certificate_providers/local_certificate/local_certificate.cc b/source/extensions/certificate_providers/local_certificate/local_certificate.cc
index 1ae79c43c3..48485524d6 100644
--- a/source/extensions/certificate_providers/local_certificate/local_certificate.cc
+++ b/source/extensions/certificate_providers/local_certificate/local_certificate.cc
@@ -1,8 +1,9 @@
 #include "source/extensions/certificate_providers/local_certificate/local_certificate.h"

-#include <cstddef>
 #include <openssl/x509.h>

+#include <cstddef>
+
 #include "source/common/common/logger.h"
 #include "source/common/config/datasource.h"
 #include "source/common/protobuf/protobuf.h"
@@ -55,7 +56,8 @@ Provider::tlsCertificates(const std::string&) const {
 }

 Envoy::CertificateProvider::OnDemandUpdateHandlePtr Provider::addOnDemandUpdateCallback(
-    const std::string sni, absl::optional<::Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
+    const std::string sni,
+    absl::optional<::Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
     Event::Dispatcher& thread_local_dispatcher,
     Envoy::CertificateProvider::OnDemandUpdateCallbacks& callback) {
   ASSERT(main_thread_dispatcher_.isThreadSafe());
@@ -69,7 +71,6 @@ Envoy::CertificateProvider::OnDemandUpdateHandlePtr Provider::addOnDemandUpdateC
   // We need to align this cache_hit with current transport socket behavior
   bool cache_hit = [&]() { return certificates_.is_in_cache(sni); }();

-
   if (metadata.has_value()) {
     if (cache_hit) {
       ENVOY_LOG(debug, "Cache hit for {}", sni);
@@ -81,8 +82,7 @@ Envoy::CertificateProvider::OnDemandUpdateHandlePtr Provider::addOnDemandUpdateC
         signCertificate(sni, metadata.value(), thread_local_dispatcher);
       });
     }
-  }
-  else {
+  } else {
     runOnDemandUpdateCallback(sni, absl::nullopt, thread_local_dispatcher, cache_hit);
   }
   return handle;
@@ -99,10 +99,10 @@ void Provider::runAddUpdateCallback() {
   update_callback_manager_.runCallbacks();
 }

-void Provider::runOnDemandUpdateCallback(const std::string& host,
-                                         absl::optional<Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
-                                         Event::Dispatcher& thread_local_dispatcher,
-                                         bool in_cache) {
+void Provider::runOnDemandUpdateCallback(
+    const std::string& host,
+    absl::optional<Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
+    Event::Dispatcher& thread_local_dispatcher, bool in_cache) {
   ASSERT(main_thread_dispatcher_.isThreadSafe());
   auto host_it = on_demand_update_callbacks_.find(host);
   if (host_it != on_demand_update_callbacks_.end()) {
@@ -110,9 +110,11 @@ void Provider::runOnDemandUpdateCallback(const std::string& host,
       auto& callbacks = pending_callbacks->callbacks_;
       pending_callbacks->cancel();
       if (in_cache) {
-        thread_local_dispatcher.post([&callbacks, host, metadata] { callbacks.onCacheHit(host, !metadata.has_value()); });
+        thread_local_dispatcher.post(
+            [&callbacks, host, metadata] { callbacks.onCacheHit(host, !metadata.has_value()); });
       } else {
-        thread_local_dispatcher.post([&callbacks, host, metadata] { callbacks.onCacheMiss(host, !metadata.has_value()); });
+        thread_local_dispatcher.post(
+            [&callbacks, host, metadata] { callbacks.onCacheMiss(host, !metadata.has_value()); });
       }
     }
     on_demand_update_callbacks_.erase(host_it);
diff --git a/source/extensions/certificate_providers/local_certificate/local_certificate.h b/source/extensions/certificate_providers/local_certificate/local_certificate.h
index 5512b96d00..ebfb037311 100644
--- a/source/extensions/certificate_providers/local_certificate/local_certificate.h
+++ b/source/extensions/certificate_providers/local_certificate/local_certificate.h
@@ -85,7 +85,8 @@ public:
       std::reference_wrapper<const envoy::extensions::transport_sockets::tls::v3::TlsCertificate>>
   tlsCertificates(const std::string& cert_name) const override;
   Envoy::CertificateProvider::OnDemandUpdateHandlePtr addOnDemandUpdateCallback(
-      const std::string cert_name, absl::optional<Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
+      const std::string cert_name,
+      absl::optional<Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
       Event::Dispatcher& thread_local_dispatcher,
       ::Envoy::CertificateProvider::OnDemandUpdateCallbacks& callback) override;
   Common::CallbackHandlePtr addUpdateCallback(const std::string& cert_name,
@@ -106,9 +107,10 @@ private:
   CertCacheImpl certificates_;

   void runAddUpdateCallback();
-  void runOnDemandUpdateCallback(const std::string& host,
-                                 absl::optional<Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
-                                 Event::Dispatcher& thread_local_dispatcher, bool in_cache = true);
+  void runOnDemandUpdateCallback(
+      const std::string& host,
+      absl::optional<Envoy::CertificateProvider::OnDemandUpdateMetadataPtr> metadata,
+      Event::Dispatcher& thread_local_dispatcher, bool in_cache = true);
   // void signCertificate(std::string sni, absl::Span<const std::string> dns_sans, const std::string
   // subject,
   //                      Event::Dispatcher& thread_local_dispatcher);
diff --git a/source/extensions/filters/network/bumping/bumping.cc b/source/extensions/filters/network/bumping/bumping.cc
index 1836b49759..2f66842555 100644
--- a/source/extensions/filters/network/bumping/bumping.cc
+++ b/source/extensions/filters/network/bumping/bumping.cc
@@ -236,8 +236,8 @@ void Filter::requestCertificate(Ssl::ConnectionInfoConstSharedPtr info) {
     // to establish connection with upstream
     config_->main_dispatcher_.post([this]() {
       this->on_demand_handle_ = config_->tls_certificate_provider_->addOnDemandUpdateCallback(
-          std::string(read_callbacks_->connection().requestedServerName()),
-          absl::nullopt, read_callbacks_->connection().dispatcher(), *this);
+          std::string(read_callbacks_->connection().requestedServerName()), absl::nullopt,
+          read_callbacks_->connection().dispatcher(), *this);
     });
     return;
   }
@@ -323,9 +323,10 @@ void Filter::onCacheHit(const std::string, bool) {
 void Filter::onCacheMiss(const std::string, bool check_only) {
   if (check_only) {
     // cert is not present in cert provider cache, and mimic cert is not generated
-    // establish connection with upstream and trigger cert mimicking based on original certification.
+    // establish connection with upstream and trigger cert mimicking based on original
+    // certification.
     establishUpstreamConnection();
-    //read_callbacks_->continueReading();
+    // read_callbacks_->continueReading();
     return;
   }
   // cert is not present in cert provider cache, and mimic cert is generated

[LuyaoZhong]

@ipuustin @poussa format is fixed, and clang tidy is executed to make sure there is no error-level Diagnostics.
Thanks Ismo for your comments.

[LuyaoZhong]

It's better to fixing warning diagnostics as well, working on that.

[LuyaoZhong]

It's better to fixing warning diagnostics as well, working on that.

Done. @ipuustin @poussa

[ipuustin]

LGTM