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

Implement full shutdown #78

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

jens-diewald
Copy link
Contributor

The implementation still has Issues, but it generally solves #75.
This is particularly documented in the new tests in shutdown_test.cpp, so I close #74 in favor of this branch.
The branch is based on top of #77.

Besides smaller things, there are two major problems still:

  • The test case small reads/async client test in stream_test.cpp fails.
    • I have no idea why, any hints are appreciated.
  • io_context.run() in echo test/async test in echo_test.cpp blocks with wintls_server.
    • I also have no idea why. It can be worked around by switching the calls to client.run() and server.run(), as shown in the last commit. But that is no proper solution, obviously. Again, any hints are appreciated.

Both problems are introduced with the commit 26fc986.

Or rather move it to the end where it is called at most once.
This resolves the TODO on sspi_handshake::state
and is intended to simplify the code to allow for further refactoring.

There is a small change in behavior:
As a server, all communication with the client is now done before manual_auth.
Before, manual_auth was called as early as possible and
possibly more data was sent through the stream after that.
What the comment stated for the AcceptSecurityContext documentation
holds for InitializeSecurityContext just as much.
Again this is to simplify the code.
I do not currently see a reason for the function to be a coroutine.

Also add a comment about posting self.complete on the first call.
Change the code such that posting self is only needed once.
Also add a comment explaining why this is necessary.
This failed once on the github CI. Assuming that the failure was due to
high load on the test server, this increases the timeout so that
hopefully this will not fail again in the future.
The handshake loop shall be used for shutdown as well.
This seemingly worked correctly before, but the documentation clearly
states that AcceptSecurityContext should be used when acting as a server.
See https://learn.microsoft.com/en-us/windows/win32/secauthn/shutting-down-an-schannel-connection
More precisely, add test where one of the endpoints closes the
underlying stream without shutting down the TLS stream.
WinTLS will currently not produce an error in such cases.
WinTLS will now produce asio::error::eof when the underlying
stream gets closed without the peer sending its close notify.
This should be changed to a separate error code analogously to
Asio.SSL.
The error code is designed to be analogously to the one in asio.ssl.
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 this pull request may close these issues.

1 participant