chore: add workspace proxies to the backend #7032
Conversation
Co-authored-by: Colin Adler <colin1adler@gmail.com>
| // Same as the first but it's optional. | ||
| apiKeyMiddlewareOptional := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ |
There was a problem hiding this comment.
Would it be cleaner if we allowed Optional as a separate argument to this function? Then they could share configs.
There was a problem hiding this comment.
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
| -- 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 |
There was a problem hiding this comment.
It seems weird to me that we need to do this, and could cause some odd bugs. Why can't we do an exact match for the hostname?
There was a problem hiding this comment.
You can't do exact match because app--agent--workspace--username.dev.coder.com is not an exact match for dev.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.
| // The format of an external proxy token is: | ||
| // <proxy id>:<proxy secret> | ||
| // |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
All of my comments were just naming, nothing daunting.
@deansheather feel free to merge once we reconcile the naming stuff!
🥳🥳🥳
|
@deansheather before merge lets rename the title to something like |
|
Keeping it as chore so it doesn't show up in changelog (it's not usable by users yet) |
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