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

Access to nexus structure members #112

Closed
martin31821 opened this issue Jun 18, 2018 · 17 comments
Closed

Access to nexus structure members #112

martin31821 opened this issue Jun 18, 2018 · 17 comments

Comments

@martin31821
Copy link
Contributor

martin31821 commented Jun 18, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

gammazero commented Jun 19, 2018

Here is what I propose to implement:

The following APIs:

  • Realm.KillSession(sessionID wamp.ID, reason wamp.URI, message string) error
  • Realm.KillSessionsByAuthid(authid string, reason wamp.URI, message string) error
  • Realm.KillSessionsByAuthrole(authrole string, reason wamp.URI, message string) error
  • Realm.KillAllSessions(reason wamp.URI, message string) error

The reason argument is the URI used as the GOODBYE.Reason. The message argument will be put into Goodbye.Details under the key "message".

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

  • wamp.session.kill
  • wamp.session.kill_by_authid
  • wamp.session.kill_by_authrole
  • wamp.session.kill_all

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:
crossbario/crossbar#1340

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

@martin31821
Copy link
Contributor Author

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

@gammazero
Copy link
Owner

Here is a PR to add new session meta procedures to WAMP specification: wamp-proto/wamp-proto#315

@gammazero
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

gammazero commented Jun 30, 2018

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

  • wamp.session.kill
  • wamp.session.kill_by_authid
  • wamp.session.kill_by_authrole
  • wamp.session.kill_all

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@gammazero
Copy link
Owner

PR #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 #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
Copy link
Owner

gammazero commented Jul 4, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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

@martin31821
Copy link
Contributor Author

Looks great, thanks!

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

No branches or pull requests

3 participants