NotificationsYou must be signed in to change notification settings
Fork1.1k
Star11.6k

chore: add workspace proxies to the backend#7032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

deansheather merged 51 commits intomainfromdreamteam/external_proxy

Apr 17, 2023

Merged

chore: add workspace proxies to the backend#7032

deansheather merged 51 commits intomainfromdreamteam/external_proxy

Apr 17, 2023

Conversation

Copy link

Member

Emyrk commentedApr 6, 2023•
edited
Loading

What this does

This is the initial implementation of workspace proxies. It is missing functionality, and not complete. Stuff that's remaining is listed at the bottom.

This initial implementation implements the basic interface contracts that allow workspace proxies to communicate with coderd. It enables basic workspace proxying! The unit tests are setup and all passing. Token smuggling is implemented, hostname validation is implemented.

This feature is experiment flag gated and not able to be used in any production environment. Currently only unit tests can leverage this code since there's no CLI yet.

More PRs will add functionality. This is the core, and makes PRs more "bite sized".... Ok it is a big bite.

Future Work

Cli command to run external workspace proxy (feat: Add workspace proxy enterprise cli commands #7123)
Registration, deregistration of external proxies
Health monitoring
Various TODOs across workspaceapps code

Emyrkand others added3 commits

April 5, 2023 14:03

feat: Implement start of external workspace proxies

ca5b50c

Add more init code

5fc7832

feat: add proxysdk and proxy tokeng

391fe74

github-actionsbot assignedEmyrk

Apr 6, 2023

deansheather self-assigned this

Apr 6, 2023

Emyrk commented

Apr 6, 2023

View reviewed changes

enterprise/coderd/workspaceproxy.go OutdatedShow resolvedHide resolved

Comments and import cleanup

23d0a4c

kylecarbs reviewed

Apr 6, 2023

View reviewed changes

Copy link

Member

kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I know this is in draft, but wanted to take a peek! Feel free to take my comments or leave em', all just naming stuff.

enterprise/externalproxy/proxy.go OutdatedShow resolvedHide resolved

enterprise/proxysdk/client.go OutdatedShow resolvedHide resolved

Emyrkand others added15 commits

April 6, 2023 11:41

Move to wsproxy, make unit test work, update audit log resources

7cce9a2

Merge remote-tracking branch 'origin/main' into dreamteam/external_proxy

2aebe77

Add proxy token provider

020b4b5

Merge remote-tracking branch 'origin/main' into dreamteam/external_proxy

dc5af55

Begin writing unit test for external proxy

c5225ae

Add option validation

d6a1217

Fix access url passing

1e163d9

Healthz and buildinfo endpoints

e86a518

do stuff

20b44c6

Linting

68c3bb1

Check workspace proxy hostnames for subdomain apps

ec04552

Path based redirects redirect to dashboardurl

07323e5

Merge remote-tracking branch 'origin/main' into dreamteam/external_proxy

ffa8b00

Just commit something

a96a73b

use query instead of proxycache

2d7e242

Emyrk changed the title~~draft: External Workspace Proxies~~chore: External workspace proxies available in testing.

Apr 12, 2023

Emyrkand others added6 commits

April 12, 2023 13:50

Merge remote-tracking branch 'origin/main' into dreamteam/external_proxy

e80e7e0

Make gen

be25c51

MAke gen

208eaf1

Linting

6cfb62c

Bump migration

a112e29

Smuggling for path apps on proxies

d6edd29

Renames from PR feedback

50fa1ca

coadler reviewed

Apr 14, 2023

View reviewed changes

coderd/workspaceagents.go OutdatedShow resolvedHide resolved

coderd/workspaceapps/proxy.goShow resolvedHide resolved

enterprise/audit/table.goShow resolvedHide resolved

enterprise/coderd/coderdenttest/proxytest.goShow resolvedHide resolved

enterprise/coderd/workspaceproxy.goShow resolvedHide resolved

Emyrkand others added11 commits

April 17, 2023 08:39

Update enterprise/audit/table.go

b7f3b86

Co-authored-by: Colin Adler <colin1adler@gmail.com>

Renames and formatting

bb032c3

Make gen

6ab0dea

Fix compile

12c6f8d

Add comments to sql columns

224fa2f

ExternalProxy -> WorkspaceProxy

06fb88b

Remove Actor function

82d10d9

comments

dc884eb

comments

dbbd2ba

Use correct MW

1322f99

Make gen/fmt/lint

784fb68

deansheather requested a review fromkylecarbs

April 17, 2023 17:00

Group vs route to fix swagger

b72ef2f

kylecarbs reviewed

Apr 17, 2023

View reviewed changes

coderd/coderd.go

Comment on lines +372 to +373

		// Same as the first but it's optional.
		apiKeyMiddlewareOptional:=httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{

Copy link

Member

kylecarbsApr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would it be cleaner if we allowedOptional as a separate argument to this function? Then they could share configs.

Copy link

Member

deansheatherApr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There are a few values that change between the 3 instances of ExtractAPIKeyMW we have like Optional, RedirectToLogin, etc. Do you propose we add all of these as bool params? Seems janky IMO

coderd/database/queries/proxies.sql

Comment on lines +65 to +72

		-- Validate that the @hostname has been sanitized and is not empty. This
		-- doesn't prevent SQL injection (already prevented by using prepared
		-- queries), but it does prevent carefully crafted hostnames from matching
		-- when they shouldn't.
		--
		-- Periods don't need to be escaped because they're not special characters
		-- in SQL matches unlike regular expressions.
		@hostname ::text SIMILAR TO'[a-zA-Z0-9.-]+'AND

Copy link

Member

kylecarbsApr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It seems weird to me that we need to do this, and could cause someodd bugs. Why can't we do an exact match for the hostname?

Copy link

Member

deansheatherApr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You can't do exact match becauseapp--agent--workspace--username.dev.coder.com is not an exact match fordev.coder.com or*.dev.coder.com.

The specific lines you commented on is to avoid regex escapes in hostnames. It's a preventative measure to make the match more consistent and to avoid security issues.

codersdk/deployment.go OutdatedShow resolvedHide resolved

coderd/httpmw/actor.goShow resolvedHide resolved

coderd/httpmw/workspaceproxy.go

Comment on lines +24 to +26

		// The format of an external proxy token is:
		// <proxy id>:<proxy secret>
		//

Copy link

Member

kylecarbsApr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't disagree with the ease of telling a difference, it just seems weird to maintain multiple types of secret keys when we already have a standard in the product.

If we need to bump the security of our keys, we'll now have multiple places for that to be done, even though they share the same purpose.

And under this format, it's not explicit when it's a proxy key... we just know it is because of the:, right?