From c7b15784ccb0d199b83e6315960c62f4c4e3cd0e Mon Sep 17 00:00:00 2001 From: Tam Mach Date: Mon, 2 Dec 2024 22:01:28 +1100 Subject: [PATCH] Revert "policy: Do not store policy reference in cilium socket option" This reverts commit 2aa20ee3acb68cd38d57669af19508bea8f0ba62. --- cilium/bpf_metadata.cc | 2 +- cilium/network_filter.cc | 9 ++------- cilium/socket_option.h | 21 ++++++++++++--------- cilium/tls_wrapper.cc | 21 ++++++++------------- tests/bpf_metadata.cc | 6 +++--- tests/metadata_config_test.cc | 9 +++++++-- 6 files changed, 33 insertions(+), 35 deletions(-) diff --git a/cilium/bpf_metadata.cc b/cilium/bpf_metadata.cc index 630ba8681..1e2f1cfd5 100644 --- 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( - mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_, dip->port(), + policy, 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 index 6bd9316c2..db67badbc 100644 --- a/cilium/network_filter.cc +++ b/cilium/network_filter.cc @@ -135,11 +135,6 @@ 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) { @@ -151,7 +146,7 @@ Network::FilterStatus Instance::onNewConnection() { destination_identity = option->resolvePolicyId(dip); if (option->ingress_source_identity_ != 0) { - auto ingress_port_policy = policy->findPortPolicy(true, destination_port_); + auto ingress_port_policy = option->initial_policy_->findPortPolicy(true, destination_port_); if (!ingress_port_policy.allowed(option->ingress_source_identity_, sni)) { ENVOY_CONN_LOG( debug, @@ -162,7 +157,7 @@ Network::FilterStatus Instance::onNewConnection() { } } - auto port_policy = policy->findPortPolicy(option->ingress_, destination_port_); + auto port_policy = option->initial_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 e2c6ab788..f05862b00 100644 --- a/cilium/socket_option.h +++ b/cilium/socket_option.h @@ -222,18 +222,19 @@ class SocketMarkOption : public Network::Socket::Option, class SocketOption : public SocketMarkOption { public: - SocketOption(uint32_t mark, uint32_t ingress_source_identity, uint32_t source_identity, - bool ingress, bool l7lb, uint16_t port, std::string&& pod_ip, + 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, Network::Address::InstanceConstSharedPtr original_source_address, Network::Address::InstanceConstSharedPtr ipv4_source_address, Network::Address::InstanceConstSharedPtr ipv6_source_address, - const std::shared_ptr& policy_resolver, uint32_t proxy_id, + const std::shared_ptr& policy_id_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), ingress_(ingress), is_l7lb_(l7lb), - port_(port), pod_ip_(std::move(pod_ip)), proxy_id_(proxy_id), sni_(sni), - policy_resolver_(policy_resolver) { + 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) { ENVOY_LOG(debug, "Cilium SocketOption(): source_identity: {}, " "ingress: {}, port: {}, pod_ip: {}, source_addresses: {}/{}/{}, mark: {:x} (magic " @@ -243,14 +244,15 @@ 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_resolver_->resolvePolicyId(ip); + return policy_id_resolver_->resolvePolicyId(ip); } const PolicyInstanceConstSharedPtr getPolicy() const { - return policy_resolver_->getPolicy(pod_ip_); + return policy_id_resolver_->getPolicy(pod_ip_); } // policyUseUpstreamDestinationAddress returns 'true' if policy enforcement should be done on the @@ -259,6 +261,7 @@ 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_; @@ -267,7 +270,7 @@ class SocketOption : public SocketMarkOption { std::string sni_; private: - const std::shared_ptr policy_resolver_; + const std::shared_ptr policy_id_resolver_; }; using SocketOptionSharedPtr = std::shared_ptr; diff --git a/cilium/tls_wrapper.cc b/cilium/tls_wrapper.cc index 34eaa28ab..100875b1a 100644 --- a/cilium/tls_wrapper.cc +++ b/cilium/tls_wrapper.cc @@ -37,11 +37,6 @@ 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) { - 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_; @@ -70,7 +65,8 @@ class SslSocketWrapper : public Network::TransportSocket { const auto& sni = option->sni_; auto remote_id = option->ingress_ ? option->identity_ : destination_identity; - auto port_policy = policy->findPortPolicy(option->ingress_, destination_port); + auto port_policy = + option->initial_policy_->findPortPolicy(option->ingress_, destination_port); const Envoy::Ssl::ContextConfig* config = nullptr; bool raw_socket_allowed = false; Envoy::Ssl::ContextSharedPtr ctx = @@ -95,7 +91,7 @@ class SslSocketWrapper : public Network::TransportSocket { // Set the callbacks socket_->setTransportSocketCallbacks(callbacks); } else { - policy->tlsWrapperMissingPolicyInc(); + option->initial_policy_->tlsWrapperMissingPolicyInc(); std::string ipStr(""); if (option->ingress_) { @@ -113,12 +109,11 @@ class SslSocketWrapper : public Network::TransportSocket { ipStr = dip->addressAsString(); } } - ENVOY_LOG_MISC( - warn, - "cilium.tls_wrapper: Could not get {} TLS context for pod {} on {} IP {} (id {}) port " - "{} sni \"{}\" and raw socket is not allowed", - is_client ? "client" : "server", option->pod_ip_, - option->ingress_ ? "source" : "destination", ipStr, remote_id, destination_port, sni); + ENVOY_LOG_MISC(warn, + "cilium.tls_wrapper: Could not get {} TLS context for {} IP {} (id {}) port " + "{} sni \"{}\" and raw socket is not allowed", + is_client ? "client" : "server", option->ingress_ ? "source" : "destination", + ipStr, remote_id, destination_port, sni); } } else { ENVOY_LOG_MISC(warn, "cilium.tls_wrapper: Can not correlate connection with Cilium Network " diff --git a/tests/bpf_metadata.cc b/tests/bpf_metadata.cc index 8f9beda26..765532c82 100644 --- a/tests/bpf_metadata.cc +++ b/tests/bpf_metadata.cc @@ -183,9 +183,9 @@ Cilium::SocketOptionSharedPtr TestConfig::getMetadata(Network::ConnectionSocket& ENVOY_LOG_MISC(info, "setRequestedApplicationProtocols({})", l7proto); } - return std::make_shared(0, 0, source_identity, is_ingress_, is_l7lb_, port, - std::move(pod_ip), nullptr, nullptr, nullptr, - shared_from_this(), 0, ""); + return std::make_shared(policy, 0, 0, source_identity, is_ingress_, + is_l7lb_, port, std::move(pod_ip), nullptr, nullptr, + nullptr, shared_from_this(), 0, ""); } } // namespace BpfMetadata diff --git a/tests/metadata_config_test.cc b/tests/metadata_config_test.cc index 62d48d558..68c5bd422 100644 --- a/tests/metadata_config_test.cc +++ b/tests/metadata_config_test.cc @@ -263,6 +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_EQ(0, option->ingress_source_identity_); // Check that Ingress security ID is used in the socket mark @@ -295,13 +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_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->getPolicy()->findPortPolicy(true, 80); + auto port_policy = option->initial_policy_->findPortPolicy(true, 80); EXPECT_TRUE(port_policy.allowed(12345678, "")); } @@ -331,13 +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_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->getPolicy()->findPortPolicy(true, 80); + auto port_policy = option->initial_policy_->findPortPolicy(true, 80); EXPECT_FALSE(port_policy.allowed(2, "")); } @@ -384,6 +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_); // Check that Endpoint's ID is used in the socket mark EXPECT_TRUE((option->mark_ & 0xffff) == 0x0900 && (option->mark_ >> 16) == 2048); @@ -413,6 +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_EQ(0, option->ingress_source_identity_); // Check that Ingress ID is used in the socket mark