search

clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Only yield client streams after successful SSL handshake #623

Closed DerGuteMoritz closed 2 years ago

DerGuteMoritz commented 2 years ago

The deferred returned by aleph.tcp/client would always be resolved as soon as the TCP connection was established, even when a :ssl-context was given and the SSL handshake had not yet taken place. With this patch, it will only be resolved once the SSL handshake was successful.

Unfortunately, since TLSv1.3 the connection may still fail with a handshake error even after the handshake has seemingly succeeded. This is because only an invalid certificate is explicitly rejected while a valid certificate will be implicitly accepted. Thus, it's only possible to determine whether the handshake has fully succeeded after having succeessfully read some data. See [1] and [2] for more details. However, this patch still seems worhtwhile even for TLSv1.3 since the stream is only yielded at the earliest time it would theoretically be possible to successfully use it.

This patch is the client-side dual of 9c911e37f305f3392b5b0c57156eff4c16bc8dc2 and as such, the common code is extracted into a on-connection-established helper.

1: https://github.com/netty/netty/issues/10502 2: https://stackoverflow.com/questions/62459802/tls-1-3-client-does-not-report-failed-handshake-when-client-certificate-verifica/62465859#62465859

KingMob commented 2 years ago

@DerGuteMoritz LGTM.

I'm not worried about TLS1.3 if the protocol is designed to allow optimistic sends before cert verification. That's a user problem (though it wouldn't hurt to remind them in docs).

Questions:

  1. To confirm, the server behavior is identical, right, just refactored now?
  2. Do we need to change anything in the HTTP code?
DerGuteMoritz commented 2 years ago
  • To confirm, the server behavior is identical, right, just refactored now?

That is correct, on-connection-established helper is verbatim extraction of the original.

  • Do we need to change anything in the HTTP code?

Ah heck, unlike the server, this should indeed also apply to the HTTP client. Thanks! Another commit coming up ...

DerGuteMoritz commented 2 years ago

@KingMob OK, you may check again!

DerGuteMoritz commented 2 years ago

Ah, the new use of on-connection-fully-established actually doesn't work with its error handling behavior. Another moment ...

DerGuteMoritz commented 2 years ago

Alright, I think I'm sufficiently happy with this now :sweat_drops: I suggest to check the individual commits to understand how I got here. Hope it makes sense!

DerGuteMoritz commented 2 years ago

@KingMob Good call, done! Would you like me to squash this into a more pointed set of patches?

KingMob commented 2 years ago

I like to rewrite history, especially when commits undo other commits, but it's not mandatory.

Do you have merge perms?

DerGuteMoritz commented 2 years ago

I like to rewrite history, especially when commits undo other commits, but it's not mandatory.

Yeah, same here, that's why I suggested it - commits were more of a WIP log. I've just squashed them all into one again, didn't really make sense to break them up.

Do you have merge perms?

I do!