search

laudrup / boost-wintls

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

add lowest_layer() that's also present in ssl::stream #50

Open DraconPern opened 2 years ago

DraconPern commented 2 years ago

Added two lowest_layer that's present in ssl::stream. For example, asio\example\cpp11\ssl\client.cpp uses this function and many people also expect it. For example a post like this https://stackoverflow.com/questions/28264313/ssl-certificates-and-boost-asio

laudrup commented 2 years ago

I actually had the lowest_layer type some time ago, but @vinniefalco pointed out this issue in asio, so I removed it again in 36e87075fcf0ce79069668e3955d08e43108271d.

I agree that it makes sense to match the interface of the asio ssl::stream. Whether that includes bugs can of course be discussed :-)

But please have a look at the issue @vinniefalco has mentioned and let me know what you think. Thanks.

Unrelated, but I had forgotten to trigger a github actions run on pull requests, so could you please either merge or rebase this against the current master. Would just be an easy way for me to see if it works as expected. Thanks a lot.

DraconPern commented 2 years ago

I think the work @vinniefalco did on beast is awesome, but regarding https://github.com/chriskohlhoff/asio/issues/251. Having a concept requirement doesn't mean the class can't have other extra functions. SyncReadStream and SyncWriteStream only requires read_some() and write_some(). As long as it has those functions the requirement is fulfilled. Also, get_executor() is a requirement for AsyncReadStream so you can't just take it out. As for the lowest_layer() function, you have to implement it. I think it's because the documentation forgot to mention that Stream needs to be of the basic_socket type (and also not have BOOST_ASIO_NO_EXTENSIONS defined). I think the confusion is the naming at template <typename Stream> class stream. Stream is actually a boost::asio::basic_socket type, not a stdio stream. They should have named it Socket. I believe it was actually referring to stream vs datagram protocol.

Looking at basic_socket.hpp, I think it might be better if it was guarded by the BOOST_ASIO_NO_EXTENSIONS. I'll update the patch.

DraconPern commented 2 years ago

Oh! I want to note that your examples should not depend on beast. If wintls is going to be a 'drop-in' replacement, it needs to show it can be used without beast. As of now, the examples depend on beast::get_lowest_layer which is not present in asio! kinda related to this pr, so I'll mention how important it is. :)

laudrup commented 2 years ago

Oh! I want to note that your examples should not depend on beast. If wintls is going to be a 'drop-in' replacement, it needs to show it can be used without beast. As of now, the examples depend on beast::get_lowest_layer which is not present in asio! kinda related to this pr, so I'll mention how important it is. :)

Only the HTTPS examples depend on boost::beast. There is a simple echo client and server that don't depend on beast.

Of course async versions of the echo/client server could be added, but I don't intend to remove the beast examples.

This library sholdn't depend on beast. Unfortunately the unit tests do because the test_stream from beast is quite handy. I have considered trying to extract that class so the tests can run using only asio and not the entire boost libraries.