-
Notifications
You must be signed in to change notification settings - Fork 12
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
document websocket support in the relay server #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -48,7 +48,8 @@ This may be relaxed in the future, much as Wormhole was. | |||
|
|||
Each Transit object has a set of "abilities". These are outbound connection | |||
mechanisms that the client is capable of using. The basic CLI tool (running | |||
on a normal computer) has two abilities: `direct-tcp-v1` and `relay-v1`. | |||
on a normal computer) has these abilities: `direct-tcp-v1`, `relay-v1`, | |||
`tor-tcp-v1`, `direct-ws-v1` and `direct-wss-v1`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like these should be called relay-ws-v1
and relay-wss-v1
but I also feel like we had this conversation before and I don't recall how it ended. Could you remind me what's up with that?
If we did go with the above, would it then make sense to rename relay-v1
to relay-tcp-v1
(in a backwards compatible way w/ deprecation warning)?
Perhaps we can add descriptions of these in the list that follows? Maybe something like "indicates that it can connect via the transit relay using <proto> transport".
@@ -48,7 +48,8 @@ This may be relaxed in the future, much as Wormhole was. | |||
|
|||
Each Transit object has a set of "abilities". These are outbound connection | |||
mechanisms that the client is capable of using. The basic CLI tool (running | |||
on a normal computer) has two abilities: `direct-tcp-v1` and `relay-v1`. | |||
on a normal computer) has these abilities: `direct-tcp-v1`, `relay-v1`, | |||
`tor-tcp-v1`, `direct-ws-v1` and `direct-wss-v1`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's kind of tangential to your change, but why would one advertise any of the other three values? What meaning do they have in the protocol/handshake? I think the only relevant question is "can we connect directly or do we need a relay"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you are saying makes sense, now that relay specification also includes the connection protocol part (as in tcp:foobar.org:4001
or ws://foobar.org:4002
). It wasn't there in the past before the WebSocket changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps those changes should be separate from this change as a "-v2" abilities? That would also help maintain backward compatibility with older relay servers and clients.
Co-authored-by: Bryan White <bryanchriswhite@gmail.com>
I'd like to discuss the semantics of having multiple protocols towards one relay server. What is the intended feature negotiation once both sides have exchanged their abilities? Usually, I'd say both sides have to agree on each (i.e. take the intersection of both sets), but with web sockets this needs not to be so: two clients could connect through a relay, one with web sockets and one over TCP. (Corollary: do both sides actually need to use TOR in order to use TOR or is only one side sufficient if additionally a relay is used? I don't know how TOR works internally at all.) |
@piegamesde I can't speak to tor because I too am not well-read on tor. Additionally, wormhole-william, which we're using for the current project, hasn't implemented tor support yet. I'm not aware of any reason that the relay wouldn't be able to speak a different protocol on each leg. @meejah implemented the transit relay side of the change so perhaps he has more concrete details here. The original motivation for us to add a WebSocket as a transport option was to support a web environment. In this context, use of Websocket transport implies use of the relay (as browsers can't listen for incoming connections*). In a scenario between clients which both support TCP and WebSocket transport, the relay is not necessarily a given; thus, I think the following would apply from the transit spec:
|
Okay, this poses a few problems. Let's assume that relays are able to bridge multiple procotols (this should be mentioned in the spec, so that clients can relay on it). Then, at the moment, there is the possibility for a nasty race:
Thus, if a relay has multiple endpoints, clients need to know this in order to deduplicate somehow. Alternatively, we can adapt the relay handshake to handle this case, but I have no straightforward idea for that. A third option would be to mandate that all relay servers always support all endpoints, but this may be troublesome for backwards compatibility reasons (esp. we'd run into the problem again when adding a third endpoint). |
@piegamesde in a scenario where both clients support multiple transports and the relay is used, I think it makes sense for the client to only connect to the relay with one transport at a time. Good observation! |
I don't really know how to move this forward. I also kind of depends on how willing we are to make big breaking changes to the relay protocol. The simplest solution I can see is to add the client side in the relay handshake:
That way the relay will know if two connection attempts stem from the same entity and can safely ignore the all except the first one. Note that this is not 100% backwards compatible on the Relay side, but at the moment there is only one and it is in our control so I don't feel that bad about it. While it solves the problem with the race hazard, it still does not resolve the other problems I see with the current abilities merging: In theory, an application supporting only websockets can communicate with an application supporting only TCP just fine, provided that the relay supports both. But there isn't really a way to tell whether a relay does, so it may just horribly fail. |
The relay server already uses |
For Tor (which is not an acronym so they prefer "Tor" over "TOR"): with client-to-server situations, a Tor connection can be thought of as just a normal TCP connection except that the server doesn't know where the original client is on the internet. So, either client can use Tor to connect to the relay server (for example) independently of each other. An even better way to use Tor would be for one side to set up an Onion service and advertise it to the other. An Onion service is a Tor way to be a server. Clients of such a server have to use Tor. In this setup, the relay server would not be needed but there also won't be any NAT issues because all connections to the Tor network are "outgoing" (including for Onion services). (We don't yet support this mode though). |
The current relay implementation can handle either TCP or WebSockets on either side (so can relay WS-to-WS or TCP-to-WS on either leg). |
Thank you for the input. In that case I suggest the following changes to the protocol:
|
I think all that sounds good @piegamesde .. it might be that we can side-step the "loops" problem: any client new enough to support WebSockets should also support the That said, we should probably still document the issue (and that new clients should always use the Edit: we could enforce this by insisting that WebSocket clients can only use the |
That's the obvious case, but no. It may end up happening whenever a client has two entries with different content to the same server. Yes, I don't expect this to ever happen, but I still think it should be documented that it might happen as it clearly is not impossible.
It'd only partially help (see above), but it's a good idea nevertheless. |
I went looking for this in the spec because I thought i recalled something about "for side ..." being a thing but didn't see it. Was this a change that just didn't get documented? |
@bryanchriswhite maybe? That handshake was already in there when I implemented WebSocket support ... |
It was already there when @vu3rdd implemented transit in the Rust port a few years ago. But I've bisected the original commit with that change, it's from the end of 2016 and it contains no further motivation: magic-wormhole/magic-wormhole@e1546bf |
Coming back to this again, this time with from the perspective of adding UDT transport (personal experiments) and advertising relay servers in the welcome message. Some random thoughts:
If you want and find this proposal acceptable, I can draft a PR containing the spec clarifications. |
I definitely think it's important to scope multiple addresses + schemes to a single logical "server". This might need slightly more thinking as to whether URLs are sufficient (certainly they are for many use-cases). Twisted's "endpoint strings" runs into this problem a little, though: "Tor" is a wrapper protocol, so you could use For example, a WebSocket server may be reachable via multiple addresses, but still wants a canonical URL ( |
First of all, now that I think more of it, there is no need for explicit TOR support: .onion servers don't make any sense (if both sides support TOR anyways, they can connect "directly"), and a server doesn't care whether you tunnel through TOR to reach it or not. (Similarly, a client can tunnel over TOR for any TCP-based protocol at well.) I did not say to only specify one URL per protocol and server. I was saying that we should group relay URLs by server so that a client only needs to pick one per server. |
I don't think that's true; they'd have to go via a relay server then. Then point of one side running an (p.s. it's "Tor" not "TOR"). |
Yeah, I get that: a list of URLs per server. That may indeed be sufficient -- I was just trying to bring up some of the weirder edge-type cases. But, you could just list |
...Also, regarding Tor, it may be that relay servers want to offer services in a network-location-hiding way. Really, that's what
|
At the moment, only the first mentioned use case is actually implemented by clients. Use case two is covered by the |
Superseded by #16. |
No description provided.