Skip to content

websockets implementation#979

Merged
slingamn merged 9 commits intoergochat:masterfrom
slingamn:websockets_draft.7
May 6, 2020
Merged

websockets implementation#979
slingamn merged 9 commits intoergochat:masterfrom
slingamn:websockets_draft.7

Conversation

@slingamn
Copy link
Copy Markdown
Member

@slingamn slingamn commented May 5, 2020

Continues from #971.

This fixes #967 and #978.

This is very fiddly so code review would be very welcome (cc @hhirtz and @ajaspers).

@slingamn slingamn mentioned this pull request May 5, 2020
3 tasks
@slingamn
Copy link
Copy Markdown
Member Author

slingamn commented May 5, 2020

This doesn't work with Chrome, because of a Chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=329884

The workaround is straightforward, I'll submit it soon.

Copy link
Copy Markdown
Contributor

@ajaspers ajaspers left a comment

Choose a reason for hiding this comment

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

Looks good to me on the first pass, but I only have a partial understanding of some of the APIs being used.

One question: How does reloading handle TLS cert key/cert changes? It looks like "stillConfigured" is only checking whether the address itself still exists in the config. Reload() just then copies the config over, but what actually applies the key/cert change?

Copy link
Copy Markdown
Contributor

@hhirtz hhirtz left a comment

Choose a reason for hiding this comment

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

This is some really nice stuff. 👍

I have gone through the changes once but I guess I'll have to read this another time (maybe tomorrow), because I didn't understand how ProxiedConnection works with websockets.

@slingamn
Copy link
Copy Markdown
Member Author

slingamn commented May 5, 2020

Looks good to me on the first pass, but I only have a partial understanding of some of the APIs being used.

One question: How does reloading handle TLS cert key/cert changes? It looks like "stillConfigured" is only checking whether the address itself still exists in the config. Reload() just then copies the config over, but what actually applies the key/cert change?

This is kinda neat actually (it's the same design as before). Immediately after each Accept() returns, ReloadableListener checks the current state of its config and acts on it:

https://github.com/oragono/oragono/blob/5ae6f6b927fb8d2f878aa6b9596afee560c36e8d/irc/utils/proxy.go#L122

applying a TLS wrapper (including the up-to-date tls.Config, which contains the cert-key pair) and reading the PROXY line as necessary, and then it returns the wrapped connection to whoever is listening (either NetListener for a stream socket, or HttpServer for a websocket).

@slingamn
Copy link
Copy Markdown
Member Author

slingamn commented May 5, 2020

I have gone through the changes once but I guess I'll have to read this another time (maybe tomorrow), because I didn't understand how ProxiedConnection works with websockets.

The name ProxiedConnection is bad and I should probably change it (maybe WrappedConnection?). Basically, the ReloadableListener gets passed into http.Server, so the connection that comes back is always a ProxiedConnection (whether or not PROXY protocol support is enabled). Then after the upgrade (hijacking), the connection is available as websocket.Conn.UnderlyingConnection() and its special fields can be used via a type assertion:

https://github.com/oragono/oragono/blob/5ae6f6b927fb8d2f878aa6b9596afee560c36e8d/irc/listeners.go#L189

@slingamn
Copy link
Copy Markdown
Member Author

slingamn commented May 6, 2020

Someone needs this so I'm going to merge it for now --- still interested in any follow-up comments.

@slingamn slingamn merged commit d37af69 into ergochat:master May 6, 2020
@slingamn slingamn deleted the websockets_draft.7 branch May 8, 2020 08:00
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.

websocket support

3 participants