Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: perform https redirect prior to ext_authz calls #4618

Merged
merged 3 commits into from
Oct 21, 2022

Conversation

LanceEa
Copy link
Contributor

@LanceEa LanceEa commented Oct 12, 2022

Description

Fixes a regression with https_redirection when an AuthService is applied. This was introduced in v3+ due to upgrading Envoy where a behavior change was introduced for the ext_authz http filter. This restores the previous behavior by disabling ext_authz calls on a per route basis when an AuthService is applied.

Before change:

{
    "match": {
      "case_sensitive": true,
      "prefix": "/backend/",
      "runtime_fraction": {
        "default_value": {
          "denominator": "HUNDRED",
          "numerator": 100
        },
        "runtime_key": "routing.traffic_shift.cluster_quote_default_default"
      }
    },
    "redirect": {
      "https_redirect": true
    }

After change:

{
    "match": {
      "case_sensitive": true,
      "prefix": "/backend/",
      "runtime_fraction": {
        "default_value": {
          "denominator": "HUNDRED",
          "numerator": 100
        },
        "runtime_key": "routing.traffic_shift.cluster_quote_default_default"
      }
    },
    "redirect": {
      "https_redirect": true
    },
    "typed_per_filter_config": {
      "envoy.filters.http.ext_authz": {
        "@type": "type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthzPerRoute",
        "disabled": true
      }
    }
  }

IMPORTANT: this is only applied to the https_redirect routes that are added by default for all Host which has requestPolicy.insecure.action = Redirect as the default action.

Related Issues

#4620

Testing

Manual Verification:

  • reproduced in a cluster with 3.2.0
  • tested dev build and verified https redirection occurs
  • visually inspected the envoy.json to ensure expected config was generated

Automtates Test:

  • Added two new Unit Test to verify generated envoy config from diagd
  • Added new KAT test to verify 301 redirect for non-tls request

Checklist

  • I made sure to update CHANGELOG.md.
  • This is unlikely to impact how Ambassador performs at scale.
  • My change is adequately tested.
  • I updated DEVELOPING.md with any any special dev tricks I had to use to work on this code efficiently.
  • The changes in this PR have been reviewed for security concerns and adherence to security best practices.

@LanceEa LanceEa force-pushed the laustin/authservice-http-redirect branch 10 times, most recently from 28d01e6 to e79ea32 Compare October 16, 2022 03:39
@LanceEa LanceEa changed the title WIP: fix http-->https redirect with AuthService fix: perform https redirect prior to ext_authz calls Oct 16, 2022
@LanceEa LanceEa added the t:bug Something isn't working label Oct 16, 2022
@LanceEa LanceEa marked this pull request as ready for review October 16, 2022 15:26
@LanceEa LanceEa requested review from ddymko and LukeShu October 17, 2022 11:16
@LanceEa
Copy link
Contributor Author

LanceEa commented Oct 17, 2022

@LukeShu @ddymko - I have the KAT testing passing now so this is good to go for a review.

As I was working on the KAT tests I added these two commits for CI improvements:

I'm happy to PR them separately if they are sticking points, in favor of landing the fix commit.

@ddymko
Copy link
Member

ddymko commented Oct 17, 2022

@LukeShu @ddymko - I have the KAT testing passing now so this is good to go for a review.

As I was working on the KAT tests I added these two commits for CI improvements:

* [d31ab7f](https://github.com/emissary-ingress/emissary/commit/d31ab7f408ba43b70661e89cfb872ecf4e651483)

* [902bed4](https://github.com/emissary-ingress/emissary/commit/902bed428da6796fd50d34730f7a9500b4dd8b4f)

I'm happy to PR them separately if they are sticking points, in favor of landing the fix commit.

I don't have a strong preference for this specific occurrence. They are small enough we can keep them here or if we want a cleaner history, then break them out into CI improvements PR.

Lance Austin added 3 commits October 17, 2022 08:35
Checkout v2 was based on Node 12 which is now deprecated. Github
now post a warning during CI runs recommending to upgrade to
Node 16+. This updates all instances of this to v3 which is based on
Node 16.

See <https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/>
for more information on Github warnings.

Signed-off-by: Lance Austin <laustin@datawire.io>
Collect the config that was used for the test pod so that
it can be used to for debugging failing tests.

Signed-off-by: Lance Austin <laustin@datawire.io>
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>
@LanceEa LanceEa force-pushed the laustin/authservice-http-redirect branch from e79ea32 to d5e9b91 Compare October 17, 2022 13:36
@LanceEa
Copy link
Contributor Author

LanceEa commented Oct 17, 2022

I don't have a strong preference for this specific occurrence. They are small enough we can keep them here or if we want a cleaner history, then break them out into CI improvements PR.

Ok, I will leave it then since they are separate commits already and are not mixed with the fix commit.


def manifests(self) -> str:
return (
self.format(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When possible, I've been preferring f-strings to the old self.format magic.

Copy link
Contributor

@LukeShu LukeShu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's the redirect-Mapping issue discussed today that still needs fixed. But the changes here lgtm.

@LanceEa
Copy link
Contributor Author

LanceEa commented Oct 21, 2022

There's the redirect-Mapping issue discussed today that still needs fixed. But the changes here lgtm.

Cool, I will go ahead and land this so it can be in the next RC and will get to work on the fix for the mapping redirect issue as well.

@LanceEa LanceEa merged commit 57d043e into master Oct 21, 2022
@LanceEa LanceEa deleted the laustin/authservice-http-redirect branch October 21, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants