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

Discussion: Securing token for socket.io auth #33

Closed
corymsmith opened this Issue Jan 25, 2016 · 12 comments

Comments

Projects
None yet
4 participants
@corymsmith
Copy link
Contributor

corymsmith commented Jan 25, 2016

Found this article and thought I'd share to start a discussion around it:

https://facundoolano.wordpress.com/2014/10/11/better-authentication-for-socket-io-no-query-strings/

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jan 28, 2016

This is a cool idea and I don't see any reason we shouldn't implement it with the auth rewrite we're doing. We could map the authenticate method that he talks about to one of the token service methods.

One thing that's not addressed is checking the token on every request. I think the only reason we wanted to do that in the first place was to watch for an expired token. Rather than checking it on every request, maybe we could grab the token expiration and set it up on the socket the same way he's doing socket.auth = true;. We do socket.expiresAt = timestamp; and only do an

if(new Date().getTime() > socket.expiresAt){
  // Do we boot them? 
  socket.disconnect('unauthorized');
  // or let them stay connected as a public user?
  socket.feathers.user = false;
}

on every request. That should spare the server from having to crunch away at the token on every request. We would still want to re-auth on reconnects after a temporary disconnection. I don't think we have to disconnect the socket if no authentication data is received, though. (but that's a good thing to show in the docs if people want to only allow auth'ed sockets.) Actually, why not make it a configuration option for the socket-io auth plugin? options.expireDisconnect = true We would still want to send a 'unauthorized' message back to the client, so the app could hide the private data or refresh if that's what they want. Maybe I'm overthinking that part.

@ekryski @daffl Am I overlooking something?

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jan 28, 2016

Is it really that expensive to do the token check? In a REST API we do the same thing all the time no? It might make sense to benchmark it before we make any workarounds for that.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jan 28, 2016

https://www.cryptopp.com/benchmarks.html
Looks like on that machine you would have to have over 100 MB/sec traffic before you'd hit a bottleneck, assuming the server is ONLY performing hashing. We're running HMAC over SHA-256, by default, which there's not a benchmark for.

So really, we have a while before we need to concern ourselves with the hashing stuff. What do you think about getting rid of query string auth / the rest of what he says?

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 1, 2016

I don't think we need to worry too much about checking the token on each request at this stage. Possibly in the future. The thing I'm more concerned about is do we want to support both methods of authentication via sockets:

  1. passing a token via query string params for auth during handshake? (which is less secure)
  2. passing the token in a socket message body or as part of the headers?

Looking at Auth0's implementation they support both. https://github.com/auth0/socketio-jwt

It's pretty much the same as that guy's implementation but how they treat namespaces for events is a little different.

I'm inclined to adopt best practices by default and not support passing the token by query string, or at least that not being the default behaviour.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Feb 1, 2016

They should probably only be passed via the headers or a socket call. We're promoting JWT as a more secure and compatible way to deal with APIs. Passing it as a query string goes a little against that. I think by now all web clients support setting custom headers.

I do think though that we have to check the token on each request (which can be easily done cross-provider as a service hook or as provider specific middleware). Imagine you have a long running connection e.g. with a mobile app and someone cancels their account or exceeds certain limits. You want to invalidate the token immediately and prevent subsequent requests not just throw an error when the user tries to reconnect.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 1, 2016

@daffl ya I agree. Right now I have it as provider specific middleware. Although I have a hunch it may turn into a hook.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 1, 2016

Good point. I'd want to be able to boot a troublesome user immediately, not just when the token expires.

If we can auth on a header, let's do that. That's consistent with the Rest provider.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 1, 2016

I vote we get rid of query string auth completely. I think the server gets to dictate the client, not the other way around. If there is some client library that won't work with it, it shouldn't be too hard for them to fix it or find a new one.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 2, 2016

Alright so on the decoupling branch I've implemented it so that it doesn't support authenticating via the socket handshake (aka. the query string method). As part of those changes it now also supports sending email/password or token for authenticating over sockets (#32).

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 2, 2016

👍 Awesome! I'm sure that was hard work. Thank you.

@ekryski ekryski modified the milestone: 1.0 release Feb 2, 2016

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 11, 2016

So it turns out that we don't need to disconnect a socket if it doesn't connect within a certain time period like that article suggests. We might actually want sockets that don't need to be authenticated for certain endpoints.

We also don't need to do the namespace hacking because we can .filter() service events. So you can filter events from a service so that only auth'd users see them.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 11, 2016

Makes sense to me.

@ekryski ekryski referenced this issue Feb 12, 2016

Merged

Decoupling #49

@ekryski ekryski closed this Feb 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.