Basic security for cube #108

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@ghost
ghost commented Feb 15, 2013

Hey guys :)
We liked Cube and wanted to give it a try, but since most of our app is hosted on Heroku, we didn't have the luxury of relying on firewalls for security :(

While we initially used nginx as proxy and let it handle the security, we felt that it was basic enough to roll into Cube itself (the change itself is rather small), and that it would make deployment and operations a bit easier.
It also makes it easier to "try out" Cube without thinking too much about it, which is always nice :)

There are tests for the http part (but not currently for udp), and it seems mostly ok.

TODO:

  • Add tests for UDP
  • Add support for SSL and WSS
@cwarden
cwarden commented Feb 18, 2013

Also see @mrflip's pluggable authentication in #92. It makes it easy to add new authenticators.

@ghost
ghost commented Feb 18, 2013

I only saw it after implementing ours :)
We mostly just wanted to feel comfortable having our backends talk with Cube without fearing someone will spam/query it, and we wanted to make the patch as compact as possible :)

If you're going to pull his request, we can port the simple password thing to his "authenticator" interface (we didn't bother with that since our use-case was strictly internal server-to-server stuff).
Do note that UDP remains unsecured, which is a bit iffy.

@mrflip
mrflip commented Feb 20, 2013

+1 for putting this in the pluggable interface, but of course I think so :)

On Mon, Feb 18, 2013 at 4:41 PM, talkasa notifications@github.com wrote:

I only saw it after implementing ours :)
We mostly just wanted to feel comfortable having our backends talk with
Cube without fearing someone will spam/query it, and we wanted to make the
patch as compact as possible :)

If you're going to pull his request, we can port the simple password thing
to his "authenticator" interface (we didn't bother with that since our
use-case was strictly internal server-to-server stuff).
Do note that UDP remains unsecured, which is a bit iffy.


Reply to this email directly or view it on GitHubhttps://github.com/square/cube/pull/108#issuecomment-13747193.

@ghost
ghost commented Feb 20, 2013

Two things:

  • What do you want to do about UDP ? We'd prefer not to worry about getting our cube instance spammed :p
  • There isn't any basic auth stuff in your implementation, so you just get kicked out and don't get a chance to authenticate :(
@RandomEtc
Collaborator

+1 for making authentication pluggable. It should be possible to run cube as a library and wire it into a server that uses a library like connect/express to allow pluggable middleware.

I feel pretty uneasy merging this pull request as-is.

The UDP security isn't really very secure - anyone else on the network could see those messages and trivially extract the password. You'd need to look at a Message Authentication Code scheme (e.g. HMAC) to encrypt your messages and validate that only you could have sent them. But... if you're on Heroku, it's probably smarter to just disable UDP collection entirely since Heroku apps can't receive UDP messages anyway.

The HTTP security also seems suspicious. Sending a password in a query string over HTTP seems like a bad idea. I'd rather see this go over HTTPS... but before I get too far into critiquing again, what I'd really like to see is less HTTP server stuff in Cube and more in a pluggable library like Connect.

I'll close this for now but feel free to re-open for further discussion if you attempt to wire Cube into Connect/Express and find that certain things aren't possible. Otherwise, there are many tried-and-true authentication schemes available as Connect middleware and that should be a safer path to happiness on Heroku etc.

@RandomEtc RandomEtc closed this Mar 5, 2013
@RandomEtc RandomEtc commented on the diff Mar 5, 2013
lib/cube/server.js
@@ -68,10 +69,20 @@ module.exports = function(options) {
});
function connect(connection, request) {
+ var requrl = url.parse(request.url, true);
+ var path = requrl.pathname;
+
+ if (options["password"]) {
+ var query = requrl.query;
@RandomEtc
RandomEtc Mar 5, 2013 collaborator

Query strings over HTTP are really easy to steal if you're on the same network. If you're using this now (without https) it's probably not offering you any real security.

@ghost
ghost Mar 5, 2013

I am well aware of that. This however wasn't a major problem in our use case:

  • we do not have anyone on our network that is capable of sniffing (stuff on Heroku are sandboxed, and afaik, they claim you cannot dump things out).
  • in most deployments, it will sit in a private network of somekind (EC2 server clusters)

The only real issue is with the lack of https for client machines (direct HTTP and WS). It's something I've been meaning to add :)

@ghost
ghost Mar 5, 2013

Also: The pluggable auth patch suffers from the same issue: if everything is on HTTP, it's easy to steal the cookies if you're at the same network :)

@RandomEtc
RandomEtc Mar 5, 2013 collaborator

Yep :) I have the same problem with the pluggable auth patch. I'm imagining rewriting the server stuff to be compatible withfunction(req,res,next) style middleware so that there is no need for an auth story inside Cube itself. This is similar to how learnboost/kue supports basic auth, for example.

@RandomEtc RandomEtc commented on the diff Mar 5, 2013
lib/cube/server.js
function ignore() {
// Responses for UDP are ignored; there's nowhere for them to go!
}
+
+// returns the username & password. Ripped from Express
@RandomEtc
RandomEtc Mar 5, 2013 collaborator

I missed this comment until just now, but it underscores what I said earlier. I'd rather put this back in Express :)

I'd like Cube to support integrating into an Express server (it would also solve the static hosting stuff and remove the need for node-static). Let me know if you look into this, I should be able to help.

@ghost
ghost Mar 5, 2013

I agree. We mostly wanted to keep the change compact, and bringing in Express just for the authentication seemed excess. But if you want to pull something like this in anyhow, then it's better, of course :)

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