Repository: envoyproxy/gateway
Pull request #3869 (2024-07-16 08:50:51)

return 500 error for failed SecurityPolicies to avoid unauthorized access to xRoutes
The current implementation of SecurityPolicy translation is in a best-effort manner, which means even if the translation of SecurityPolicy fails, the targeted xRoute will still be translated to xDS. This would result in unauthorized access to the xRoute.

This PR adds a 500 error direct response to the targeted xRoute when the SecurityPolicy translation fails.

fix https://github.com/envoyproxy/gateway/issues/2837

A `FailOpen` knob can be added to the SecurityPolicy API later if needed.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/envoyproxy/gateway/pull/3869?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 69.25%. Comparing base [(`435a4dc`)](https://app.codecov.io/gh/envoyproxy/gateway/commit/435a4dc10f92349c7c6951dc8077ad911513ee2b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) to head [(`24f224d`)](https://app.codecov.io/gh/envoyproxy/gateway/commit/24f224d1e848e21f8c9c6f83c70a9e48e95704ee?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy).

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #3869      +/-   ##
==========================================
+ Coverage   69.24%   69.25%   +0.01%     
==========================================
  Files         176      176              
  Lines       21740    21750      +10     
==========================================
+ Hits        15054    15064      +10     
  Misses       5594     5594              
  Partials     1092     1092              
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/envoyproxy/gateway/pull/3869?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy).

Xunzhuo: /retest
zhaohuabing: > Is this approach reasonable? Since this is a SecurityPolicy, should we accpet the best-effort logic to Security related features?

It's reasonable because the current best-effort logic allows unauthorized access to the protected resources, which is a security risk.  A FailOpen knob can be added to the SecurityPolicy API later if needed.
Xunzhuo: @zhaohuabing make senses, thanks for clarifying 
arkodg: hey @zhaohuabing should we create a seperate issue for BackendTrafficPolicy ?
arkodg: hey @zhaohuabing can we also add a e2e for this ?

Patch:
diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go
index 57153bc7e..9a0fefbaf 100644
--- a/internal/gatewayapi/securitypolicy.go
+++ b/internal/gatewayapi/securitypolicy.go
@@ -394,8 +394,6 @@ func (t *Translator) translateSecurityPolicyForRoute(
 	}
 
 	// Apply IR to all relevant routes
-	// Note: there are multiple features in a security policy, even if some of them
-	// are invalid, we still want to apply the valid ones.
 	prefix := irRoutePrefix(route)
 	parentRefs := GetParentReferences(route)
 	for _, p := range parentRefs {
@@ -430,6 +428,12 @@ func (t *Translator) translateSecurityPolicyForRoute(
 							ExtAuth:       extAuth,
 							Authorization: authorization,
 						}
+						if errs != nil {
+							// Return a 500 direct response to avoid unauthorized access
+							r.DirectResponse = &ir.DirectResponse{
+								StatusCode: 500,
+							}
+						}
 					}
 				}
 			}
@@ -523,7 +527,6 @@ func (t *Translator) translateSecurityPolicyForGateway(
 			if r.Security != nil {
 				continue
 			}
-
 			r.Security = &ir.SecurityFeatures{
 				CORS:          cors,
 				JWT:           jwt,
@@ -532,6 +535,12 @@ func (t *Translator) translateSecurityPolicyForGateway(
 				ExtAuth:       extAuth,
 				Authorization: authorization,
 			}
+			if errs != nil {
+				// Return a 500 direct response to avoid unauthorized access
+				r.DirectResponse = &ir.DirectResponse{
+					StatusCode: 500,
+				}
+			}
 		}
 	}
 	return errs


Test patch:
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-matching-port.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-matching-port.out.yaml
index 6494f2d68..c5bf4237f 100644
--- a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-matching-port.out.yaml
+++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-matching-port.out.yaml
@@ -160,6 +160,8 @@ xdsIR:
               port: 8080
             protocol: HTTP
             weight: 1
+        directResponse:
+          statusCode: 500
         hostname: www.foo.com
         isHTTP2: false
         metadata:
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-port.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-port.out.yaml
index 619025bcf..3f5e60f11 100644
--- a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-port.out.yaml
+++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-port.out.yaml
@@ -160,6 +160,8 @@ xdsIR:
               port: 8080
             protocol: HTTP
             weight: 1
+        directResponse:
+          statusCode: 500
         hostname: www.foo.com
         isHTTP2: false
         metadata:
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-reference-grant.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-reference-grant.out.yaml
index e3f8f65f7..1f8fd280a 100644
--- a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-reference-grant.out.yaml
+++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-reference-grant.out.yaml
@@ -161,6 +161,8 @@ xdsIR:
               port: 8080
             protocol: HTTP
             weight: 1
+        directResponse:
+          statusCode: 500
         hostname: www.foo.com
         isHTTP2: false
         metadata:
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-service.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-service.out.yaml
index 3b63963bc..294267b90 100644
--- a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-service.out.yaml
+++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-service.out.yaml
@@ -160,6 +160,8 @@ xdsIR:
               port: 8080
             protocol: HTTP
             weight: 1
+        directResponse:
+          statusCode: 500
         hostname: www.foo.com
         isHTTP2: false
         metadata:
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-jwt-and-invalid-oidc.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-jwt-and-invalid-oidc.out.yaml
index 3ad2f4f10..d5731870d 100644
--- a/internal/gatewayapi/testdata/securitypolicy-with-jwt-and-invalid-oidc.out.yaml
+++ b/internal/gatewayapi/testdata/securitypolicy-with-jwt-and-invalid-oidc.out.yaml
@@ -258,6 +258,8 @@ xdsIR:
               port: 8080
             protocol: HTTP
             weight: 1
+        directResponse:
+          statusCode: 500
         hostname: gateway.envoyproxy.io
         isHTTP2: false
         metadata:
@@ -290,6 +292,8 @@ xdsIR:
               port: 8080
             protocol: HTTP
             weight: 1
+        directResponse:
+          statusCode: 500
         hostname: gateway.envoyproxy.io
         isHTTP2: false
         metadata:
diff --git a/test/e2e/testdata/securitypolicy-translation-failed.yaml b/test/e2e/testdata/securitypolicy-translation-failed.yaml
new file mode 100644
index 000000000..01cdec37b
--- /dev/null
+++ b/test/e2e/testdata/securitypolicy-translation-failed.yaml
@@ -0,0 +1,39 @@
+---
+apiVersion: gateway.networking.k8s.io/v1
+kind: HTTPRoute
+metadata:
+  name: http-with-oidc
+  namespace: gateway-conformance-infra
+spec:
+  parentRefs:
+  - name: same-namespace
+  hostnames: ["www.example.com"]
+  rules:
+  - matches:
+    - path:
+        type: PathPrefix
+        value: /myapp              # This is the path that will be protected by OIDC
+    backendRefs:
+    - name: infra-backend-v1
+      port: 8080
+---
+apiVersion: gateway.envoyproxy.io/v1alpha1
+kind: SecurityPolicy
+metadata:
+  name: oidc-test
+  namespace: gateway-conformance-infra
+spec:
+  targetRefs:
+  - group: gateway.networking.k8s.io
+    kind: HTTPRoute
+    name: http-with-oidc
+  oidc:
+    provider:
+      issuer: "http://keycloak.gateway-conformance-infra/realms/master"
+      authorizationEndpoint: "http://keycloak.gateway-conformance-infra/realms/master/protocol/openid-connect/auth"
+      tokenEndpoint: "http://keycloak.gateway-conformance-infra/realms/master/protocol/openid-connect/token"
+    clientID: "oidctest"
+    clientSecret:
+      name: "oidctest-secret"
+    redirectURL: "http://www.example.com/myapp/oauth2/callback"
+    logoutPath: "/myapp/logout"
diff --git a/test/e2e/tests/securitypolicy_transaltion_failed.go b/test/e2e/tests/securitypolicy_transaltion_failed.go
new file mode 100644
index 000000000..539fd4f72
--- /dev/null
+++ b/test/e2e/tests/securitypolicy_transaltion_failed.go
@@ -0,0 +1,62 @@
+// Copyright Envoy Gateway Authors
+// SPDX-License-Identifier: Apache-2.0
+// The full text of the Apache license is available in the LICENSE file at
+// the root of the repo.
+
+//go:build e2e
+// +build e2e
+
+package tests
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/types"
+	gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
+	gwapiv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
+	"sigs.k8s.io/gateway-api/conformance/utils/http"
+	"sigs.k8s.io/gateway-api/conformance/utils/kubernetes"
+	"sigs.k8s.io/gateway-api/conformance/utils/suite"
+
+	"github.com/envoyproxy/gateway/internal/gatewayapi"
+)
+
+func init() {
+	ConformanceTests = append(ConformanceTests, FailedSecurityPolicyDirectResponseTest)
+}
+
+// FailedSecurityPolicyDirectResponseTest tests the direct 500 response for HTTPRoute targeted by a failed SecurityPolicy.
+var FailedSecurityPolicyDirectResponseTest = suite.ConformanceTest{
+	ShortName:   "FailedSecurityPolicyDirectResponse",
+	Description: "Test direct 500 response when failed to translate SecurityPolicy",
+	Manifests:   []string{"testdata/securitypolicy-translation-failed.yaml"},
+	Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
+		t.Run("http route with failed SecurityPolicy", func(t *testing.T) {
+			ns := "gateway-conformance-infra"
+			routeNN := types.NamespacedName{Name: "http-with-oidc", Namespace: ns}
+			gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns}
+			gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)
+
+			ancestorRef := gwapiv1a2.ParentReference{
+				Group:     gatewayapi.GroupPtr(gwapiv1.GroupName),
+				Kind:      gatewayapi.KindPtr(gatewayapi.KindGateway),
+				Namespace: gatewayapi.NamespacePtr(gwNN.Namespace),
+				Name:      gwapiv1.ObjectName(gwNN.Name),
+			}
+			SecurityPolicyMustFail(t, suite.Client, types.NamespacedName{Name: "oidc-test", Namespace: ns}, suite.ControllerName, ancestorRef, "")
+
+			expectedResponse := http.ExpectedResponse{
+				Request: http.Request{
+					Host: "www.example.com",
+					Path: "/myapp",
+				},
+				Response: http.Response{
+					StatusCode: 500,
+				},
+				Namespace: ns,
+			}
+
+			http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectedResponse)
+		})
+	},
+}
diff --git a/test/e2e/tests/utils.go b/test/e2e/tests/utils.go
index 33d215168..d620a9baa 100644
--- a/test/e2e/tests/utils.go
+++ b/test/e2e/tests/utils.go
@@ -104,6 +104,33 @@ func SecurityPolicyMustBeAccepted(t *testing.T, client client.Client, policyName
 	require.NoErrorf(t, waitErr, "error waiting for SecurityPolicy to be accepted")
 }
 
+// SecurityPolicyMustFail waits for an SecurityPolicy to fail with the specified reason.
+func SecurityPolicyMustFail(
+	t *testing.T, client client.Client, policyName types.NamespacedName,
+	controllerName string, ancestorRef gwapiv1a2.ParentReference, message string,
+) {
+	t.Helper()
+
+	policy := &egv1a1.SecurityPolicy{}
+	waitErr := wait.PollUntilContextTimeout(
+		context.Background(), 1*time.Second, 60*time.Second,
+		true, func(ctx context.Context) (bool, error) {
+			err := client.Get(ctx, policyName, policy)
+			if err != nil {
+				return false, fmt.Errorf("error fetching SecurityPolicy: %w", err)
+			}
+
+			if policyFailAcceptedByAncestor(policy.Status.Ancestors, controllerName, ancestorRef, message) {
+				t.Logf("SecurityPolicy has been failed: %v", policy)
+				return true, nil
+			}
+
+			return false, nil
+		})
+
+	require.NoErrorf(t, waitErr, "error waiting for SecurityPolicy to fail with message: %s policy %v", message, policy)
+}
+
 // BackendTrafficPolicyMustBeAccepted waits for the specified BackendTrafficPolicy to be accepted.
 func BackendTrafficPolicyMustBeAccepted(t *testing.T, client client.Client, policyName types.NamespacedName, controllerName string, ancestorRef gwapiv1a2.ParentReference) {
 	t.Helper()