GEP 1767: CORS Filter

[lianglli]

What type of PR is this?
/kind gep

What this PR does / why we need it:
This GEP proposes to add a new field HTTPCORSFilter to HTTPRouteFilter.

Which issue(s) this PR fixes:
Fixes #1767

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lianglli
Once this PR has been reviewed and has the lgtm label, please assign mlavacca for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• geps/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @lianglli. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[youngnick]

Thanks for the energy here @liangli, but the correct process here is for you to add some discussion about this feature to the proposed set of Experimental changes in #3403, and then, if this CORS GEP is selected for an Experimental slot by the community's voting, this PR will be able to move forward.

This is a useful feature that already has a GEP, has been discussed before, and is not too big, so it's reasonably likely that it will be included if we can free up enough Experimental slots by moving things to Standard.

I note that you added this to the 1.2 Experimental discussion as well, so it's fine to just reuse the same description in the v1.3 scoping discussion there so that any new folks will have the context to vote.

Until then, sadly, this PR will need to be on hold.

/hold

[robscott]

Thanks for writing up this GEP @lianglli! This made the cut for v1.3 release scoping, it's great to already have a GEP PR ready for review, will take a look shortly.

/ok-to-test

[robscott]

Thanks for all your work on this @lianglli! This is incredibly detailed and well written. I took a first pass and left some initial comments.

/cc @strongjz @howardjohn @youngnick

[robscott]

What happens if a user has configured * here? Should the Gateway implementation reject the request? Or do we just need to add CEL validation to prevent allowCredentials being set to true at the same time as this is set to *?

[howardjohn]

I think you may be mixing up the user HTTPRoute config and the server's expected response. a user can legitimately set * in the config certainly -- this seems to be saying they cannot set it in the response header.

However, I am not sure why? The spec does not seem to reject *? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers#syntax

[strongjz]

We had someone in ingress-nginx, request that we updated CORS to accept * and null

kubernetes/ingress-nginx#12402

[spacewander]

Note that Envoy can't return * directly because it treats * specially:
https://github.com/envoyproxy/envoy/blob/6445d0dcdd33413e47454221fa80b3cfabea7d4e/source/extensions/filters/http/cors/cors_filter.cc#L159-L163.

[lianglli]

What happens if a user has configured * here? Should the Gateway implementation reject the request? Or do we just need to add CEL validation to prevent allowCredentials being set to true at the same time as this is set to *?

When responding to a credentialed requests, the response headers Access-Control-Allow-Origin, Access-Control-Expose-Headers, Access-Control-Allow-Methods and Access-Control-Allow-Headers can NOT use * as value.

However, if a HTTPCORSFilter sets AllowCredentials as true and configures * for AllowHeaders, the gateway will return HTTP response header Access-Control-Allow-Credentials: true and Access-Control-Allow-Headers: * to the "preflight" request based on the HTTPCORSFilter specifically.

Then, clients will NOT be able to send actual cross-origin request.

[lianglli]

Note that Envoy can't return * directly because it treats * specially: https://github.com/envoyproxy/envoy/blob/6445d0dcdd33413e47454221fa80b3cfabea7d4e/source/extensions/filters/http/cors/cors_filter.cc#L159-L163.

Most gateways support the * wildcard as the value of a HTTP header.
Moreover, * wildcard is a valid character of header value based on rfc7230.

Hence, the envoy should support it in future.

[youngnick]

This field (and this spec in general) is pretty unclear as to which "user" we're talking about.

I suggest using different names for the HTTPRoute owner (maybe "user" is okay here), and the client, (should probably use that word instead).

So, I think what @robscott was saying is "What happens if the HTTPRoute owner has configured * here?"

The text says:

    // When responding to a credentialed requests, the gateway must specify 
    // one or more HTTP headers in the value of the Access-Control-Allow-Headers response header, 
    // instead of specifying the * wildcard.

What headers should the implementation respond with? Below, @spacewander suggests responding with whatever was sent in the Access-Control-Request-Headers field in the OPTIONS request. Whatever we want to do MUST be defined in this field, as what to do in the other cases:

• * is configured in this field, and Access-Control-Request-Headers is not sent
• One or more values are configured in this field and Access-Control-Request-Headers is sent
• One or more values are configured in this field and Access-Control-Request-Headers is not sent

What we are aiming for is that someone who reads the spec should be able to understand enough to write conformance tests for the spec, ideally without looking at any other documents.

[robscott]

I'm not 100% sure if this should be a filter vs a top level field in Routes similar to timeouts.

In almost all cases, filters modify a request or response (RequestHeaderModifier, ResponseHeaderModifier, RequestRedirect, URLRewrite). RequestMirror is a bit of an exception, but it's effectively creating a copy of a request and sending it somewhere else, so there's some overlap.

This admittedly can modify a request or response, so it could qualify as a filter, but it also feels conceptually similar to "timeouts" in that it's describing what kinds of requests are "valid" or "invalid".

[howardjohn]

I don't have a strong preference but it does seem more filter-like. If we consider filters logically as a

fn run_filter(req: HTTPRequest) -> FilterResponse;
enum FilterResponse {
  Request(HTTPRequest), // A request, maybe the same as the input or maybe modified
  Response(HTTPResonse) // Respond directly
}

A timeout doesn't really meet that, but CORs does:

fn run_filter(req: HTTPRequest) -> FilterResponse {
  if req.method == origin { return Response(cors_response()) }
  return Request(req) // return the original request
}

(the function is not 100% correct in CORS semantics but close enough)

[lianglli]

CORS is a HTTP feature based on HTTP-header.
Hence, this GEP proposes to add a new field HTTPCORSFilter to HTTPRouteFilter.

[youngnick]

I agree this fits into a Filter pretty well.

[robscott]

Seems like a pretty clear consensus that this fits as a filter, thanks for the responses here! @lianglli do you mind capturing some part of this thought process under a section like "Alternatives Considered" for future reference?

[lianglli]

Seems like a pretty clear consensus that this fits as a filter, thanks for the responses here! @lianglli do you mind capturing some part of this thought process under a section like "Alternatives Considered" for future reference?

ok.

[robscott]

Why have a default here at all? This seems like it could be surprising to users unless it's common for implementations to have a default.

[lianglli]

Based on Fetch Living Standard, the default value of Access-Control-Max-Age is 5 (seconds).

If a HTTPCORSFilter does NOT set 'MaxAge' specifically, the gateway will return the header "Access-Control-Max-Age: 5" to clients by default.

For example, the default value of Access-Control-Max-Age for Ingress-NGINX Enable CORS is 1728000 seconds.

[howardjohn]

The question I have: if unset, should the server respond Access-Control-Max-Age: 5, or should the server not include anything and let the client decide (where it will likely default to 5s)?

Envoy prior art is "not include"

[mlavacca]

Since the default value for MDN is 5 seconds, I think it's good to set an explicit default here: in case the gateway does not include such a header, it's up to the client such a decision, which is never a good thing in my opinion. Setting a default here makes the whole thing explicit and always defined.

[howardjohn]

If the default is 'unset', we support all cases: if a user wants it to be unset (and let client decide) they can, and if a user wants it set they can explicitly set it.

Not sure how valid this is though

[howardjohn]

Thanks for the GEP -- top notch details!

[howardjohn]

Is this accurate? AFAIK CORS is a client side security mechanism -- the thing that would reject something is the browser

[howardjohn]

Which is a common mistake when testing CORS; someone would use curl and expect it to fail but it works fine since curl does not implement CORS

[lianglli]

Which is a common mistake when testing CORS; someone would use curl and expect it to fail but it works fine since curl does not implement CORS

All major browsers support CORS.
However, some web application also support CORS.
For example, a web application is able to use CORS based on Apache Pekko HTTP’s cors module or ASP.NET Core.

Hence, when referring to "client" in the context of CORS, it means the web browser or applications and services inherently support Cross-Origin Resource Sharing (CORS).

[lianglli]

Is this accurate? AFAIK CORS is a client side security mechanism -- the thing that would reject something is the browser

Based on CORS spec,

for a successful HTTP response to a CORS-preflight request, it is restricted to an ok status, e.g., 200 or 204.

Any other kind of HTTP response is not successful and will either end up not being shared or fail the CORS-preflight request. Be aware that any work the server performs might nonetheless leak through side channels, such as timing. If server developers wish to denote this explicitly, the 403 status can be used, coupled with omitting the relevant headers.

Alternatively, if the server doesn’t want to allow cross-origin access, it will respond with an error message to the client.

[howardjohn]

I don't think that is accurate. That says "anything except 200 or 204 means it failed", but it doesn't mean if the Cross origin request is not supported it should return a 403. Just that if it happened to do so it would be treated as failed.

I have never seen a server respond with a 403 from a CORS request?

[lianglli]

The specification for CORS is included as part of the WHATWG's Fetch Living Standard.

Yes, it is. The 403 is not mandatory for fail the CORS-preflight request.

For most Gateway implementation, we always return 204/200 to the the CORS-preflight request even if the request Origin dos not match the configured allowed origins.

Alternatively, If CORS is not enabled or no CORS rule matches the preflight request, the server responds with status code 403 (Forbidden) to the CORS-preflight request.

[howardjohn]

My concern is we are taking a very weak statement "If you wish you can return a 403" and saying "you must return a 403".

[howardjohn]

I don't have a strong preference but it does seem more filter-like. If we consider filters logically as a

fn run_filter(req: HTTPRequest) -> FilterResponse;
enum FilterResponse {
  Request(HTTPRequest), // A request, maybe the same as the input or maybe modified
  Response(HTTPResonse) // Respond directly
}

A timeout doesn't really meet that, but CORs does:

fn run_filter(req: HTTPRequest) -> FilterResponse {
  if req.method == origin { return Response(cors_response()) }
  return Request(req) // return the original request
}

(the function is not 100% correct in CORS semantics but close enough)

[howardjohn]

I don't think this is how CORS works generally. Or, at the very least, not how Envoy's CORS filter works which is what I am familiar with so I will desribe that.

In envoy, if a request is a CORS request then there will be a direct response from the CORS filter. A "CORS request" means:

• method is OPTIONS
• "origin" header is not empty
• "Access-Control-Request-Method" is not empty"

If it meets this criteria, it will always return a 200 with the various CORS response headers. If its not allowed, it will be rejected by the browser -- NOT the server.

If it doesn't meet the criteria, the request is just passed through

[spacewander]

If it doesn't meet the criteria, the request is just passed through

Note that Envoy Gateway has changed this default behavior in envoyproxy/gateway#3002 to "aligns more closely with users' intuition." CC @zhaohuabing

Personally, I would vote for returning an OK status, which is chosen by some Nginx implementations like Kong.

[mlavacca]

+1. The OPTIONS pre-flight response status should always be 200 or 204 (no content) and the actual result of the pre-flight should be included in the various headers.

[lianglli]

Before the actual cross-origin requests, a client sends a cross-origin "preflight" request for asking a server whether it would allow an actual cross-origin request.

Based on CORS spec of Fetch Living Standard, the status code of a successful response to a "preflight" request is an OK status (i.e., 204 or 200).

Any other kind of HTTP response is not successful and will either end up not being shared or fail the CORS-preflight request. Be aware that any work the server performs might nonetheless leak through side channels, such as timing. If server developers wish to denote this explicitly, the 403 status can be used, coupled with omitting the relevant headers.

In the Gateway API context, the server is a Gateway implementation.

A "preflight" response will be generated by the Gateway. Moreover, the gateway will send the "preflight" response to the client directly.

Hence, for the "preflight" request, if the request Origin does not match the configured allowed origins, alternatively, the Gateway will respond with an error message 403 to the client and omit the relevant CORS response headers.

[howardjohn]

I think you may be mixing up the user HTTPRoute config and the server's expected response. a user can legitimately set * in the config certainly -- this seems to be saying they cannot set it in the response header.

However, I am not sure why? The spec does not seem to reject *? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers#syntax

[howardjohn]

The question I have: if unset, should the server respond Access-Control-Max-Age: 5, or should the server not include anything and let the client decide (where it will likely default to 5s)?

Envoy prior art is "not include"

[spacewander]

I would recommend using regex to specify the matched Origins. Otherwise, there is no way to configure it to match any subdomain of a given domain, or any port of a given domain.

Also, it would be better no export the * wildcard directly, so the user doesn't need to study what a credentialed request means. She can use the .* regex instead.

[lianglli]

I would recommend using regex to specify the matched Origins. Otherwise, there is no way to configure it to match any subdomain of a given domain, or any port of a given domain.

Also, it would be better no export the * wildcard directly, so the user doesn't need to study what a credentialed request means. She can use the .* regex instead.

The type of AllowOrigins is a list of string.
Based on CORS spec, the valid origins include wildcard "*".

wildcard                         = "*"
Access-Control-Allow-Origin      = origin-or-null / wildcard

An origin consists of three parts: the scheme, host and port.
An HTTP host can be a wildcard, meaning you can use an asterisk () in the hostname to match any subdomain within a domain.

For example:

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: http-route-cors
spec:
  hostnames:
  - http.route.cors.com
  parentRefs:
  - group: gateway.networking.k8s.io
    kind: Gateway
    name: http-gateway
  rules:
  - backendRefs:
    - kind: Service
      name: http-route-cors
      port: 80
    matches:
    - path:
        type: PathPrefix
        value: /resource/foo
    filters:
    - cors:
      - allowOrigins:
        - http://*.foo.example
        - https://*.foo.example
        allowMethods: 
        - GET
        - PUT
        - POST
        - DELETE
        - PATCH
        - OPTIONS
        allowHeaders: 
        - DNT
        - X-CustomHeader
        - Keep-Alive
        - User-Agent
        - X-Requested-With
        - If-Modified-Since
        - Cache-Control
        - Content-Type
        - Authorization
        exposeHeaders: 
        - Content-Security-Policy
        maxAge: 1728000
      type: CORS

[youngnick]

I added some suggested wording about possible formats for this field up above, which I think should make this clearer.

If that suggestion is okay, I'd suggest also adding an example to this godoc that shows how an Origin with scheme plus wildcarded host would work (as in the example @lianglli posted just above).

[lianglli]

ok

[spacewander]

The host part of the origin may contain the wildcard character *.

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin,

Limiting the possible Access-Control-Allow-Origin values to a set of allowed origins requires code on the server side to check the value of the Origin request header, compare that to a list of allowed origins, and then if the Origin value is in the list, set the Access-Control-Allow-Origin value to the same value as the Origin value.

the Access-Control-Allow-Origin should return the Origin sent by the client. So if the client sends "a.com", the server can't reply with "*.com".

[spacewander]

I think it again, maybe you mean "if configured with '*.com', the server should return ‘a.com’ when the client sends 'a.com' and 'b.com' when the client sends 'b.com'"?

What's the advantage of this wildcard host match expression?

[youngnick]

To be honest, I don't know. I'm just going by trying to clarify what I understand of the language - I haven't done a lot of work with CORS though.

[lianglli]

I think it again, maybe you mean "if configured with '*.com', the server should return ‘a.com’ when the client sends 'a.com' and 'b.com' when the client sends 'b.com'"?

What's the advantage of this wildcard host match expression?

The following var $http_origin is the request header 'origin'.

For a simple cross-origin interaction, the gateway sets value of the header
Access-Control-Allow-Origin same as the Origin header provided by the client. However, if AllowOrigins has the * wildcard, the gateway will return a response header Access-Control-Allow-Origin: *.

    if (allowOrigins has "*") {
        set $http_origin "*";
    }

    more_set_headers 'Access-Control-Allow-Origin: $http_origin';

[youngnick]

Another example in the godoc here would serve to clarify this behavior, and remove any ambiguity.

[spacewander]

If it doesn't meet the criteria, the request is just passed through

Note that Envoy Gateway has changed this default behavior in envoyproxy/gateway#3002 to "aligns more closely with users' intuition." CC @zhaohuabing

Personally, I would vote for returning an OK status, which is chosen by some Nginx implementations like Kong.

[spacewander]

As said in this proposal:

The server response for the CORS "preflight" request include the following headers:
Access-Control-Allow-Origin response header indicates whether the response can be shared with requested resource from the given Origin.
Access-Control-Allow-Methods response header specifies one or more HTTP methods are accepted by the server when accessing the requested resource.
Access-Control-Allow-Headers response header indicates which HTTP headers can be used during the actual cross-origin request.

Access-Control-Allow-Methods is not an optional response header. So we can't make it optional unless we provide a good default value. I have created a similar issue to Envoy before: envoyproxy/envoy#37533.

I would recommend echoing back the Access-Control-Request-Method request header instead of *, so the user doesn't need to study what a credentialed request means.

By the way, Envoy can't configure plain * because it treats the * specially.

[lianglli]

Using wildcard or regular expressions may create vulnerabilities.
However, based on the CORS spec of Fetch Living Standard, the wildcard "*" is a valid value of header ccess-Control-Allow-Origin.

wildcard                         = "*"
Access-Control-Allow-Origin      = origin-or-null / wildcard

The Envoy should supports wildcard "*" in the CORS filter specifically.

Credentials is important for the CORS protocol.
Most gateway (E.g., Istio, Ingress-NGINX and etc.) supports Credential.

[youngnick]

I think that the point that @spacewander is making here is that, if we make this field +optional, then we need to include some default value in this spec, because the config itself is not optional when an implementation is responding - the Gateway implementation must return something, so we should be configuring that in this object, rather than having some implicit behavior.

I think that this field should instead be +required (or have no +optional tag, which is the same thing), as setting any default could be insecure depending on the user. So it's better to force users of this config to have to specify something here.

[spacewander]

Note that Envoy can't return * directly because it treats * specially:
https://github.com/envoyproxy/envoy/blob/6445d0dcdd33413e47454221fa80b3cfabea7d4e/source/extensions/filters/http/cors/cors_filter.cc#L159-L163.

[spacewander]

As MDN mentions,

This header is required if the preflight request contains Access-Control-Request-Headers

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers

I would recommend echoing back the Access-Control-Request-Headers request header by default, so the user doesn't need to study what a credentialed request means.

[mlavacca]

This is a great introduction! One thing I see implicit here is the default port for the schema: in the examples below, you outline how http://example.com:80 is equal to http://example.com and how http://example.com is different from https://example.com:80, which is great. I think we could benefit from explicitly stating here how ports can be implicitly bound to the scheme.

[mlavacca]

+1. The OPTIONS pre-flight response status should always be 200 or 204 (no content) and the actual result of the pre-flight should be included in the various headers.

[mlavacca]

Since the default value for MDN is 5 seconds, I think it's good to set an explicit default here: in case the gateway does not include such a header, it's up to the client such a decision, which is never a good thing in my opinion. Setting a default here makes the whole thing explicit and always defined.

[mlavacca]

Since we are introducing the GEP with this PR, and it aims at becoming experimental in the next release, we should set provisional here.

[lianglli]

provisional

OK, I get it.

[youngnick]

That's not how boolean serialization works for YAML. YAML accepts truthy values like true, True, On, etc for true, and similar ones for false.

If we want to specify that this can only be true, or false, precisely, then this should be a string enumerated field with constants for true and false.

Also, if we do stick with boolean, we need to be sure that we won't want to add any other values later, because once this goes in, we can't change the type later to string (which we would need to). It doesn't seem likely since this is implementing a defined-elsewhere boolean field, but it's worth noting.

[youngnick]

This should also include a mention that requests with AllowCredentials set to true are referred to as credentialled requests in other parts of the spec.

[lianglli]

I get it.

[youngnick]

This should be an aliased string type, with included constants for all the valid HTTP methods, and should have kubebuilder Enum tags that set the permitted values. That will ensure that users will get immediate feedback if they provide an invalid value.

[youngnick]

Does "exposed to clients" mean "returned as headers in requests"? Seems like it, but if that's the case, that should be said specifically here. Once we define that, then we can use "exposed to clients" and "returned as headers" interchangeably.

[youngnick]

This section really should be before the API section and after the Introduction section. I think we may need a template update.

[k8s-ci-robot]

@lianglli: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gateway-api-verify	4caf94e	link	true	/test pull-gateway-api-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.