Apply Disable ExtAuth from GlobalExtAuth and Remove Auth from HTTP Up…

…grade (#6661) * fix #6617 and #6659 Changes: - use dagRoute's AuthContext and AuthDisabled in HTTPS-Upgrade to fix 6659 - Use globalExtAuth.AuthPolicy.Disabled to calculate dagRoute.AuthDisabled - Fix Tests Signed-off-by: Saman Mahdanian <[email protected]> * add changelog Signed-off-by: Saman Mahdanian <[email protected]> * fix indentation Signed-off-by: Saman Mahdanian <[email protected]> * fix indentation in route.go Signed-off-by: Saman Mahdanian <[email protected]> * disable ext_auth when upgrading to HTTPS Signed-off-by: Saman Mahdanian <[email protected]> * fix tests for "disable ext_auth when upgrading to HTTPS" Signed-off-by: Saman Mahdanian <[email protected]> * fix CHANGELOG for "disable ext_auth when upgrading to HTTPS" Signed-off-by: Saman Mahdanian <[email protected]> * add tests for globalExtAuth.AuthPolicy.disabled proper behaviour Signed-off-by: Saman Mahdanian <[email protected]> * fix gofumpt issue with global_authorization_test.go Signed-off-by: Saman Mahdanian <[email protected]> * fix nil authorization extref issue while overwriting GlobalExtAuth Signed-off-by: Saman Mahdanian <[email protected]> * fix linting issue Signed-off-by: Saman Mahdanian <[email protected]> * Update changelogs/unreleased/6661-SamMHD-minor.md Signed-off-by: Saman Mahdanian <[email protected]> --------- Signed-off-by: Saman Mahdanian <[email protected]>
projectcontour · Nov 22, 2024 · 3589fc0 · 3589fc0
1 parent 4607ac2
commit 3589fc0
Show file tree

Hide file tree

Showing 11 changed files with 408 additions and 206 deletions.
diff --git a/apis/projectcontour/v1/helpers.go b/apis/projectcontour/v1/helpers.go
@@ -40,6 +40,11 @@ func (v *VirtualHost) DisableAuthorization() bool {
 	return false
 }
 
+// IsConfigured returns whether service ref is configured
+func (r *ExtensionServiceReference) IsConfigured() bool {
+	return r.Name != ""
+}
+
 // AuthorizationContext returns the authorization policy context (if present).
 func (v *VirtualHost) AuthorizationContext() map[string]string {
 	if v.AuthorizationConfigured() {

diff --git a/changelogs/unreleased/6661-SamMHD-minor.md b/changelogs/unreleased/6661-SamMHD-minor.md
@@ -0,0 +1,10 @@
+## Disable ExtAuth by default if GlobalExtAuth.AuthPolicy.Disabled is set
+
+Global external authorization can now be disabled by default and enabled by overriding the vhost and route level auth policies.
+This is achieved by setting the `globalExtAuth.authPolicy.disabled` in the configuration file or `ContourConfiguration` CRD to `true`, and setting the `authPolicy.disabled` to `false` in the vhost and route level auth policies.
+The final authorization state is determined by the most specific policy applied at the route level.
+
+## Disable External Authorization in HTTPS Upgrade
+
+When external authorization is enabled, no authorization check will be performed for HTTP to HTTPS redirection.
+Previously, external authorization was checked before redirection, which could result in a 401 Unauthorized error instead of a 301 Moved Permanently status code.
diff --git a/internal/dag/httpproxy_processor.go b/internal/dag/httpproxy_processor.go
@@ -852,16 +852,31 @@ func (p *HTTPProxyProcessor) computeRoutes(
 		// enable it on the route and propagate defaults
 		// downwards.
 		if rootProxy.Spec.VirtualHost.AuthorizationConfigured() || p.GlobalExternalAuthorization != nil {
+			// Global external or vhost-level authorization is enabled by default
+			// unless an AuthPolicy explicitly disables it. By default, `disabled`
+			// is set to false, meaning authorization is active. This global setting
+			// can be overridden by vhost-level AuthPolicy, which can further be
+			// overridden by route-specific AuthPolicy.
+			// Therefore, the final authorization state is determined by the
+			// most specific policy applied at the route level.
+			disabled := false
+
+			if p.GlobalExternalAuthorization != nil && p.GlobalExternalAuthorization.AuthPolicy != nil {
+				disabled = p.GlobalExternalAuthorization.AuthPolicy.Disabled
+			}
+
 			// When the ext_authz filter is added to a
 			// vhost, it is in enabled state, but we can
 			// disable it per route. We emulate disabling
 			// it at the vhost layer by defaulting the state
 			// from the root proxy.
-			disabled := rootProxy.Spec.VirtualHost.DisableAuthorization()
+			if rootProxy.Spec.VirtualHost.AuthorizationConfigured() {
+				disabled = rootProxy.Spec.VirtualHost.DisableAuthorization()
+			}
 
 			// Take the default for enabling authorization
-			// from the virtual host. If this route has a
-			// policy, let that override.
+			// from the virtualhost/global-extauth. If this
+			// route has a policy, let that override.
 			if route.AuthPolicy != nil {
 				disabled = route.AuthPolicy.Disabled
 			}
@@ -1441,14 +1456,14 @@ func determineExternalAuthTimeout(responseTimeout string, validCond *contour_v1.
 }
 
 func (p *HTTPProxyProcessor) computeSecureVirtualHostAuthorization(validCond *contour_v1.DetailedCondition, httpproxy *contour_v1.HTTPProxy, svhost *SecureVirtualHost) bool {
-	if httpproxy.Spec.VirtualHost.AuthorizationConfigured() && !httpproxy.Spec.VirtualHost.DisableAuthorization() {
+	if httpproxy.Spec.VirtualHost.AuthorizationConfigured() && !httpproxy.Spec.VirtualHost.DisableAuthorization() && httpproxy.Spec.VirtualHost.Authorization.ExtensionServiceRef.IsConfigured() {
 		authorization := p.computeVirtualHostAuthorization(httpproxy.Spec.VirtualHost.Authorization, validCond, httpproxy)
 		if authorization == nil {
 			return false
 		}
 
 		svhost.ExternalAuthorization = authorization
-	} else if p.GlobalExternalAuthorization != nil && !httpproxy.Spec.VirtualHost.DisableAuthorization() {
+	} else if p.GlobalExternalAuthorization != nil {
 		globalAuthorization := p.computeVirtualHostAuthorization(p.GlobalExternalAuthorization, validCond, httpproxy)
 		if globalAuthorization == nil {
 			return false

diff --git a/internal/envoy/v3/route.go b/internal/envoy/v3/route.go
@@ -121,14 +121,18 @@ func buildRoute(dagRoute *dag.Route, vhostName string, secure bool) *envoy_confi
 		// envoy.RouteRoute. Currently the DAG processor adds any HTTP->HTTPS
 		// redirect routes to *both* the insecure and secure vhosts.
 		route.Action = UpgradeHTTPS()
+
+		// Disable External Authorization it is being redirected to HTTPS route
+		route.TypedPerFilterConfig = map[string]*anypb.Any{}
+		route.TypedPerFilterConfig[ExtAuthzFilterName] = routeAuthzDisabled()
 	case dagRoute.DirectResponse != nil:
 		route.TypedPerFilterConfig = map[string]*anypb.Any{}
 
 		// Apply per-route authorization policy modifications.
 		if dagRoute.AuthDisabled {
-			route.TypedPerFilterConfig["envoy.filters.http.ext_authz"] = routeAuthzDisabled()
+			route.TypedPerFilterConfig[ExtAuthzFilterName] = routeAuthzDisabled()
 		} else if len(dagRoute.AuthContext) > 0 {
-			route.TypedPerFilterConfig["envoy.filters.http.ext_authz"] = routeAuthzContext(dagRoute.AuthContext)
+			route.TypedPerFilterConfig[ExtAuthzFilterName] = routeAuthzContext(dagRoute.AuthContext)
 		}
 
 		route.Action = routeDirectResponse(dagRoute.DirectResponse)
@@ -592,6 +596,19 @@ func UpgradeHTTPS() *envoy_config_route_v3.Route_Redirect {
 	}
 }
 
+// DisabledExtAuthConfig returns a route TypedPerFilterConfig that disables ExtAuth
+func DisabledExtAuthConfig() map[string]*anypb.Any {
+	return map[string]*anypb.Any{
+		ExtAuthzFilterName: protobuf.MustMarshalAny(
+			&envoy_filter_http_ext_authz_v3.ExtAuthzPerRoute{
+				Override: &envoy_filter_http_ext_authz_v3.ExtAuthzPerRoute_Disabled{
+					Disabled: true,
+				},
+			},
+		),
+	}
+}
+
 // headerValueList creates a list of Envoy HeaderValueOptions from the provided map.
 func headerValueList(hvm map[string]string, app bool) []*envoy_config_core_v3.HeaderValueOption {
 	var hvs []*envoy_config_core_v3.HeaderValueOption

diff --git a/internal/featuretests/v3/authorization_test.go b/internal/featuretests/v3/authorization_test.go
@@ -267,13 +267,6 @@ func authzOverrideDisabled(t *testing.T, rh ResourceEventHandlerWrapper, c *Cont
 	// same authorization enablement as the root proxy, and
 	// the other path should have the opposite enablement.
 
-	disabledConfig := withFilterConfig(envoy_v3.ExtAuthzFilterName,
-		&envoy_filter_http_ext_authz_v3.ExtAuthzPerRoute{
-			Override: &envoy_filter_http_ext_authz_v3.ExtAuthzPerRoute_Disabled{
-				Disabled: true,
-			},
-		})
-
 	c.Request(routeType).Equals(&envoy_service_discovery_v3.DiscoveryResponse{
 		TypeUrl: routeType,
 		Resources: resources(t,
@@ -287,7 +280,7 @@ func authzOverrideDisabled(t *testing.T, rh ResourceEventHandlerWrapper, c *Cont
 					&envoy_config_route_v3.Route{
 						Match:                routePrefix("/default"),
 						Action:               routeCluster("default/app-server/80/da39a3ee5e"),
-						TypedPerFilterConfig: disabledConfig,
+						TypedPerFilterConfig: envoy_v3.DisabledExtAuthConfig(),
 					},
 				),
 			),
@@ -297,7 +290,7 @@ func authzOverrideDisabled(t *testing.T, rh ResourceEventHandlerWrapper, c *Cont
 					&envoy_config_route_v3.Route{
 						Match:                routePrefix("/disabled"),
 						Action:               routeCluster("default/app-server/80/da39a3ee5e"),
-						TypedPerFilterConfig: disabledConfig,
+						TypedPerFilterConfig: envoy_v3.DisabledExtAuthConfig(),
 					},
 					&envoy_config_route_v3.Route{
 						Match:  routePrefix("/default"),
@@ -309,22 +302,26 @@ func authzOverrideDisabled(t *testing.T, rh ResourceEventHandlerWrapper, c *Cont
 				"ingress_http",
 				envoy_v3.VirtualHost(disabled,
 					&envoy_config_route_v3.Route{
-						Match:  routePrefix("/enabled"),
-						Action: withRedirect(),
+						Match:                routePrefix("/enabled"),
+						Action:               withRedirect(),
+						TypedPerFilterConfig: envoy_v3.DisabledExtAuthConfig(),
 					},
 					&envoy_config_route_v3.Route{
-						Match:  routePrefix("/default"),
-						Action: withRedirect(),
+						Match:                routePrefix("/default"),
+						Action:               withRedirect(),
+						TypedPerFilterConfig: envoy_v3.DisabledExtAuthConfig(),
 					},
 				),
 				envoy_v3.VirtualHost(enabled,
 					&envoy_config_route_v3.Route{
-						Match:  routePrefix("/disabled"),
-						Action: withRedirect(),
+						Match:                routePrefix("/disabled"),
+						Action:               withRedirect(),
+						TypedPerFilterConfig: envoy_v3.DisabledExtAuthConfig(),
 					},
 					&envoy_config_route_v3.Route{
-						Match:  routePrefix("/default"),
-						Action: withRedirect(),
+						Match:                routePrefix("/default"),
+						Action:               withRedirect(),
+						TypedPerFilterConfig: envoy_v3.DisabledExtAuthConfig(),
 					},
 				),
 			),
@@ -406,8 +403,9 @@ func authzMergeRouteContext(t *testing.T, rh ResourceEventHandlerWrapper, c *Con
 				"ingress_http",
 				envoy_v3.VirtualHost(fqdn,
 					&envoy_config_route_v3.Route{
-						Match:  routePrefix("/"),
-						Action: withRedirect(),
+						Match:                routePrefix("/"),
+						Action:               withRedirect(),
+						TypedPerFilterConfig: envoy_v3.DisabledExtAuthConfig(),
 					},
 				),
 			),
@@ -433,8 +431,8 @@ func authzInvalidReference(t *testing.T, rh ResourceEventHandlerWrapper, c *Cont
 
 	invalid.Spec.VirtualHost.Authorization.ExtensionServiceRef = contour_v1.ExtensionServiceReference{
 		APIVersion: "foo/bar",
-		Namespace:  "",
-		Name:       "",
+		Namespace:  "missing",
+		Name:       "extension",
 	}
 
 	rh.OnDelete(invalid)

diff --git a/internal/featuretests/v3/envoy.go b/internal/featuretests/v3/envoy.go
@@ -166,8 +166,9 @@ func routeHostRewriteHeader(cluster, hostnameHeader string) *envoy_config_route_
 
 func upgradeHTTPS(match *envoy_config_route_v3.RouteMatch) *envoy_config_route_v3.Route {
 	return &envoy_config_route_v3.Route{
-		Match:  match,
-		Action: envoy_v3.UpgradeHTTPS(),
+		Match:                match,
+		Action:               envoy_v3.UpgradeHTTPS(),
+		TypedPerFilterConfig: envoy_v3.DisabledExtAuthConfig(),
 	}
 }