Skip to content

Commit

Permalink
SocketOption: Only store a weak policy reference
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@jrajahalme
jrajahalme committed Nov 24, 2024
1 parent 930ea44 commit 63d0b4a
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 18 deletions.
11 changes: 9 additions & 2 deletions cilium/network_filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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,
Expand All @@ -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)) {
Expand Down
22 changes: 16 additions & 6 deletions cilium/socket_option.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,16 @@ namespace Cilium {

class PolicyInstance;
using PolicyInstanceConstSharedPtr = std::shared_ptr<const PolicyInstance>;
using PolicyInstanceConstWeakPtr = std::weak_ptr<const PolicyInstance>;

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;
};

Expand Down Expand Up @@ -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,
Expand All @@ -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 "
Expand All @@ -244,14 +250,18 @@ 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);
}

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_);
}

Expand All @@ -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<PolicyResolver> policy_id_resolver_;
Expand Down
12 changes: 9 additions & 3 deletions cilium/tls_wrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down Expand Up @@ -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 =
Expand All @@ -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_) {
Expand Down
14 changes: 7 additions & 7 deletions tests/metadata_config_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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, ""));
}

Expand Down Expand Up @@ -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, ""));
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 63d0b4a

Please sign in to comment.