Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Add /wss dialing support #15

Closed
wants to merge 1 commit into from
Closed

Add /wss dialing support #15

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 2, 2017

We can't actually do much with it yet because we lack the ability to describe HTTP and TLS/SNI hostnames, but whatever, here goes :)

At least it enables us to understand and announce /wss multiaddrs, which is nice for browsers/interop UX.

@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage increased (+2.0%) to 66.883% when pulling 356e92b on feat/wss-multiaddr into 9c8230f on master.

@dignifiedquire
Copy link
Member

What's missing for this to be merged @lgierth @whyrusleeping

var WsFmt = mafmt.And(mafmt.TCP, mafmt.Base(WsProtocol.Code))
var WsFmt = mafmt.Or(
mafmt.And(mafmt.TCP, mafmt.Base(WsProtocol.Code)),
mafmt.And(mafmt.TCP, mafmt.Base(WssProtocol.Code)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more readable as TCP and (WS or WSS) instead of (TCP and WS) or (TCP and WSS)

@Stebalien
Copy link
Member

Status: This needs to be updated for the new websockets library. Also, in case anyone is wondering,

We can't actually do much with it yet because we lack the ability to describe HTTP and TLS/SNI hostnames, but whatever, here goes :)

Is not a security problem, it just means one can't actually dial (as far as I can tell).


Speaking of which, this will make it possible to talk about wss addresses but I don't see how it will allow us to advertise them (as we still can't listen on wss addresses).

@ghost
Copy link
Author

ghost commented Sep 5, 2017

Speaking of which, this will make it possible to talk about wss addresses but I don't see how it will allow us to advertise them (as we still can't listen on wss addresses).

For the /wss listening part, It'll still require a TLS reverse proxy (nginx et al) in front.

If we wanna actually support TLS in go-ipfs itself (we should at some point), we should look into using Caddy as a library. Same applies to the gateway.

@ghost
Copy link
Author

ghost commented Sep 5, 2017

how it will allow us to advertise them

We can advertise pretty much anything we like now (through Addresses.Announce in the config) :)

@Stebalien
Copy link
Member

We can advertise pretty much anything we like now (through Addresses.Announce in the config) :)

Ah. I thought we only advertised addresses when we succeeded on listening on it.

If we wanna actually support TLS in go-ipfs itself (we should at some point), we should look into using Caddy as a library. Same applies to the gateway.

We should also consider implementing a handshake like the one I described in libp2p/go-libp2p#34 (comment) to avoid double encryption.

@ghost
Copy link
Author

ghost commented Sep 5, 2017

We should also consider implementing a handshake like the one I described in libp2p/go-libp2p#34 (comment) to avoid double encryption.

Yeah! It'll be next to impossible to get key/cert information in the browser though, which would be neccessary to verify the communication channel. But let's keep our eyes open -- other impossible things have become possible.

@Stebalien
Copy link
Member

Browsers can probably rely on origins (as much as I hate relying on the CA system when we don't actually need to do so). We just need some way to identify the connection.

@ghost
Copy link
Author

ghost commented Sep 5, 2017

Browsers can probably rely on origins (as much as I hate relying on the CA system when we don't actually need to do so). We just need some way to identify the connection.

I mean verifying the actual encrypted and signed communication channel -- there's stuff like TLS interception and so. I think it's a bad idea to sacrifice the integrity of the connection for performance gains.

@Stebalien
Copy link
Member

Assuming the CA system hasn't been compromised, the domain of the website you're connecting to should be sufficient. We just need a name that the client can verify.

@ghost
Copy link
Author

ghost commented Sep 5, 2017

That's a pretty bold assumption ;) There's regular commercial MitM boxes on the one hand, and CAs run by quasi-state actors on the other.

But yes if we somewhat trust the underlying TLS connection, we can verify the remote end either by relying on future APIs in the browser, or by doing a proper secio handshake that we throw away right after.

@Stebalien
Copy link
Member

That's a pretty bold assumption

Well, yes. But you work with what you have.

There's regular commercial MitM boxes on the one hand

Those generally assume you can install certs in the browser.

@ghost ghost mentioned this pull request Oct 29, 2017
@Stebalien
Copy link
Member

@lgierth FYI, this still needs some TLC before it can be merged. And I now agree, let's just not do security at this level; we can consider doing that later (should be fairly easy to do with multistream).

@Stebalien
Copy link
Member

This'll probably need to be redone given the refactor.

@ConorTighe
Copy link

Any progress on this? this wont be resolved until the next version of IPFS I take it?

@Stebalien
Copy link
Member

This is something we'd like but isn't a high priority at the moment. However, we'd be happy to take a patch if you can get this working!

@Jorropo Jorropo mentioned this pull request May 28, 2019
10 tasks
@raulk
Copy link
Member

raulk commented Jan 4, 2022

There were other changes needed to take this over the finish line. #46 was an attempt to continue this PR, but ultimately was superseded by #72, which 0x has deployed in production.

@raulk raulk closed this Jan 4, 2022
@raulk raulk deleted the feat/wss-multiaddr branch January 4, 2022 12:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants