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

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

Open
jens-diewald opened this issue Dec 14, 2022 · 2 comments · May be fixed by #77
Open

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

jens-diewald opened this issue Dec 14, 2022 · 2 comments · May be fixed by #77

Comments

@jens-diewald
Copy link
Contributor

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
Copy link
Contributor Author

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 jens-diewald linked a pull request Feb 11, 2023 that will close this issue
@jens-diewald
Copy link
Contributor Author

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.

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 a pull request may close this issue.

1 participant