feat: Add serving applications on subdomains and port-based proxying #3753

Emyrk · 2022-08-30T12:59:43Z

What this does

This PR adds the ability to serve applications at / on subdomains. It also allows accessing ports inside a workspace with an anonymous application.

Examples:

Status Quo. An app on a path
- https://dev.coder.com/@Emyrk/dev-dogfood/apps/code-server/
NEW App on subdomain
- https://emyrk--dev-dogfood--main-code-server.dev.coder.com/
NEW Port on path
- https://dev.coder.com/@Emyrk/dev-dogfood/apps/8080/
NEW Port on subdomain
- https://emyrk--dev-dogfood--main-8080.dev.coder.com/

Conversation Wanted

I hit a wall working on this, and want some discussion before I proceed.

Token Scoping + Security

We need to scope the session_token used for application authentication. It should only have access to application endpoints, and nothing else. This is so a malicious application cannot make authenticated requests on a user's behalf.

We can do this with RBAC token scopes as defined in the RFC, or by making a new token type like we did for agent authentication. The new token can live in coder_application_token.

If the application is using the path based routing, then the session_token of the primary auth can still be used. We need to either have a security setting to always host applications on another subdomain, or somehow reject api calls from these paths (??).

Applications on another domain

Eventually we want to serve applications on another domain for security reasons. To do this, we need to implement a way to insert a cookie on that subdomain, like we did with devurls in V1. I suggest we do this as a follow up PR.

Applications on subpath

We should be checking if authenticated requests come from an app via the Origin header. If it is, we should not allow that Origin to make authenticated requests.

At present

At present we do not allow sharing these application URLs to other users. So all of the security considerations can be follow ups.

Future Work

Support serving apps on other domains
Scoped tokens to just application authentication
Security check on the things above, might need some setting toggles to disallow certain things.

…apps

bpmct · 2022-08-30T13:24:41Z

I'll defer to @sreya and @kylecarbs on the token scoping. From a product POV, supporting the following as a first iteration seems solid:

coder.mydomain.com # host
*.coder.mydomain.com # subdomains for apps

We can open another issue "Run Coder apps on a separate domain" as some users may eventually want

coder.mydomain.com
*.different-coder-apps-domain.com

(We have a few Coder Classic customers who do this today for security reasons)

Emyrk · 2022-08-30T13:33:20Z

(We have a few Coder Classic customers who do this today for security reasons)

Yes, it's more secure to run from another domain. I think we will want to implement some sort of toggle on the BE to block the same domain in the future.

Awesome on first iteration though 👍. For now, I think I'll just throw a cookie on both .domain and domain to handle this. We will need to tighten security in future.

coderd/subdomain.go

bpmct · 2022-08-30T15:42:23Z

For named applications, will this PR make the relative_path property for coder_app function as expected? (e.g. links from to apps from the web UI will go to the subdomain)

Emyrk · 2022-08-30T16:43:42Z

For named applications, will this PR make the relative_path property for coder_app function as expected? (e.g. links from to apps from the web UI will go to the subdomain)

It will not. The deployment still needs to setup their certs with wildcards to support this feature. So relative_path still requires some deployment side config. We need to figure out how to go about displaying this 🤔

The subdomain url will work, but the FE still has the path based link

coderd/httpapi/username.go

coderd/subdomain.go

kylecarbs · 2022-08-31T13:58:23Z

@Emyrk since our session_token is HTTPOnly, is there any way to steal the cookie even with a malicious application? I'm thinking no, but I'm not sure.

f0ssel · 2022-08-31T14:25:26Z

@Emyrk since our session_token is HTTPOnly, is there any way to steal the cookie even with a malicious application? I'm thinking no, but I'm not sure.

We also strip the session token header on the proxy as well, correct?

Emyrk · 2022-08-31T15:10:47Z

@Emyrk since our session_token is HTTPOnly, is there any way to steal the cookie even with a malicious application? I'm thinking no, but I'm not sure.

We strip the cookie in the proxy app:

coder/coderd/workspaceapps.go

Lines 186 to 188 in 0578588

    
           for _, cookieHeader := range cookieHeaders { 
        
           	r.Header.Add("Cookie", httpapi.StripCoderCookies(cookieHeader)) 
        
           }

There is something you can do though with a malicious app. You can send authenticated requests to coderd. This is an issue I put in the description. We should not allow sharing apps until we close that

BrunoQuaresma

LGTM

BrunoQuaresma · 2022-09-09T17:19:39Z

@deansheather right now, I'm doing this in the FE:

# Subdomain
`${port}--${agentName}--${workspaceName}--${username}.dev.coder.com`

# Subpath
`https://dev.coder.com/@${username}/${workspaceName}/apps/${port}`

Is that right?

deansheather · 2022-09-09T17:21:40Z

Yes @BrunoQuaresma, but the subpath should be /@$username/$workspace.$agent/apps/$port. The port is also interchangeable with an application name in both cases

coderd/workspaceapps.go

kylecarbs · 2022-09-12T16:00:33Z

coderd/workspaceapps.go

+// ParseSubdomainAppURL parses an application from the subdomain of r's Host
+// header. If the subdomain is not a valid application URL hostname, returns a
+// non-nil error.
+//
+// Subdomains should be in the form:
+//
+//	{PORT/APP_NAME}--{AGENT_NAME}--{WORKSPACE_NAME}--{USERNAME}
+//	(eg. http://8080--main--dev--dean.hi.c8s.io)
+func ParseSubdomainAppURL(hostname string) (ApplicationURL, error) {
+	subdomain, rest, err := SplitSubdomain(hostname)
+	if err != nil {
+		return ApplicationURL{}, xerrors.Errorf("split host domain %q: %w", hostname, err)
+	}
+
+	matches := appURL.FindAllStringSubmatch(subdomain, -1)
+	if len(matches) == 0 {
+		return ApplicationURL{}, xerrors.Errorf("invalid application url format: %q", subdomain)
+	}
+	matchGroup := matches[0]
+
+	appName, port := AppNameOrPort(matchGroup[appURL.SubexpIndex("AppName")])
+	return ApplicationURL{
+		AppName:       appName,
+		Port:          port,
+		AgentName:     matchGroup[appURL.SubexpIndex("AgentName")],
+		WorkspaceName: matchGroup[appURL.SubexpIndex("WorkspaceName")],
+		Username:      matchGroup[appURL.SubexpIndex("Username")],
+		BaseHostname:  rest,
+	}, nil
+}


I think httpapi might be a better spot for this too, but it's a bit weird, so chef's choice on this one. Regardless, I think it'd be better to remove this from being exposed and use an _internal.go test for it.

how do you do the internal test thing? I've never seen that before

Moved to httpapi

cli/server.go

kylecarbs · 2022-09-12T16:02:35Z

coderd/workspaceapps.go

+	// We only support setting this cookie on the access URL subdomains. This is
+	// to ensure we don't accidentally leak the auth cookie to subdomains on
+	// another hostname.
+	appCookie.Domain = "." + api.AccessURL.Hostname()


This is interesting... I didn't know we could apply to subdomains without requiring an authentication redirect, this is awesome.

We probably won't be doing this soon as we'll be implementing proper devurl auth similarly to v1

@kylecarbs this is a stop gap. It will not work on apps hosted on another domain.

Also currently using the same full auth token. We should used scoped auth tokens.

kylecarbs

@deansheather are we going to use [relative_path}(https://registry.terraform.io/providers/coder/coder/latest/docs/resources/app#relative_path) to determine the client-side routing?

It seems like we'll need to add a server-side option for the wildcard path so we can redirect users on the frontend properly, or we could assume a wildcard above the root, but we'd just have to document that.

deansheather · 2022-09-12T16:22:49Z

It seems like we'll need to add a server-side option for the wildcard path so we can redirect users on the frontend properly, or we could assume a wildcard above the root, but we'd just have to document that.

@kylecarbs this PR only does wildcards underneath the site --access-url (i.e. dev.coder.com => app--agent--ws--user.dev.coder.com), and not if the access URL is an IP address as that's obviously not supported.

In a future PR when we implement proper devurl auth (with their own tokens, similar to v1) we will be able to support separate domains, and it'll be configurable through a separate --devurl-domain flag or something.

This PR is the MVP for hostname based routing

sreya

Seems reasonable overall

sreya · 2022-09-12T16:15:35Z

coderd/users.go

+	devurlCookie := api.applicationCookie(cookie)
+	if devurlCookie != nil {
+		http.SetCookie(rw, devurlCookie)
+	}


api.setAuthCookie

coderd/workspaceapps.go

bpmct

The frontend uses an environment variable which decides whether links in the dashboard should go to a wildcard or path (if the admin didn't set up wildcard DNS, for example).

coder/site/webpack.common.ts

Line 17 in 38a0f7b

CODER_ENABLE_WILDCARD_APPS: "",

What do you think about having this also impact the functionality (e.g. whether the app is routed) versus it being purely a cosmetic/dashboard variable?

If it's the latter, perhaps we should rename the var in #3812. I suggested the name, but originally assumed it'd be used across both.

deansheather · 2022-09-12T16:56:02Z

@bpmct the env var can be temporary until we do the changes I said in my above comment. I'll add a check for the env var

deansheather · 2022-09-12T17:00:58Z

Actually @bpmct that's a build time env var, not a proper one. I'm just going to merge this as is and we can figure out what we want to do about it when we solve the devurl auth problem

changes done

kylecarbs

My comment mentioning relative_path indicates that in its current form, there's no way to access a wildcard application from the frontend. We should respect that value, and direct a user to the subdomain if it's specified (maybe it's called wildcard instead, so we don't break current behavior).

We also need to specify whether the wildcard is enabled or not, and have that information surfaced to the frontend so the "Open Port" button open in a subsequent PR functions.

coderd/users_test.go

kylecarbs · 2022-09-12T17:53:11Z

coderd/workspaceapps_test.go

+		}).String()
+	}
+
+	t.Run("NoAuthShould401", func(t *testing.T) {


Could we redirect to /login with a redirect query parameter back to the subdomain URL instead? Seems like this would be a questionable UX.

I've made it redirect to login but it's impossible to redirect back to the subdomain because we can't determine whether the accessed scheme was HTTP or HTTPS. If users are using a reverse proxy in front of coder (which they likely will) then coder will think it's receiving HTTP traffic when the actual scheme is HTTPS.

deansheather · 2022-09-12T18:07:40Z

@kylecarbs Bruno is working on this in a separate frontend PR. He will be reading that value to figure out whether to link to wildcard or path-based route.

BrunoQuaresma · 2022-09-12T18:45:25Z

@kylecarbs here is the related PR #3812

coderd/workspaceapps.go

Emyrk added 5 commits August 26, 2022 08:52

chore: Add subdomain parser for applications

35ee5d6

Add basic router for applications using same codepath

a7e5bd6

Merge remote-tracking branch 'origin/main' into stevenmasley/unnamed-…

03c697d

…apps

Handle ports as app names

3e30cdd

Add comments

8397306

Cleanup

f994ec3

Emyrk added 2 commits August 30, 2022 09:48

Linting

fda80b8

Push cookies to subdomains on the access url as well

6b09a0f

Emyrk marked this pull request as ready for review August 30, 2022 14:03

Emyrk added 2 commits August 30, 2022 10:26

Fix unit test

634cd2e

Fix comment

82df6f1

kylecarbs requested changes Aug 30, 2022

View reviewed changes

coderd/subdomain.go Outdated Show resolved Hide resolved

bpmct mentioned this pull request Aug 30, 2022

Add link to open any port in dashboard #1624

Closed

Emyrk requested review from kylecarbs, f0ssel, sreya and mafredri August 30, 2022 16:53

Reuse regex from validation

49084e2

kylecarbs previously requested changes Aug 31, 2022

View reviewed changes

coderd/httpapi/username.go Outdated Show resolved Hide resolved

coderd/subdomain.go Outdated Show resolved Hide resolved

coderd/subdomain.go Outdated Show resolved Hide resolved

coderd/subdomain.go Outdated Show resolved Hide resolved

Emyrk added 3 commits August 31, 2022 10:57

Export valid name regex

4696bf9

Move to workspaceapps.go

b5d1f6a

Change app url name order

0578588

Import order

77d3452

BrunoQuaresma approved these changes Sep 9, 2022

View reviewed changes

devurls working + tests

f3c6645

kylecarbs reviewed Sep 12, 2022

View reviewed changes

Merge branch 'main' into stevenmasley/unnamed-apps

75c4713

sreya approved these changes Sep 12, 2022

View reviewed changes

bpmct reviewed Sep 12, 2022

View reviewed changes

Move stuff to httpapi

1f8a1f0

deansheather changed the title ~~feat: Add serving applications at / on subdomains~~ feat: Add serving applications on subdomains and port-based proxying Sep 12, 2022

deansheather added 2 commits September 12, 2022 16:56

fixup! Move stuff to httpapi

2c7bcc1

Merge branch 'main' into stevenmasley/unnamed-apps

dc0d348

deansheather requested a review from kylecarbs September 12, 2022 17:01

kylecarbs reviewed Sep 12, 2022

View reviewed changes

kyle comments

58653d4

deansheather requested a review from kylecarbs September 12, 2022 19:09

kylecarbs approved these changes Sep 13, 2022

View reviewed changes

coderd/workspaceapps.go Show resolved Hide resolved

deansheather mentioned this pull request Sep 13, 2022

Custom proxy error page #4040

Closed

deansheather added 2 commits September 13, 2022 16:09

fixup! kyle comments

5321bef

fixup! kyle comments

ad53b42

deansheather merged commit 9ab437d into main Sep 13, 2022

deansheather deleted the stevenmasley/unnamed-apps branch September 13, 2022 17:31

BrunoQuaresma mentioned this pull request Sep 13, 2022

Allow serving apps at / #3378

Closed

feat: Add serving applications on subdomains and port-based proxying #3753

feat: Add serving applications on subdomains and port-based proxying #3753

Uh oh!

Conversation

Emyrk commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

Conversation Wanted

Token Scoping + Security

Applications on another domain

Applications on subpath

At present

Future Work

Uh oh!

bpmct commented Aug 30, 2022

Uh oh!

Emyrk commented Aug 30, 2022

Uh oh!

Uh oh!

bpmct commented Aug 30, 2022

Uh oh!

Emyrk commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kylecarbs commented Aug 31, 2022

Uh oh!

f0ssel commented Aug 31, 2022

Uh oh!

Emyrk commented Aug 31, 2022

Uh oh!

BrunoQuaresma left a comment

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma commented Sep 9, 2022

Uh oh!

deansheather commented Sep 9, 2022

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!

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!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

deansheather commented Sep 12, 2022

Uh oh!

sreya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bpmct left a comment • 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!

deansheather commented Sep 12, 2022

Uh oh!

deansheather commented Sep 12, 2022

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk commented Aug 30, 2022 •

edited

Loading

Emyrk commented Aug 30, 2022 •

edited

Loading

bpmct left a comment •

edited

Loading