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

Allow forcing insecure HTTP for local development only #48

Merged
merged 2 commits into from Apr 12, 2017

Conversation

Projects
None yet
2 participants
@jankeromnes
Copy link
Member

jankeromnes commented Mar 16, 2017

This pull request introduces two db.json security parameters:

{
  "security": {
    "forceHttp": true,
    "forceInsecure": true
  }
}

They can be used by Janitor developers to completely disable HTTPS support, Let's Encrypt certificate generation, and several security policies (like hostname verification and some HTTP headers).

Additionally, forcing "insecure mode" adds an obvious warning message to all web pages, so that any accident on a production app becomes immediately visible to all users.

It also prints all sign-in keys in the server logs, because signing in via email is generally not an option when developing locally.

Note: I also sneaked in the unrelated http.ServerResponse bug fix again, respecting your nits from the previous pull request. Sorry for making the diff view confusing! (It might help to review comments separately).

@jankeromnes jankeromnes self-assigned this Mar 16, 2017

@jankeromnes jankeromnes requested a review from bnjbvr Mar 16, 2017

@jankeromnes

This comment has been minimized.

Copy link
Member

jankeromnes commented Mar 16, 2017

@bnjbvr Could you please have a look at your convenience? :) Thanks!

@jankeromnes jankeromnes changed the title Allow forcing insecure HTTP for local development only. Allow forcing insecure HTTP for local development only Mar 16, 2017

@jankeromnes

This comment has been minimized.

Copy link
Member

jankeromnes commented Mar 16, 2017

Note: Combined with JanitorTechnology/dockerfiles#15, this will allow us to #31 🎉🎈🍕🍾

@bnjbvr

This comment has been minimized.

Copy link
Contributor

bnjbvr commented Mar 21, 2017

A few questions:

  • are there useful modes where forceHttp !== forceInsecure? If yes, can you explain them? I have issues identifying what should be guarded by each.
  • could the websocket redirection happen by a special message sent over the websocket, indicating that the client code must redirect through window.location?
@jankeromnes

This comment has been minimized.

Copy link
Member

jankeromnes commented Mar 21, 2017

Thanks a lot for having a look!

are there useful modes where forceHttp !== forceInsecure? If yes, can you explain them? I have issues identifying what should be guarded by each.

  • forceHttp is just there to make the server serve HTTP instead of HTTPS (and disable Let's Encrypt certificate generation). This can be useful on its own for example with a reverse proxy that handles caching and HTTPS certificates
  • forceInsecure basically disables all other security features, e.g. requested hostname verification, security-related HTTP headers like HSTS, CSP, iframe blocking, stricter mime types for script detection, secure cookies, and goes deeper into security hacks by printing secret email-login keys in the server logs etc. It should only ever be used for local development (which is why a visible warning is added at the top of each page), but you could use it without forceHttp to hack on something like https://localhost (Let's Encrypt generation would fail, but we could make forceInsecure && !forceHttp generate self-signed certificates)

could the websocket redirection happen by a special message sent over the websocket, indicating that the client code must redirect through window.location?

Good question! From what I understand, once the WebSocket handshake is complete and you upgrade to binary transfer, there is no way to push a special message (e.g. a script) to browsers and make them redirect, except if you add such a feature to your own custom protocol (but we're proxying noVNC connections, and I don't think they implement such a redirect feature).

However, you could in theory reply with a "302 Found" HTTP status during the WebSocket handshake, but it seems that browsers are allowed to ignore these (see websockets/ws#812) and I believe there would be worrying security implications if browsers accepted redirections.

In practice, dropping the connection will just show a "Connection failed" error in the cached noVNC interface, and a simple page refresh will allow the first request (which is not a WebSocket upgrade) to be hijacked for a transparent OAuth2 redirection and round-trip. ("Connection failed" error, the user refreshes, now it works again).

@jankeromnes jankeromnes force-pushed the jankeromnes:localdev branch from d49efd9 to 0f6a934 Mar 28, 2017

@jankeromnes

This comment has been minimized.

Copy link
Member

jankeromnes commented Mar 28, 2017

Note: Rebased!

Edit: On top of the recent quick fix to lib/routes.js.

@bnjbvr

bnjbvr approved these changes Apr 3, 2017

Copy link
Contributor

bnjbvr left a comment

Approved, makes sense, thanks!

app.js Outdated
documentRoot: process.cwd() + '/static',
port: ports.https,
secure: true,
secure: security.forceHttp !== true,

This comment has been minimized.

@bnjbvr

bnjbvr Apr 3, 2017

Contributor

Just an opinion, but checking only for !security.forceHttp would allow 0 and 1 as possible values, which could be intuitively used in this context. (There are a few other places where it is assumed that security.forceHttp is either true or anything else; if you want to enforce a certain subset of possible values, maybe worth asserting these somewhere and documenting this)

This comment has been minimized.

@jankeromnes

jankeromnes Apr 11, 2017

Member

I thought it would be safer to only accept security.forceInsecure === true as the exact value that disables important security features, but maybe it isn't much more secure. I'll make it so that both forcing variables can be truthy/falsy regardless of the exact value.

@@ -47,7 +48,7 @@ exports.get = function (request, callback) {
if (request.cookies) {
request.cookies.set('token', token, {
expires: new Date('2038-01-19T03:14:07Z'),
secure: true
secure: security.forceInsecure !== true

This comment has been minimized.

@bnjbvr

bnjbvr Apr 3, 2017

Contributor

You can also hoist the value of secure in the global context, since if I understand correctly, forceInsecure can't be switched at runtime.

@@ -63,7 +64,7 @@ exports.destroy = function (request, callback) {
// Destroy the cookie.
request.cookies.set('token', '', {
overwrite: true,
secure: true
secure: security.forceInsecure !== true

This comment has been minimized.

@bnjbvr

bnjbvr Apr 3, 2017

Contributor

(see above remark)

@@ -110,17 +111,26 @@ exports.sendLoginEmail = function (email, request, callback) {
return 'Janitor Sign-in link';
},
htmlMessage (key) {
const url = (security.forceHttp === true ? 'http' : 'https') + '://' +

This comment has been minimized.

@bnjbvr

bnjbvr Apr 3, 2017

Contributor

Here the prefix could be hoisted as well.

@@ -164,6 +165,13 @@ function ensureOAuth2Access (request, response, next) {
return;
}

// We can only use `http.ServerResponse`s to initiate OAuth2 authentication,
// not raw `net.Socket`s (as in WebSocket connections).
if (!(response instanceof http.ServerResponse)) {

This comment has been minimized.

@bnjbvr

bnjbvr Apr 3, 2017

Contributor

Maybe log something on the console? Then you'll know if it's a recurrent issue and it means more attention.

This comment has been minimized.

@jankeromnes
@jankeromnes

This comment has been minimized.

Copy link
Member

jankeromnes commented Apr 4, 2017

💯👍🎆 Yay! Thanks a lot for yet another great review @bnjbvr 😄 Your remarks are very helpful, and I'll try to reply to and act on them shortly.

@jankeromnes jankeromnes force-pushed the jankeromnes:localdev branch from 0f6a934 to 6ff8008 Apr 12, 2017

@jankeromnes

This comment has been minimized.

Copy link
Member

jankeromnes commented Apr 12, 2017

Note: Rebased + all nits addressed!

@jankeromnes jankeromnes force-pushed the jankeromnes:localdev branch from 6ff8008 to 15e439b Apr 12, 2017

@jankeromnes jankeromnes merged commit 7e054a9 into JanitorTechnology:master Apr 12, 2017

@jankeromnes jankeromnes deleted the jankeromnes:localdev branch Apr 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment