Skip to content

Refactor handshake #77

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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Refactor handshake #77

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eab1d14
Move manual_auth out of the handshake loop.
jens-diewald Feb 11, 2023
66a435c
Also send leftover handshake data as client.
jens-diewald Feb 11, 2023
4ba64de
Remove unused member from async_handshake
jens-diewald Feb 11, 2023
88ce93c
Remove redundant code
jens-diewald Feb 11, 2023
b437f7a
Make async_handshake::operator() not be a coroutine
jens-diewald Feb 11, 2023
9971739
Revert "Make async_handshake::operator() not be a coroutine"
jens-diewald Feb 16, 2023
0d8b104
Make use of async_handshake being a coroutine
jens-diewald Feb 16, 2023
c6bb0b0
Simplify code and add comment
jens-diewald Feb 16, 2023
fd3aea5
Improve handling of immediately failing async operations
jens-diewald Mar 1, 2023
7243bad
Add Boost 1.80 and Boost 1.81 to CI Unittests
jens-diewald Mar 2, 2023
12aec4a
Set toolset for new boost versions and install-boost
jens-diewald Mar 8, 2023
269342b
Increase waiting time for OCSP responder in test
jens-diewald Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 21 additions & 62 deletions include/boost/wintls/detail/async_handshake.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,16 @@ namespace boost {
namespace wintls {
namespace detail {

template <typename NextLayer>
template<typename NextLayer>
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 <typename Self>
template<typename Self>
void operator()(Self& self, boost::system::error_code ec = {}, std::size_t length = 0) {
if (ec) {
self.complete(ec);
Expand All @@ -40,94 +39,54 @@ struct async_handshake : boost::asio::coroutine {
return entry_count_ > 1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to return void ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a small lambda that returns bool.
Although i do not know why this is a lambda and not a simple bool variable. Technically, even the entry_count_ member could be a bool i suppose.
Then again, looking into the source of asio::detail::composed_op, it seems that it keeps track of its invocations itself and that we could access that via asio::asio_handler_is_continuation(&self). Is that how it should be done?

};

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;
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 {
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) {
if (handshake_state == 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;
}

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 == 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));
}
if (handshake_state == sspi_handshake::state::error) {
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 (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); });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); });
net::post(self.get_io_executor(), asio::append(std::move(self), ec, length));

This is bad because you lose the assocated executor, allocator & cancellation slot. Use append instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint!
asio::append is available since boost 1.80 and from your blog post you linked above, i gather that self.get_io_executor() is available since boost 1.81.
Currently wintls tests against older boost versions also and i think it is reasonable to support older versions for now, to keep the potential testing audience larger.
Hm, so maybe we should add a distinction based on BOOST_VERSION for now? @laudrup?

}
}
BOOST_ASSERT(!handshake_.last_error());
self.complete(handshake_.last_error());
}
}

private:
NextLayer& next_layer_;
detail::sspi_handshake& handshake_;
sspi_handshake& handshake_;
int entry_count_;
std::vector<char> input_;
enum class state {
idle,
reading,
writing
} state_;
};

} // namespace detail
Expand Down
67 changes: 24 additions & 43 deletions include/boost/wintls/detail/sspi_handshake.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -73,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;
}
Expand Down Expand Up @@ -129,6 +120,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;
}
Expand Down Expand Up @@ -209,28 +203,20 @@ 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 {
// 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 last_error_ == SEC_E_OK ? state::done_with_data : state::error_with_data;
}
}
return state::done;
// sspi handshake ok. Manual authentication will be done after the handshake loop.

// 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:
Expand Down Expand Up @@ -274,7 +260,6 @@ class sspi_handshake {
check_revocation_ = check;
}

private:
SECURITY_STATUS manual_auth(){
if (!context_.verify_server_certificate_) {
return SEC_E_OK;
Expand All @@ -284,16 +269,12 @@ class sspi_handshake {
if (last_error_ != SEC_E_OK) {
return last_error_;
}

cert_context_ptr remote_cert{ctx_ptr};

last_error_ = static_cast<SECURITY_STATUS>(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_;
Expand Down
24 changes: 5 additions & 19 deletions include/boost/wintls/stream.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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);
}
}
}
Expand Down