Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP proxy #1453

Merged
merged 20 commits into from
Apr 8, 2020
Merged

HTTP proxy #1453

merged 20 commits into from
Apr 8, 2020

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented Mar 24, 2020

This PR adds the ability to proxy HTTP ports through code-server.

To do this, you pass a domain on the command line. For example:

code-server --proxy-domain coder.com --proxy-domain test.coder.com

Domains can be in the format coder.com or *.coder.com and the flag can be repeated to add as many domains as you want.

When code-server encounters [number].[domain] in the host header it'll proxy to the port indicated by [number] if [domain] matches one of the provided proxy domains. For example, going to 8080.coder.com in this example would proxy code-server to itself.

When authenticating the cookie will be set against the highest-level proxy. So in this example if you logged in at 8080.test.coder.com the cookie would be set against coder.com since that's the highest level that matches in the provided list of domains. That way you only have to authenticate once.

Edit: /proxy/[number] will also work. No command-line flags are necessary to enable this.

@code-asher code-asher force-pushed the proxy branch 2 times, most recently from ab87523 to 89ec5fe Compare March 24, 2020 19:52
@kylecarbs
Copy link
Member

We should add some docs on this (prob throw it in the README too). Super cool feature.

@kylecarbs
Copy link
Member

We should make sure that no caching on CDNs can take place: https://support.cloudflare.com/hc/en-us/articles/115003206852-Understanding-Origin-Cache-Control

@kylecarbs
Copy link
Member

If I try to connect to an invalid port, code-server crashes with:

Error: connect ECONNREFUSED 127.0.0.1:8080
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1129:14) {
  errno: 'ECONNREFUSED',
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 8080
}

@t-d-d
Copy link

t-d-d commented Mar 28, 2020

Is it possible to use a (sub-)path instead or as well as the host name?

@benz0li
Copy link
Contributor

benz0li commented Mar 28, 2020

@t-d-d See #1309 (comment)

[...] I didn't mention it in the PR but /proxy/ will also work.

Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

don't understand the codebase too well so sry for all the questions

src/node/app/proxy.ts Outdated Show resolved Hide resolved
src/node/app/proxy.ts Outdated Show resolved Hide resolved
src/node/app/proxy.ts Outdated Show resolved Hide resolved
src/node/app/proxy.ts Outdated Show resolved Hide resolved
src/node/app/proxy.ts Outdated Show resolved Hide resolved
src/node/app/proxy.ts Outdated Show resolved Hide resolved
@@ -118,6 +124,7 @@ const main = async (args: Args): Promise<void> => {

if (typeof sshPort !== "undefined") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have a SSH server?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this has something to do with the Chrome extension that was planned but I'm not 100% sure on the status of that. @kylecarbs @wbobeirne

Choose a reason for hiding this comment

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

Yes, this was for the browser extension. That's pretty much on ice at this point, so I think it's probably best for safety / code cleanliness that we take it out since nothing is using it right now, and it's probably a liability to have such a powerful thing up by default. Or at the very least, disable by default.

Would be happy to make a PR to remove that if we're definitely not going to pursue the extension, but I'll defer to mr @kylecarbs on that.

doc/FAQ.md Outdated Show resolved Hide resolved
@kylecarbs
Copy link
Member

@cmoog I'd love your thoughts on this. Especially in relation to the docs/setup.

doc/FAQ.md Outdated Show resolved Hide resolved
- Add type to HTTP options.
- Fix certificate message always saying it was generated.
- Dedent output not directly related to the HTTP server.
- Remove unnecessary comma.
This will be used for proxying ports.
This allows it to be used in subdomains.
It'll be able to handle /proxy requests as well as subdomains.
The cookie will be set for the proxy domain so it'll work for all of its
subdomains.
This simplifies the logic a bit.
Otherwise they'll crash code-server.
Otherwise the request will just hang.
@code-asher code-asher merged commit 5aded14 into master Apr 8, 2020
@code-asher code-asher deleted the proxy branch April 8, 2020 17:44
@Grimeton
Copy link

This PR adds the ability to proxy HTTP ports through code-server.

To do this, you pass a domain on the command line. For example:

code-server --proxy-domain coder.com --proxy-domain test.coder.com

Domains can be in the format coder.com or *.coder.com and the flag can be repeated to add as many domains as you want.

When code-server encounters [number].[domain] in the host header it'll proxy to the port indicated by [number] if [domain] matches one of the provided proxy domains. For example, going to 8080.coder.com in this example would proxy code-server to itself.

When authenticating the cookie will be set against the highest-level proxy. So in this example if you logged in at 8080.test.coder.com the cookie would be set against coder.com since that's the highest level that matches in the provided list of domains. That way you only have to authenticate once.

Edit: /proxy/[number] will also work. No command-line flags are necessary to enable this.

Make sure to add some documentation on this and disable it by default. This is SUPER DANGEROUS. A lot of systems run on 127.0.0.1 without any security in place just relying on the loopback interface and the fact that net.ipv4.conf.all.route_localnet is set to 0 which disables routing of the 127/8 subnet, not even iptables will make anything happen here while this is 0.

When your proxy allows me to test all the ports on the local system just by iterating over /proxy/$number this will become really dangerous.

Yes, independent if the testing system has a cookie set or not.

I usually run code-server only behind nginx with the auth_request module against some http auth, e.g. Nextloud. (http://nginx.org/en/docs/http/ngx_http_auth_request_module.html)

This blocks everyone and everything from accessing anything code-server without auth.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 19, 2020

@Grimeton not sure I follow, you still have to be authorized, you can't just iterate over every port.

@mklechan
Copy link

mklechan commented Dec 9, 2020

How do I disable this feature? Is that possible?

@nhooyr
Copy link
Contributor

nhooyr commented Dec 10, 2020

How do I disable this feature? Is that possible?

Why do you want to disable it?

@sar
Copy link

sar commented Dec 15, 2020

It'd be great to add docs on nginx rewrite, proxy_pass rules for /proxy/[number] usage.

Currently this requires modifying app source as requests on https://$host:$server_port/proxy/$1 will end up at base path HTTP GET 404 /common.js instead of https://$host:$server_port/proxy/[number]/common.js.

@code-asher
Copy link
Member Author

code-asher commented Dec 15, 2020 via email

@nhooyr
Copy link
Contributor

nhooyr commented Dec 15, 2020

I was thinking maybe we should just stop doing that since I think most
applications don't use relative paths.

Perhaps we do need to make this configurable. Just seems wrong to me that people would rather want to specify a base path in their app vs just doing this relatively like HTTP supports.

@code-asher
Copy link
Member Author

code-asher commented Dec 16, 2020

Just seems wrong to me that people would rather want to specify a base path in their app vs just doing this relatively like HTTP supports.

Agreed! I thought the same thing when we initially wrote the proxy haha. But now that a few issues/comments have been made I'm starting to suspect that using relative paths is quite rare.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 18, 2020

Agreed! I thought the same thing when we initially wrote the proxy haha. But now that a few issues/comments have been made I'm starting to suspect that using relative paths is quite rare.

If we made it configurable, then the path user applications expect is going to be coupled with the port they're listening on. and it'll be extremely weird to access on localhost lol.

I'm inclined to just write a FAQ entry on how why relative paths are what you should use instead.

@nhooyr
Copy link
Contributor

nhooyr commented Dec 18, 2020

Opened #2485

@erlloyd
Copy link

erlloyd commented Jan 6, 2021

Just to further the conversation, some SPA frameworks (like react via create-react-app, for example) actually set up configuring the base path "in the app" (it's handled by react-scripts, in this case) by default. See my comments here on why this is problematic. I'd like to encourage you to still consider making this configurable at the very least: #2222 (comment)
#2222 (comment)

@melissa-barca
Copy link

I am also interested in an option to disable this for reasons similar to those @Grimeton mentioned. The predictable path opens the door for CSRF attacks which occur when an authorized user is tricked into running code by an attacker: https://owasp.org/www-community/attacks/csrf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.