Add CORS policy to app spec by avatus · Pull Request #45558 · gravitational/teleport

avatus · 2024-08-16T16:45:50Z

Requires: #45512 (not a hard requirement, but I'd like for it to go in after)
Part of: #17588

The purpose of this PR is to solve the CORS preflight request issue when an app makes a request to another app. The problem currently manifests because preflight requests sent by the browser do not include credentials, therefore Teleport throws the request out and does not forward to the destination app. The three solutions that came up when brainstorming this idea were

Set a CORS policy on the proxy service that could respond to all OPTIONS requests that came in. This would have to be a pretty "open" CORS policy to respond to all preflights for X amount of apps enabled in the app service so it seemed really weak in terms of security and control.
Set a CORS policy in the app spec. This seemed to be the most "teleport-y" way to do things. This would keep unauthenticated requests from going through to the destination app. The downside of this approach is that this CORS policy cannot be dynamic per request, and if the CORS policy is frequently changed in the destination app code itself, this spec policy might get out of sync.
Allow unauthenticated requests through to the destination app to respond to with its own CORS policy. While this way is default how most of the internet works, it seems best to not change the 'no unauthenticated requests' paradigm that the app service currently uses.

This PR implements option number 2. The main benefit here is that, unless a CORS policy is set in the app spec (after this change goes in), then nothing will change.

All of the CORS fields are optional so if a user only wants to set Access-Control-Allow-Headers, that is possible.

Example app spec

app_service:
  enabled: "yes"
  debug_app: true
  apps:
    - name: "api22"
      uri: "http://localhost:4000"
      public_addr: "api.teleport.zarquon.sh"
      cors:
        allowed_origins:
          - "https://client.teleport.zarquon.sh:3080"
        allowed_methods:
          - "POST"
          - "GET"
          - "OPTIONS"
          - "PUT"
          - "DELETE"
        allowed_headers:
          - "Content-Type"
          - "Authorization"
          - "X-Custom-Header"
        allow_credentials: true
        max_age: 86400
    - name: "client"
      uri: "http://localhost:3002"
      public_addr: "client.teleport.zarquon.sh"
      required_apps:
        - "api22"

I've uploaded my repo that I used during testing to work through the required apps PR as well as this CORS pr. You can use it here if you'd like! https://github.com/avatus/corstest

TODO: add docs when we agree on the contents of the spec

changelog: You can now specify a CORS policy per application in the application config.

github-actions · 2024-08-16T16:46:25Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

tigrato · 2024-08-19T19:44:01Z

+// CORS defines the CORS policy for AppSpecV3
+message CORS {
+  // AllowedOrigins specifies which origins are allowed to access the app.
+  repeated string AllowedOrigins = 1 [(gogoproto.jsontag) = "allowed_origins,omitempty"];


Can we use the new proto style rules with names using snake case instead of camel?

You mean like this? I'll make any recommended change to the naming conventions, just want to make sure I know what you mean

repeated string allowed_origins = 1 [(gogoproto.jsontag) = "allowed_origins,omitempty"];

tigrato · 2024-08-19T19:45:26Z

 }

+// CORS defines the CORS policy for AppSpecV3
+message CORS {


Should we name it CORSSpec?

tigrato · 2024-08-19T19:48:28Z


+// makeApplicationCORS converts a servicecfg.CORS to a types.CORS.
+func makeApplicationCORS(c *servicecfg.CORS) *types.CORS {
+	if c == nil {


Does Teleport infers any default rules as of today or it forwards the CORS requests directly into the application?

Currently any preflight requests are completely thrown out because they are unauthenticated. So no defaults. (technically its not "thrown out", its just redirected to the /web/launch route, but preflights cannot contain a redirect and anything other than a 2XX response counts as a fail anyway)

tigrato · 2024-08-19T19:54:45Z

+	foundApp := servers[0].GetApp()
+	corsPolicy := foundApp.GetCORS()
+	if corsPolicy == nil {
+		return


Given that we intercept every single cors request, won't this break existing apps without cors specified?
Should we mark it as breaking change?

The wouldn't be a breaking change.

Every prelight request is already "broken" so nothing would change

its opt in, so only users who wanted CORS to work would have to explicitly set it.

github-actions · 2024-09-04T19:10:45Z

🤖 Vercel preview here: https://docs-7bjd0wxeg-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-09-04T20:05:54Z

🤖 Vercel preview here: https://docs-4t2o3ogrr-goteleport.vercel.app/docs/ver/preview

rosstimothy · 2024-09-05T15:12:57Z

+// CORSSpec defines the CORS policy for AppSpecV3
+message CORSSpec {


Suggestoin: s/Spec/Policy to more accurately represent this type?

Suggested change

// CORSSpec defines the CORS policy for AppSpecV3

message CORSSpec {

// CORSPolicy defines the CORS policy for AppSpecV3

message CORSPolicy {

rosstimothy · 2024-09-05T15:16:23Z

+
+	servers, err := app.Match(r.Context(), h.handler.cfg.AccessPoint, app.MatchPublicAddr(publicAddr))
+	if err != nil {
+		h.handler.log.Error("failed to match application with public addr %s", publicAddr)


Is this truly an error? Or is this more information? In other words might it be expected that we don't find a match for all preflight requests?

I suppose it wouldn't be an error error because the app itself would still function (granted, CORS wont work, but thats the same as now). I'll switch to info

rosstimothy · 2024-09-05T15:16:42Z

+	}
+
+	if len(servers) == 0 {
+		h.handler.log.Error("failed to match application with public addr %s", publicAddr)


Same question as above.

rosstimothy · 2024-09-05T15:17:18Z

+	foundApp := servers[0].GetApp()
+	corsPolicy := foundApp.GetCORS()
+	if corsPolicy == nil {


Is there any chance that we might have multiple matching apps, each with a slightly different CORS policy?

rosstimothy · 2024-09-05T15:20:19Z

+		return nil
+	}
+
+	return &types.CORSSpec{


Do we need to sanitize any of this data? Does it matter if it contains a bunch of empty strings? Or a really long string? Is there any possibility that these values could be defined in such a way that it would prevent the preflight request from completing successfully?

the max length of a custom header is dependent on web server but not defined in the web spec. Apache is 8kb for a single header length.

extra spaces or trailing spaces in the origins would cause the preflight to fail. should that be left to correct configuration or do we normally remove stuff like leading/trailing spaces elsewhere? perhaps note it in the docs? (which are coming after all this is approved). happy either way

github-actions · 2024-09-05T18:01:33Z

🤖 Vercel preview here: https://docs-jrpum396p-goteleport.vercel.app/docs/ver/preview

avatus · 2024-09-07T02:17:48Z

rebased onto master and cleaned up some of the make derive and make grpc type of commits

github-actions · 2024-09-07T02:24:35Z

🤖 Vercel preview here: https://docs-45jy80nki-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-09-07T02:27:58Z

🤖 Vercel preview here: https://docs-o4echbhrf-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-09-07T02:33:51Z

🤖 Vercel preview here: https://docs-6zpweudgt-goteleport.vercel.app/docs/ver/preview

github-actions Bot added the size/md label Aug 16, 2024

github-actions Bot requested review from GavinFrazar and jakule August 16, 2024 16:46

avatus force-pushed the avatus/cors branch from 48e2241 to 9c5eb7a Compare August 16, 2024 17:48

avatus force-pushed the avatus/redirect branch from 3f18fbe to fa8c486 Compare August 18, 2024 14:49

avatus force-pushed the avatus/cors branch from 9c5eb7a to e401159 Compare August 18, 2024 14:49

avatus requested a review from tigrato August 19, 2024 13:45

avatus force-pushed the avatus/cors branch from e401159 to 31e9b8b Compare August 19, 2024 19:39

tigrato reviewed Aug 19, 2024

View reviewed changes

avatus force-pushed the avatus/cors branch from 57025a7 to 4ce1728 Compare September 4, 2024 19:00

avatus temporarily deployed to vercel September 4, 2024 19:00 — with GitHub Actions Inactive

avatus requested a review from tigrato September 4, 2024 19:01

avatus force-pushed the avatus/redirect branch from 9f31667 to 7fdc1f6 Compare September 4, 2024 19:12

avatus force-pushed the avatus/cors branch from 4ce1728 to 626337c Compare September 4, 2024 19:54

avatus temporarily deployed to vercel September 4, 2024 19:54 — with GitHub Actions Inactive

tigrato approved these changes Sep 5, 2024

View reviewed changes

avatus requested a review from rosstimothy September 5, 2024 15:05

rosstimothy reviewed Sep 5, 2024

View reviewed changes

avatus temporarily deployed to vercel September 5, 2024 17:51 — with GitHub Actions Inactive

avatus requested a review from rosstimothy September 6, 2024 20:45

avatus force-pushed the avatus/redirect branch 3 times, most recently from 6dc1fcb to 4698e76 Compare September 6, 2024 23:09

Base automatically changed from avatus/redirect to master September 7, 2024 01:11

Add CORS policy to app spec

64beb02

avatus force-pushed the avatus/cors branch from ee3fff9 to 0308fba Compare September 7, 2024 02:14

avatus temporarily deployed to vercel September 7, 2024 02:14 — with GitHub Actions Inactive

Add exposed_headers to cors spec

7253ddb

avatus force-pushed the avatus/cors branch from 0308fba to 85f4f6c Compare September 7, 2024 02:17

avatus temporarily deployed to vercel September 7, 2024 02:17 — with GitHub Actions Inactive

Update to CORSPolicy

07aa65e

avatus force-pushed the avatus/cors branch from 85f4f6c to 07aa65e Compare September 7, 2024 02:23

avatus temporarily deployed to vercel September 7, 2024 02:23 — with GitHub Actions Inactive

rosstimothy approved these changes Sep 8, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed request for GavinFrazar and jakule September 8, 2024 14:47

avatus added this pull request to the merge queue Sep 9, 2024

Merged via the queue into master with commit 8d8df0e Sep 9, 2024

avatus deleted the avatus/cors branch September 9, 2024 15:01

avatus mentioned this pull request Sep 9, 2024

[v16] Add CORS policy to app spec #46389

Merged

		// CORSSpec defines the CORS policy for AppSpecV3
		message CORSSpec {

Conversation

avatus commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 4, 2024

Uh oh!

github-actions Bot commented Sep 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 5, 2024

Uh oh!

avatus commented Sep 7, 2024

Uh oh!

github-actions Bot commented Sep 7, 2024

Uh oh!

github-actions Bot commented Sep 7, 2024

Uh oh!

github-actions Bot commented Sep 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

avatus commented Aug 16, 2024 •

edited

Loading

avatus Aug 19, 2024 •

edited

Loading

avatus Aug 19, 2024 •

edited

Loading