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

Server, user and client - current socket communication scheme problem #82

Closed
valichek opened this issue Oct 13, 2014 · 18 comments
Closed

Comments

@valichek
Copy link

Hi, I would like proceed with #65 discussion.
Server cannot push message to specific client connected by websocket, only to user.
Filtering messages on client side is not good, because it uses necessary server resources to send all messages to all user clients and for high loaded apps it is critical.
I don't understand why the current scheme when server sends messages by uid is preferable. Maybe for common use apps it is OK, but in general case it is highly objectionable.
I've investigated the code and don't see big problem to give server a little more control on clients.

So here is the changes I propose.
0) generate uuids by server (to guarantee uuids uniqueness), so move from client-uuid to uuid to distinct it

  1. store channels by uuid
    current {<uid> <#{hk-chs}>}
    proposed {<uid> {<uuid> <hk-chs>}},
  2. add send-uuid-fn or change send-fn definition to fn [user-id uuid ev & [{:as opts :keys [flush?]}]] and set uuid to nil by default to leave active current scheme
  3. add client related events :chsk/uuidport-close and :chsk/uuidport-open

It is just common solution and it won't break current communication scheme while adding possibility to build more flexible apps.

@ptaoussanis
Copy link
Member

Hi valichek, apologies for the delay getting back to you!

This is something I'm going to need some time to think deeply about. Really pinned with work atm though, so unfortunately no way I'm going to have that time over the next few weeks. Will keep this issue open and come back to you once I've had the necessary time to think about it?

Sorry I couldn't be more helpful right away!

@sritchie
Copy link
Collaborator

sritchie commented Dec 2, 2014

@ptaoussanis, I'm actually hitting this right now as well. I've built a really nice server/client side navigation scheme on top of Sente that I'm hoping to open source in the next few weeks.

Each browser tab maintains a websocket that it uses to send navigation events to the server - the server responds with updated page state information for each route.

I'm using the client's session ID as my :uid now, but this means that if a user has multiple tabs open, all tabs get the redirect events from the server.

I see that you supply a :client-uuid to socket loop. This looks like exactly what I need. Maybe the easiest way forward is to merge :client-uuid into the incoming ring request, so that the :user-id-fn can get access to it? Let me know. Happy to make a pull req trying this out.

@ptaoussanis
Copy link
Member

Hey Sam, taking a look into this now.

@ptaoussanis
Copy link
Member

Okay! Took the opportunity while I was in the code to clean up some things I'd been meaning to.

Have an experimental + untested release up on Clojars: [com.taoensso/sente "1.3.0-SNAPSHOT"].

Relevant changes:

  • Ajax and WebSocket conns now both get a stable client-generated client-uuid (WebSocket client-uuids were previously reset on disconnection).
  • The (fn user-id-fn [ring-req]) Ring request now includes the stable :client-uuid as per your suggestion (thanks for that, btw).
  • Refactored some internals to possibly allow the server-side push fn to later send to client-uuids, more in line with what @valichek suggested.

So present situation:

  • Server push still requires a user-id (no sending directly to client-uuids yet since I still have some reservations there).
  • The user-id may now freely depend in part or whole on the client-uuid.

The latter of course means that it's possible to do something like set a user id as the combination of a server-side login uid and client-side uuid (per tab, etc.).

I also made the client-uuid configurable, so you can actually decide whether you want the uuid to be, say, chsk local (the default), or tab local over multiple chsk connections.

Feedback and/or bug reports on this release would be welcome!

Cheers :-)

@valichek
Copy link
Author

valichek commented Dec 2, 2014

It looks really great! I would like to suggest another feature. Maybe generating client-uuids at the server side is better idea. It could be added as option.

@ptaoussanis
Copy link
Member

Hi Valichek,

The trouble there is that there's no reliable way of actually generating a server-side uuid for clients reliably. If you generated (say) a random uuid for each connection, then you'd lose client identity on reconnects. If you generated (say) a random uuid for the first connection, then gave it to the client to relay - you'd still be subject to the client potentially lying about the id it's been given.

An alternative would be for the server to somehow try hash the client's handshake request and try to identify the client that way (e.g. via IP address, etc.) - but that can be inaccurate, and would still depends on the client's honesty to not be forging headers, etc.

The current approach instead proposes that we treat all client uuids as forgeable, and derives security in two ways:

  1. We can attach arbitrary server-side data to the client-provided id (like a session value or cookie id) while generating the user id. So the user id can be something like <secure-server-value>:<untrustworthy-client-value>. The combination will be secure, but gives us the consistent identity we're looking for when clients are honest (the common case).
  2. We can treat the client uuids as secret information. If Mallory is using a modified client and trying to impersonate Alice, she'll need to know Alice's client-uuid. But if Alice's client-uuid was generated randomly and protected by HTTPs, then impersonation will be hard in the same way guessing a cookie id is hard.

Does that make sense?

@sritchie
Copy link
Collaborator

sritchie commented Dec 3, 2014

Hmm, so I tried this out, making sure to clean and rebuild my clojure and clojurescript, and I'm now seeing this error server side:

WARN [taoensso.sente] - Client's Ring request doesn't have a client uuid. Does your server have the necessary keyword Ring middleware?:

and my websocket isn't connecting. I do have the proper middleware, but I see that the params field is empty. I'll see what I can do - @ptaoussanis, I know you said this is untested, but have you gotten a basic socket connection working? Will dig a little post dinner.

Thanks for this, by the way! Once I get this working this'll be huge.

@sritchie
Copy link
Collaborator

sritchie commented Dec 3, 2014

Hold that, I may have done a poor job cleaning clojurescript. I just looked in the cljsbuild folder and I still see old sente. Trying again.

@sritchie
Copy link
Collaborator

sritchie commented Dec 3, 2014

Yeah, user error. Works great!

@ptaoussanis
Copy link
Member

No problem, happy to hear you have it working. Let me know how well this does/n't end up fitting your use case?

Good luck with the project! Cheers :-)

@sritchie
Copy link
Collaborator

sritchie commented Dec 3, 2014

I'm now about to pass :client-uuid as the user-id-fn and send my nav events like this:

(let [reply (or ?reply-fn #(send-fn client-uuid %))]
          (reply resp))

Looks perfect to me.

@valichek
Copy link
Author

valichek commented Dec 4, 2014

Hi, Peter
Thank you for the answer. My question about client-uuid was more about data consistency, than security. Let me explain my position. You're absolutely right about all the security things about session check and lying clients. Application data is binded to client-uuid, so server considers client-uuid as something really unique. But uniqueness cannot be 100% guaranteed when clients provide and generate their uuids independently even if they have super-puper generating algo and the chance for generating duplicate is very small but it still exists. Clients are allowed to be designed with any technology that supports http/tcp and the implementations of uuid generating can differ, and that fact can have its own impact. I cannot say that it's a problem, but it is just not perfect. On my opinion client-uuid is something very internal or even intimate for server to rely on client side.

@ptaoussanis
Copy link
Member

But uniqueness cannot be 100% guaranteed when clients provide and generate their uuids independently even if they have super-puper generating algo and the chance for generating duplicate is very small but it still exists.

If you're worried about poor client uuid implementations:

  1. You can add as much entropy server-side as you like by concatenating the client uuid with info generated server-side. You can even ignore the client-provided uuid entirely. You can transmit this unique token back to clients and ask them to relay it on future requests.
  2. The general (+ default) way of using client uuids is to concatenate them with the server-side generated user id as above.

Any alternative will need to address the points I raised before.

So the client [uu]id is currently being used only for the one thing the server cannot reliably do, which is provide some form of id that persists across disconnections and that's stable from first connection. What the server chooses to do with that id is up to the server.

@valichek
Copy link
Author

valichek commented Dec 5, 2014

I agree with you. That make sense. Thank you

@ptaoussanis
Copy link
Member

No problem. And just to clarify - I'm not suggesting that the current approach is the only, or best one. Just suggesting that any alternatives would need to address our particular needs+limits.

I haven't been able to give the client id stuff much thought yet, so it's very possible that there's a better idea to be had. Your thoughts have been (+ would continue to be) very helpful.

Cheers!

tobias pushed a commit to tobias/sente that referenced this issue Jan 29, 2015
@sritchie
Copy link
Collaborator

Hmm. As I mentioned in the other issue I posted, in the latest snapshots my clients aren't receiving messages sent via UUID. Has some change come in since the first :client-uuid impl of this?

@ptaoussanis
Copy link
Member

ptaoussanis commented Feb 28, 2015

Has some change come in since the first :client-uuid impl of this?

Nothing intentional that I recall. My first guess would be that your build or build environment have somehow gone bad. Since these may be related, let's keep the discussion at issue #105 for now.

Going to follow up with some questions at #105 now.

@ptaoussanis
Copy link
Member

Closing for now as it's likely I won't be able to assist with this in at least the short/medium term.

Might be something to investigate again some time in future if someone can provide a clear + updated summary of specific motivations + can address how the current user-id based recommendations are inadequate.

Appreciate all the input given so far! Cheers :-)

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

No branches or pull requests

3 participants