Skip to content

update transit url to use WebSocket over TLS with standard port#174

Closed
wuan wants to merge 1 commit into
LeastAuthority:mainfrom
wuan:update_transit_url
Closed

update transit url to use WebSocket over TLS with standard port#174
wuan wants to merge 1 commit into
LeastAuthority:mainfrom
wuan:update_transit_url

Conversation

@wuan

@wuan wuan commented Sep 24, 2022

Copy link
Copy Markdown
Contributor

This change should fix two problems:


Code Review Checklist

  • Description accurately reflects what changes are being made.
  • Description explains why the changes are being made (or references an issue containing one).
  • The PR appropriately sized.
  • New code has enough tests.
  • New code has enough documentation to answer "how do I use it?" and "what does it do?".
  • Existing documentation is up-to-date, if impacted.

ewanas
ewanas previously approved these changes Sep 25, 2022
@donpui

donpui commented Sep 26, 2022

Copy link
Copy Markdown
Contributor

The best would be that we could include both WebSocket and TCP relay addresses, then depending which succeeds first, works, but this might require extra work

@wuan

wuan commented Sep 26, 2022

Copy link
Copy Markdown
Contributor Author

Having a mobile app (which I expect will be the main use-case for Destiny) we need to get away from using non standard ports for our connections. This will get us a number of bad reviews, because for the end user the app will just not work when it is being used in a network which restricts access. This happens quite often if you are using public wireless offerings.

I would even go in the direction of enabling the CLI to use WebSocket to get away from that non standard port usage. (If there are no technical reasons against it).

@ewanas ewanas self-requested a review September 26, 2022 12:51
@ewanas ewanas dismissed their stale review September 26, 2022 12:52

Need to decide if switching to WS will allow backwards compatibility after noise changes are merged

@donpui

donpui commented Sep 26, 2022

Copy link
Copy Markdown
Contributor

We are depending from this PR: LeastAuthority/wormhole-william#80, because which if we switch to early, we will not be interoperable with older destiny versions (or initial released version). Require some internal discussion for this.

@meejah

meejah commented Sep 26, 2022

Copy link
Copy Markdown
Collaborator

You can use the TCP relay on 443 (if that's what you're getting at).

@meejah

meejah commented Sep 26, 2022

Copy link
Copy Markdown
Collaborator

We are depending from this PR: LeastAuthority/wormhole-william#80, because which if we switch to early, we will not be interoperable with older destiny versions (or initial released version). Require some internal discussion for this.

What? No, why are we talking about changing the transit protocol??
Next-gen file transfer will use Dilation, right?

@donpui

donpui commented Nov 30, 2022

Copy link
Copy Markdown
Contributor

As there is no more changes regarding websocket framing fix, maybe we can merge this PR and have default websocket connect via relay for Destiny. To my understanding, there should be no interoperability issues with older versions or other implementations. Any objections @vu3rdd @ewanas @meejah?

@meejah

meejah commented Nov 30, 2022

Copy link
Copy Markdown
Collaborator

Right, half of the reason for this PR is gone.

(If there really are good reasons to use "standard" ports maybe a bug about that in particular should be filed, since we we could in principal listen for both kinds of connections on one port e.g. 443).

I put "standard" in quotes because 4001 is the standard relay port for TCP. I think what is meant here is "work around dumb port-based blocking by re-using the HTTPS port for everything"...?

@wuan

wuan commented Nov 30, 2022

Copy link
Copy Markdown
Contributor Author

I put "standard" in quotes because 4001 is the standard relay port for TCP. I think what is meant here is "work around dumb port-based blocking by re-using the HTTPS port for everything"...?

Neither IANA nor Wikipedia give any hint about port 4001 being part of some established standard. As the app properly supports wss:// there should be no problem using it.

Already had the experience with an Android App fetching data on port 7080. Over the years there were many bad reviews because the App did not work properly in some restricted network environments. The intention of this PR has been to avoid that we need to learn that again.

@donpui

donpui commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

I think main question or discussion here: keeping default ports of magic-wormhole or using standard HTTPS port.

Is default magic-wormhole ports documented and recommended to be used? What are benefits or downside keeping 4001 and 4002?

From HTTPS port usage, another benefit is that it "hides" under regular traffic to my understanding. If someone monitor activity on network, I think it is easier to notice uncommon port usage, then traffic via 443. Question, can usage of HTTPS port conflict with other traffic?

@meejah

meejah commented Dec 6, 2022

Copy link
Copy Markdown
Collaborator

I think main question or discussion here: keeping default ports of magic-wormhole or using standard HTTPS port.

Seems moreso the question is "use WebSocket for Transit" versus "use TCP for Transit" from Destiny.

@donpui

donpui commented Dec 13, 2022

Copy link
Copy Markdown
Contributor

Proposal: pass both TCP and WS addresses. Other side will receive two hints and will try to call both.

Simples solution would be just add another parameter and pass to dart and C transitRelayUrlWs. Or make more flexible and do more coding to pass array of urls

@meejah

meejah commented Dec 13, 2022

Copy link
Copy Markdown
Collaborator

Proposal: pass both TCP and WS addresses. Other side will receive two hints and will try to call both.

I think this is the least-good way to do it.
This means every client has to care about every kind of transport, forever.

@wuan

wuan commented Dec 14, 2022

Copy link
Copy Markdown
Contributor Author

I think this is the least-good way to do it. This means every client has to care about every kind of transport, forever.

So the idea behind this PR was originally to move all communication to standard ports. Now after all that discussion it seems to be not right to move in any direction.

So I propose to follow the direction to focus on using WebSocket in the future. With Winden we have no other choice anyway.

@wuan

wuan commented Dec 14, 2022

Copy link
Copy Markdown
Contributor Author

Anyway closing this PR in favour of #209.

@wuan wuan closed this Dec 14, 2022
@meejah

meejah commented Dec 14, 2022

Copy link
Copy Markdown
Collaborator

Maybe I misunderstood something then? Or was confused with the PRs about interop .. if this is just about putting more hints from one side (so long as Destiny is indeed listening/connecting on them all) that should work. If we need the other side to put in hints, then it won't.

@donpui

donpui commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

Maybe I misunderstood something then? Or was confused with the PRs about interop .. if this is just about putting more hints from one side (so long as Destiny is indeed listening/connecting on them all) that should work. If we need the other side to put in hints, then it won't.

We put two hints instead of one, which is allowed from protocol perspective. Sender will send two relay addresses via hints. These address are common to Sender and Receiver on w-w, so it means that Receiver also will add two hints instead of one and exchange with whatever client Sender is. But this should not be a problem for Destiny and Winden, especially after if we merge this LeastAuthority/wormhole-william#95 as it removes duplicates, but even know, receiver is consuming only Sender hints for relay.

@meejah

meejah commented Dec 14, 2022

Copy link
Copy Markdown
Collaborator

Yes, sending multiple hints are fine from the protocol perspective (I must have been confused with the interop PRs, i.e. depending on a client to send a WebSocket hint when it doesn't know about WS).

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

Successfully merging this pull request may close these issues.

4 participants