policy: Do not store policy reference in socket_option

Remove the member 'initial_policy_' from the Cilium SocketOption class, as that reference was possibly kept after the policy had already been deleted. This made it possible for the policy to be destructed from the worker thread, which can lead to Envoy crash. 'initial_policy_' was stored as we already did the policy lookup and the same policy is needed in other Cilium filters. Have the cilium.network and cilium.tls_wrapper filters do their own policy lookups instead, so that we do not need to keep the reference in SocketOption. Signed-off-by: Jarno Rajahalme <[email protected]>
cilium · Nov 22, 2024 · 080d03a · 080d03a
1 parent b817a52
commit 080d03a
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 18 deletions.
diff --git a/cilium/bpf_metadata.cc b/cilium/bpf_metadata.cc
@@ -443,7 +443,7 @@ Cilium::SocketOptionSharedPtr Config::getMetadata(Network::ConnectionSocket& soc
     mark = ((is_ingress_) ? 0x0A00 : 0x0B00) | cluster_id | identity_id;
   }
   return std::make_shared<Cilium::SocketOption>(
-      policy, mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_, dip->port(),
+      mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_, dip->port(),
       std::move(pod_ip), std::move(src_address), std::move(source_addresses.ipv4_),
       std::move(source_addresses.ipv6_), shared_from_this(), proxy_id_, sni);
 }

diff --git a/cilium/network_filter.cc b/cilium/network_filter.cc
@@ -135,6 +135,13 @@ Network::FilterStatus Instance::onNewConnection() {
       ENVOY_LOG(warn, "cilium.network (egress): No destination address ");
       return false;
     }
+
+    const auto& policy = option->getPolicy();
+    if (!policy) {
+      ENVOY_LOG_MISC(warn, "cilium.network: No policy found for pod {}", option->pod_ip_);
+      return false;
+    }
+
     if (!option->ingress_) {
       const auto dip = dst_address->ip();
       if (!dip) {
@@ -146,7 +153,7 @@ Network::FilterStatus Instance::onNewConnection() {
       destination_identity = option->resolvePolicyId(dip);
 
       if (option->ingress_source_identity_ != 0) {
-        auto ingress_port_policy = option->initial_policy_->findPortPolicy(true, destination_port_);
+        auto ingress_port_policy = policy->findPortPolicy(true, destination_port_);
         if (!ingress_port_policy.allowed(option->ingress_source_identity_, sni)) {
           ENVOY_CONN_LOG(
               debug,
@@ -157,7 +164,7 @@ Network::FilterStatus Instance::onNewConnection() {
       }
     }
 
-    auto port_policy = option->initial_policy_->findPortPolicy(option->ingress_, destination_port_);
+    auto port_policy = policy->findPortPolicy(option->ingress_, destination_port_);
 
     remote_id_ = option->ingress_ ? option->identity_ : destination_identity;
     if (!port_policy.allowed(remote_id_, sni)) {

diff --git a/cilium/socket_option.h b/cilium/socket_option.h
@@ -222,19 +222,18 @@ class SocketMarkOption : public Network::Socket::Option,
 
 class SocketOption : public SocketMarkOption {
 public:
-  SocketOption(PolicyInstanceConstSharedPtr policy, uint32_t mark, uint32_t ingress_source_identity,
-               uint32_t source_identity, bool ingress, bool l7lb, uint16_t port,
-               std::string&& pod_ip,
+  SocketOption(uint32_t mark, uint32_t ingress_source_identity, uint32_t source_identity,
+               bool ingress, bool l7lb, uint16_t port, std::string&& pod_ip,
                Network::Address::InstanceConstSharedPtr original_source_address,
                Network::Address::InstanceConstSharedPtr ipv4_source_address,
                Network::Address::InstanceConstSharedPtr ipv6_source_address,
-               const std::shared_ptr<PolicyResolver>& policy_id_resolver, uint32_t proxy_id,
+               const std::shared_ptr<PolicyResolver>& policy_resolver, uint32_t proxy_id,
                absl::string_view sni)
       : SocketMarkOption(mark, source_identity, original_source_address, ipv4_source_address,
                          ipv6_source_address),
-        ingress_source_identity_(ingress_source_identity), initial_policy_(policy),
-        ingress_(ingress), is_l7lb_(l7lb), port_(port), pod_ip_(std::move(pod_ip)),
-        proxy_id_(proxy_id), sni_(sni), policy_id_resolver_(policy_id_resolver) {
+        ingress_source_identity_(ingress_source_identity), ingress_(ingress), is_l7lb_(l7lb),
+        port_(port), pod_ip_(std::move(pod_ip)), proxy_id_(proxy_id), sni_(sni),
+        policy_resolver_(policy_resolver) {
     ENVOY_LOG(debug,
               "Cilium SocketOption(): source_identity: {}, "
               "ingress: {}, port: {}, pod_ip: {}, source_addresses: {}/{}/{}, mark: {:x} (magic "
@@ -244,15 +243,14 @@ class SocketOption : public SocketMarkOption {
               ipv4_source_address_ ? ipv4_source_address_->asString() : "",
               ipv6_source_address_ ? ipv6_source_address_->asString() : "", mark_, mark & 0xff00,
               mark & 0xff, mark >> 16, proxy_id_, sni_);
-    ASSERT(initial_policy_ != nullptr);
   }
 
   uint32_t resolvePolicyId(const Network::Address::Ip* ip) const {
-    return policy_id_resolver_->resolvePolicyId(ip);
+    return policy_resolver_->resolvePolicyId(ip);
   }
 
   const PolicyInstanceConstSharedPtr getPolicy() const {
-    return policy_id_resolver_->getPolicy(pod_ip_);
+    return policy_resolver_->getPolicy(pod_ip_);
   }
 
   // policyUseUpstreamDestinationAddress returns 'true' if policy enforcement should be done on the
@@ -261,7 +259,6 @@ class SocketOption : public SocketMarkOption {
 
   // Additional ingress policy enforcement is performed if ingress_source_identity is non-zero
   uint32_t ingress_source_identity_;
-  const PolicyInstanceConstSharedPtr initial_policy_; // Never NULL
   bool ingress_;
   bool is_l7lb_;
   uint16_t port_;
@@ -270,7 +267,7 @@ class SocketOption : public SocketMarkOption {
   std::string sni_;
 
 private:
-  const std::shared_ptr<PolicyResolver> policy_id_resolver_;
+  const std::shared_ptr<PolicyResolver> policy_resolver_;
 };
 
 using SocketOptionSharedPtr = std::shared_ptr<SocketOption>;

diff --git a/cilium/tls_wrapper.cc b/cilium/tls_wrapper.cc
@@ -35,6 +35,13 @@ class SslSocketWrapper : public Network::TransportSocket {
     // TLS a raw socket is used instead,
     const auto option = Cilium::GetSocketOption(callbacks.connection().socketOptions());
     if (option) {
+      const auto& policy = option->getPolicy();
+      if (!policy) {
+        policy->tlsWrapperMissingPolicyInc();
+        ENVOY_LOG_MISC(warn, "cilium.tls_wrapper: No policy found for pod {}", option->pod_ip_);
+        return;
+      }
+
       // Resolve the destination security ID and port
       uint32_t destination_identity = 0;
       uint32_t destination_port = option->port_;
@@ -63,8 +70,7 @@ class SslSocketWrapper : public Network::TransportSocket {
       const auto& sni = option->sni_;
 
       auto remote_id = option->ingress_ ? option->identity_ : destination_identity;
-      auto port_policy =
-          option->initial_policy_->findPortPolicy(option->ingress_, destination_port);
+      auto port_policy = policy->findPortPolicy(option->ingress_, destination_port);
       const Envoy::Ssl::ContextConfig* config = nullptr;
       bool raw_socket_allowed = false;
       Envoy::Ssl::ContextSharedPtr ctx =
@@ -89,7 +95,7 @@ class SslSocketWrapper : public Network::TransportSocket {
         // Set the callbacks
         socket_->setTransportSocketCallbacks(callbacks);
       } else {
-        option->initial_policy_->tlsWrapperMissingPolicyInc();
+        policy->tlsWrapperMissingPolicyInc();
 
         std::string ipStr("<none>");
         if (option->ingress_) {