Do a clean TLS shutdown for HTTPS #2565
Conversation
|
I think we have discussed this in issue 2564 and the bottom line is that if you call Link Close() on a TLS connection, the correct shutdown sequence happens. This code here simply duplicates the code in Link Close(). So, I propose we a) verify that this is true, and b) if true, delete the HTTP specific code to use the already implemented code for TLS connections. If that implementation is not correct then it should be fixed. In any case, having duplicate handling is not what we want here. |
|
The whole point is to consolidate the code not to replicate things. What would be your suggestion to have this handled in one central place? |
I'd create a separate function and call it in both places. Right now since the code is inside a class it's kinda hard to share. |
d43f943 to
6f325f8
Compare
Unlike XrdTlsSocket, the HTTP protocol did a one-sided TLS shutdown. This saved network a round-trip but at the cost of correctness: if the server shut down the connection after its response while the client was still sending data then the client may recieve a TCP reset prior to reading out the response. This exact behavior was observed in unit tests and the correct approach is outlined in latest HTTP 1.1 RFC: https://datatracker.ietf.org/doc/html/rfc9112#name-tls-connection-closure Basically, we now do the same as `XrdTlsSocket` and perform a bidirectional TLS shutdown.
6f325f8 to
7f97d8a
Compare
|
Although we still need to address the non-blocking I/O case as I mentioned in my comment, what we have here is definitely an improvement over what is currently done, so I'm picking this up for XRootD 5.9. |
Unlike XrdTlsSocket, the HTTP protocol did a one-sided TLS shutdown. This saved network a round-trip but at the cost of correctness: if the server shut down the connection after its response while the client was still sending data then the client may recieve a TCP reset prior to reading out the response.
This exact behavior was observed in unit tests and the correct approach is outlined in latest HTTP 1.1 RFC:
https://datatracker.ietf.org/doc/html/rfc9112#name-tls-connection-closure
Basically, we now do the same as
XrdTlsSocketand perform a bidirectional TLS shutdown.This takes care of the HTTPS portion of #2564.