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

updated websockify to be compatible with python3 #400

Closed
wants to merge 9 commits into from

Conversation

rpodgorny
Copy link

file() and has_key() are long gone in new python. still, the new syntax works fine in python2.

@kanaka
Copy link
Member

kanaka commented Sep 30, 2014

@rpodgorny the changes to support python 3 look good but you have a host/port patch mixed in with the python 3 support patches.

Please see novnc/websockify#3 for discussion of why allowing the client (JS) to specify the host and port is a security concern.

@rpodgorny
Copy link
Author

ah! i've made the original patch, then created the pull request, then added more changes. i thought github pull requests are at patch-level and now it seems they work on whole branches. i will rework it and resubmnit.

as for the direct host/port specification, i do realize the security implications but that's why this has to be specified at the command line explicitly. sooner or later someone needs this functionality and it should be left to the user whether it's worth the risk or not.

@rpodgorny rpodgorny closed this Oct 1, 2014
@kanaka
Copy link
Member

kanaka commented Oct 1, 2014

@rpodgorny yeah, pull requests are against a branch and not a commit, so any new changes to that branch also update the pull request. It's a little weird, but it does allow for easy update of pull requests as is often the case before a pull request is merged.

Enabling the direct host/port support on the command line isn't enough. The host and port need to be validated by something that the administrator controls before a connection is allowed. Allowing the client to choose the target without validation is especially egregious because it is a choice made by untrusted JS code running in the browser that is able to do this. And the websockify service is often going to be running on the inside of people's firewalls which means it silently bypasses the best defense that exists against network attacks. It looks like a web server because it runs on port 80/443, but it would effectively be a webserver that has been completely compromised. Even worse, since the purpose of websockify is often to connect to VNC desktops (using noVNC), it probably is not protected by the same DMZ firewall rules that webservers usually are.

All that to say, do this at your own risk. But websockify won't be including this sort of functionality and please don't encourage others to do this.

The --target-config option is designed for situations where you have a lot of host/port targets and need them to be dynamically updated (it takes a directory of configs and re-reads them every time a new client connection is made). Often the solution used in practice is create an authenticated web UI that modifies the target config to with an token for a particular host/port target and the web UI opens the noVNC page using the token just created (the config can be removed after a few seconds because existing websockify connections stay open). Perhaps you could consider something like that.

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.

2 participants