Allow proxying to remote host

[jacobtomlinson]

This PR fixes #152 by allowing you to proxy to another host using the format:

/proxy/{host}:{port}/{path}

I appreciate that there may be some security concerns around this but considering that Jupyter already provides a remote execution environment and a bad actor could simply use nc along with the existing proxy to acheive port forwarding to any host within the network I don't think this increases the existing attack surface.

Feedback and discussion very welcome!

[ian-r-rose]

I can take a look at this later, but for the moment there is some discussion of the security implications of this in #128.

[yuvipanda]

Thanks for opening this, @jacobtomlinson!

I'm mostly worried about bypassing cross origin protection in other third party websites through this from malicious js that can't otherwise run backend code.

I'd like us to whitelist domains with a configurable here to whitelist domains. How does that sound?

[jacobtomlinson]

Ah I see.

My only concern is that the dask-kubernetes service names are random UUIDs and so will be unique for every cluster. Could there be a way to dynamically whitelist? A fixed whitelist wouldn't work for this use case.

[manics]

How about allowing a regex or glob in the whitelist?

[jacobtomlinson]

That would work, are you happy with that @yuvipanda?

[todo]

Get whitelist from config

jupyter-server-proxy/jupyter_server_proxy/handlers.py

Lines 171 to 176 in 87d4955

	# TODO Get whitelist from config
	whitelist = [r'localhost']
	return any([bool(re.match(pattern, host)) for pattern in whitelist])
	@web.authenticated
	async def proxy(self, host, port, proxied_path):

---

This comment was generated by todo based on a TODO comment in 87d4955 in #154. cc @jacobtomlinson.

[jacobtomlinson]

I've quickly implemented this using a regex based whitelist. I'm not familiar enough with the project to have a feeling for the best way to get the config from within the handler.

https://github.com/jupyterhub/jupyter-server-proxy/pull/154/files#diff-bc4411a876f25fa78f564f274f779fd9R171

Coud someone please point me in the right direction here?

[mrocklin]

Checking in, what is the current status on this? @jacobtomlinson is this blocking the dask-kubernetes rewrite and release?

[manics]

I've just looked through some of the related issues, I'm a bit confused as to how in your use case (internally hosted dask dashboard) a user will know the /proxy/{host}:{port}/{path} to go to since it's an external process, not managed by jupyter-server-proxy's simpervisor.

For config I'd guess @yuvipanda meant adding a new top-level config property to

jupyter-server-proxy/jupyter_server_proxy/config.py

Line 117 in 0e67e1a

class ServerProxy(Configurable):

[jacobtomlinson]

@manics Dask knows the address of the dashboard. This is provided programmatically to the Dask JL Extension and also the widget (with a link the user clicks).

Thanks for the pointer to the config!

@mrocklin yeah this is blocking the release.

[yuvipanda]

When me and @ian-r-rose had this conversation last, we ended up with https://github.com/dask/dask-labextension/blob/master/dask_labextension/dashboardhandler.py#L16. You can proxy to remote hosts by overriding this in your own extension. Does your rewrite have your own extension to put that code in? If so, IMO that's the 'right' thing to do - you control the hosts, but you also control the URL path to be whatever you want. If it's in config, you still have to find a way to add it to the jupyter_notebook_config.py file.

If you do want this to be a config, I think having that be function that can dynamically take a hostname and return True / False is better than a RegEx.

Thank you for working on this! I'm sorry about the tardy responses. Happy to spend some time in a more immediate medium (chat / voice) if that'll help.

[yuvipanda]

As for creating config, you'll have to use traitlets

nbresuse offers useful example:

1. https://github.com/yuvipanda/nbresuse/blob/master/nbresuse/__init__.py#L56 creates a Config object, and defines the aspects that can be configurable
2. https://github.com/yuvipanda/nbresuse/blob/master/nbresuse/__init__.py#L96 it's populated from jupyter_notebook_config.py (and other files Jupyter checks) and put in a place that web handlers can see
3. Accessed in the handlers https://github.com/yuvipanda/nbresuse/blob/master/nbresuse/__init__.py#L17

[jacobtomlinson]

@yuvipanda This isn't for an extension so I don't think I have anywhere to write a custom handler.

Not used traitlets yet, looks neat. I'll have a go at adding it, but if I struggle I may ping you for a sync.

[rcthomas]

@jacobtomlinson is the only thing needed here the traitlets config stuff for the whitelist function?

[jacobtomlinson]

Yes but this has dropped down my priority list slightly.

[rcthomas]

@jacobtomlinson if you review+merge the PR I just sent you then I think that might address the configuration question and then @yuvipanda can review.

[jacobtomlinson]

@rcthomas done, many thanks for pushing this along.

[rcthomas]

There might be another thing to fix here I realized. The handlers need to be swapped (make Remote handler first) or the regex match in the handlers refined more. When I was testing my fix I was using a setup that had a Docker network and a name like "service1:8000" and that matched the proper handler. But ordering matters, and when I tried late last night with "10.128.0.20:8787" it matched the Local one (because \d's matched). I did a live fix swapping the handlers and that made the behavior correct.

[rcthomas]

I think that looks good. @yuvipanda What do you think?

[yuvipanda]

Traitlet nitpicking, but otherwise +1 I think.

[yuvipanda]

Thanks, @rcthomas and @jacobtomlinson! I think this is good to go once the merge conflict is resolved.

[rcthomas]

I think it's just here https://github.com/jupyterhub/jupyter-server-proxy/blob/master/jupyter_server_proxy/config.py#L12 @jacobtomlinson do you want to handle that? I probably should have just submitted a different PR directly, but we're almost done here...

[yuvipanda]

w00t, thanks @rcthomas and @jacobtomlinson!

I'll try make a release shortly

[mrocklin]

Thanks everyone!

…

On Fri, Nov 15, 2019 at 9:41 AM Yuvi Panda ***@***.***> wrote: w00t, thanks @rcthomas <https://github.com/rcthomas> and @jacobtomlinson <https://github.com/jacobtomlinson>! I'll try make a release shortly — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#154?email_source=notifications&email_token=AACKZTDWCQC3W7ZLYKHOLLLQT3NM7A5CNFSM4I4KLCI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEGFVFQ#issuecomment-554457750>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKZTHCOKBUF36XUMPQOHTQT3NM7ANCNFSM4I4KLCIQ> .

[jacobtomlinson]

Thanks all!

[yuvipanda]

Released!