laudrup / boost-wintls

Native Windows TLS stream wrapper for use with Asio
https://wintls.dev
Boost Software License 1.0
54 stars 13 forks source link

Stream Shutdown does not produce Error on incomplete protocol shutdown #75

Open jens-diewald opened 1 year ago

jens-diewald commented 1 year ago

As originally mentioned in #56, Asio.SSL has error::stream_truncated. This StackOverflow Post does explain very well what it is good for: https://stackoverflow.com/questions/25587403/boost-asio-ssl-async-shutdown-always-finishes-with-an-error (I think, that stream_truncated did not exist when that post was written and corresponds to the mentioned SSL_R_SHORT_READ.)

I have added #74 to show the current behavior of Asio.SSL and WinTLS in the relevant cases.

Looking at the Schannel Docs, i suspect that the sspi_shutdown class may have to be more complex than it currently is..?

As a sidenote: The StackOverflow post mentions that Asio should produce error::eof during shutdown in some cases. I did not observe that in my tests. However, this is handled in the beast https sync/async examples and in the WinTLS async example, butn not in the WinTLS sync example. Could it be, that the information is outdated and error::eof will not occur anymore? At the very least, it seems inconsistent, that error::eof is handled in the async example, but not in the sync example.

jens-diewald commented 1 year ago

The beast https server examples have a helpful comment on this topic. For example here. In particular: When using with HTTP (or another "self-terminated" protocol) it is not necessary to wait for the peers close_notify. For better performance (or simpler code?), many implementations just close the underlying transport after sending their close_notify, without waiting for the peer. This also seems to be, what the current sspi_shutdown code in wintls is doing. Although i do not quite understand why it is okay to always use InitializeSecurityContext in sspi_shutdown::operator() and not AcceptSecurityContext when running as a server, as the documentation clearly states. @laudrup, can you comment on that?

Altogether, i think it may be reasonable to keep the current shutdown functions as a more efficient variant to use with HTTP or other "self-terminated" protocols. But as this library is supposed to be more general than that, the default shutdown functions should make sure that the shutdown procedure was completed properly. I could not find an existing implementation as a good reference, but according to the documentation, the shutdown should be implemented very similarly to the handshake with a loop similar to the one in stream::handshake(handshake_type, error_code&). Then, a new error code stream_truncated should be introduced analogously to asio::ssl and it should explicitly be set, when asio::error::eof occurs when trying to write our close_notify to the stream or when trying to read the peers close_notify from the stream.

jens-diewald commented 1 year ago

As for a good reference implementation i have found https://github.com/steffengy/schannel-rs, which is rust but otherwise similar to wintls. Notably, they use one function for the initial handshake and also for the shutdown. (https://github.com/steffengy/schannel-rs/blob/e93c3bcf588630717bc592fe9f36fc4b740589b5/src/tls_stream.rs#L583) I believe that this is generally a good idea and that a lot of code can be share between the initial handshake and the shutdown. As a first effort towards this end, i have added #77 to simplify the handshake code.

InternalHigh commented 3 months ago

Please complete the PR

laudrup commented 3 months ago

@InternalHigh

I guess it's up to you to complete this @jens-diewald :-)

jens-diewald commented 3 months ago

Hi, I will try to find some time to look into this again. At first, I should rebase this to the current master. This applies to #77 as well, on which I based #78. @laudrup, it would be nice to merge this before, as it is an independent refactoring which should improve the code in its own right. It looks like the lowest tested boost version is 1.81 now, so I can even remove the post_self case distinction I introduced there.