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

Websocket address type: attempt 2 #1068

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Apr 26, 2023

#891 seemed to be dead and does not properly support things like TLS.

This is a re-attempt to bring back the proposal while addressing the concerns. It is mostly the same as @rustyrussell's but changed it so be the same as the DNS hostname address descriptor so things like TLS can be supported and doesn't require scanning all the other network addresses.

Co-authored-by: Rusty Russell <rusty@rustcorp.com.au>
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

A few things we need to consider here: (a) We really need to support wss as well, to do this we need a hostname to check the TLS cert against, (b) if we weren't supporting wss we'd probably not want a hostname at all here but rather simply a feature flag indicating that WS is supported. These two goals are somewhat contradictory, sadly, so I'm not sure what the right design is, maybe a feature flag indicating websockets are supported for all connection modes but also a wss hostname type? Or maybe a ws hostname type that has a secure bit in it? I'm open to either, but we need to support wss somehow.

@rustyrussell
Copy link
Collaborator

My plan was to abandon #891 altogether. Allowing a websocket is cool, advertising it less so. In particular, I have no plans to support wss in CLN, so it will be hard to get two interoperable implementations. And in fact a standalone connector to Apache / nginx probably makes more sense than existing implementations doing it.

I have a patch (missed this release, will be in next) to drop advertising the websocket.

@TheBlueMatt
Copy link
Collaborator

Are there popularly available wrappers which map websockets to TCP? IIUC the majority of relevant wrappers just apply TLS over unencrypted websockets, so the node software also needs to support websockets (I assume you mean you're going to stop announcing, but keep the websocket code itself?)

I do disagree that we shouldn't announce wss - if a node wants to accept connections from browser nodes (or browser extension wallets, which are a common request), they have to put the hostname they accept TLS on somewhere, and imo it might as well be in the gossip.

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.

None yet

3 participants