fix: disable ext_authz filter on https redirect route

Fix regression introduced in v3 series when Envoy was upgraded to a
version after 1.20 (behavior change introduced). This commit restores
the expected behavior that an https redirect will occur prior to
calling the ext_authz service.

fixes #4620

Signed-off-by: Lance Austin <laustin@datawire.io>

.github/actions/collect-logs/action.yml

@@ -39,7 +39,9 @@ runs:
           tools/bin/kubectl describe pods --all-namespaces >/tmp/test-logs/cluster/all-pods-described.txt || true
 
           tools/bin/kubectl get pods --all-namespaces -ocustom-columns="name:.metadata.name,namespace:.metadata.namespace" --no-headers | while read -r name namespace; do
+            echo "Pulling logs for ${name}.${namespace}"
             tools/bin/kubectl --namespace="$namespace" logs "$name" >"/tmp/test-logs/cluster/pod.${namespace}.${name}.log" || true
+            tools/bin/kubectl exec -n $namespace $name -- tar -czf /tmp/ambassador-config-base.tgz /tmp/ambassador --exclude=secrets-decoded --exclude=webui --exclude=sidecars && kubectl cp -n "$namespace" "$name":/tmp/ambassador-config-base.tgz /tmp/test-logs/cluster/${namespace}-${name}-pod-config-base.tgz || true
           done
 
           tools/bin/kubectl cp xfpredirect:/tmp/ambassador/snapshots /tmp/test-logs/cluster/xfpredirect.snapshots || true

CHANGELOG.md

@@ -93,6 +93,17 @@ it will be removed; but as it won't be user-visible this isn't considered a brea
 - Security: Updated Golang to 1.19.2 to address the CVEs: CVE-2022-2879, CVE-2022-2880,
   CVE-2022-41715.
 
+- Bugfix: By default Emissary-ingress adds routes for http to https redirection. When an AuthService
+  is applied in v2.Y of Emissary-ingress, Envoy would skip the ext_authz call for non-tls http
+  request and would perform the https  redirect. In Envoy 1.20+ the behavior has changed where Envoy
+  will  always call the ext_authz filter and must be disabled on a per route  basis. 
+  This new
+  behavior change introduced a regression in v3.0 of  Emissary-ingress when it was upgraded to Envoy
+  1.22. The http to https  redirection no longer works when an AuthService was applied. This fix 
+  restores the previous behavior by disabling the ext_authz call on the  https redirect routes. ([#4620])
+
+[#4620]: https://github.com/emissary-ingress/emissary/issues/4620
+
 ## [3.2.0] September 26, 2022
 [3.2.0]: https://github.com/emissary-ingress/emissary/compare/v3.1.0...v3.2.0
 

docs/releaseNotes.yml

@@ -41,6 +41,25 @@ items:
         body: >-
           Updated Golang to 1.19.2 to address the CVEs: CVE-2022-2879, CVE-2022-2880, CVE-2022-41715.
 
+      - title: Fix regression in http to https redirects with AuthService
+        type: bugfix
+        body: >-
+          By default $productName$ adds routes for http to https redirection. When
+          an AuthService is applied in v2.Y of $productName$, Envoy would skip the
+          ext_authz call for non-tls http request and would perform the https 
+          redirect. In Envoy 1.20+ the behavior has changed where Envoy will 
+          always call the ext_authz filter and must be disabled on a per route 
+          basis. 
+          
+          This new behavior change introduced a regression in v3.0 of 
+          $productName$ when it was upgraded to Envoy 1.22. The http to https 
+          redirection no longer works when an AuthService was applied. This fix 
+          restores the previous behavior by disabling the ext_authz call on the 
+          https redirect routes.
+        github:
+        - title: "#4620"
+          link: https://github.com/emissary-ingress/emissary/issues/4620
+
   - version: 3.2.0
     prevVersion: 3.1.0
     date: '2022-09-26'

python/ambassador/envoy/v3/v3route.py

@@ -238,6 +238,16 @@ def action_route(self, variant) -> None:
     def action_redirect(self, variant) -> None:
         variant.pop("route", None)
         variant["redirect"] = {"https_redirect": True}
+        for filter in self.route._group.ir.filters:
+            if filter.kind == "IRAuth":
+                # Required to ensure that the redirect occurs prior to calling ext_authz
+                # when an AuthService is applied
+                variant["typed_per_filter_config"] = {
+                    "envoy.filters.http.ext_authz": {
+                        "@type": "type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthzPerRoute",
+                        "disabled": True,
+                    }
+                }
 
 
 # Model an Envoy route.

python/tests/kat/t_extauth.py

@@ -961,7 +961,6 @@ def config(self) -> Generator[Union[str, Tuple[Node, str]], None, None]:
 - X-Foo
 - X-Bar
 - Extauth
-
 """
         )
         yield self, self.format(
@@ -1117,6 +1116,93 @@ def check(self):
         # are overridden, e.g. Authorization.
 
 
+class AuthenticationHTTPSRedirectTest(AmbassadorTest):
+    """
+    AuthenticationHTTPSREdirect: We expect that the https redirect from 8080 to 8443 will occur
+    and that the AuthenticationService will only be called on the TLS request.
+    """
+
+    target: ServiceType
+    auth: ServiceType
+
+    def init(self):
+        if EDGE_STACK:
+            self.xfail = "custom AuthServices not supported in Edge Stack"
+        self.target = HTTP()
+        self.auth = AHTTP(name="auth")
+        self.add_default_http_listener = False
+        self.add_default_https_listener = False
+
+    def manifests(self) -> str:
+        return (
+            self.format(
+                """
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Listener
+metadata:
+  name: {self.path.k8s}
+spec:
+  ambassador_id: [{self.ambassador_id}]
+  port: 8080
+  protocol: HTTP
+  securityModel: XFP
+  l7Depth: 1
+  hostBinding:
+    namespace:
+      from: ALL
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Host
+metadata:
+  name: {self.path.k8s}-host
+  labels:
+    kat-ambassador-id: {self.ambassador_id}
+spec:
+  ambassador_id: [ {self.ambassador_id} ]
+  hostname: "*"
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Mapping
+metadata:
+  name:  {self.target.path.k8s}
+spec:
+  ambassador_id: [{self.ambassador_id}]
+  hostname: "*"
+  prefix: /target/
+  service: {self.target.path.fqdn}
+"""
+            )
+            + super().manifests()
+        )
+
+    def requirements(self):
+        # The client doesn't follow redirects so we must force checks to
+        # match the XFP https route. The Listener is configured with
+        # l7depth: 1 so that Envoy trusts the header XFP header forwarded
+        # by the client.
+        yield (
+            "url",
+            Query(self.url("ambassador/v0/check_ready"), headers={"X-Forwarded-Proto": "https"}),
+        )
+        yield (
+            "url",
+            Query(self.url("ambassador/v0/check_alive"), headers={"X-Forwarded-Proto": "https"}),
+        )
+
+    def queries(self):
+        # send http request
+        yield Query(self.url("target/", scheme="http"), headers={"X-Forwarded-Proto": "http"})
+
+    def check(self):
+        print("results=%s" % repr(self.results))
+        # we should NOT make a call to the AuthService nor the backend service
+        # rather envoy should have redirected to https
+        assert self.results[0].backend is None
+        assert self.results[0].status == 301
+        assert self.results[0].headers["location"] == [f"https://{self.path.fqdn}/target/"]
+
+
 class AuthenticationWebsocketTest(AmbassadorTest):
 
     auth: ServiceType

python/tests/unit/test_irauth.py

@@ -170,3 +170,72 @@ def test_irauth_grpcservice_version_default():
         errors[0]["error"]
         == 'AuthService: protocol_version v2 is unsupported, protocol_version must be "v3"'
     )
+
+
+@pytest.mark.compilertest
+def test_basic_http_redirect_with_no_authservice():
+    """Test that http --> https redirect route exists when no AuthService is provided
+    and verify that the typed_per_filter_config is NOT included
+    """
+
+    yaml = """
+apiVersion: getambassador.io/v3alpha1
+kind: Mapping
+metadata:
+  name: ambassador
+  namespace: default
+spec:
+  hostname: "*"
+  prefix: /httpbin/
+  service: httpbin
+    """
+    econf = _get_envoy_config(yaml)
+
+    for rv in econf.route_variants:
+        if rv.route.get("match").get("prefix") == "/httpbin/":
+            xfp_http_redirect = rv.variants.get("xfp-http-redirect")
+            assert xfp_http_redirect
+            assert "redirect" in xfp_http_redirect
+            assert "typed_per_filter_config" not in xfp_http_redirect
+
+
+@pytest.mark.compilertest
+def test_basic_http_redirect_disables_ext_authz():
+    """Test that http --> https redirect route exists along with
+    typed_per_filter_config for disabling ext_authz when an AuthService exists
+    """
+
+    if EDGE_STACK:
+        pytest.xfail("XFailing for now, custom AuthServices not supported in Edge Stack")
+
+    yaml = """
+---
+apiVersion: getambassador.io/v3alpha1
+kind: AuthService
+metadata:
+  name:  mycoolauthservice
+  namespace: default
+spec:
+  auth_service: someservice
+  proto: grpc
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Mapping
+metadata:
+  name: ambassador
+  namespace: default
+spec:
+  hostname: "*"
+  prefix: /httpbin/
+  service: httpbin
+    """
+    econf = _get_envoy_config(yaml)
+
+    for rv in econf.route_variants:
+        if rv.route.get("match").get("prefix") == "/httpbin/":
+            xfp_http_redirect = rv.variants.get("xfp-http-redirect")
+            assert xfp_http_redirect
+            assert "redirect" in xfp_http_redirect
+            per_filter_config = xfp_http_redirect.get("typed_per_filter_config")
+            assert per_filter_config.get("envoy.filters.http.ext_authz")
+            assert per_filter_config.get("envoy.filters.http.ext_authz").get("disabled") == True