CORS: support wildcard matching for AllowMethods and AllowHeaders

[zhaohuabing]

Support wildcard matching for AllowMethods and AllowHeaders
Related: #4196

[codecov]

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 66.45%. Comparing base (480b387) to head (fa0ddab).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/xds/translator/cors.go	76.92%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4168      +/-   ##
==========================================
- Coverage   66.51%   66.45%   -0.06%     
==========================================
  Files         195      195              
  Lines       23734    23745      +11     
==========================================
- Hits        15786    15780       -6     
- Misses       6831     6841      +10     
- Partials     1117     1124       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[guydc]

LGTM. Do we need to label this somehow so that we include a breaking change notice in release notes?

[zhaohuabing]

Although the Envoy documentation doesn’t explicitly mention it, the implementation currently allows the * symbol to match all HTTP methods and allowheaders.

The current implementation:

allowOrigins: * is used for wildcard matching, and EG translate it to a regex in the generated xDS.
allowMethods: * is treated as a normal character in EG, but Envoy use it to match all methods.
allowHeaders: * is treated as a normal character in EG, but Envoy use it to match all headers.
exposeHeaders * is treated as a normal character in both EG and Envoy. (I have no idea why it's not treated as the same matching logic as the allowHeaders 🤷‍♂️)

While allowing * can be convenient, whitelisting all headers/methods has a better security posture. Should we using wildcard maching for allowHeaders and allowMethods? @envoyproxy/gateway-maintainers @envoyproxy/gateway-reviewers

Test:

cors:
  allowHeaders:
  - '*'
  allowMethods:
  - '*'
  allowOrigins:
  - '*'
  exposeHeaders:
  - '*'

curl -H "Origin: http://www.foo.com/" \
-H "Host: www.example.com" \
-H "Access-Control-Request-Method: GET" \
-H "Access-Control-Request-Headers: Content-Type, x-resp" \
-H "Access-Control-Expose-Headers: xxxxxx, yyyyy" \
-X OPTIONS -v -s \
http://172.18.255.200/  \
1> /dev/null

< HTTP/1.1 200 OK
< access-control-allow-origin: http://www.foo.com/
< access-control-allow-methods: GET
< access-control-allow-headers: Content-Type, x-resp
< access-control-expose-headers: *
< date: Tue, 10 Sep 2024 08:43:47 GMT
< content-length: 0

Slack discussion: https://envoyproxy.slack.com/archives/C03E6NHLESV/p1725956439557039?thread_ts=1725899843.349039&cid=C03E6NHLESV

CORS implementations in some existing gateways, all of them support wildcard in allowOrigins, and some of them support wildcard in headers and methods.

• Nginx: https://docs.nginx.com/nginx-management-suite/acm/how-to/policies/cors/
• APISIX: https://apisix.apache.org/docs/apisix/plugins/cors/
• Contour: https://projectcontour.io/docs/1.28/config/cors/
• Istio: https://istio.io/latest/docs/reference/config/networking/virtual-service/#CorsPolicy
• emissary: https://www.getambassador.io/docs/emissary/latest/topics/using/cors

[luvk1412]

Adding my use case here where we require some way to mention Allow all.

• We want to have a global CORS SecurityPolicy where cluster Operators can restrict the origins only. If Cluster Operator controls the allowHeaders or exposeHeaders, then devs have to come to operators every time they add a new header to their req/resp. Devs can ideally create a separate SecurityPolicy and override, but we would ideally wan't devs to use only predefined ones via label selectors and not allow them to create new SecurityPolicy.
• For allowMethods, we can add all enums to policy, as enum set is limited, but adding * or some other cleaner approach to allow all would be preferred. I have one more use case in mind but i am not sure if that would be faced practically: Currently allowed enum set contains all the standard Methods but technically methods don't need to be only those, for eg WebDAV protocol(we happen to run a WebDAV server) has methods like PROPFIND etc., but I don't think any browser/js allows hitting non standard methods. So this might not be an issue for CORS, just mentioning in case i am missing something here and someone else can add.

So for us it would be preferable if there is a way to define allow all for allowOrigins , allowMethods, allowHeaders and exposeHeaders
@zhaohuabing

[luvk1412]

From Access-Control-Expose-Headers

For requests without credentials, a server can also respond with a wildcard value

So for requests without creds/cookies, below

  exposeHeaders:
  - '*'

would make it behave like allow all @zhaohuabing

[zhaohuabing]

From Access-Control-Expose-Headers

For requests without credentials, a server can also respond with a wildcard value

So for requests without creds/cookies, below

  exposeHeaders:
  - '*'

would make it behave like allow all @zhaohuabing

I misunderstood the access-control-expose-headers header. It's enforced by the browser when the browser receive responses. So just allowing inputting * in the exposeHeaders is good enough. :-)

[arkodg]

LGTM thanks !