From eab1d148fd6143ea2dbdaab6beb565804087fb46 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Sat, 11 Feb 2023 13:33:00 +0100 Subject: [PATCH 01/19] Move manual_auth out of the handshake loop. 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. --- .../boost/wintls/detail/async_handshake.hpp | 24 +------------ .../boost/wintls/detail/sspi_handshake.hpp | 34 +++++++------------ include/boost/wintls/stream.hpp | 24 +++---------- 3 files changed, 18 insertions(+), 64 deletions(-) diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index 0a353ae3..b9a45e1e 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -82,29 +82,6 @@ struct async_handshake : boost::asio::coroutine { self.complete(handshake_.last_error()); return; } - - if (handshake_state == detail::sspi_handshake::state::done_with_data) { - BOOST_ASIO_CORO_YIELD { - state_ = state::writing; - net::async_write(next_layer_, handshake_.out_buffer(), std::move(self)); - } - break; - } - - if (handshake_state == detail::sspi_handshake::state::error_with_data) { - BOOST_ASIO_CORO_YIELD { - state_ = state::writing; - net::async_write(next_layer_, handshake_.out_buffer(), std::move(self)); - } - if (!is_continuation()) { - BOOST_ASIO_CORO_YIELD { - auto e = self.get_executor(); - net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); - } - } - self.complete(handshake_.last_error()); - return; - } } if (!is_continuation()) { @@ -114,6 +91,7 @@ struct async_handshake : boost::asio::coroutine { } } BOOST_ASSERT(!handshake_.last_error()); + handshake_.manual_auth(); self.complete(handshake_.last_error()); } } diff --git a/include/boost/wintls/detail/sspi_handshake.hpp b/include/boost/wintls/detail/sspi_handshake.hpp index 6315513e..3fb90878 100644 --- a/include/boost/wintls/detail/sspi_handshake.hpp +++ b/include/boost/wintls/detail/sspi_handshake.hpp @@ -28,15 +28,11 @@ namespace detail { class sspi_handshake { public: - // TODO: enhancement: done_with_data and error_with_data can be removed if - // we move the manual validate logic out of the handshake loop. enum class state { - data_needed, // data needs to be read from peer - data_available, // data needs to be write to peer - done_with_data, // handshake success, but there is leftover data to be written to peer - error_with_data, // handshake error, but there is leftover data to be written to peer - done, // handshake success - error // handshake error + data_needed, // data needs to be read from peer + data_available, // data needs to be write to peer + done, // handshake success + error // handshake error }; sspi_handshake(context& context, ctxt_handle& ctxt_handle, cred_handle& cred_handle) @@ -129,6 +125,9 @@ class sspi_handshake { } state operator()() { + if (last_error_ == SEC_E_OK) { + return state::done; + } if (last_error_ != SEC_I_CONTINUE_NEEDED && last_error_ != SEC_E_INCOMPLETE_MESSAGE) { return state::error; } @@ -209,13 +208,9 @@ class sspi_handshake { return has_buffer_output ? state::data_available : state::data_needed; } case SEC_E_OK: { - // sspi handshake ok. perform manual auth here. - manual_auth(); - if (handshake_type_ == handshake_type::client) { - if (last_error_ != SEC_E_OK) { - return state::error; - } - } else { + // sspi handshake ok. Manual authentication will be done after the handshake loop. + + if (handshake_type_ == handshake_type::server) { // Note: we are not checking (out_flags & ASC_RET_MUTUAL_AUTH) is true, // but instead rely on our manual cert validation to establish trust. // "The AcceptSecurityContext function will return ASC_RET_MUTUAL_AUTH if a @@ -227,7 +222,7 @@ class sspi_handshake { // "If function generated an output token, the token must be sent to the client process." // This happens when client cert is requested. if (has_buffer_output) { - return last_error_ == SEC_E_OK ? state::done_with_data : state::error_with_data; + return state::data_available; } } return state::done; @@ -274,7 +269,6 @@ class sspi_handshake { check_revocation_ = check; } -private: SECURITY_STATUS manual_auth(){ if (!context_.verify_server_certificate_) { return SEC_E_OK; @@ -284,16 +278,12 @@ class sspi_handshake { if (last_error_ != SEC_E_OK) { return last_error_; } - cert_context_ptr remote_cert{ctx_ptr}; - last_error_ = static_cast(context_.verify_certificate(remote_cert.get(), server_hostname_, check_revocation_)); - if (last_error_ != SEC_E_OK) { - return last_error_; - } return last_error_; } +private: context& context_; ctxt_handle& ctxt_handle_; cred_handle& cred_handle_; diff --git a/include/boost/wintls/stream.hpp b/include/boost/wintls/stream.hpp index 0e1e0747..bcedc048 100644 --- a/include/boost/wintls/stream.hpp +++ b/include/boost/wintls/stream.hpp @@ -137,9 +137,8 @@ class stream { void handshake(handshake_type type, boost::system::error_code& ec) { sspi_stream_->handshake(type); - detail::sspi_handshake::state state; - while((state = sspi_stream_->handshake()) != detail::sspi_handshake::state::done) { - switch (state) { + while (true) { + switch (sspi_stream_->handshake()) { case detail::sspi_handshake::state::data_needed: { std::size_t size_read = next_layer_.read_some(sspi_stream_->handshake.in_buffer(), ec); if (ec) { @@ -159,24 +158,11 @@ class stream { case detail::sspi_handshake::state::error: ec = sspi_stream_->handshake.last_error(); return; - case detail::sspi_handshake::state::done_with_data:{ - std::size_t size_written = net::write(next_layer_, sspi_stream_->handshake.out_buffer(), ec); - if (ec) { - return; - } - sspi_stream_->handshake.size_written(size_written); - return; - } - case detail::sspi_handshake::state::error_with_data:{ - std::size_t size_written = net::write(next_layer_, sspi_stream_->handshake.out_buffer(), ec); - if (ec) { - return; + case detail::sspi_handshake::state::done: + if (sspi_stream_->handshake.manual_auth() != SEC_E_OK) { + ec = sspi_stream_->handshake.last_error(); } - sspi_stream_->handshake.size_written(size_written); return; - } - case detail::sspi_handshake::state::done: - BOOST_UNREACHABLE_RETURN(0); } } } From 66a435cf521daf4ba03d8d5680df74867f6651ff Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Sat, 11 Feb 2023 13:34:12 +0100 Subject: [PATCH 02/19] Also send leftover handshake data as client. What the comment stated for the AcceptSecurityContext documentation holds for InitializeSecurityContext just as much. --- .../boost/wintls/detail/sspi_handshake.hpp | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/include/boost/wintls/detail/sspi_handshake.hpp b/include/boost/wintls/detail/sspi_handshake.hpp index 3fb90878..2b41b85d 100644 --- a/include/boost/wintls/detail/sspi_handshake.hpp +++ b/include/boost/wintls/detail/sspi_handshake.hpp @@ -210,22 +210,18 @@ class sspi_handshake { case SEC_E_OK: { // sspi handshake ok. Manual authentication will be done after the handshake loop. - if (handshake_type_ == handshake_type::server) { - // Note: we are not checking (out_flags & ASC_RET_MUTUAL_AUTH) is true, - // but instead rely on our manual cert validation to establish trust. - // "The AcceptSecurityContext function will return ASC_RET_MUTUAL_AUTH if a - // client certificate was received from the client and schannel was - // successfully able to map the certificate to a user account in AD" - // As observed in tests, this check would wrongly reject openssl client with valid certificate. - - // AcceptSecurityContext documentation: - // "If function generated an output token, the token must be sent to the client process." - // This happens when client cert is requested. - if (has_buffer_output) { - return state::data_available; - } - } - return state::done; + // Note: When we requested client auth as a server, + // we are not checking (out_flags & ASC_RET_MUTUAL_AUTH) is true, + // but instead rely on our manual cert validation to establish trust. + // "The AcceptSecurityContext function will return ASC_RET_MUTUAL_AUTH if a + // client certificate was received from the client and schannel was + // successfully able to map the certificate to a user account in AD" + // As observed in tests, this check would wrongly reject openssl client with valid certificate. + + // AcceptSecurityContext/InitializeSecurityContext documentation for return value SEC_E_OK: + // "If function generated an output token, the token must be sent to the client/server." + // This happens when client cert is requested. + return has_buffer_output ? state::data_available : state::done; } case SEC_I_INCOMPLETE_CREDENTIALS: From 4ba64de15d6363842f99d3ed961813a3ef152b71 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Sat, 11 Feb 2023 13:35:40 +0100 Subject: [PATCH 03/19] Remove unused member from async_handshake --- include/boost/wintls/detail/async_handshake.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index b9a45e1e..8e35d94a 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -100,7 +100,6 @@ struct async_handshake : boost::asio::coroutine { NextLayer& next_layer_; detail::sspi_handshake& handshake_; int entry_count_; - std::vector input_; enum class state { idle, reading, From 88ce93cbcc8c89564936b535cb72582ca74b11d4 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Sat, 11 Feb 2023 13:56:19 +0100 Subject: [PATCH 04/19] Remove redundant code --- include/boost/wintls/detail/sspi_handshake.hpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/include/boost/wintls/detail/sspi_handshake.hpp b/include/boost/wintls/detail/sspi_handshake.hpp index 2b41b85d..f3709ed3 100644 --- a/include/boost/wintls/detail/sspi_handshake.hpp +++ b/include/boost/wintls/detail/sspi_handshake.hpp @@ -69,16 +69,11 @@ class sspi_handshake { BOOST_UNREACHABLE_RETURN(0); }(); - auto server_cert = context_.server_cert(); - if (handshake_type_ == handshake_type::server && server_cert != nullptr) { - creds.cCreds = 1; - creds.paCred = &server_cert; - } - // TODO: rename server_cert field since it is also used for client cert. // Note: if client cert is set, sspi will auto validate server cert with it. // Even though verify_server_certificate_ in context is set to false. - if (handshake_type_ == handshake_type::client && server_cert != nullptr) { + auto server_cert = context_.server_cert(); + if (server_cert != nullptr) { creds.cCreds = 1; creds.paCred = &server_cert; } From b437f7a7381878c398b6c3bd9e6a290cfc0712ad Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Sat, 11 Feb 2023 13:59:23 +0100 Subject: [PATCH 05/19] Make async_handshake::operator() not be a coroutine 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. --- .../boost/wintls/detail/async_handshake.hpp | 80 ++++++++----------- 1 file changed, 33 insertions(+), 47 deletions(-) diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index 8e35d94a..0396688b 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -12,23 +12,21 @@ #include -#include - namespace boost { namespace wintls { namespace detail { -template -struct async_handshake : boost::asio::coroutine { +template +struct async_handshake { async_handshake(NextLayer& next_layer, detail::sspi_handshake& handshake, handshake_type type) - : next_layer_(next_layer) - , handshake_(handshake) - , entry_count_(0) - , state_(state::idle) { + : next_layer_(next_layer) + , handshake_(handshake) + , entry_count_(0) + , state_(state::idle) { handshake_(type); } - template + template void operator()(Self& self, boost::system::error_code ec = {}, std::size_t length = 0) { if (ec) { self.complete(ec); @@ -40,7 +38,7 @@ struct async_handshake : boost::asio::coroutine { return entry_count_ > 1; }; - switch(state_) { + switch (state_) { case state::reading: handshake_.size_read(length); state_ = state::idle; @@ -53,45 +51,33 @@ struct async_handshake : boost::asio::coroutine { break; } - detail::sspi_handshake::state handshake_state; - BOOST_ASIO_CORO_REENTER(*this) { - while((handshake_state = handshake_()) != detail::sspi_handshake::state::done) { - if (handshake_state == detail::sspi_handshake::state::data_needed) { - BOOST_ASIO_CORO_YIELD { - state_ = state::reading; - next_layer_.async_read_some(handshake_.in_buffer(), std::move(self)); - } - continue; - } - - if (handshake_state == detail::sspi_handshake::state::data_available) { - BOOST_ASIO_CORO_YIELD { - state_ = state::writing; - net::async_write(next_layer_, handshake_.out_buffer(), std::move(self)); - } - continue; - } - - if (handshake_state == detail::sspi_handshake::state::error) { - if (!is_continuation()) { - BOOST_ASIO_CORO_YIELD { - auto e = self.get_executor(); - net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); - } - } - self.complete(handshake_.last_error()); - return; - } + switch ((handshake_())) { + case detail::sspi_handshake::state::data_needed: { + state_ = state::reading; + next_layer_.async_read_some(handshake_.in_buffer(), std::move(self)); + return; } - - if (!is_continuation()) { - BOOST_ASIO_CORO_YIELD { - auto e = self.get_executor(); - net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); - } + case detail::sspi_handshake::state::data_available: { + state_ = state::writing; + net::async_write(next_layer_, handshake_.out_buffer(), std::move(self)); + return; } - BOOST_ASSERT(!handshake_.last_error()); - handshake_.manual_auth(); + case detail::sspi_handshake::state::error: { + break; + } + case detail::sspi_handshake::state::done: { + BOOST_ASSERT(!handshake_.last_error()); + handshake_.manual_auth(); + break; + } + } + // If this is the first call to this function, it would cause the completion handler + // (invoked by self.complete()) to be executed on the wrong executor. + // Ensure that doesn't happen by posting the completion handler instead of calling it directly. + if (!is_continuation()) { + auto e = self.get_executor(); + net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); + } else { self.complete(handshake_.last_error()); } } From 997173968ee5c4b77029c4966a25c5d753c383bc Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Thu, 16 Feb 2023 15:54:05 +0100 Subject: [PATCH 06/19] Revert "Make async_handshake::operator() not be a coroutine" This reverts commit b437f7a7381878c398b6c3bd9e6a290cfc0712ad. --- .../boost/wintls/detail/async_handshake.hpp | 80 +++++++++++-------- 1 file changed, 47 insertions(+), 33 deletions(-) diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index 0396688b..8e35d94a 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -12,21 +12,23 @@ #include +#include + namespace boost { namespace wintls { namespace detail { -template -struct async_handshake { +template +struct async_handshake : boost::asio::coroutine { async_handshake(NextLayer& next_layer, detail::sspi_handshake& handshake, handshake_type type) - : next_layer_(next_layer) - , handshake_(handshake) - , entry_count_(0) - , state_(state::idle) { + : next_layer_(next_layer) + , handshake_(handshake) + , entry_count_(0) + , state_(state::idle) { handshake_(type); } - template + template void operator()(Self& self, boost::system::error_code ec = {}, std::size_t length = 0) { if (ec) { self.complete(ec); @@ -38,7 +40,7 @@ struct async_handshake { return entry_count_ > 1; }; - switch (state_) { + switch(state_) { case state::reading: handshake_.size_read(length); state_ = state::idle; @@ -51,33 +53,45 @@ struct async_handshake { break; } - switch ((handshake_())) { - case detail::sspi_handshake::state::data_needed: { - state_ = state::reading; - next_layer_.async_read_some(handshake_.in_buffer(), std::move(self)); - return; - } - case detail::sspi_handshake::state::data_available: { - state_ = state::writing; - net::async_write(next_layer_, handshake_.out_buffer(), std::move(self)); - return; - } - case detail::sspi_handshake::state::error: { - break; + detail::sspi_handshake::state handshake_state; + BOOST_ASIO_CORO_REENTER(*this) { + while((handshake_state = handshake_()) != detail::sspi_handshake::state::done) { + if (handshake_state == detail::sspi_handshake::state::data_needed) { + BOOST_ASIO_CORO_YIELD { + state_ = state::reading; + next_layer_.async_read_some(handshake_.in_buffer(), std::move(self)); + } + continue; + } + + if (handshake_state == detail::sspi_handshake::state::data_available) { + BOOST_ASIO_CORO_YIELD { + state_ = state::writing; + net::async_write(next_layer_, handshake_.out_buffer(), std::move(self)); + } + continue; + } + + if (handshake_state == detail::sspi_handshake::state::error) { + if (!is_continuation()) { + BOOST_ASIO_CORO_YIELD { + auto e = self.get_executor(); + net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); + } + } + self.complete(handshake_.last_error()); + return; + } } - case detail::sspi_handshake::state::done: { - BOOST_ASSERT(!handshake_.last_error()); - handshake_.manual_auth(); - break; + + if (!is_continuation()) { + BOOST_ASIO_CORO_YIELD { + auto e = self.get_executor(); + net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); + } } - } - // If this is the first call to this function, it would cause the completion handler - // (invoked by self.complete()) to be executed on the wrong executor. - // Ensure that doesn't happen by posting the completion handler instead of calling it directly. - if (!is_continuation()) { - auto e = self.get_executor(); - net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); - } else { + BOOST_ASSERT(!handshake_.last_error()); + handshake_.manual_auth(); self.complete(handshake_.last_error()); } } From 0d8b104cf47ba81f5a912d2074cd083075e071c7 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Thu, 16 Feb 2023 15:56:24 +0100 Subject: [PATCH 07/19] Make use of async_handshake being a coroutine --- .../boost/wintls/detail/async_handshake.hpp | 35 +++++-------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index 8e35d94a..9b77eacc 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -18,17 +18,16 @@ namespace boost { namespace wintls { namespace detail { -template +template struct async_handshake : boost::asio::coroutine { async_handshake(NextLayer& next_layer, detail::sspi_handshake& handshake, handshake_type type) - : next_layer_(next_layer) - , handshake_(handshake) - , entry_count_(0) - , state_(state::idle) { + : next_layer_(next_layer) + , handshake_(handshake) + , entry_count_(0) { handshake_(type); } - template + template void operator()(Self& self, boost::system::error_code ec = {}, std::size_t length = 0) { if (ec) { self.complete(ec); @@ -40,35 +39,22 @@ struct async_handshake : boost::asio::coroutine { return entry_count_ > 1; }; - switch(state_) { - case state::reading: - handshake_.size_read(length); - state_ = state::idle; - break; - case state::writing: - handshake_.size_written(length); - state_ = state::idle; - break; - case state::idle: - break; - } - detail::sspi_handshake::state handshake_state; BOOST_ASIO_CORO_REENTER(*this) { - while((handshake_state = handshake_()) != detail::sspi_handshake::state::done) { + while ((handshake_state = handshake_()) != detail::sspi_handshake::state::done) { if (handshake_state == detail::sspi_handshake::state::data_needed) { BOOST_ASIO_CORO_YIELD { - state_ = state::reading; next_layer_.async_read_some(handshake_.in_buffer(), std::move(self)); } + handshake_.size_read(length); continue; } if (handshake_state == detail::sspi_handshake::state::data_available) { BOOST_ASIO_CORO_YIELD { - state_ = state::writing; net::async_write(next_layer_, handshake_.out_buffer(), std::move(self)); } + handshake_.size_written(length); continue; } @@ -100,11 +86,6 @@ struct async_handshake : boost::asio::coroutine { NextLayer& next_layer_; detail::sspi_handshake& handshake_; int entry_count_; - enum class state { - idle, - reading, - writing - } state_; }; } // namespace detail From c6bb0b0a8d0dd03a901d18ad0c3fc4191591a9cd Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Thu, 16 Feb 2023 15:59:00 +0100 Subject: [PATCH 08/19] Simplify code and add comment Change the code such that posting self is only needed once. Also add a comment explaining why this is necessary. --- .../boost/wintls/detail/async_handshake.hpp | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index 9b77eacc..158fb268 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -39,10 +39,11 @@ struct async_handshake : boost::asio::coroutine { return entry_count_ > 1; }; - detail::sspi_handshake::state handshake_state; + sspi_handshake::state handshake_state; BOOST_ASIO_CORO_REENTER(*this) { - while ((handshake_state = handshake_()) != detail::sspi_handshake::state::done) { - if (handshake_state == detail::sspi_handshake::state::data_needed) { + while (true) { + handshake_state = handshake_(); + if (handshake_state == sspi_handshake::state::data_needed) { BOOST_ASIO_CORO_YIELD { next_layer_.async_read_some(handshake_.in_buffer(), std::move(self)); } @@ -50,7 +51,7 @@ struct async_handshake : boost::asio::coroutine { continue; } - if (handshake_state == detail::sspi_handshake::state::data_available) { + if (handshake_state == sspi_handshake::state::data_available) { BOOST_ASIO_CORO_YIELD { net::async_write(next_layer_, handshake_.out_buffer(), std::move(self)); } @@ -58,33 +59,33 @@ struct async_handshake : boost::asio::coroutine { continue; } - if (handshake_state == detail::sspi_handshake::state::error) { - if (!is_continuation()) { - BOOST_ASIO_CORO_YIELD { - auto e = self.get_executor(); - net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); - } - } - self.complete(handshake_.last_error()); - return; + if (handshake_state == sspi_handshake::state::error) { + break; + } + + if (handshake_state == sspi_handshake::state::done) { + BOOST_ASSERT(!handshake_.last_error()); + handshake_.manual_auth(); + break; } } + // If this is the first call to this function, it would cause the completion handler + // (invoked by self.complete()) to be executed on the wrong executor. + // Ensure that doesn't happen by posting the completion handler instead of calling it directly. if (!is_continuation()) { BOOST_ASIO_CORO_YIELD { auto e = self.get_executor(); net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); } } - BOOST_ASSERT(!handshake_.last_error()); - handshake_.manual_auth(); self.complete(handshake_.last_error()); } } private: NextLayer& next_layer_; - detail::sspi_handshake& handshake_; + sspi_handshake& handshake_; int entry_count_; }; From fd3aea566267136672f2f7193b41a606b32bfe09 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Wed, 1 Mar 2023 21:28:49 +0100 Subject: [PATCH 09/19] Improve handling of immediately failing async operations See comments in post_self.hpp. --- .../boost/wintls/detail/async_handshake.hpp | 7 +-- include/boost/wintls/detail/async_read.hpp | 4 +- .../boost/wintls/detail/async_shutdown.hpp | 4 +- include/boost/wintls/detail/post_self.hpp | 48 +++++++++++++++++++ include/boost/wintls/stream.hpp | 12 +++-- 5 files changed, 62 insertions(+), 13 deletions(-) create mode 100644 include/boost/wintls/detail/post_self.hpp diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index 158fb268..7a50e695 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -10,6 +10,7 @@ #include +#include #include #include @@ -70,13 +71,9 @@ struct async_handshake : boost::asio::coroutine { } } - // If this is the first call to this function, it would cause the completion handler - // (invoked by self.complete()) to be executed on the wrong executor. - // Ensure that doesn't happen by posting the completion handler instead of calling it directly. if (!is_continuation()) { BOOST_ASIO_CORO_YIELD { - auto e = self.get_executor(); - net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); + post_self(self, next_layer_, ec, length); } } self.complete(handshake_.last_error()); diff --git a/include/boost/wintls/detail/async_read.hpp b/include/boost/wintls/detail/async_read.hpp index fda02a35..a7500391 100644 --- a/include/boost/wintls/detail/async_read.hpp +++ b/include/boost/wintls/detail/async_read.hpp @@ -8,6 +8,7 @@ #ifndef BOOST_WINTLS_DETAIL_ASYNC_READ_HPP #define BOOST_WINTLS_DETAIL_ASYNC_READ_HPP +#include #include #include @@ -50,8 +51,7 @@ struct async_read : boost::asio::coroutine { if (state == detail::sspi_decrypt::state::error) { if (!is_continuation()) { BOOST_ASIO_CORO_YIELD { - auto e = self.get_executor(); - net::post(e, [self = std::move(self), ec, size_read]() mutable { self(ec, size_read); }); + post_self(self, next_layer_, ec, size_read); } } ec = decrypt_.last_error(); diff --git a/include/boost/wintls/detail/async_shutdown.hpp b/include/boost/wintls/detail/async_shutdown.hpp index 8d613349..b17ffa2e 100644 --- a/include/boost/wintls/detail/async_shutdown.hpp +++ b/include/boost/wintls/detail/async_shutdown.hpp @@ -8,6 +8,7 @@ #ifndef BOOST_WINTLS_DETAIL_ASYNC_SHUTDOWN_HPP #define BOOST_WINTLS_DETAIL_ASYNC_SHUTDOWN_HPP +#include #include #include @@ -49,8 +50,7 @@ struct async_shutdown : boost::asio::coroutine { } else { if (!is_continuation()) { BOOST_ASIO_CORO_YIELD { - auto e = self.get_executor(); - net::post(e, [self = std::move(self), ec, size_written]() mutable { self(ec, size_written); }); + post_self(self, next_layer_, ec, size_written); } } self.complete(ec); diff --git a/include/boost/wintls/detail/post_self.hpp b/include/boost/wintls/detail/post_self.hpp new file mode 100644 index 00000000..b37abd72 --- /dev/null +++ b/include/boost/wintls/detail/post_self.hpp @@ -0,0 +1,48 @@ +// +// Copyright (c) 2020 Kasper Laudrup (laudrup at stacktrace dot dk) +// +// Distributed under the Boost Software License, Version 1.0. (See accompanying +// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) +// + +#ifndef BOOST_WINTLS_DETAIL_POST_SELF_HPP +#define BOOST_WINTLS_DETAIL_POST_SELF_HPP + +#if BOOST_VERSION >= 108000 +#include +#endif +#include +#include +#include + +namespace boost { +namespace wintls { +namespace detail { + +// If a composed asynchronous operation completes immediately (due to an error) +// we do not want to call self.complete() directly as this may produce an infinite recursion in some cases. +// Instead, we post the intermediate completion handler (self) once. +// To achieve consistent behavior to non-erroneous cases, we post to the executor of the I/O object. +// Note that this only got accessible through self by get_io_executor since boost 1.81. +template +auto post_self(Self& self, IoObject& io_object, boost::system::error_code ec, std::size_t length) { +#if BOOST_VERSION >= 108100 + boost::ignore_unused(io_object); + auto ex = self.get_io_executor(); + return boost::asio::post(ex, boost::asio::append(std::move(self), ec, length)); +#elif BOOST_VERSION >= 108000 + return boost::asio::post(io_object.get_executor(), boost::asio::append(std::move(self), ec, length)); +#else + auto ex = io_object.get_executor(); + // If the completion token associated with self had an associated executor, + // allocator or cancellation slot, we loose these here. + // Therefore, above solutions are better! + return boost::asio::post(ex, [self = std::move(self), ec, length]() mutable { self(ec, length); }); +#endif +} + +} // namespace detail +} // namespace wintls +} // namespace boost + +#endif //BOOST_WINTLS_DETAIL_POST_SELF_HPP \ No newline at end of file diff --git a/include/boost/wintls/stream.hpp b/include/boost/wintls/stream.hpp index bcedc048..35a19636 100644 --- a/include/boost/wintls/stream.hpp +++ b/include/boost/wintls/stream.hpp @@ -212,7 +212,7 @@ class stream { template auto async_handshake(handshake_type type, CompletionToken&& handler) { return boost::asio::async_compose( - detail::async_handshake{next_layer_, sspi_stream_->handshake, type}, handler); + detail::async_handshake{next_layer_, sspi_stream_->handshake, type}, handler, *this); } /** Read some data from the stream. @@ -306,7 +306,9 @@ class stream { template auto async_read_some(const MutableBufferSequence& buffers, CompletionToken&& handler) { return boost::asio::async_compose( - detail::async_read{next_layer_, buffers, sspi_stream_->decrypt}, handler); + detail::async_read{next_layer_, buffers, sspi_stream_->decrypt}, + handler, + *this); } /** Write some data to the stream. @@ -396,7 +398,9 @@ class stream { template auto async_write_some(const ConstBufferSequence& buffers, CompletionToken&& handler) { return boost::asio::async_compose( - detail::async_write{next_layer_, buffers, sspi_stream_->encrypt}, handler); + detail::async_write{next_layer_, buffers, sspi_stream_->encrypt}, + handler, + *this); } /** Shut down TLS on the stream. @@ -451,7 +455,7 @@ class stream { template auto async_shutdown(CompletionToken&& handler) { return boost::asio::async_compose( - detail::async_shutdown{next_layer_, sspi_stream_->shutdown}, handler); + detail::async_shutdown{next_layer_, sspi_stream_->shutdown}, handler, *this); } private: From 7243bad0f447b0207e4b00f898cd84bf28550c09 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Thu, 2 Mar 2023 09:52:26 +0100 Subject: [PATCH 10/19] Add Boost 1.80 and Boost 1.81 to CI Unittests --- .github/workflows/unittest.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/unittest.yml b/.github/workflows/unittest.yml index 382c3105..32c3d9dd 100644 --- a/.github/workflows/unittest.yml +++ b/.github/workflows/unittest.yml @@ -12,7 +12,7 @@ jobs: strategy: fail-fast: false matrix: - boost_version: ["1.75.0", "1.76.0", "1.77.0", "1.78.0", "1.79.0"] + boost_version: ["1.75.0", "1.76.0", "1.77.0", "1.78.0", "1.79.0", "1.80.0", "1.81.0"] os: [windows-2019, windows-2022] toolset: [v141, v142, v143, ClangCL] build_type: [Debug, Release] @@ -32,6 +32,8 @@ jobs: - { boost_version: "1.77.0", toolset: v143 } - { boost_version: "1.78.0", toolset: v141 } - { boost_version: "1.79.0", toolset: v141 } + - { boost_version: "1.80.0", toolset: v141 } + - { boost_version: "1.81.0", toolset: v141 } include: - boost_version: "1.79.0" os: windows-2022 From 12aec4a72d87bda14874446a23f221d5a150db6e Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Wed, 8 Mar 2023 20:04:19 +0100 Subject: [PATCH 11/19] Set toolset for new boost versions and install-boost --- .github/workflows/unittest.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/unittest.yml b/.github/workflows/unittest.yml index 32c3d9dd..11cec1a9 100644 --- a/.github/workflows/unittest.yml +++ b/.github/workflows/unittest.yml @@ -64,8 +64,9 @@ jobs: with: fetch-depth: 0 + # For Boost Versions >= 1.78, the toolset parameter has to be specified to install-boost. - name: Add boost toolset to environment - if: contains(fromJson('["1.78.0", "1.79.0"]'), matrix.boost_version) + if: contains(fromJson('["1.78.0", "1.79.0", "1.80.0", "1.81.0"]'), matrix.boost_version) run: echo BOOST_TOOLSET=$([[ "${{matrix.generator}}" == "MinGW Makefiles" ]] && echo "mingw" || echo "msvc") >> $GITHUB_ENV # The platform_version passed to boost-install determines the msvc toolset version for which static libs are installed. From 269342b97ab4ac404ffcdd4e1a25c0ef27854c71 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Wed, 8 Mar 2023 20:29:51 +0100 Subject: [PATCH 12/19] Increase waiting time for OCSP responder in test 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. --- test/certificate_test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/certificate_test.cpp b/test/certificate_test.cpp index 7370b9c0..24419506 100644 --- a/test/certificate_test.cpp +++ b/test/certificate_test.cpp @@ -245,13 +245,13 @@ TEST_CASE("check certificate revocation (integration test)", "[.integration]") { // 'running()' does not mean that it is responding yet, but that we did start the process correctly CHECK(ocsp_responder.running()); // It takes a few seconds for the OCSP responder to become available. - // Therefore, we try to verify the certificate in a loop, but for at most 30 seconds. + // Therefore, we try to verify the certificate in a loop, but for at most 60 seconds. auto res_with_responder = CRYPT_E_REVOCATION_OFFLINE; const auto start = std::chrono::system_clock::now(); while (res_with_responder == CRYPT_E_REVOCATION_OFFLINE) { res_with_responder = ctx_certs.verify_certificate(cert_ocsp.get(), "", true); - if (std::chrono::system_clock::now() - start > std::chrono::seconds(30)) { - WARN("OCSP responder did not provide a response within 30 seconds."); + if (std::chrono::system_clock::now() - start > std::chrono::seconds(60)) { + WARN("OCSP responder did not provide a response within 60 seconds."); break; } } From 52d49ee3dabec63fadc37830d6496666b262d267 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Mon, 27 Mar 2023 17:53:58 +0200 Subject: [PATCH 13/19] Extract synchronous handshake loop into function The handshake loop shall be used for shutdown as well. --- .../boost/wintls/detail/sspi_handshake.hpp | 35 ++++++++++++++++++- include/boost/wintls/stream.hpp | 33 ++--------------- 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/include/boost/wintls/detail/sspi_handshake.hpp b/include/boost/wintls/detail/sspi_handshake.hpp index f3709ed3..01c7c3e4 100644 --- a/include/boost/wintls/detail/sspi_handshake.hpp +++ b/include/boost/wintls/detail/sspi_handshake.hpp @@ -260,7 +260,7 @@ class sspi_handshake { check_revocation_ = check; } - SECURITY_STATUS manual_auth(){ + SECURITY_STATUS manual_auth() { if (!context_.verify_server_certificate_) { return SEC_E_OK; } @@ -289,6 +289,39 @@ class sspi_handshake { bool check_revocation_ = false; }; +template +void handshake(NextLayer& next_layer, sspi_handshake& handshake, handshake_type type, boost::system::error_code& ec) { + handshake(type); + while (true) { + switch (handshake()) { + case detail::sspi_handshake::state::data_needed: { + std::size_t size_read = next_layer.read_some(handshake.in_buffer(), ec); + if (ec) { + return; + } + handshake.size_read(size_read); + continue; + } + case detail::sspi_handshake::state::data_available: { + std::size_t size_written = net::write(next_layer, handshake.out_buffer(), ec); + if (ec) { + return; + } + handshake.size_written(size_written); + continue; + } + case detail::sspi_handshake::state::error: + ec = handshake.last_error(); + return; + case detail::sspi_handshake::state::done: + if (handshake.manual_auth() != SEC_E_OK) { + ec = handshake.last_error(); + } + return; + } + } +} + } // namespace detail } // namespace wintls } // namespace boost diff --git a/include/boost/wintls/stream.hpp b/include/boost/wintls/stream.hpp index 35a19636..8b576d6e 100644 --- a/include/boost/wintls/stream.hpp +++ b/include/boost/wintls/stream.hpp @@ -135,36 +135,7 @@ class stream { * @param ec Set to indicate what error occurred, if any. */ void handshake(handshake_type type, boost::system::error_code& ec) { - sspi_stream_->handshake(type); - - while (true) { - switch (sspi_stream_->handshake()) { - case detail::sspi_handshake::state::data_needed: { - std::size_t size_read = next_layer_.read_some(sspi_stream_->handshake.in_buffer(), ec); - if (ec) { - return; - } - sspi_stream_->handshake.size_read(size_read); - continue; - } - case detail::sspi_handshake::state::data_available: { - std::size_t size_written = net::write(next_layer_, sspi_stream_->handshake.out_buffer(), ec); - if (ec) { - return; - } - sspi_stream_->handshake.size_written(size_written); - continue; - } - case detail::sspi_handshake::state::error: - ec = sspi_stream_->handshake.last_error(); - return; - case detail::sspi_handshake::state::done: - if (sspi_stream_->handshake.manual_auth() != SEC_E_OK) { - ec = sspi_stream_->handshake.last_error(); - } - return; - } - } + detail::handshake(next_layer_, sspi_stream_->handshake, type, ec); } /** Perform TLS handshaking. @@ -209,7 +180,7 @@ class stream { * this function. Invocation of the handler will be performed in a * manner equivalent to using `net::post`. */ - template + template auto async_handshake(handshake_type type, CompletionToken&& handler) { return boost::asio::async_compose( detail::async_handshake{next_layer_, sspi_stream_->handshake, type}, handler, *this); From 245a51a49cfb19dc6742d8ccb301236a2cbf3f2e Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Mon, 27 Mar 2023 18:24:23 +0200 Subject: [PATCH 14/19] Move shutdown code into sspi_handshake --- .../boost/wintls/detail/async_handshake.hpp | 17 +++- .../boost/wintls/detail/async_shutdown.hpp | 72 ----------------- .../boost/wintls/detail/sspi_handshake.hpp | 61 ++++++++++++-- include/boost/wintls/detail/sspi_shutdown.hpp | 79 ------------------- include/boost/wintls/detail/sspi_stream.hpp | 9 +-- include/boost/wintls/stream.hpp | 26 +++--- 6 files changed, 83 insertions(+), 181 deletions(-) delete mode 100644 include/boost/wintls/detail/async_shutdown.hpp delete mode 100644 include/boost/wintls/detail/sspi_shutdown.hpp diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index 7a50e695..cafe96b7 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -21,11 +21,19 @@ namespace detail { template struct async_handshake : boost::asio::coroutine { - async_handshake(NextLayer& next_layer, detail::sspi_handshake& handshake, handshake_type type) + async_handshake(NextLayer& next_layer, detail::sspi_handshake& handshake, handshake_mode mode) : next_layer_(next_layer) , handshake_(handshake) + , mode_(mode) , entry_count_(0) { - handshake_(type); + switch (mode) { + case handshake_mode::init: + handshake_.init(); + break; + case handshake_mode::shutdown: + handshake_.shutdown(); + break; + } } template @@ -66,7 +74,9 @@ struct async_handshake : boost::asio::coroutine { if (handshake_state == sspi_handshake::state::done) { BOOST_ASSERT(!handshake_.last_error()); - handshake_.manual_auth(); + if (mode_ == handshake_mode::init) { + handshake_.manual_auth(); + } break; } } @@ -83,6 +93,7 @@ struct async_handshake : boost::asio::coroutine { private: NextLayer& next_layer_; sspi_handshake& handshake_; + handshake_mode mode_; int entry_count_; }; diff --git a/include/boost/wintls/detail/async_shutdown.hpp b/include/boost/wintls/detail/async_shutdown.hpp deleted file mode 100644 index b17ffa2e..00000000 --- a/include/boost/wintls/detail/async_shutdown.hpp +++ /dev/null @@ -1,72 +0,0 @@ -// -// Copyright (c) 2020 Kasper Laudrup (laudrup at stacktrace dot dk) -// -// Distributed under the Boost Software License, Version 1.0. (See accompanying -// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) -// - -#ifndef BOOST_WINTLS_DETAIL_ASYNC_SHUTDOWN_HPP -#define BOOST_WINTLS_DETAIL_ASYNC_SHUTDOWN_HPP - -#include -#include - -#include - -namespace boost { -namespace wintls { -namespace detail { - -template -struct async_shutdown : boost::asio::coroutine { - async_shutdown(NextLayer& next_layer, detail::sspi_shutdown& shutdown) - : next_layer_(next_layer) - , shutdown_(shutdown) - , entry_count_(0) { - } - - template - void operator()(Self& self, boost::system::error_code ec = {}, std::size_t size_written = 0) { - if (ec) { - self.complete(ec); - return; - } - - ++entry_count_; - auto is_continuation = [this] { - return entry_count_ > 1; - }; - - ec = shutdown_(); - - BOOST_ASIO_CORO_REENTER(*this) { - if (!ec) { - BOOST_ASIO_CORO_YIELD { - net::async_write(next_layer_, shutdown_.buffer(), std::move(self)); - } - shutdown_.size_written(size_written); - self.complete({}); - return; - } else { - if (!is_continuation()) { - BOOST_ASIO_CORO_YIELD { - post_self(self, next_layer_, ec, size_written); - } - } - self.complete(ec); - return; - } - } - } - -private: - NextLayer& next_layer_; - detail::sspi_shutdown& shutdown_; - int entry_count_; -}; - -} // namespace detail -} // namespace wintls -} // namespace boost - -#endif // BOOST_WINTLS_DETAIL_ASYNC_SHUTDOWN_HPP diff --git a/include/boost/wintls/detail/sspi_handshake.hpp b/include/boost/wintls/detail/sspi_handshake.hpp index 01c7c3e4..10a1c6ad 100644 --- a/include/boost/wintls/detail/sspi_handshake.hpp +++ b/include/boost/wintls/detail/sspi_handshake.hpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -26,6 +27,11 @@ namespace boost { namespace wintls { namespace detail { +enum class handshake_mode { + init, // initial handshake to establish the protocol + shutdown // shutdown the protocol +}; + class sspi_handshake { public: enum class state { @@ -44,9 +50,13 @@ class sspi_handshake { input_buffers_[0].pvBuffer = reinterpret_cast(input_data_.data()); } - void operator()(handshake_type type) { + // Specify whether the handshake is performed as a client or as a server. + // This is used for the initial handshake and for the shutdown. + void set_type(handshake_type type) { handshake_type_ = type; + } + void init() { SCHANNEL_CRED creds{}; creds.dwVersion = SCHANNEL_CRED_VERSION; creds.grbitEnabledProtocols = static_cast(context_.method_); @@ -119,16 +129,41 @@ class sspi_handshake { } } - state operator()() { - if (last_error_ == SEC_E_OK) { - return state::done; + void shutdown() { + shutdown_buffers buffers; + last_error_ = detail::sspi_functions::ApplyControlToken(ctxt_handle_.get(), buffers.desc()); + if (last_error_ != SEC_E_OK) { + return; } - if (last_error_ != SEC_I_CONTINUE_NEEDED && last_error_ != SEC_E_INCOMPLETE_MESSAGE) { + DWORD out_flags = 0; + last_error_ = detail::sspi_functions::InitializeSecurityContext(cred_handle_.get(), + ctxt_handle_.get(), + nullptr, + client_context_flags, + 0, + SECURITY_NATIVE_DREP, + nullptr, + 0, + ctxt_handle_.get(), + buffers.desc(), + &out_flags, + nullptr); + if (last_error_ != SEC_E_OK) { + return; + } + out_buffer_ = sspi_context_buffer{buffers[0].pvBuffer, buffers[0].cbBuffer}; + } + + state operator()() { + if (last_error_ != SEC_I_CONTINUE_NEEDED && last_error_ != SEC_E_INCOMPLETE_MESSAGE && last_error_ != SEC_E_OK) { return state::error; } if (!out_buffer_.empty()) { return state::data_available; } + if (last_error_ == SEC_E_OK) { + return state::done; + } if (input_buffers_[0].cbBuffer == 0) { return state::data_needed; } @@ -290,8 +325,18 @@ class sspi_handshake { }; template -void handshake(NextLayer& next_layer, sspi_handshake& handshake, handshake_type type, boost::system::error_code& ec) { - handshake(type); +void handshake(NextLayer& next_layer, + sspi_handshake& handshake, + handshake_mode mode, + boost::system::error_code& ec) { + switch (mode) { + case handshake_mode::init: + handshake.init(); + break; + case handshake_mode::shutdown: + handshake.shutdown(); + break; + } while (true) { switch (handshake()) { case detail::sspi_handshake::state::data_needed: { @@ -314,7 +359,7 @@ void handshake(NextLayer& next_layer, sspi_handshake& handshake, handshake_type ec = handshake.last_error(); return; case detail::sspi_handshake::state::done: - if (handshake.manual_auth() != SEC_E_OK) { + if (mode == handshake_mode::init && handshake.manual_auth() != SEC_E_OK) { ec = handshake.last_error(); } return; diff --git a/include/boost/wintls/detail/sspi_shutdown.hpp b/include/boost/wintls/detail/sspi_shutdown.hpp deleted file mode 100644 index 8983493e..00000000 --- a/include/boost/wintls/detail/sspi_shutdown.hpp +++ /dev/null @@ -1,79 +0,0 @@ -// -// Copyright (c) 2021 Kasper Laudrup (laudrup at stacktrace dot dk) -// -// Distributed under the Boost Software License, Version 1.0. (See accompanying -// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) -// - -#ifndef BOOST_WINTLS_DETAIL_SSPI_SHUTDOWN_HPP -#define BOOST_WINTLS_DETAIL_SSPI_SHUTDOWN_HPP - -#include -#include -#include -#include -#include -#include - -#include - -namespace boost { -namespace wintls { -namespace detail { - -class sspi_shutdown { -public: - sspi_shutdown(ctxt_handle& ctxt_handle, cred_handle& cred_handle) - : ctxt_handle_(ctxt_handle) - , cred_handle_(cred_handle) { - } - - boost::system::error_code operator()() { - shutdown_buffers buffers; - - SECURITY_STATUS sc = detail::sspi_functions::ApplyControlToken(ctxt_handle_.get(), buffers.desc()); - if (sc != SEC_E_OK) { - return error::make_error_code(sc); - } - - DWORD out_flags = 0; - sc = detail::sspi_functions::InitializeSecurityContext(cred_handle_.get(), - ctxt_handle_.get(), - nullptr, - client_context_flags, - 0, - SECURITY_NATIVE_DREP, - nullptr, - 0, - ctxt_handle_.get(), - buffers.desc(), - &out_flags, - nullptr); - if (sc != SEC_E_OK) { - return error::make_error_code(sc); - } - - buffer_ = sspi_context_buffer{buffers[0].pvBuffer, buffers[0].cbBuffer}; - return {}; - } - - net::const_buffer buffer() { - return buffer_.asio_buffer(); - } - - void size_written(std::size_t size) { - BOOST_VERIFY(size == buffer_.size()); - buffer_ = sspi_context_buffer{}; - } - -private: - ctxt_handle& ctxt_handle_; - cred_handle& cred_handle_; - sspi_context_buffer buffer_; -}; - -} // namespace detail -} // namespace wintls -} // namespace boost - -#endif // BOOST_WINTLS_DETAIL_SSPI_SHUTDOWN_HPP diff --git a/include/boost/wintls/detail/sspi_stream.hpp b/include/boost/wintls/detail/sspi_stream.hpp index 22c211ee..dc5f73a9 100644 --- a/include/boost/wintls/detail/sspi_stream.hpp +++ b/include/boost/wintls/detail/sspi_stream.hpp @@ -11,7 +11,6 @@ #include #include #include -#include #include namespace boost { @@ -21,10 +20,9 @@ namespace detail { class sspi_stream { public: sspi_stream(context& ctx) - : handshake(ctx, ctxt_handle_, cred_handle_) - , encrypt(ctxt_handle_) - , decrypt(ctxt_handle_) - , shutdown(ctxt_handle_, cred_handle_) { + : handshake(ctx, ctxt_handle_, cred_handle_) + , encrypt(ctxt_handle_) + , decrypt(ctxt_handle_) { } sspi_stream(sspi_stream&&) = delete; @@ -38,7 +36,6 @@ class sspi_stream { sspi_handshake handshake; sspi_encrypt encrypt; sspi_decrypt decrypt; - sspi_shutdown shutdown; }; } // namespace detail diff --git a/include/boost/wintls/stream.hpp b/include/boost/wintls/stream.hpp index 8b576d6e..311c24da 100644 --- a/include/boost/wintls/stream.hpp +++ b/include/boost/wintls/stream.hpp @@ -13,7 +13,6 @@ #include #include -#include #include #include @@ -135,7 +134,8 @@ class stream { * @param ec Set to indicate what error occurred, if any. */ void handshake(handshake_type type, boost::system::error_code& ec) { - detail::handshake(next_layer_, sspi_stream_->handshake, type, ec); + sspi_stream_->handshake.set_type(type); + detail::handshake(next_layer_, sspi_stream_->handshake, detail::handshake_mode::init, ec); } /** Perform TLS handshaking. @@ -182,8 +182,11 @@ class stream { */ template auto async_handshake(handshake_type type, CompletionToken&& handler) { + sspi_stream_->handshake.set_type(type); return boost::asio::async_compose( - detail::async_handshake{next_layer_, sspi_stream_->handshake, type}, handler, *this); + detail::async_handshake{next_layer_, sspi_stream_->handshake, detail::handshake_mode::init}, + handler, + *this); } /** Read some data from the stream. @@ -383,14 +386,7 @@ class stream { * @param ec Set to indicate what error occurred, if any. */ void shutdown(boost::system::error_code& ec) { - ec = sspi_stream_->shutdown(); - if (ec) { - return; - } - std::size_t size_written = net::write(next_layer_, sspi_stream_->shutdown.buffer(), ec); - if (!ec) { - sspi_stream_->shutdown.size_written(size_written); - } + detail::handshake(next_layer_, sspi_stream_->handshake, detail::handshake_mode::shutdown, ec); } /** Shut down TLS on the stream. @@ -423,10 +419,14 @@ class stream { *); * @endcode */ - template + template auto async_shutdown(CompletionToken&& handler) { return boost::asio::async_compose( - detail::async_shutdown{next_layer_, sspi_stream_->shutdown}, handler, *this); + detail::async_handshake{next_layer_, + sspi_stream_->handshake, + detail::handshake_mode::shutdown}, + handler, + *this); } private: From b3f392a1e6ffa39d92a5cfe425c93f6280ad4078 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Mon, 27 Mar 2023 18:29:15 +0200 Subject: [PATCH 15/19] Distinguish between client and server in shutdown 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 --- .../boost/wintls/detail/sspi_handshake.hpp | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/include/boost/wintls/detail/sspi_handshake.hpp b/include/boost/wintls/detail/sspi_handshake.hpp index 10a1c6ad..ddebb8d3 100644 --- a/include/boost/wintls/detail/sspi_handshake.hpp +++ b/include/boost/wintls/detail/sspi_handshake.hpp @@ -136,18 +136,34 @@ class sspi_handshake { return; } DWORD out_flags = 0; - last_error_ = detail::sspi_functions::InitializeSecurityContext(cred_handle_.get(), + switch (handshake_type_) { + case handshake_type::client: + last_error_ = detail::sspi_functions::InitializeSecurityContext(cred_handle_.get(), + ctxt_handle_.get(), + nullptr, + client_context_flags, + 0, + SECURITY_NATIVE_DREP, + nullptr, + 0, + ctxt_handle_.get(), + buffers.desc(), + &out_flags, + nullptr); + break; + case handshake_type::server: { + TimeStamp expiry; + last_error_ = detail::sspi_functions::AcceptSecurityContext(cred_handle_.get(), ctxt_handle_.get(), nullptr, - client_context_flags, - 0, + server_context_flags, SECURITY_NATIVE_DREP, - nullptr, - 0, ctxt_handle_.get(), buffers.desc(), &out_flags, - nullptr); + &expiry); + } + } if (last_error_ != SEC_E_OK) { return; } From 0d765c4962c1e9668f9d0ee5d353bb0a0c220c7d Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Tue, 4 Apr 2023 21:08:28 +0200 Subject: [PATCH 16/19] Add shutdown test 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. --- test/CMakeLists.txt | 1 + test/echo_client.hpp | 8 +++ test/shutdown_test.cpp | 119 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+) create mode 100644 test/shutdown_test.cpp diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 81bbc241..d74db1ff 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -33,6 +33,7 @@ set(test_sources sspi_buffer_sequence_test.cpp stream_test.cpp decrypted_data_buffer_test.cpp + shutdown_test.cpp ) add_executable(unittest diff --git a/test/echo_client.hpp b/test/echo_client.hpp index fc31a2d6..aee8f77f 100644 --- a/test/echo_client.hpp +++ b/test/echo_client.hpp @@ -27,6 +27,14 @@ class echo_client : public Stream { stream.shutdown(); } + auto launch_shutdown() { + return std::async(std::launch::async, [&]() { + boost::system::error_code ec; + stream.shutdown(ec); + return ec; + }); + } + void read() { net::read_until(stream, buffer_, '\0'); } diff --git a/test/shutdown_test.cpp b/test/shutdown_test.cpp new file mode 100644 index 00000000..d5ca6fe1 --- /dev/null +++ b/test/shutdown_test.cpp @@ -0,0 +1,119 @@ +// +// Copyright (c) 2020 Kasper Laudrup (laudrup at stacktrace dot dk) +// +// Distributed under the Boost Software License, Version 1.0. (See accompanying +// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) +// + +#include "echo_server.hpp" +#include "echo_client.hpp" +#include "async_echo_server.hpp" +#include "async_echo_client.hpp" +#include "asio_ssl_client_stream.hpp" +#include "asio_ssl_server_stream.hpp" +#include "wintls_client_stream.hpp" +#include "wintls_server_stream.hpp" +#include "unittest.hpp" + +#include + +template +auto stream_truncated_error(const asio_ssl::stream&) { + return asio_ssl::error::stream_truncated; +} + +template +auto stream_truncated_error(const boost::wintls::stream&) { + return boost::system::errc::success; // #TODO: This should be an error +} + +using TestTypes = std::tuple, + std::tuple, + std::tuple, + std::tuple>; + +TEMPLATE_LIST_TEST_CASE("shutdown test", "", TestTypes) { + using ClientStream = typename std::tuple_element<0, TestType>::type; + using ServerStream = typename std::tuple_element<1, TestType>::type; + + SECTION("stream truncated - client closes after server shutdown") { + net::io_context io_context; + echo_client client(io_context); + echo_server server(io_context); + + client.stream.next_layer().connect(server.stream.next_layer()); + + auto handshake_result = server.handshake(); + client.handshake(); + REQUIRE_FALSE(handshake_result.get()); + + auto shutdown_result = server.shutdown(); + client.stream.next_layer().close(); + CHECK(shutdown_result.get() == stream_truncated_error(server.stream)); + } + + SECTION("stream truncated - client closes before server shutdown") { + net::io_context io_context; + echo_client client(io_context); + echo_server server(io_context); + + client.stream.next_layer().connect(server.stream.next_layer()); + + auto handshake_result = server.handshake(); + client.handshake(); + REQUIRE_FALSE(handshake_result.get()); + + client.stream.next_layer().close(); + auto shutdown_result = server.shutdown(); + CHECK(shutdown_result.get() == stream_truncated_error(server.stream)); + } + + SECTION("stream truncated - server closes after client shutdown") { + net::io_context io_context; + echo_client client(io_context); + echo_server server(io_context); + + client.stream.next_layer().connect(server.stream.next_layer()); + + auto handshake_result = server.handshake(); + client.handshake(); + REQUIRE_FALSE(handshake_result.get()); + + auto client_shutdown_result = client.launch_shutdown(); + server.stream.next_layer().close(); + CHECK(client_shutdown_result.get() == stream_truncated_error(client.stream)); + } + + SECTION("stream truncated - server closes before client shutdown") { + net::io_context io_context; + echo_client client(io_context); + echo_server server(io_context); + + client.stream.next_layer().connect(server.stream.next_layer()); + + auto handshake_result = server.handshake(); + client.handshake(); + REQUIRE_FALSE(handshake_result.get()); + + server.stream.next_layer().close(); + CHECK(client.launch_shutdown().get() == stream_truncated_error(client.stream)); + } + + SECTION("okay") { + net::io_context io_context; + echo_client client(io_context); + echo_server server(io_context); + + client.stream.next_layer().connect(server.stream.next_layer()); + + auto handshake_result = server.handshake(); + client.handshake(); + REQUIRE_FALSE(handshake_result.get()); + + auto shutdown_result = server.shutdown(); + client.shutdown(); + // according to https://stackoverflow.com/a/25703699/16814536 this should produce asio::error::eof + // is that information outdated? + CHECK_FALSE(shutdown_result.get()); + } +} From 26fc986d8ea64b29d9f8f34c0bc4cec08cc53032 Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Tue, 4 Apr 2023 21:08:28 +0200 Subject: [PATCH 17/19] Implement full shutdown 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. --- include/boost/wintls/detail/sspi_handshake.hpp | 16 +++++++++++++++- test/shutdown_test.cpp | 2 +- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/include/boost/wintls/detail/sspi_handshake.hpp b/include/boost/wintls/detail/sspi_handshake.hpp index ddebb8d3..92fb96bf 100644 --- a/include/boost/wintls/detail/sspi_handshake.hpp +++ b/include/boost/wintls/detail/sspi_handshake.hpp @@ -168,6 +168,12 @@ class sspi_handshake { return; } out_buffer_ = sspi_context_buffer{buffers[0].pvBuffer, buffers[0].cbBuffer}; + // Setting SEC_I_CONTINUE_NEEDED here means that we will wait for the peer to send its close notify. + // Not doing so may open up the possibility of a truncation attack. + // If the underlying protocol is self-terminated (as is http) it would be okay not to wait for the close notify. + // We could implement such a shutdown as a "fast/unsafe_shutdown" and not change last_error_ here for that case. + // Cmp. https://security.stackexchange.com/questions/82028/ssl-tls-is-a-server-always-required-to-respond-to-a-close-notify + last_error_ = SEC_I_CONTINUE_NEEDED; } state operator()() { @@ -245,7 +251,7 @@ class sspi_handshake { } bool has_buffer_output = out_buffers[0].cbBuffer != 0 && out_buffers[0].pvBuffer != nullptr; - if(has_buffer_output){ + if (has_buffer_output) { out_buffer_ = sspi_context_buffer{out_buffers[0].pvBuffer, out_buffers[0].cbBuffer}; } @@ -270,6 +276,14 @@ class sspi_handshake { return has_buffer_output ? state::data_available : state::done; } + // If we get here during shutdown, this means we initiated the shutdown and now received the + // close notify from the peer. That means that we are done. + // #TODO: could this happen during the initial handshake where it should be an error? + case SEC_I_CONTEXT_EXPIRED: { + last_error_ = SEC_E_OK; // #TODO: better solve this in the handshake loop? + return state::done; + } + case SEC_I_INCOMPLETE_CREDENTIALS: BOOST_ASSERT_MSG(false, "client authentication not implemented"); diff --git a/test/shutdown_test.cpp b/test/shutdown_test.cpp index d5ca6fe1..1290623b 100644 --- a/test/shutdown_test.cpp +++ b/test/shutdown_test.cpp @@ -24,7 +24,7 @@ auto stream_truncated_error(const asio_ssl::stream&) { template auto stream_truncated_error(const boost::wintls::stream&) { - return boost::system::errc::success; // #TODO: This should be an error + return net::error::eof; // #TODO: This should be a separate error code. } using TestTypes = std::tuple, From 3b79f6d9830c539363614344a6ad35bbce4b96ae Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Tue, 4 Apr 2023 21:08:28 +0200 Subject: [PATCH 18/19] Add Error Code stream_truncated_error The error code is designed to be analogously to the one in asio.ssl. --- .../boost/wintls/detail/async_handshake.hpp | 8 +++- .../boost/wintls/detail/sspi_handshake.hpp | 48 ++++++++++--------- include/boost/wintls/error.hpp | 41 ++++++++++++++++ test/shutdown_test.cpp | 2 +- 4 files changed, 74 insertions(+), 25 deletions(-) diff --git a/include/boost/wintls/detail/async_handshake.hpp b/include/boost/wintls/detail/async_handshake.hpp index cafe96b7..8acb2a39 100644 --- a/include/boost/wintls/detail/async_handshake.hpp +++ b/include/boost/wintls/detail/async_handshake.hpp @@ -69,24 +69,28 @@ struct async_handshake : boost::asio::coroutine { } if (handshake_state == sspi_handshake::state::error) { + ec = handshake_.last_error(); break; } if (handshake_state == sspi_handshake::state::done) { BOOST_ASSERT(!handshake_.last_error()); if (mode_ == handshake_mode::init) { - handshake_.manual_auth(); + ec = error::make_error_code(handshake_.manual_auth()); } break; } } + if (ec == net::error::eof) { + ec = wintls::error::stream_truncated; + } if (!is_continuation()) { BOOST_ASIO_CORO_YIELD { post_self(self, next_layer_, ec, length); } } - self.complete(handshake_.last_error()); + self.complete(ec); } } diff --git a/include/boost/wintls/detail/sspi_handshake.hpp b/include/boost/wintls/detail/sspi_handshake.hpp index 92fb96bf..87ca90ee 100644 --- a/include/boost/wintls/detail/sspi_handshake.hpp +++ b/include/boost/wintls/detail/sspi_handshake.hpp @@ -368,33 +368,37 @@ void handshake(NextLayer& next_layer, break; } while (true) { - switch (handshake()) { - case detail::sspi_handshake::state::data_needed: { - std::size_t size_read = next_layer.read_some(handshake.in_buffer(), ec); - if (ec) { - return; - } - handshake.size_read(size_read); - continue; + auto handshake_state = handshake(); + if (handshake_state == sspi_handshake::state::data_needed) { + std::size_t size_read = next_layer.read_some(handshake.in_buffer(), ec); + if (ec) { + break; } - case detail::sspi_handshake::state::data_available: { - std::size_t size_written = net::write(next_layer, handshake.out_buffer(), ec); - if (ec) { - return; - } - handshake.size_written(size_written); - continue; + handshake.size_read(size_read); + continue; + } + if (handshake_state == sspi_handshake::state::data_available) { + std::size_t size_written = net::write(next_layer, handshake.out_buffer(), ec); + if (ec) { + break; } - case detail::sspi_handshake::state::error: + handshake.size_written(size_written); + continue; + } + if (handshake_state == sspi_handshake::state::error) { + ec = handshake.last_error(); + break; + } + if (handshake_state == sspi_handshake::state::done) { + if (mode == handshake_mode::init && handshake.manual_auth() != SEC_E_OK) { ec = handshake.last_error(); - return; - case detail::sspi_handshake::state::done: - if (mode == handshake_mode::init && handshake.manual_auth() != SEC_E_OK) { - ec = handshake.last_error(); - } - return; + } + break; } } + if (ec == net::error::eof) { + ec = wintls::error::stream_truncated; + } } } // namespace detail diff --git a/include/boost/wintls/error.hpp b/include/boost/wintls/error.hpp index 11df0148..fde7a713 100644 --- a/include/boost/wintls/error.hpp +++ b/include/boost/wintls/error.hpp @@ -15,6 +15,36 @@ namespace boost { namespace wintls { namespace error { +enum stream_errors { + /// The underlying stream closed before the ssl stream gracefully shut down. + stream_truncated = 1 +}; + +class stream_category : public boost::system::error_category { +public: + const char* name() const BOOST_ASIO_ERROR_CATEGORY_NOEXCEPT { + return "wintls.stream"; + } + + std::string message(int value) const { + switch (value) { + case stream_truncated: + return "stream truncated"; + default: + return "wintls.stream error"; + } + } +}; + +inline const boost::system::error_category& get_stream_category() { + static stream_category instance; + return instance; +} + +inline boost::system::error_code make_error_code(stream_errors e) { + return boost::system::error_code(static_cast(e), get_stream_category()); +} + inline boost::system::error_code make_error_code(SECURITY_STATUS sc) { return boost::system::error_code(static_cast(sc), boost::system::system_category()); } @@ -23,4 +53,15 @@ inline boost::system::error_code make_error_code(SECURITY_STATUS sc) { } // namespace wintls } // namespace boost +namespace boost { +namespace system { + +template<> +struct is_error_code_enum { + static const bool value = true; +}; + +} // namespace system +} // namespace boost + #endif // BOOST_WINTLS_ERROR_HPP diff --git a/test/shutdown_test.cpp b/test/shutdown_test.cpp index 1290623b..eeb210df 100644 --- a/test/shutdown_test.cpp +++ b/test/shutdown_test.cpp @@ -24,7 +24,7 @@ auto stream_truncated_error(const asio_ssl::stream&) { template auto stream_truncated_error(const boost::wintls::stream&) { - return net::error::eof; // #TODO: This should be a separate error code. + return boost::wintls::error::stream_truncated; } using TestTypes = std::tuple, From 0b13afae3617234000af13b9bc0c1de1ff8d323b Mon Sep 17 00:00:00 2001 From: Jens Diewald Date: Thu, 6 Apr 2023 20:49:06 +0200 Subject: [PATCH 19/19] temp fix --- test/echo_test.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/echo_test.cpp b/test/echo_test.cpp index f7265622..9c04375f 100644 --- a/test/echo_test.cpp +++ b/test/echo_test.cpp @@ -75,8 +75,10 @@ TEMPLATE_LIST_TEST_CASE("echo test", "", TestTypes) { async_echo_server server(io_context); async_echo_client client(io_context, test_data); client.stream.next_layer().connect(server.stream.next_layer()); - server.run(); + // #TODO: calling server.run() before client.run() causes io_context.run() to block indefinitely + // in the shutdown handshake loop while the server waits for the client close notify. client.run(); + server.run(); io_context.run(); CHECK(client.received_message() == test_data); }