onConnecting implementation #1170

meejah · 2019-04-03T04:29:07Z

As per latest discussion in #1158

oberstet · 2019-04-13T07:24:09Z

had a quick look: rgd the new type classes TransportDetails and ConnectingRequest: I think we might want to make all ctor arguments optional, as certain transports (twisted/asyncio level) such as serial, pipes and UDS don't have host/port, and further add other things (path for UDS, device path for UDS and such) ..

meejah · 2019-04-15T17:52:09Z

Okay, I was just looking at the asyncio and Twisted "transport" interfaces and it looked like they always provide getHost .. i.e. ITransport has getHost.

Looks like getHost on a Unix socket returns address.UNIXAddress(self.socket.getsockname()) and similar for getPeer. It seems like asyncio aped these interfaces too (i.e. any "socket" has this stuff).

Examining how we used this currently/before, request += "Host: %s:%d\x0d\x0a" % (self.factory.host, self.factory.port) so ConnectingRequest does need a host + port (of "whatever is visible on the Internet" I guess?) but TransportDetails doesn't. Currently, TransportDetails (in this PR) just has host and peer which are defined as "whatever the low-level framework returned". I believe these will always work (and if you're using Twisted, you'll get an IAddress for both, which would be a UNIXAddress if you've got a unix socket etc).

Put another way, if we want to use ConnectingRequest in place of "things in the factory", we currently do blindly use (and thus require) host, port, and resource -- these being "what the client will see" , not necessarily what is actually being used. I added a little clarification in the docs.

oberstet

The code and tests look great! For IP based transports.

Which is my main worry: eg in Crossbar.io, we depend on UDS at the very least. Also we support Tor. etc. The change goes deep .. and I am worried it breaks or doesn't fit the non-IP transport we support.

autobahn/websocket/types.py

oberstet · 2019-04-16T06:04:41Z

autobahn/websocket/types.py

+    # is_secure
+    # peer_certificate  # getPeerCertificate(), .get_extra_info('peercert')
+
+    def __init__(self, peer, host):


Similar to my above comment: what about UDS, pipes, serial, ..?

I believe they all return "something" on their respective platforms (i.e. ITransport in Twisted has getHost() and it returns an IAddress which is .. basically "transport specific" (e.g. TCPAddress or UNIXAddress, etc). That said, I'll double-check pipes though.

oberstet · 2019-04-16T15:22:29Z

not sure if that is of use, but we have 2 helpers:

autobahn.twisted.util.peer2str
autobahn.asyncio.util.peer2str

meejah · 2019-04-16T19:32:56Z

Ahhh, interesting. I wonder if it's better to use those (and then TransportDetails always has strings for those two items) or if it's better to just declare "you will get whatever your framework returns for the peer and host"...? (These will be IAddress implementations in Twisted, not sure what asyncio returns/promises)

oberstet · 2019-04-17T04:20:43Z

I wonder if it's better to use those (and then TransportDetails always has strings for those two items)

yeah, that would be better IMO. we can have only one item: peer|str == peer2str(..) => this is what we need in CB and AB (WAMP)

we also need this https://github.com/crossbario/autobahn-python/blob/master/autobahn/twisted/util.py#L94

this is the channel ID over the underlying TLS (when the transport is running TLS)

I don't know how to get it on asyncio

also, looking at def transport_channel_id(transport, is_server, channel_id_type)::

transport and is_server => those are available (implicitly) for onConnecting to fill
channel_id_type => for now, the only value it accepts is "tls-unique", so practically we could kick it as it is not available in onConnecting
we could instead have TransportDetails.channel_ids be a dict with a "tls-unique" key filled - so that we can fill in other bindings
I am not sure 100% if the latter will really work, as I haven't worked with other bindings
the relevant RFC for TLS channel bindings is this https://tools.ietf.org/html/rfc5929

meejah · 2019-04-17T04:33:32Z

Okay, so convert peer and host to strings, add channel_id (maybe "None" if asyncio can't get it?). And maybe channel_id_type but if so it's just hard-coded to tls-unique? (or maybe we just leave it out for now, and document that it's always tls-unique currently and if that changes it'll grow a new attribute?). Do we want is_secure (basically "we used TLS" or not?) too? (i.e. if is_secure is False then channel_id and channel_id_type [if it exists] is None then I guess?)

oberstet · 2019-04-17T05:05:51Z

How about this?

TransportDetails.peer: str
TransportDetails.is_secure: bool
TransportDetails.secure_channel_id: dict or None

and

TransportDetails.secure_channel_id[u'tls-unique']: bytes

meejah · 2019-04-17T16:40:23Z

Sounds good. Drop host? Or it also gets run through peer2str as well ? (it's also an IAddress so that should work)

oberstet · 2019-04-17T18:57:59Z

Yeah, this looks all good for merge from my side now! Awesome. Hope it'll do for JP too;) So if don't want anything else to push on this PR from your side, please merge

meejah · 2019-04-17T19:09:21Z

@exarkun do you see anything missing as far as the use-case for Tahoe-LAFS? (It has "headers" so I think we're good...)

meejah · 2019-04-18T01:11:47Z

I added some documentation, a changelog entry, and put the onConnecting method in the interface.

oberstet · 2019-04-19T07:39:43Z

going to merge it now, as I'd like to run it with ongoing development of crossbar/crossbarfx - to see if there is any unexpected fallout. should @exarkun have any itches with how this now is landed, we can do that via another PR if necessary ..

exarkun · 2019-04-19T14:38:22Z

autobahn/websocket/types.py

+        self.port = port if port is not None else 80
+        self.resource = resource if resource is not None else "/"
+        # optional
+        self.headers = headers if headers else dict()


Kind of an anti-pattern. If headers is an empty dict then this sets self.headers to a different empty dict. It doesn't really matter for simple, straightforward code but it creates quite a surprise for nasty code that mutates that dict later on.

I'd suggest preserving identity:

self.headers = headers if headers is not None else dict()

And likewise for protocols below.

ah, right! fwiw, the classes in "types" were envisioned as pure value holding classes - but the code as it stands is neither nor. if we'd go with "pure value types", I guess we should (deep)copy everything coming in via ctors. lets see @meejah 's opinion ..

We can definitely do what @exarkun suggests (which I agree is a good idea). At some point I had some "XXX" comments about this; we can "slow everything down" by deep-copying or make it "up to the caller" to make sure they're not mutating (or, only mutating in a way that makes sense?). We should definitely do the is not None thing ... I'm not so sure about deep-copying everything.

ok, so no deep copy, and preserve identity for dict: +1

while discussing this, why is dict() better than {}?

I prefer dict() to avoid confusion with set()s. A dict with one item: {"foo": "bar"} a set with two items: {"foo", "bar"}...you can't make an empty set with { and } AFAIK, but ....

exarkun · 2019-04-19T14:39:50Z

autobahn/websocket/interfaces.py

+            used for this connection. If you wish to use the default
+            behavior, `None` may be returned (this is the default).
+        """
+


Are all non-subclassing IWebSocketChannel implementations broken now, because they lack this method? Or is the only supported use through subclassing the ABC?

yeah, people that implemented their own sub-classing stuff have broken classes now until they add the new callback (which is strictly expected by the calling library code). not sure if anyone has done that (wrote own classes for channel). adding dynamic attribute checking to work around that .. not sure if it is worth.

Well, but also the only way to "implement" an ABC is by subclassing it, isn't it? So then they'll get this empty method (which returns None and thus "asks for defaults"). I should double-check that, but I'm fairly certain that's how ABCs work...

Hrm, I think I'm wrong here ... "A class containing at least one method declared with this decorator that hasn't been overridden yet cannot be instantiated. Such methods may be called from the overriding method in the subclass (using super or direct invocation)."

That's from PEP3119. So, I think what that means is that yes, anyone who created their own classes from scratch and declared the IWebSocketChannel "interface" via subclassing will now be broken (because they haven't implemented onConnecting). A quick test in the repl confirms this -- you'll get TypeError: Can't instantiate abstract class Foo with abstract methods onClose, onConnect, onConnecting, onMessage, onOpen, onPing, onPong, sendClose, sendMessage, sendPing, sendPong.

meejah added 4 commits April 9, 2019 16:30

first-cut of an onConnecting implementation

e098567

add TransportDetails

1528cf1

more explicit

ff6d9f8

cleanups

6dd5889

meejah force-pushed the ticket1158-websocket-headers branch from ef21816 to 6dd5889 Compare April 10, 2019 00:42

meejah added 2 commits April 9, 2019 19:03

mocks

aa25513

test fixes

28697c4

clarify in docs

ed251a0

meejah added 2 commits April 15, 2019 13:01

pass all possible options from Factory

11a0f91

tests for onConnecting

fde823a

meejah changed the title ~~WIP: first-cut of an onConnecting implementation~~ onConnecting implementation Apr 15, 2019

meejah requested a review from oberstet April 15, 2019 20:53

flake8

6361c3e

oberstet reviewed Apr 16, 2019

View reviewed changes

host, port, resource are optional

d03bd5b

meejah added 3 commits April 17, 2019 01:12

asyncio transport_channel_id() and TransportDetails fixes from review

58aa219

only need ISSLTransport for twisted

a240713

use fakes, not mocks

8dbe3fa

meejah added 2 commits April 17, 2019 10:42

extend example

f5d6a6f

flake8

22c8ad0

meejah added 2 commits April 17, 2019 19:10

documentation

60744d8

changelog entry

368353a

oberstet merged commit 6ae03e7 into crossbario:master Apr 19, 2019

This was referenced Apr 19, 2019

WIP: websocket Agent interfaces #1174

Closed

Process object has no attribute getPeer #1177

Closed

exarkun reviewed Apr 19, 2019

View reviewed changes

meejah mentioned this pull request Apr 19, 2019

Ticket1158 onconnecting followup #1180

Merged

oberstet mentioned this pull request Apr 20, 2019

Implement transport channel binding for aio #729

Closed

meejah deleted the ticket1158-websocket-headers branch May 5, 2019 06:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

onConnecting implementation #1170

onConnecting implementation #1170

meejah commented Apr 3, 2019

oberstet commented Apr 13, 2019 •

edited

meejah commented Apr 15, 2019 •

edited

oberstet left a comment

oberstet Apr 16, 2019

meejah Apr 16, 2019

oberstet commented Apr 16, 2019

meejah commented Apr 16, 2019

oberstet commented Apr 17, 2019

meejah commented Apr 17, 2019

oberstet commented Apr 17, 2019

meejah commented Apr 17, 2019

oberstet commented Apr 17, 2019

meejah commented Apr 17, 2019

meejah commented Apr 18, 2019

oberstet commented Apr 19, 2019

exarkun Apr 19, 2019

oberstet Apr 19, 2019

meejah Apr 19, 2019

oberstet Apr 19, 2019

meejah Apr 19, 2019

exarkun Apr 19, 2019

oberstet Apr 19, 2019

meejah Apr 19, 2019

meejah Apr 19, 2019

onConnecting implementation #1170

onConnecting implementation #1170

Conversation

meejah commented Apr 3, 2019

oberstet commented Apr 13, 2019 • edited

meejah commented Apr 15, 2019 • edited

oberstet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oberstet commented Apr 16, 2019

meejah commented Apr 16, 2019

oberstet commented Apr 17, 2019

meejah commented Apr 17, 2019

oberstet commented Apr 17, 2019

meejah commented Apr 17, 2019

oberstet commented Apr 17, 2019

meejah commented Apr 17, 2019

meejah commented Apr 18, 2019

oberstet commented Apr 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oberstet commented Apr 13, 2019 •

edited

meejah commented Apr 15, 2019 •

edited