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

Publisher/caller disclosure #57

Open
oberstet opened this issue Mar 5, 2014 · 27 comments

Comments

Projects
None yet
8 participants
@oberstet
Copy link
Member

commented Mar 5, 2014

Provide options so we can provide any of:

  • WAMP session ID
  • authrole of WAMP session
  • authid of WAMP session
@mbonneau

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2015

👍

I would like to see the disclose_caller/disclose_publisher option provide all this information if the router has it.

@darkl

This comment has been minimized.

Copy link

commented Aug 1, 2015

👍
Can this be added to the spec? It seems that Thruway already supports this, and I think it could be very useful for callees.

darkl added a commit to Code-Sharp/WampSharp that referenced this issue Aug 7, 2015

darkl added a commit to Code-Sharp/WampSharp that referenced this issue Aug 7, 2015

@oberstet oberstet added this to the RC6 milestone Aug 27, 2015

@oberstet

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2015

@darkl Crossbar.io had implemented that in the past. And we removed it again. The reason: the next thing after authid/authrole asked for was transport level details: "we need access to the WebSocket cookie in callees". CB has more information about the Caller than just session ID, authid and authrole. Where do we stop? It doesn't make sense to transmit all that information in each and every call.

So regarding the question "What info should be disclosed", here are the options:

  1. only sessionid
  2. sessionid, authid and authrole
  3. sessionid, authid, authrole, transport_details, ...

Now it was suggested in the past to let the Subscriber/Callee not only ask for "details yes/no", but ask for specific details ("I need only authrole. Gimme that and nothing else").

The problem with that is: when different Subscribers ask for different details, the router cannot get away with serializing an event once - as the details asked for differ. The same problem arises with Callees and shared registrations.

@goeddea

This comment has been minimized.

Copy link
Member

commented Sep 3, 2015

Since we want to keep the routing core performant, we probably don't want to think about having configurable details.

Absent this, providing sessionid, authid and authrole seems to cover a large number of use cases (this has been requested a few times) while not increasing wire traffic by too much.

My suggestion is to go with this slightly expanded set and close the issue.

@oddjobz

This comment has been minimized.

Copy link

commented Nov 13, 2015

Just a thought, how about something like disclose_my_transport as an additional option in the client? Or, just going out on a limb, disclose_fields=[..] .. or if that's too fine-grained, then maybe four options, nothing , session , (session,auth,role) , kitchen sink ... ??

@oberstet

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2015

@oddjobz no, because, see above.

@oddjobz

This comment has been minimized.

Copy link

commented Nov 21, 2015

@oberstet , Ok, in principle I can see that if the client is allowed to choose the fields it wants returned, then when you publish an event, you may need to send different identification fields to each client, and I appreciate there is an undesirable performance hit if you need to manipulate the packet to each client separately depending on their requirements.

However .. in practice (Ok, so I'm speaking about "my" code so this may be a flawed perspective) the code that calls a given topic on the client side, will be common to all clients, so the client identification requirements will 'always' be the same for a given topic. Whereas I may well re-use the same topic in different places in the code, again the identification requirements will always remain the same as this is a function of the topic implementation.

i.e. If the topic "needs" the "auth" in order to function, then the client must always disclose the "auth", or the topic call will fail.

I guess I'm looking at this "the other way around", the required fields are determined by the topic implementation and not by the client, yet currently the fields available are specified by the client. Perhaps (?) we need a handshake here, the client specifies the details it's prepared to disclose, and the server specifies the details it "needs" to function (?)

On the first invocation the client may send more than is needed, which can be filtered out by Crossbar, then on return the server can notify the client of the subset of fields it needs, so subsequent calls can be 100% efficient .. for example many of my RPC implementations need "auth" only and not "session" or anything else .. so this scheme would produce a notable reduction in traffic over sending session/auth/role for everything.

This adds to the complexity, but on the other hand it does give you an additional layer of error checking in terms of handshaking required data .vs. available data, and assuming you need to pass more than just the session by default, this will minimise the traffic flow.

If you take into account the additional complexity being added (well, that I find I need) to cope with the fact I'm "just" getting a session, the performance / complexity difference may be self-cancelling, and indeed it shifts complexity away from user code and back onto infrastructure .. which IMHO would be the correct direction of travel.

Alternatively (?) if "disclose_me" was relative to the Websocket connection, then each topic implementation specified the fields required ... don't know .. but the current implementation seems to be application centric to a degree in that for what I'm doing, I don't actually want a client-side disclose_me "at all" as security is handled server side (it just gets in the way).

Given all the client-side code is sent "from" the server, "disclose_me" has no security function, and in terms of bandwidth the choice is dependent on the topic's requirements ... sorry, I'm going in circles, does that make sense?

@goeddea

This comment has been minimized.

Copy link
Member

commented Nov 22, 2015

@oddjobz - Just a couple of points:

  • The client currently authorizes the disclosure of information, but it does not send this. Any information about the caller is added by the router.
  • The current spec and implementation of having the caller decide whether its identity should be disclosed has rightfully been criticized (see e.g. crossbario/crossbar#283). We'll likely not keep it that way.
@oddjobz

This comment has been minimized.

Copy link

commented Nov 22, 2015

@goeddea

The client currently authorizes the disclosure of information ..

Sure, appreciate that.

The current spec and implementation ..

Ok, from my perspective, bandwidth is an issue because the link between the router and the topic server is always a network connection and will have to carry all the variables required, at the moment this is just "session", but the point of "where to draw the line" is a very good one.

Whereas I appreciate there are different use-cases at play here, for me the client-side disclose_me has zero value with regards to security. Yet the inability to get immediate access to the variables I need is very expensive in terms of development resource and indeed back-end processing in terms of making those variables available. Currently I need to maintain a session listing in a shared database so topic implementations can resolve sessions back to authid's. I know there are metadata calls to do this but I'm currently not happy with is this mechanism.

For what it's worth I'm still thinking an "I need .." definition in the topic implementation might be a good solution ...

@goeddea

This comment has been minimized.

Copy link
Member

commented Nov 22, 2015

@oddjobz - I think drawing the line at sessionID, authID and authrole makes sense, since this is what most people need.

Regarding the configuration of where to trigger the disclose: For RPCs this could equally be the callee when it registers the procedure or a configuration in the router. For PubSub with multiple subscribers we run into the serialization issue again - one subscriber requests the details, the others don't want them, two serializations. In this case having it in the router configuration and set for the entire application might make more sense.

@oddjobz

This comment has been minimized.

Copy link

commented Nov 23, 2015

Sure, but again I guess it's use-case dependent. For me, all subscribers would have the same requirements, so having it set by the publisher for all subscribers would be fine. I could imagine that the same publication may attract two subscribers who want different details .. but in practice, how often would this happen?

meejah referenced this issue in crossbario/autobahn-python Nov 26, 2015

@nbrochu

This comment has been minimized.

Copy link

commented Nov 26, 2015

If I'm mostly interested in the Callee's IP address, is this the best option with the current state of the protocol?

# Anywhere inside an ApplicationSession
self._transport.http_headers.get("x-real-ip") or \
self._transport.http_headers.get("x-forwarded-for") or \
self._transport.peer.split(":")[1]

I was looking to update http://stackoverflow.com/questions/26725770/accessing-rpc-callers-ip-and-http-connect-headers-inside-applicationsessions-r but looking at this issue, it looks like passing the Caller Transport was only in for a single release.

@meejah

This comment has been minimized.

Copy link
Member

commented Nov 26, 2015

@nbrochu something like:

    @inlineCallbacks
    def onJoin(self, _):

        @inlineCallbacks
        def method(deets):
            session = yield self.call('wamp.session.get', details.caller)
            print "session_info:", session
            peer = session['transport']['peer']
            print "peer's address", peer
            headers = session['transport']['http_headers_received']
            print "headers:"
            print '\n'.join(['{}: {}'.format(k, v) for (k, v) in headers.items()])

        yield self.register(
            method, 'some_method',
            types.RegisterOptions(details_arg='details''),
        )

I haven't tried this exact code behind nginx etc to confirm the x-real-ip etc headers. This uses "official" APIs, instead of self._transport which by convention is a private API (because it starts with _).

@oddjobz

This comment has been minimized.

Copy link

commented Nov 26, 2015

@meejah just as a matter of interest, what features of NGINX are you using? i.e. what is driving the use of crossbar 'behind' NGINX?

@meejah

This comment has been minimized.

Copy link
Member

commented Nov 27, 2015

@oddjobz I'm not, but @nbrochu's question implied he was (X-Forwarded-For etc).

@nbrochu

This comment has been minimized.

Copy link

commented Nov 27, 2015

I wouldn't worry about it. That was my setup a long time ago, before Crossbar, when I had a standalone Autobahn Component. I would proxy pass the web socket connection in NGINX for TLS.

That being said, when Crossbar ends up getting multi-node support, some load-balancing setups could use some form of reverse-proxying and this becomes relevant again.

I also know that some people like doing more advanced forms of A/B testing with NGINX: Proxy pass to stable or experimental based on a cookie value (generally). With Crossbar though, it makes more sense to simply connect to a different realm with some frontend logic in the browser.

@oddjobz

This comment has been minimized.

Copy link

commented Nov 27, 2015

Mmm, have you tried "ipvs" ?

@oberstet

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2016

This is how it is implemented in Crossbar.io now:

  1. Both publisher and caller disclosure can be enabled on a per URI basis in the router configuration
  2. The detail attributes in EVENT messages are: publisher (the WAMP session ID of the publisher), publisher_authid and publisher_authrole
  3. The detail attributes in the INVOCATION messages are: caller (the WAMP session ID of the caller), caller_authid and caller_authrole

I do think this reflects the end result of our discussion of the feature. @mbonneau does it?

It isn't reflected in the specification though. And eg Thruway uses different attribute names (see here.

@oddjobz

This comment has been minimized.

Copy link

commented Jul 23, 2016

Hi, is there any documentation on this yet? (do you have a link?)

@oberstet

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2016

@oddjobz rgd Crossbar.io, no, the docs are not there=( crossbario/crossbar#841

@oberstet oberstet changed the title Disclose publisher/caller information Publisher/caller disclosure Mar 19, 2017

@oberstet oberstet added the RFC label Mar 19, 2017

@oberstet

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2017

Essentially, the semantics have converged now, the feature is well defined - but we need to go over the spec text.

@oberstet oberstet added the Bug label Jul 3, 2017

darkl added a commit to Code-Sharp/WampSharp that referenced this issue Jul 20, 2017

@oberstet oberstet removed this from the RC6-AP milestone Feb 21, 2018

@oberstet oberstet added this to the spec-fixes-and-polish milestone Mar 3, 2018

@gammazero gammazero closed this May 21, 2018

@gammazero gammazero reopened this May 21, 2018

@gammazero

This comment has been minimized.

Copy link
Contributor

commented May 21, 2018

I understand that the consensus here is to include the authid and authrole fields in support of publisher and caller identification. To clarify, what is the agreed name for these fields:
publisher_authid, publisher_authrole, caller_authid, caller_authrole or simply authid and authrole.

To me, it seems that the publisher_ or caller_ prefix is unnecessary, as this is apparent from the message type in which they are delivered. I think this will only require a minor exit to the existing spec text.

Also, is there any agreed formatting for the custom x_... fields. Looks like the convention is to have x_ then some abbreviation of the implementation, such as tw_ or cb_ and then the field name. Any agreement here?

@darkl

This comment has been minimized.

Copy link

commented May 21, 2018

@gammazero, unfortunately the agreed names include the prefix, i.e. publisher_authid, publisher_authrole, caller_authid, caller_authrole. Please don't change this again, as this is already reflected in the implementations (see here, here and here).

@gammazero

This comment has been minimized.

Copy link
Contributor

commented May 21, 2018

@darkl Thank you for the clarifying - will not change (the close-reopen was accidental). I was not sure about decision as it looked like Thruway did not include the role prefix: https://github.com/voryx/ThruwayCommon/blob/master/src/Message/EventMessage.php#L149-L150

nexus router now also supports these fields for caller and publisher identification as per your feedback.

@oberstet

This comment has been minimized.

Copy link
Member Author

commented May 21, 2018

FWIW, there is a deeper running feature request somewhere .. I think filed by @mbonneau : allow to assigned one or multiple authroles when authenticating a client .. which is sth we should discuss IMO ..

@mbonneau

This comment has been minimized.

Copy link
Contributor

commented May 22, 2018

@gammazero Thruway is a little out of sync with the spec.

@oberstet Would still like to see the authroles officially supported see #99

@oberstet

This comment has been minimized.

Copy link
Member Author

commented May 25, 2018

@mbonneau I am +1 taking a vote on #99 : I think this has clear use cases, but also has deep implementation impacts

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.