Contour: Feature request: CORS support

[glerchundi]

As far as I can see, in v2 API, it can be implemented only as a http filter: https://github.com/envoyproxy/data-plane-api/blob/master/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto.

In order to have secure enough defaults I think this should be disabled by default and let people enable explicitly with a new command-line parameter.

The implementation is fairly easy I can send a PR if you feel this can get into contour.

WDYT @davecheney?

---

[@rosskukulinski copy/paste] Proposed spec:

[...]
tls:
# required, the name of a secret in the current namespace
secretName: google-tls
# other properties like cipher suites may be added later
# if present describes the CORS policy.
cors:
# Specifies the origins that will be allowed to do CORS requests.
allowOrigin:
- www.google.com
- google.com
# Specifies the content for the access-control-allow-methods
# header.
allowMethods:
- GET
- POST
# Specifies the content for the access-control-allow-headers
# header.
allowHeaders:
- Content-Type
# Specifies the content for the access-control-expose-headers
# header.
exposeHeaders:
- Content-Type
# Specifies the content for the access-control-max-age header.
maxAge: 24h
# Specifies whether the resource allows credentials.
allowCredentials: true
strategy: RoundRobin # (Optional) LB Algorithm to apply to all services, defaults for all services
[...]

[glerchundi]

I know that you probably don't want to infect ingress with annotations but IMHO the cleanest way to do this is to allow the user specify each cors policies in there with annotations, something like this:

contour.heptio.com/enable-cors
contour.heptio.com/cors-allow-origin
contour.heptio.com/cors-allow-methods
contour.heptio.com/cors-allow-headers
contour.heptio.com/cors-allow-credentials
contour.heptio.com/cors-max-age

This directly maps to envoy CorsPolicy and allows to do everything related to CORS.

For now and until defined in the IngressRoute, this is only valid for standard k8s Ingress objects.

Regards,

[davecheney]

Thanks for raising this issue.

TBH I'm going to punt on this til IngressRoute (project) is done. I don't want to see another seven annotations on the ingress document.

[glerchundi]

@davecheney what do you think about approaching this now once IngressRoute (project) seems to be settled?

[davecheney]

@glerchundi i'm going to remove the milestone for this issue; we're not going to get it done for 0.7 for two reasons

1. There's about 10 development days until the end of the cycle, I will be travelling during some of those.
2. There is no design outlined in this issue or a design document. I can't help with this due to (1) and also I have no idea what CORS is.

I'm going to make this as needs a design document; either in this issue or as a separate PR.

[glerchundi]

In the meantime, take a look to this blog post so that you can have more
CORS related background:
http://performantcode.com/web/do-you-really-know-cors

I’ll try to re-include the proposal I made in another issue as soon as
possible so that we can start discussing where would be the best place to
include this.

Thanks,

On Mon, 8 Oct 2018 at 07:00, Dave Cheney [e-mail] wrote:

@glerchundi https://github.com/glerchundi i'm going to remove the
milestone for this issue; we're not going to get it done for 0.7 for two
reasons

1. There's about 10 development days until the end of the cycle, I
   will be travelling during some of those.
2. There is no design outlined in this issue or a design document. I
   can't help with this due to (1) and also I have no idea what CORS is.

I'm going to make this as needs a design document; either in this issue or
as a separate PR.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/heptio/contour/issues/437#issuecomment-427722352, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACIPlnhagyWkKwuCidGaHsibzv-baNpgks5uitwJgaJpZM4Uke23
.

[glerchundi]

@davecheney as we're going to need this sooner than later I would like to know if you found a free slot to take a look to the blog post I mentioned in my last comment?

The configuration parameters for CORS are very strict and static so I think there is no doubt in there. My main concern is the location of this cors key inside de ingressroute spec. As CORS is available in route level we can make it available at a virtualhost or route level, whatever you feel more confortable is ok for me. It doesn't apply to tcp proxy of course, these are only HTTP/S related response headers.

In case you prefer to use these keyset (cors) on both levels (virtualhost and route), the hierarchy should imply inheritance so that children always have precedence but inherits from parent.

Referencing @stevesloka as he was the one who managed this request in the past.

WDYT?

I already made a fork with the changes in case you wanna take a look: https://github.com/glerchundi/contour/commit/d309b09c15c8835c314dbeae1d31cfab4fcbae52

[glerchundi]

Design doc: https://github.com/glerchundi/contour/blob/d309b09c15c8835c314dbeae1d31cfab4fcbae52/design/ingressroute-design.md

[davecheney]

Thanks for the ping. I don't think anyone is going to be able to look at
this til the 0.9 release next year.

On Wed, 28 Nov 2018 at 09:26, Gorka Lerchundi Osa [e-mail]
wrote:

Design doc:
https://github.com/glerchundi/contour/blob/d309b09c15c8835c314dbeae1d31cfab4fcbae52/design/ingressroute-design.md

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/heptio/contour/issues/437#issuecomment-442241732, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAcA9nUW0mskOMEadlelIUuRhX6pwM6ks5uzbwpgaJpZM4Uke23
.

[glerchundi]

@davecheney ok, thanks. As an update, we're deploying our own fork with this implemented and it's working as expected.

[glerchundi]

Hei, sorry for being a little bit heavy on this but this would enable to
stop maintaining our own fork...

What could I do in order to push this forward @rosskukulinski and
@davecheney?

On Wed, 28 Nov 2018 at 00:43, Dave Cheney [e-mail] wrote:

Thanks for the ping. I don't think anyone is going to be able to look at
this til the 0.9 release next year.

On Wed, 28 Nov 2018 at 09:26, Gorka Lerchundi Osa <
[email protected]>
wrote:

Design doc:

https://github.com/glerchundi/contour/blob/d309b09c15c8835c314dbeae1d31cfab4fcbae52/design/ingressroute-design.md

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/heptio/contour/issues/437#issuecomment-442241732,
or mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AAAcA9nUW0mskOMEadlelIUuRhX6pwM6ks5uzbwpgaJpZM4Uke23

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/heptio/contour/issues/437#issuecomment-442261562, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACIPlmzHH5shD1NyscTvBUxmRgtVdh6Kks5uzc4XgaJpZM4Uke23
.

[davecheney]

The root problem is I do not understand what CQRS is or how it relates to a reverse proxy load balancer.

If someone can submit a design document that takes me _out of the loop_ then we can move forward. I cannot be a blocker for this, people have tried to explain what CQRS is, but it hasn’t helped, my brain just does not think in frontend.

[glerchundi]

Thanks for replying this quick!

CQRS (Command Query Request Seggregation) and CORS (Cross Origin Resource
Sharing) are two completely different things!

CORS is a mechanisn used through HTTP headers to tell a browser to let a
webapp running at one domain have permission to access resources from a
different domain.

I recommend you to read this post which sum ups what is and for what is
being used on a very easy and comprensible way:
http://performantcode.com/web/do-you-really-know-cors.

As it is very straighforward I thought a design doc wasn’t needed. Will
send a PR with the design doc during this week.

On Tue, 26 Feb 2019 at 11:33, Dave Cheney [e-mail] wrote:

The root problem is I do not understand what CQRS is or how it relates to
a reverse proxy load balancer.

If someone can submit a design document that takes me out of the loop
then we can move forward. I cannot be a blocker for this, people have tried
to explain what CQRS is, but it hasn’t helped, my brain just does not think
in frontend.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/heptio/contour/issues/437#issuecomment-467386424, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACIPlmr1s7df0eA382r-dGiJrPPgI-zZks5vRQ1qgaJpZM4Uke23
.

[davecheney]

Please understand that I say this out of complete exasperation with my own lack of understanding of the subject. Please do not misinterpret my words as malicious or condescending.

Please do not post links to other eve pages, people have tried, it has had no effect. I am unable to grasp the concept.

Please do post links to the relevant data structures in the envoy documentation that you want to change.

[glerchundi]

Ok!

On Tue, 26 Feb 2019 at 11:46, Dave Cheney [e-mail] wrote:

Please understand that I say this out of complete exasperation with my own
lack of understanding of the subject. Please do not misinterpret my words
as malicious or condescending.

Please do not post links to other eve pages, people have tried, it has had
no effect. I am unable to grasp the concept.

Please do post links to the relevant data structures in the envoy
documentation that you want to change.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/heptio/contour/issues/437#issuecomment-467390570, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACIPllHHGhM-yWz5iSO9qY6pBVu7BMvbks5vRRBrgaJpZM4Uke23
.

[davecheney]

See also https://kubernetes.slack.com/archives/C8XRH2R4J/p1550089170015000

[glerchundi]

For the sake of tracing this feature progress, here the design PR: #906.

[sylwit]

@glerchundi @davecheney For unblocking this feature and having a more simple draft, I suggest to start with a partial implementation which I think would be good for most use cases.

I would start by implementing only the allowOrigin and allowMethods

Here is an example: https://github.com/envoyproxy/envoy/blob/master/examples/cors/backend/front-envoy.yaml#L19

apiVersion: contour.heptio.com/v1beta1
kind: IngressRoute
metadata:
  name: google
  namespace: prod
spec:
  virtualhost:
    fqdn: www.google.com
  routes:
  - match: /
    service:
    - name: google
      port: 9000
    # if present describes the CORS policy.
    cors:
      # Specifies the origins that will be allowed to do CORS requests.
      allowOrigin:
        - www.google.com
        - google.com
      # Specifies the content for the *access-control-allow-methods* header.
      allowMethods:
        - GET
        - POST

as for grpc-web, cors with default values could be added on each route

cors:
  allowOrigin:
    {virtualhost.fqdn}
  allowMethods: 
    - *

If we really want to simplify the usage of cors we could add a shortcut like
cors: true which will be equivalent to :

allowOrigin:
  - *
allowMethods:
  - *

[yoitsro]

I think @sylwit's suggestions are a good start for now and would unblock the majority of use cases without needing to add seven more annotations.

What needs to be done to make progress on this @davecheney?

[glerchundi]

There is an incredible well explained design document in #1012, lets follow the discussion there. @davecheney is it possible to block comments here in order to focus the discussion in one place?

[davecheney]

Temporarily locked, please see https://github.com/heptio/contour/pull/1012#issuecomment-500091198

[davecheney]

Procedural note: I have added this to the 0.14 milestone. I am pushing for a decision from PM in the next two weeks. If we decide to implement this feature, hopefully it will land in the 0.14 cycle.

[davecheney]

Thank you for your patience while this issue was locked. I have an update I want to share

First, the good news. We want to include CORS in Contour 1.0. I have added the 0.15 milestone to this issue mainly because there is nobody to work on this in 0.14.

Now the bad news.

First, the 0.15 milestone is provisional. We don't have any resources available to do this work in 0.15.

Second, and this is going to be the sticking point, we do not want to add the current design outlined in #1012. This is because there are too many parameters. Yup, I know this is the same complaint as three months ago, but that's the bottom line; our support team and our PM are not comfortable with exposing all the CORS knobs to our users in Contour 1.0.

The plan of action going forward, iff (and this is unlikely at this stage) there is development bandwidth in 0.15 someone on my team will be tasked with an alternative CORS implementation design. This is likely to be pushed off to a later milestone.

How can you help? I appreciate that this process has been long and frustrating for everyone involved. I've tried to be as transparent as possible about our resourcing constraints and the difficulties in adding CORS as proposed in #1012 to Contour. If you would like to help, what we need, but do not have, is a design for CORS that has the _minimum_ possible number of configuration parameters. Every configuration parameter removed, inferred, or hard coded has an exponential decrease in the complexity of supporting this feature in Contour 1.0.

[aberasarte]

Thanks @davecheney for the status update and for keeping the discussion open.

Second, and this is going to be the sticking point, we do not want to add the current design outlined in #1012. This is because there are too many parameters. Yup, I know this is the same complaint as three months ago, but that's the bottom line; our support team and our PM are not comfortable with exposing all the CORS knobs to our users in Contour 1.0.

Fair enough.

If you would like to help, what we need, but do not have, is a design for CORS that has the minimum possible number of configuration parameters. Every configuration parameter removed, inferred, or hard coded has an exponential decrease in the complexity of supporting this feature in Contour 1.0.

As I said in #1012, from the proposed six parameters, I think that we could just remove MaxAge setting a reasonable default. Could we set some defaults for the rest of the parameters? Sure! we could set the defaults I mentioned for the parameters allowCredentials, allowMethods and allowOrigin. This way, we'd just expose two parameters (exposeHeaders and allowHeaders) which are very application specific. But the question is: do we want to set default values for all those parameters? I don't think so. If we take that path, we'll end up having a too loose CORS configuration from a security point of view. Precisely, the use of what apparently were "good CORS defaults", has led to many vulnerabilities. In this article they talk about the "recommended CORS defaults" problem and they use the recent Zoom vulnerability as an example.

I know that CORS is hard. To understand and configure it properly is tricky. I'd love to hear what the community thinks about the topic and the possibility of inferring the configuration without exposing the backend more than needed.

[davecheney]

As mentioned in https://github.com/heptio/contour/issues/437#issuecomment-509065117 we didn't have anyone who could work on this during the 0.15. Moving to the beta1 milestone with the caveat that we still don't have any resources to work on this so this feature is at risk of being bumped til after Contour 1.0

[jipperinbham]

With the recently added feature to configure parts of contour/envoy (tls.minimumProtocolVersion/disablePermitInsecure) via a config file, I wonder if we couldn't define most (if not all) of the CORS settings at the top-level in the config file and only require a single option on IngressRoute to enable CORS (i.e. cors: true)?

I feel like this would address the concerns of polluting IngressRoute with too many knobs for CORS while also giving users the ability to configure CORS to their respective needs with the caveat being the settings are global rather than route specific.

If this feels like an option worth pursuing, I'd be happy to take the existing design in https://github.com/heptio/contour/pull/1012 and adjust it accordingly, as well as work through the implementation should the design be accepted.

[davecheney]

What would those settings be? IMO the blocker on CORS support is defining the default values for its various settings.

On 28 Aug 2019, at 23:45, JP Phillips [e-mail] wrote:

With the recently added feature to configure parts of contour/envoy (tls.minimumProtocolVersion/disablePermitInsecure) via a config file, I wonder if we couldn't define most (if not all) of the CORS settings at the top-level in the config file and only require a single option on IngressRoute to enable CORS (i.e. cors: true)?

I feel like this would address the concerns of polluting IngressRoute with too many knobs for CORS while also giving users the ability to configure CORS to their respective needs with the caveat being the settings are global rather than route specific.

If this feels like an option worth pursuing, I'd be happy to take the existing design in #1012 and adjust it accordingly, as well as work through the implementation should the design be accepted.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[jipperinbham]

Given each one would be overridable via the config file, I feel we can opt for rather loose/simple defaults:

+--------------+--------------------------------+
| allowOrigins | *                              |
+--------------+--------------------------------+
| allowMethods | PUT DELETE OPTIONS PATCH       |
+--------------+--------------------------------+
| allowHeaders | Content-Type X-Requested-With  |
+--------------+--------------------------------+
| maxAge       | 86400 (24 hours)               |
+--------------+--------------------------------+

Of the other options (allowCredentials and exposeHeaders), we take the CORS defaults and have to be explicitly provided as it's likely to cover the majority of users IMO. Of the four above, we could potentially drop maxAge and allow browser defaults to kick in.

The only other change that would differ from the current proposal that I would like to push for is to have allowOrigins be a list of regular expressions to be used in allow_origin_regex. One side effect of having an amazing service like Let's Encrypt, is it affords users to have a wide array of subdomains. Because of this, I feel being able to easily have *.example.com for allowOrigins would be beneficial to users.

[annismckenzie]

@jipperinbham While that looks easy on the surface, this will break / not work for example with credentialed requests that require CORS. In these cases, you can't use *.example.com as an allowed origin and browsers will yell at you. And that's one of the things that will never work with that one config file. :/

[jipperinbham]

@annismckenzie thanks for bringing that up as it definitely questioned my understanding of how Envoy handles the allow_origin_regex field.

https://github.com/envoyproxy/envoy/pull/3769 looks to be the original patch to support regexes and digging into the tests added, my interpretation is Envoy will match on the provided values in allow_origin_regex and set the access-control-allow-origin accordingly. The test does show that if Contour does move forward with regex support, the default should be .* instead of just *.

[davecheney]

I'm sorry but we will not be attempting this feature before Contour 1.0. Assigning to the backlog to reassess it after Contour 1.0 is out.

[aberasarte]

Contour 1.0 was released a while ago and I wonder if there has been any update regarding this matter.

I still think that is not easy to set some default values or at least not in a secure way but let me know if I can be of any help. The design doc I wrote last year could be a good starting point.

[youngnick]

Thanks for bringing this back up. As part of onboarding as the new tech lead for Contour, I've been going over some of the product philosophy that interacts with features like this, and I'm hoping to write up something soon.

Which is a long way of saying, thanks for making sure this is on our radar.

[jpeach]

Current status - I think that we have broad consensus on the design in https://github.com/projectcontour/contour/pull/1012. I'll follow up with @aberasarte and get the design doc landed. I hope that @aberasarte and other interested parties (@glerchundi ?) can implement 👍

[aberasarte]

The design doc has been merged. In the following days I will start working on the implementation.