From 63d0b4a0622ce38384a3e7f0f4b7c5a990537612 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Sun, 24 Nov 2024 09:17:48 +0100 Subject: [PATCH] SocketOption: Only store a weak policy reference Change 'initial_policy_' from a shared to a weak pointer, so that it does not hold a reference to the PolicyInstance, 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. Change 'getPolicy()' to convert the initial policy to a shared pointer, and only performing the new policy lookup if that can not be done. This speeds up existing uses of 'getPolicy()' a bit as they can use the initial_policy_ if still valid. Signed-off-by: Jarno Rajahalme --- cilium/network_filter.cc | 11 +++++++++-- cilium/socket_option.h | 22 ++++++++++++++++------ cilium/tls_wrapper.cc | 12 +++++++++--- tests/metadata_config_test.cc | 14 +++++++------- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/cilium/network_filter.cc b/cilium/network_filter.cc index db67badbc..b86474a2d 100644 --- 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 index f05862b00..2014ca7c9 100644 --- a/cilium/socket_option.h +++ b/cilium/socket_option.h @@ -15,12 +15,16 @@ namespace Cilium { class PolicyInstance; using PolicyInstanceConstSharedPtr = std::shared_ptr; +using PolicyInstanceConstWeakPtr = std::weak_ptr; class PolicyResolver { public: virtual ~PolicyResolver() = default; virtual uint32_t resolvePolicyId(const Network::Address::Ip*) const PURE; + + // getPolicy gets a shared pointer to the policy instance, looking up a fresh one if the + // initial policy is no longer valid. virtual const PolicyInstanceConstSharedPtr getPolicy(const std::string&) const PURE; }; @@ -221,8 +225,10 @@ class SocketMarkOption : public Network::Socket::Option, }; class SocketOption : public SocketMarkOption { + friend class MetadataConfigTest; + public: - SocketOption(PolicyInstanceConstSharedPtr policy, uint32_t mark, uint32_t ingress_source_identity, + SocketOption(PolicyInstanceConstWeakPtr policy, 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, @@ -232,9 +238,9 @@ class SocketOption : public SocketMarkOption { 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), + initial_policy_(policy), policy_id_resolver_(policy_id_resolver) { ENVOY_LOG(debug, "Cilium SocketOption(): source_identity: {}, " "ingress: {}, port: {}, pod_ip: {}, source_addresses: {}/{}/{}, mark: {:x} (magic " @@ -244,7 +250,6 @@ 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 { @@ -252,6 +257,11 @@ class SocketOption : public SocketMarkOption { } const PolicyInstanceConstSharedPtr getPolicy() const { + const auto& policy = initial_policy_.lock(); + if (policy) { + return policy; + } + // Get fresh policy if the initial policy no longer exists return policy_id_resolver_->getPolicy(pod_ip_); } @@ -261,13 +271,13 @@ 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_; std::string pod_ip_; uint32_t proxy_id_; std::string sni_; + const PolicyInstanceConstWeakPtr initial_policy_; private: const std::shared_ptr policy_id_resolver_; diff --git a/cilium/tls_wrapper.cc b/cilium/tls_wrapper.cc index acbf9ef96..0dfb68b1e 100644 --- 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(""); if (option->ingress_) { diff --git a/tests/metadata_config_test.cc b/tests/metadata_config_test.cc index 68c5bd422..941b7af7c 100644 --- a/tests/metadata_config_test.cc +++ b/tests/metadata_config_test.cc @@ -263,7 +263,7 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbMetadata) { EXPECT_EQ("[face::42]:0", option->ipv6_source_address_->asString()); EXPECT_EQ(80, option->port_); EXPECT_EQ("10.1.1.42", option->pod_ip_); - EXPECT_NE(nullptr, option->initial_policy_); + EXPECT_NE(nullptr, option->initial_policy_.lock()); EXPECT_EQ(0, option->ingress_source_identity_); // Check that Ingress security ID is used in the socket mark @@ -296,14 +296,14 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbIngressEnforcedMetadata) { EXPECT_EQ("[face::42]:0", option->ipv6_source_address_->asString()); EXPECT_EQ(80, option->port_); EXPECT_EQ("10.1.1.42", option->pod_ip_); - EXPECT_NE(nullptr, option->initial_policy_); + EXPECT_NE(nullptr, option->initial_policy_.lock()); EXPECT_EQ(12345678, option->ingress_source_identity_); // Check that Ingress security ID is used in the socket mark EXPECT_TRUE((option->mark_ & 0xffff) == 0x0B00 && (option->mark_ >> 16) == 8); // Expect policy accepts security ID 12345678 on ingress on port 80 - auto port_policy = option->initial_policy_->findPortPolicy(true, 80); + auto port_policy = option->getPolicy()->findPortPolicy(true, 80); EXPECT_TRUE(port_policy.allowed(12345678, "")); } @@ -333,14 +333,14 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbIngressEnforcedCIDRMetadata) { EXPECT_EQ("[face::42]:0", option->ipv6_source_address_->asString()); EXPECT_EQ(80, option->port_); EXPECT_EQ("10.1.1.42", option->pod_ip_); - EXPECT_NE(nullptr, option->initial_policy_); + EXPECT_NE(nullptr, option->initial_policy_.lock()); EXPECT_EQ(2, option->ingress_source_identity_); // Check that Ingress security ID is used in the socket mark EXPECT_TRUE((option->mark_ & 0xffff) == 0x0B00 && (option->mark_ >> 16) == 8); // Expect policy does not accept security ID 2 on ingress on port 80 - auto port_policy = option->initial_policy_->findPortPolicy(true, 80); + auto port_policy = option->getPolicy()->findPortPolicy(true, 80); EXPECT_FALSE(port_policy.allowed(2, "")); } @@ -387,7 +387,7 @@ TEST_F(MetadataConfigTest, EastWestL7LbMetadata) { EXPECT_EQ("[face::1:1:1]:41234", option->ipv6_source_address_->asString()); EXPECT_EQ(80, option->port_); EXPECT_EQ("10.1.1.1", option->pod_ip_); - EXPECT_NE(nullptr, option->initial_policy_); + EXPECT_NE(nullptr, option->initial_policy_.lock()); // Check that Endpoint's ID is used in the socket mark EXPECT_TRUE((option->mark_ & 0xffff) == 0x0900 && (option->mark_ >> 16) == 2048); @@ -417,7 +417,7 @@ TEST_F(MetadataConfigTest, EastWestL7LbMetadataNoOriginalSource) { EXPECT_EQ("[face::42]:0", option->ipv6_source_address_->asString()); EXPECT_EQ(80, option->port_); EXPECT_EQ("10.1.1.42", option->pod_ip_); - EXPECT_NE(nullptr, option->initial_policy_); + EXPECT_NE(nullptr, option->initial_policy_.lock()); EXPECT_EQ(0, option->ingress_source_identity_); // Check that Ingress ID is used in the socket mark