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

delay SSL socket wrapping #1504

Closed
totaam opened this issue Apr 21, 2017 · 7 comments
Closed

delay SSL socket wrapping #1504

totaam opened this issue Apr 21, 2017 · 7 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Apr 21, 2017

This would allow us to peek at the packet data and decide how to handle the client connection better, partially solving #1213#comment:5.

We would then know if we're dealing with a browser / websocket request or with a plain SSL + TCP client connection.

The ssl=MODE option could then be used for bind-ssl sockets.

The difficulty with this is to ensure that we always eventually wrap the socket with an SSL layer if the user selected bind-ssl and not allow plain TCP / HTTP.

@totaam
Copy link
Collaborator Author

totaam commented Jun 20, 2017

Having to choose which client protocols will be able to handle SSL is annoying. Raising.

@totaam
Copy link
Collaborator Author

totaam commented Aug 2, 2017

2017-08-02 09:14:21: antoine uploaded file ssl-late-wrap.patch (24.8 KiB)

work in progress patch - also adds bind-ws and bind-wss options

@totaam
Copy link
Collaborator Author

totaam commented Aug 2, 2017

2017-08-02 14:06:50: antoine uploaded file ssl-late-wrap-v2.patch (39.3 KiB)

better patch - works in almost all cases (just not wss connect to upgradable tcp)

@totaam
Copy link
Collaborator Author

totaam commented Aug 3, 2017

2017-08-03 11:13:31: antoine commented


Merged in r16599. Starting a server with:

xpra start --ssl=auto --ssl-cert=self.pem -d network,http \
    --html=on
    --bind-tcp=0.0.0.0:10000 \
    --bind-ssl=0.0.0.0:10001 \
    --bind-ws=0.0.0.0:10002 \
    --bind-wss=0.0.0.0:10003

Then connecting to each port with all the possible connection strings (ie: tcp:localhost:10000, ssl:localhost:10001, ...)

  • OK means the connection succeeded
  • ERROR OK means the connection failed, as expected (wrong protocol)

|| ||||||||# Target Server Port||
||# Client Connection||# TCP||# SSL||# WS||# WSS||
||TCP || OK || ERROR OK || ERROR OK || ERROR OK ||
||SSL || OK (upgraded) || OK || ERROR OK || ERROR OK ||
||WS || OK (upgraded) || ERROR OK || OK || ERROR OK ||
||WSS || error: upgraded to SSL but not http / websocket || error: not upgraded to http / websocket || OK (upgraded) || OK ||

With html=off, it becomes impossible to connect to TCP sockets using WS.
With ssl=www, it becomes possible to connect to SSL ports using WSS.
With ssl=www and html=on, it becomes possible to connect to TCP sockets using WSS. (socket is upgraded to SSL and then websockets)

The problem with WSS connections not being upgraded to http / websocket is that the socket peek data we get before wrapping the socket with SSL doesn't have any distinguishable HTTP header data in it. (and we still cannot peak at SSL sockets - silly python API limitation)

Maybe we could workaround that: read from the ssl socket before constructing the protocol object, inspect it then wrap it and re-inject the data. (would mean wrapping the whole ssl socket object since we can't override socket.recv)
It might be easier to modify websockify's recv_frames (re-submit the split-out patches and check for method support at runtime), or just override that method completely.

@totaam
Copy link
Collaborator Author

totaam commented Aug 4, 2017

Helped by the information from Subclassing and built-in methods in Python, r16607 overrides the recv socket method to inject peek support. This allows us to detect HTTP headers in SSL packets, which means that the two error cases from comment:2 are now fixed:

  • we can connect using WSS to plain TCP sockets: they get upgraded twice, first to SSL then to websocket
  • we can connect using WSS to SSL sockets: they get upgraded to websocket

There is a small cost associated with the extra socket.recv call we intercept, but since this is on an IO function I don't think this matters.

@maxmylin: something to be aware of, feel free to just close.

@totaam
Copy link
Collaborator Author

totaam commented Aug 4, 2017

2017-08-04 17:59:17: maxmylyn commented


Noted and closing.

(btw it's maxmylyn with two ys. Not sure what my dad was thinking but he sure did come up with a creative spelling.)

@totaam totaam closed this as completed Aug 4, 2017
@totaam
Copy link
Collaborator Author

totaam commented Sep 10, 2017

See also #1636, r27656

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

No branches or pull requests

1 participant