Ambassador: Using includeSubdomains: true in a Filter causes crash in Ambassador 1.5.4

[Lemmons]

Describe the bug
Using includeSubdomains: true in a Filter causes ambassador to crash at startup, or to raise an error when the Filter is added.

To Reproduce
Steps to reproduce the behavior:

1. Apply a Filter with includeSubdomains: true set for one of its spec.OAuth2.protectedOrigins
2. Check logs for error or attempt to access a site included as part of the Filter

Expected behavior
The Filter should not error and should properly protect subdomains as expected.

Versions (please complete the following information):

• Ambassador: 1.5.4
• Kubernetes environment: EKS
• Version: 1.14

Additional context
Here is the Filter which causes issues:

apiVersion: getambassador.io/v2
kind: Filter
metadata:
  name: my-test
spec:
  OAuth2:
    authorizationURL: https://cognito-idp.us-east-1.amazonaws.com/{sensitive}
    clientID: {sensitive}
    secret: {sensitive}
    protectedOrigins:
      - origin: https://my-test.{sensitive}
        includeSubdomains: true

Removing includeSubdomains: true, as shown below, allows the Filter to apply correctly and start working.

apiVersion: getambassador.io/v2
kind: Filter
metadata:
  name: my-test
spec:
  OAuth2:
    authorizationURL: https://cognito-idp.us-east-1.amazonaws.com/{sensitive}
    clientID: {sensitive}
    secret: {sensitive}
    protectedOrigins:
      - origin: https://my-test.{sensitive}

gz#453

[nbkrause]

@Lemmons Thank you for reporting this.

This is a bug that appears to be introduced in 1.5.3 with the internalOrigin addition to the protectedOrigins field.

The workaround for it is to manually set internalOrigin to the origin configured with includeSubdomains: true set.

apiVersion: getambassador.io/v2
kind: Filter
metadata:
  name: my-test
spec:
  OAuth2:
    authorizationURL: https://cognito-idp.us-east-1.amazonaws.com/{sensitive}
    clientID: {sensitive}
    secret: {sensitive}
    protectedOrigins:
      - origin: https://my-test.{sensitive}
        internalOrigin: https://my-test.{sensitive}
        includeSubdomains: true

We apologize for this issue and are working to resolve it. Thanks again for reporting it!

[Lemmons]

While the workaround lets the filter now not fail when being applied, it still doesn't actually enable the expected behavior from includeSubdomains: true.

I expect with the workaround for a request to https://subdomain.my-test.{sensitive} to go through the filter, but instead it raises an error filter misconfiguration: https://subdomain.my-test.{sensitive} is not a valid origin for my-test.default

[yanickbelanger]

Same problem here, the workaround makes ambassador stop crashing at startup, but includeSubdomains doesn't work. From the stack trace, it seems like a simple bug in the filter validation? @khussey, @LukeShu, @nbkrause, any other workaround for us? Or an ETA on a fix? This is blocking the deployment of our authentication system. Thanks.

[khussey]

The crashing issue has been fixed in the 1.6.0 release that is now available.

[yanickbelanger]

@khussey I've tested this morning and it looks like the crashing issue is gone. However I can't get the includeSubdomains flag to work.

My filter contains:

    protectedOrigins:
      - origin: https://xxxxxx.xxxx.xx
        includeSubdomains: true

but when I hit https://test.xxxxxx.xxxx.xx, I get this error message:

"message": "filter misconfiguration: https://test.xxxxxx.xxxx.xx is not a valid origin for xxxxxx.xxxxxx"

My goal is to able to onboard new services internally, with new subdomains, without changing the oauth2 filter configuration (I still have to whitelist their redirect URI in the IdP, but that's another topic).

Maybe I don't get what's this includeSubdomains flag is doing?

[khussey]

Thanks for checking, @yanickbelanger! We're still working to make the flag function as expected and should be able to ship a more complex fix in an upcoming release.

[yanickbelanger]

@khussey Any update on this? I can help you with the testing of that feature if needed.

[khussey]

Thanks for the kind offer, @yanickbelanger. We're currently testing a fix and anticipate shipping it as part of the upcoming 1.7.0 release.

[khussey]

Oops, closed prematurely. We have a fix and will be shipping it with 1.7.0 in the coming days.

[khussey]

This fix is in 1.7.0, which is now available.

[yanickbelanger]

Tested in our environment, it seems to work great. Thanks! As a bonus, the redirect URIs are now all pointing to the same domain, so no more whitelisting of redirect URIs required for all our apps in our IdP.