gammazero / nexus

Full-feature WAMP v2 router and client written in Go
MIT License
268 stars 58 forks source link

Access to nexus structure members #112

Closed martin31821 closed 6 years ago

martin31821 commented 6 years ago

I (and @johannwagner) am developing an advanced wamp router providing more useful meta features and some extended authentication features. For this use case (specifically extending nexus) it would be useful to have access to the local realm object members (https://github.com/gammazero/nexus/blob/master/router/realm.go#L62) such as the list of session objects.

Specific use case is that I want to provide an endpoint to terminate active sessions. Any thoughts for this?

gammazero commented 6 years ago

In order to provide administrative capabilities on top of the nexus router, to do things like active session termination, I would prefer that the realm exposes a well-defined API, instead of giving access directly to internal data members. It will be much easier to continue to support an API, even if internal structures need to change. Whereas, if implementers depend on access the internal members then that can never change without breaking someone's router implementation.

Also, what you considering may be somewhat of a natural extension of the Registration Revocation meta API (also here).

What I mean, consider whether this session termination capability is something that only the router implementation itself is going to be capable of doing, or if a properly authenticated/authorized client should be able to invoke an extended session meta API to do this. It may even be worth proposing the latter as an extension to the wamp session meta API (if it has not already been done).

martin31821 commented 6 years ago

An API would be enough for our specific use case and (ofc) we want to create an extension to the session meta API and finally get it accepted upstream.

gammazero commented 6 years ago

Here is what I propose to implement:

The following APIs:

A above APIs will correspond, respectively, to the following session-meta procedures:

The first three meta procedures take one positional argument, the key that selects the session(s) to kill, and they all take two optional keyword arguments, reason and message, that serve as the last two arguments when calling the above APIs.

The wamp.session.kill will be equivalent and compatible with Crossbar's session kill feature referenced here: https://groups.google.com/forum/#!topic/crossbario/OgweKHge7Rs and here: https://crossbar.io/docs/Session-Metaevents-and-Procedures/#killing-a-session

This will implement the following in nexus: https://github.com/crossbario/crossbar/issues/1340

Will have this as soon as some consensus is reached in WAMP organization (probably a few days).

martin31821 commented 6 years ago

That would be great, actually didn't know this was already implemented in crossbar.

gammazero commented 6 years ago

Here is a PR to add new session meta procedures to WAMP specification: https://github.com/wamp-proto/wamp-proto/pull/315

gammazero commented 6 years ago

I have a working implementation for all of this. I am hesitant to have the router expose any new API if the session meta API is sufficient for this.

martin31821 commented 6 years ago

Nevertheless it would be good (for our specific use case) to have access to the sessions used within the router (think about adding/removing/changing authroles at runtime or other session details). /cc @johannwagner

gammazero commented 6 years ago

OK, so the proposal to update the WAMP spec was approved (merged https://github.com/wamp-proto/wamp-proto/pull/315), so now there is official spec for:

Which means I will at least provide the new session meta procedures above. However, for the API I want to be very careful about what gets released, because nexus will need to support it essentially forever.

What you are looking for is an administrative API which is essentially the session meta procedures, plus an API to modify the sessions the sessions at run-time. Modifying the session will need to be done carefully since the broker, dealer, message handler, and meta goroutines may all be reading the session details, and doing things like changing/removing/changing session details will need to be synchronized.

martin31821 commented 6 years ago

Administrative API is a pretty good summary for our project, I think. Ofc we try to stick to the session meta procedures as far as possible. At the moment I don't even know whether our code/idea works out in the end, since there are lots of edge cases which need to be handled properly. Maybe we start with a fork and try to see what members need to be exposed.

However, it's great to see the kill API implemented.

johannwagner commented 6 years ago

Yep, good work! I look forward to implement and integrate this into my existing application! Many thanks!

gammazero commented 6 years ago

PR https://github.com/gammazero/nexus/pull/123 provides all the new session kill meta procedures. A new session meta procedure, wamp.session.modify_details is provided to allow a session's details to be modified.

I did not provide a router/realm API, since the session meta API already provides access to the admin functionality discussed in this issue. A local (in process) client calling the session meta procedures should be just as capable as code that directly calls a router API. This prevents having to maintain another code path. I did actually implement the API experimentally, but decided that it was duplicating functionality.

Take a look at https://github.com/gammazero/nexus/pull/123 as see what you think. The new lockable router.session type was necessary since changing session details could cause races with the multiple goroutines that may be reading the details.

gammazero commented 6 years ago

The wamp.session.modify_details seems particularly dangerous. Probably necessary to at least prevent modification of the session ID. This is something that should require client authorization, as it would be bad for clients to change their authid and authrole to have privileges that they should not. Maybe the modify_details API is not such a good idea as a session meta API.

I am afraid that this PR will open a vulnerability for existing users who update nexus and do not have an authorizer specifically to protect clients from calling the new modify_details and session kill meta procedures. So, I will require that the modify and kill session meta procedures are explicitly enabled.

martin31821 commented 6 years ago

I'd really like to have this endpoint, but IMHO we should check the authrole of the caller to be trusted, so it is possible to use it within an embedded context, but not from the outside world. If a user wants to have it accessible, he needs to create custom code which performs vulnerability checks.

But having them enabled explicitly would also be fine for me.

Furthermore I agree that the session ID should never change, since other users may use it ti associate information with specific callers/subscribers.

martin31821 commented 6 years ago

I just read your changes and in fact it looks very promising. Session ID can't be changed since it's not contained within the details dict of the session, so that is not a problem.

gammazero commented 6 years ago

I guess the modify meta procedure is not that much more dangerous than the kill procedures, so you can have it. It must be enabled separately: https://github.com/gammazero/nexus/pull/123/files#diff-d3b9b3ec2dcc8b044cc18aad595af3d3R54

gammazero commented 6 years ago

Finally merged. If this provides everything you need, please close issue. Enjoy.

martin31821 commented 6 years ago

Looks great, thanks!