Add authentication plugin capabilities to node version of websockify (v2) #285

samfrances · 2017-05-24T15:12:22Z

Duplicate of pull request #282 , but without the unwanted merge commits.

I have adapted the node.js version of Websockify, to take advantage of the ws library's authorization capabilities. Authorisation plugins can now be specified using the --auth-plugin and --auth-source command line arguments. --auth-plugin specifies the <module>.<member> function to use as an auth plugin. --auth-source provides additional configuration as a string, which is passed as the first argument to the plugin function. This command line interface mirrors that in the Python version of websockify.

Authorisation plugins are functions that take a source argument, and return a function that can be used as the verifyClient option to the ws.WebSocket.Server class from https://github.com/websockets/ws.

The following abridged extract from the ws module documentation at https://github.com/websockets/ws/blob/master/doc/ws.md should clarify:

new WebSocket.Server(options[, callback])

options {Object}
...

verifyClient {Function} A function which can be used to validate incoming
connections. See description below.

...

callback {Function}

Create a new server instance. One of port, server or noServer must be
provided or an error is thrown.

If verifyClient is not set then the handshake is automatically accepted. If
it is is provided with a single argument then that is:

info {Object}

origin {String} The value in the Origin header indicated by the client.

req {http.IncomingMessage} The client HTTP GET request.

secure {Boolean} true if req.connection.authorized or
req.connection.encrypted is set.

The return value (Boolean) of the function determines whether or not to accept
the handshake.

if verifyClient is provided with two arguments then those are:

info {Object} Same as above.

cb {Function} A callback that must be called by the user upon inspection
of the info fields. Arguments in this callback are:

result {Boolean} Whether or not to accept the handshake.

code {Number} When result is false this field determines the HTTP
error status code to be sent to the client.

name {String} When result is false this field determines the HTTP
reason phrase.

URL

environment variable

ever for demo purposes, never used in any sort of production)

kanaka · 2017-05-26T20:36:26Z

other/js/auth_plugin_examples.js

@@ -0,0 +1,90 @@
+/*
+ * An auth plugin must be a function which returns a function conforing to the


conforing -> conforming

Fixed in dc31fab

could you squash dc31fab into 7d6f638 ?

kanaka · 2017-05-26T20:45:29Z

other/js/auth_plugin_examples.js

+     * Parse the url path, extract the `token` querystring value, and check if
+     * it matches the token argument. If verbose is set to true, log messages
+     * are enabled.
+     *


The "token" query parameter already has a different meaning in the main python implementation which is used for selecting among multiple target hosts: https://github.com/novnc/websockify/wiki/Token-based-target-selection. The python implementation also supports basic authentication via the Authorization header. I think it would be better to have an example of basic auth as that would probably be more generally applicable. But certainly it shouldn't be using "token" for authentication.

Replaced token based auth examples with origin-based examples (these being easier to test out using NoVNC than basic auth): a543c35

kanaka · 2017-05-26T20:54:28Z

other/js/websockify.js

+    let plugin_name = auth_plugin_arg.pop();
+    let module_path = auth_plugin_arg.join(".");
+
+    let auth_plugin = require(module_path)[plugin_name];


In node it's not unusual for the module itself to have a single export (exports = myFunc). That case should probably be supported.

Addressed in ff1897e

kanaka · 2017-05-26T20:57:08Z

other/js/websockify.js

+    let auth_source = argv["auth-source"] || undefined;
+    websocket_server_opts = {
+        server: webServer,
+        verifyClient: auth_plugin(auth_source)


What about the case (that is symmetrical to the python version) where auth_plugin is a class function and not just a plain function returning the auth function?

I think it would actually be more idiomatic JavaScript to have the auth_source bound as the first argument of an authorizer function rather than generating a new function in the auth_plugin itself. For example:

verifyClient: auth_plugin.bind(auth_plugin, auth_source)

Obviously, the structure of your example authentication function would change (one less layer of abstraction).

Addressed in 5c3ac52. The auth plugin can now either be a class with a authenticate method (similar to the Python version), or a factory function which returns an object with an authenticate method.

samfrances · 2017-05-26T23:43:10Z

Thanks for the review points, @kanaka . I'll get onto those ASAP.

CendioOssman · 2017-10-18T14:51:24Z

How are the fixes going?

samfrances · 2017-10-18T15:00:27Z

Sorry @CendioOssman , this has been on the back burner for me. I'll get on it this weekend. Thanks for the reminder.

issue with ws library retiring upgradeReq property

samfrances · 2017-11-03T18:39:42Z

@CendioOssman @kanaka I believe I have addressed all of the review comments now. See what you think.

CendioOssman · 2017-11-16T11:35:06Z

@kanaka?

kanaka · 2017-12-09T20:46:06Z

@samfrances I would love to be able to review this in detail, but I just haven't been able to find the time lately. @CendioOssman if you have time to do brief review and maybe test it quickly, I have no objection to merging it.

@samfrances I guess the only other question I have is if we get issues filed about this functionality are you willing to follow them up?

samfrances · 2017-12-11T10:25:43Z

@kanaka Certainly happy to follow up on any issues.

CendioOssman · 2017-12-19T14:16:51Z

I can try to review it, but right now I'm a bit swamped with other things.

samhed

Hi @samfrances. Thank you for working on this! Apologies for the very long response times.

I would like to get this merged. I had a look at your commits and want to ask you to clean it up a bit. It would be nice if we could avoid "correct typo from previous commit"-types of commits.

Also if you add something in one of the earlier commits only to be removed later, you should consider if that first commit was relevant in the first place.

If you can look over your commits I think we can finish this up soon.

samhed · 2018-03-06T15:05:07Z

other/js/websockify.js

@@ -92,7 +93,7 @@ http_request = function (request, response) {

    var uri = url.parse(request.url).pathname
        , filename = path.join(argv.web, uri);
-    
+


This white space change is not really close to other modified code. I would prefer if you could move it to a separate commit

samhed · 2018-03-06T15:08:15Z

other/js/auth_plugin_examples.js

@@ -0,0 +1,90 @@
+/*
+ * An auth plugin must be a function which returns a function conforing to the


could you squash dc31fab into 7d6f638 ?

samhed · 2018-03-06T15:11:31Z

other/js/auth_plugin_examples.js

-     * a token provided as the argument to the --auth-source command line
-     * argument
+     * a token provided as the argument to the `--auth-source` command line
+     * argument.


Could you squash the changes from this commit (22ffd2c) into the commit that originally introduced these comments please

samhed · 2018-03-06T15:12:46Z

other/js/password.txt

@@ -1 +0,0 @@
-passpass12


Squash this commit (e5e0e5d) with 0e1824c please

samhed · 2018-03-06T15:15:30Z

other/js/websockify.js

    var clientAddr = client._socket.remoteAddress, log;
-    console.log(upgradeReq ? upgradeReq.url : client.upgradeReq.url);
+    console.log(req ? req.url : client.req.url);


You should rebase your changes on master to avoid commits like this one

samfrances · 2019-07-05T10:53:20Z

Closing, as this was from a while ago and I don't think I'll have time to do the requested changes.

samfrances added 8 commits May 5, 2017 12:46

Add option of providing authorization plugin

845f9f2

Add basic example auth plugin that checks the NoVNC token from the

7d6f638

URL

Refactor example auth functions, and add one which gets password from

0991e25

environment variable

Add example auth plugin that reads password from a file

4143b00

Add log message for no token provided

0e1824c

Add comments to example authorisation plugins

c6d7444

Update comments

22ffd2c

Remove demo password file (no need to remove history, as it was only

e5e0e5d

ever for demo purposes, never used in any sort of production)

samfrances mentioned this pull request May 24, 2017

Add authentication plugin capabilities to node version of websockify #282

Closed

kanaka requested changes May 26, 2017

View reviewed changes

Correct typo in comments

dc31fab

samfrances added 6 commits November 3, 2017 15:12

Allow for auth plugin to be a class or non-class function, and resolve

5c3ac52

issue with ws library retiring upgradeReq property

Rename upgradeReq in http_client to make identical with master

41cd3f8

Correct typo

94fba81

Add ability to handle auth modules which export a single function

ff1897e

Replace token auth examples

a543c35

Es6-ify variable declarations

078f5cd

CendioOssman assigned kanaka Nov 9, 2017

CendioOssman added feature New feature or request js labels Nov 9, 2017

kanaka mentioned this pull request Jan 16, 2018

multiple targets apply #200

Closed

samhed requested changes Mar 6, 2018

View reviewed changes

samfrances closed this Jul 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authentication plugin capabilities to node version of websockify (v2) #285

Add authentication plugin capabilities to node version of websockify (v2) #285

samfrances commented May 24, 2017

kanaka May 26, 2017

samfrances Jun 6, 2017

samhed Mar 6, 2018

kanaka May 26, 2017

samfrances Nov 3, 2017

kanaka May 26, 2017

samfrances Nov 3, 2017

kanaka May 26, 2017

samfrances Nov 3, 2017

samfrances commented May 26, 2017

CendioOssman commented Oct 18, 2017

samfrances commented Oct 18, 2017 •

edited

samfrances commented Nov 3, 2017

CendioOssman commented Nov 16, 2017

kanaka commented Dec 9, 2017 •

edited

samfrances commented Dec 11, 2017

CendioOssman commented Dec 19, 2017

samhed left a comment

samhed Mar 6, 2018

samhed Mar 6, 2018

samhed Mar 6, 2018

samhed Mar 6, 2018

samhed Mar 6, 2018

samfrances commented Jul 5, 2019

		@@ -0,0 +1,90 @@
		/*
		* An auth plugin must be a function which returns a function conforing to the

		@@ -92,7 +93,7 @@ http_request = function (request, response) {

		var uri = url.parse(request.url).pathname
		, filename = path.join(argv.web, uri);

		@@ -1 +0,0 @@
		passpass12

Add authentication plugin capabilities to node version of websockify (v2) #285

Add authentication plugin capabilities to node version of websockify (v2) #285

Conversation

samfrances commented May 24, 2017

new WebSocket.Server(options[, callback])

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samfrances commented May 26, 2017

CendioOssman commented Oct 18, 2017

samfrances commented Oct 18, 2017 • edited

samfrances commented Nov 3, 2017

CendioOssman commented Nov 16, 2017

kanaka commented Dec 9, 2017 • edited

samfrances commented Dec 11, 2017

CendioOssman commented Dec 19, 2017

samhed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samfrances commented Jul 5, 2019

samfrances commented Oct 18, 2017 •

edited

kanaka commented Dec 9, 2017 •

edited