policy: Drop on missing SDS secret and no default value inline

When a header is matched based on a SDS secret and there is no inline value to use as a backup, the verdict should be deny if the secret is missing. Also, if the secret value is empty, then the match should be a presence match only. Signed-off-by: Jarno Rajahalme <[email protected]>
cilium · Oct 29, 2024 · 5e76843 · 5e76843
1 parent 3dc90ef
commit 5e76843
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 42 deletions.
diff --git a/cilium/network_policy.cc b/cilium/network_policy.cc
@@ -63,25 +63,28 @@ class HeaderMatch : public Logger::Loggable<Logger::Id::config> {
     bool matches = false;
     const std::string* match_value = &value_;
     const auto header_value = Http::HeaderUtility::getAllOfHeaderAsString(headers, name_);
-    bool isPresentMatch = (value_.length() == 0 && !secret_);
 
+    // Get secret value?
+    if (secret_) {
+      auto* secret_value = secret_->value();
+      if (secret_value)
+        match_value = secret_value;
+      else if (value_.length() == 0) {
+        // fail if secret has no value and the inline value to match is also empty
+        ENVOY_LOG(info, "Cilium HeaderMatch missing SDS secret value for header {}", name_);
+        return false;
+      }
+    }
+
+    // Perform presence match if the value to match is empty
+    bool isPresentMatch = match_value->length() == 0;
     if (isPresentMatch)
       matches = header_value.result().has_value();
-    else {
-      // Value match, update secret?
-      if (secret_) {
-        auto* secret_value = secret_->value();
-        if (secret_value)
-          match_value = secret_value;
-        else if (value_.length() == 0)
-          ENVOY_LOG(info, "Cilium HeaderMatch missing SDS secret value for header {}", name_);
-      }
-      if (header_value.result().has_value()) {
-        const absl::string_view val = header_value.result().value();
-        if (val.length() == match_value->length()) {
-          // Use constant time comparison for security reason
-          matches = CRYPTO_memcmp(val.data(), match_value->data(), match_value->length()) == 0;
-        }
+    else if (header_value.result().has_value()) {
+      const absl::string_view val = header_value.result().value();
+      if (val.length() == match_value->length()) {
+        // Use constant time comparison for security reason
+        matches = CRYPTO_memcmp(val.data(), match_value->data(), match_value->length()) == 0;
       }
     }
 
@@ -137,7 +140,7 @@ class HeaderMatch : public Logger::Loggable<Logger::Id::config> {
       }
     }
     IS_ENVOY_BUG("HeaderMatch reached unreachable return");
-    return true;
+    return false;
   }
 
   void toString(int indent, std::string& res) const {

diff --git a/tests/cilium_http_integration_test.cc b/tests/cilium_http_integration_test.cc
@@ -1179,9 +1179,6 @@ TEST_P(SDSIntegrationTest, TestMissingSDSSecretOnUpdate) {
 
   // Validate that missing headers are access logged correctly
   EXPECT_TRUE(expectAccessLogRequestTo([](const ::cilium::LogEntry& entry) {
-    auto source_ip = Network::Utility::parseInternetAddressAndPort(entry.source_address())
-                         ->ip()
-                         ->addressAsString();
     const auto& http = entry.http();
 
     ENVOY_LOG_MISC(info, "Access Log entry: {}", entry.DebugString());
@@ -1206,20 +1203,17 @@ TEST_P(SDSIntegrationTest, TestMissingSDSSecretOnUpdate) {
   absl::SleepFor(absl::Milliseconds(100));
 
   // 2nd round, on updated policy
-  Accepted({{":method", "GET"}, {":path", "/allowed"}, {":authority", "host"}});
+  Denied({{":method", "GET"}, {":path", "/allowed"}, {":authority", "host"}});
 
   // Validate that missing headers are access logged correctly
-  EXPECT_TRUE(expectAccessLogRequestTo([](const ::cilium::LogEntry& entry) {
-    auto source_ip = Network::Utility::parseInternetAddressAndPort(entry.source_address())
-                         ->ip()
-                         ->addressAsString();
+  EXPECT_TRUE(expectAccessLogDeniedTo([](const ::cilium::LogEntry& entry) {
     const auto& http = entry.http();
     const auto& missing = http.missing_headers();
 
     ENVOY_LOG_MISC(info, "Access Log entry: {}", entry.DebugString());
 
-    return http.rejected_headers_size() == 0 && http.missing_headers_size() == 2 &&
-           hasHeader(missing, "header42") && hasHeader(missing, "bearer-token", "[redacted]");
+    return http.rejected_headers_size() == 0 && http.missing_headers_size() == 1 &&
+           hasHeader(missing, "header42");
   }));
 
   // 3rd round back to the initial policy
@@ -1236,9 +1230,6 @@ TEST_P(SDSIntegrationTest, TestMissingSDSSecretOnUpdate) {
 
   // Validate that missing headers are access logged correctly
   EXPECT_TRUE(expectAccessLogDeniedTo([](const ::cilium::LogEntry& entry) {
-    auto source_ip = Network::Utility::parseInternetAddressAndPort(entry.source_address())
-                         ->ip()
-                         ->addressAsString();
     const auto& http = entry.http();
 
     ENVOY_LOG_MISC(info, "Access Log entry: {}", entry.DebugString());

diff --git a/tests/cilium_http_upstream_integration_test.cc b/tests/cilium_http_upstream_integration_test.cc
@@ -873,17 +873,14 @@ TEST_P(SDSIntegrationTest, TestMissingSDSSecretOnUpdate) {
   absl::SleepFor(absl::Milliseconds(100));
 
   // 2nd round, on updated policy
-  Accepted({{":method", "GET"}, {":path", "/allowed"}, {":authority", "host"}});
+  Denied({{":method", "GET"}, {":path", "/allowed"}, {":authority", "host"}});
 
   // Validate that missing headers are access logged correctly
-  EXPECT_TRUE(expectAccessLogRequestTo([](const ::cilium::LogEntry& entry) {
-    auto source_ip = Network::Utility::parseInternetAddressAndPort(entry.source_address())
-                         ->ip()
-                         ->addressAsString();
+  EXPECT_TRUE(expectAccessLogDeniedTo([](const ::cilium::LogEntry& entry) {
     const auto& http = entry.http();
     const auto& missing = http.missing_headers();
-    return http.rejected_headers_size() == 0 && http.missing_headers_size() == 2 &&
-           hasHeader(missing, "header42") && hasHeader(missing, "bearer-token", "[redacted]");
+    return http.rejected_headers_size() == 0 && http.missing_headers_size() == 1 &&
+           hasHeader(missing, "header42");
   }));
 
   // 3rd round back to the initial policy
@@ -900,9 +897,6 @@ TEST_P(SDSIntegrationTest, TestMissingSDSSecretOnUpdate) {
 
   // Validate that missing headers are access logged correctly
   EXPECT_TRUE(expectAccessLogDeniedTo([](const ::cilium::LogEntry& entry) {
-    auto source_ip = Network::Utility::parseInternetAddressAndPort(entry.source_address())
-                         ->ip()
-                         ->addressAsString();
     const auto& http = entry.http();
     return http.rejected_headers_size() == 0 && http.missing_headers_size() == 0;
   }));

diff --git a/tests/cilium_network_policy_test.cc b/tests/cilium_network_policy_test.cc
@@ -1097,8 +1097,8 @@ TEST_F(CiliumNetworkPolicyTest, HttpPolicyUpdateToMissingSDS) {
 )EOF"));
   EXPECT_EQ(version, "2");
   EXPECT_TRUE(policy_map_->exists("10.1.2.3"));
-  // Allowed remote ID, port, & path:
-  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 80, {{":path", "/allowed"}}));
+  // Drop due to the missing SDS secret
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 80, {{":path", "/allowed"}}));
   // Wrong remote ID:
   EXPECT_FALSE(IngressAllowed("10.1.2.3", 40, 80, {{":path", "/allowed"}}));
   // Wrong port: