A number of extra features/changes for ASIO

[nickfajones]

I want to contribute a number of changes to ASIO that I have made for use in our products at Network Box.

The changes come under three categories. Each of the categories does require some work,
so they couldn't be included in ASIO straight away without further discussion and refinement.

The changes are:

1. New interfaces for loading SSL certificates:
   e08db1f05d5e19b5e10d87235247e46b593300c4

Interfaces to load CAs, Certificates and Private Keys stored in the form of std::string.
Useful for cases where these objects have been loaded from a database.

Additional interfaces therefore no ABI breakage.

90% complete, as the interface for loading CAs in ASN1 format hasn't been defined.
Loading Certs and PKs supports both PEM and ASN1 which is consistent with other
ASIO interfaces available.

2. Using already buffered data to feed an SSL handshake
   56ec4891d4e9ee530d990adeca3c263b7a58778b
   09db9f10f606b86e39869547f48cd9c157ed944a

Our planned use for this is cases where data is read from a socket, then subsequently identified as being
a SSL handshake header. Caller may then re-use the already read data in a handshake.
New function overload for async_handshake, so should be no ABI breakage.

This change is theory only at this stage, so is more of a request for comments. It works by using the
user provided buffer object as the initial value of the SSL layer input_ buffer, so the read routines find
this data before they try reading from the wire. As stated, not tested fully, and as I am writing this pull
request, I realise there is no corresponding non-async interface. I will submit that soon.

3. Altered logic on SSL shutdown
   d966bf238727f10fa9853b3959913becb8b1696c

Provide a distinct error value for ssl shutdown in the form of a new error_code: ssl_eof. On receipt of
this, the user can then initiate a shutdown in their own chosen method: async or non-async.
I seem to recall that in the current logic, the code path leads to an implicit call to SSL_shutdown,
which is out of control of the user.

This change is not backwards compatible and a brand new error_code value is being returned, so
using this interface will require proper support in code, as only exists in my company software.
But seeing as there is a problem with the logic in regard to ssl shutdown, any fix to this area may
require a new version release of ASIO with sufficient documentation to explain the new behaviour.
But with this fix, I am able to produce a pleasing, although probably redundant test example:
A client and server connect with TCP and can exchange data in plaintext.
Either side may initiate a secure connection by writing 'starttls\n' on a new line.
Either side may stop a secure connection by writing 'stoptls\n' on a new line, and continue talking
on the same connection in plaintext.
This may be repeated on the same connection at will.

Thanks for taking the time to consider these submissions.

[chriskohlhoff]

Hi Nick,

Thanks for taking the time to work on these patches. I have a few comments.

1. New interfaces for loading SSL certificates:

These look good. However I do wonder whether std::string equivalents should be added for all of the existing *_file() functions. What do you think?

2. Using already buffered data to feed an SSL handshake

I think these functions should be templated to take a ConstBufferSequence rather than a single const_buffer. This data can then be buffer_copy()-ed in to the stream's core's input buffer space, and the input_ set to point to that.

3. Altered logic on SSL shutdown

The intention is that asio::error::eof be used to indicate closure of the SSL session, and a new error code shouldn't be needed. However I did fix a bug recently in that area. Can you please check whether that covers your use case? Please see: 93bf0f4

Cheers,
Chris

[nickfajones]

On Wed, Dec 14, 2011 at 4:56 PM, chriskohlhoff <
reply@reply.github.com

wrote:

Hi Nick,

Thanks for taking the time to work on these patches. I have a few comments.

1. New interfaces for loading SSL certificates:

These look good. However I do wonder whether std::string equivalents
should be added for all of the existing *_file() functions. What do you
think?

Yes, to make the feature more complete, where openssl supports the ability
to do as such, more of the _file functions could be changed. I'll have a
look over the api and the openssl docs to find which other non _file
functions can be added.

I recall the reason why there is no interface for CA loading from an asn1
format byte array (as there is for certificates), and that was because
openssl didn't seem to support it.

2. Using already buffered data to feed an SSL handshake

I think these functions should be templated to take a ConstBufferSequence
rather than a single const_buffer. This data can then be buffer_copy()-ed
in to the stream's core's input buffer space, and the input_ set to point
to that.

Yes, originally when I started this feature, I pondered the code in ASIO
for a while to see if there was a way to replace the
asio::ssl::detail::stream_core::input_ member with a
ConstBufferSequence but quickly gave up because it seemed too difficult at
the time and keeping the existing single buffer was enough to get started
proving the concept. We dealt with fragmented pages by linearising our own
buffers before wrapping them in asio::buffer, but this is not ideal.

Instead of keeping the asio::ssl::detail::stream_core::input_ member, we
could look at replacing this with a pair of buffers_iterator members:
input_begin_ and input_end_.
If the author calls async_handshake with no buffer sequence (the
traditional interface), input_begin_ and input_end_ are initialised in a
way to make them equal.
If the author calls async_handshake with a ConstBufferSequence,
input_begin_ and input_end_ are initialised with this.
During normal operation, if input_begin_ == input_end_, fill input_buffer_,
the iterators are re-initialised using the newly filled input_buffer_.
The interface to stream_core::put_input can be changed to use the iterators
rather than a buffer.

3. Altered logic on SSL shutdown

The intention is that asio::error::eof be used to indicate closure of the
SSL session, and a new error code shouldn't be needed. However I did fix a
bug recently in that area. Can you please check whether that covers your
use case? Please see: 93bf0f4

Your bug fix is logically the same as what I have, except I set the
error_code to my own defined ssl_eof.

I felt that the caller would need a distinct indication that only the SSL
aspect of the connection was being shutdown by the peer, not the underlying
connection itself.

I guess that upon receiving asio::error::eof in an SSL session, the author
could call is_open() on the underlying socket to differentiate? Then there
may be no need for my feature.

Given that the only place where asio::error::eof is set as the error code
is in engine::perform, where you already do a check of the
SSL_RECEIVED_SHUTDOWN flag, the map_error_code function is probably
redundant.

On a kind of related note:
in ssl/detail/io.hpp:259
c compilers don't guarantee the order of argument evaluation. So if
map_error_code stays, and some time in future maps an error to non-error
but there was a data transfer, then the author could get a non-error read
or write of zero, it's usually safer to evaluate arguments before a
function. I have had bugs like this before, and will probably have some
again.

Thanks

[chriskohlhoff]

I'll have a look over the api and the openssl docs to find which other non _file functions can be added.

Thanks. I guess if there's no easy equivalent for some of them there's no point trying too hard.

We dealt with fragmented pages by linearising our own buffers before wrapping them in asio::buffer, but this is not ideal.

Can you expand on what problems you had with linearisation?

In any case support for multiple buffers is essential IMHO, as in my experience things like circular buffers are commonplace.

I felt that the caller would need a distinct indication that only the SSL aspect of the connection was being shutdown by the peer, not the underlying connection itself.

The current mapping should provide this. The asio::error::eof is only for clean termination of the SSL session. Eof from the underlying socket is remapped to another error, hence map_error_code().

Given that the only place where asio::error::eof is set as the error code is in engine::perform

It may also be set by the underlying stream. E.g. the read_some() operation. It is then mapped to another error.

On a kind of related note: in ssl/detail/io.hpp:259 c compilers don't guarantee the order of argument evaluation.

Thanks of the tip. I will change this.

[nickfajones]

On Wed, Dec 14, 2011 at 8:32 PM, chriskohlhoff <
reply@reply.github.com

wrote:

I'll have a look over the api and the openssl docs to find which other
non _file functions can be added.

Thanks. I guess if there's no easy equivalent for some of them there's no
point trying too hard.

There are still a number of functions that can be converted, I'm sure, but
they'll need to be done case at a time.

I'll keep these specific functions in my tree until I can map as many more
functions as possible, I'll send another pull request then.

We dealt with fragmented pages by linearising our own buffers before
wrapping them in asio::buffer, but this is not ideal.

Can you expand on what problems you had with linearisation?

In any case support for multiple buffers is essential IMHO, as in my
experience things like circular buffers are commonplace.

For us, it is possible that an SSL handshake header would cross the
boundary of one buffer page and onto the next, although it never happened
in the lab without a bit of force, it's of course entirely possible in the
field, especially for protocols where the SSL session doesn't begin at the
start of a stream connection, or if you are re-using a buffer object. As a
precaution to deal with these theoretical cases, I have to do a memory copy
to a precisely sized buffer, which would then be wrapped in asio::buffer
when passed as a parameter to my async_handshake function.

But I fully agree, supporting a buffer sequence is the way to go.

I felt that the caller would need a distinct indication that only the
SSL aspect of the connection was being shutdown by the peer, not the
underlying connection itself.

The current mapping should provide this. The asio::error::eof is only for
clean termination of the SSL session. Eof from the underlying socket is
remapped to another error, hence map_error_code().

Given that the only place where asio::error::eof is set as the error
code is in engine::perform

It may also be set by the underlying stream. E.g. the read_some()
operation. It is then mapped to another error.

Ok, sounds fine, then basically the short_read error maps to stream
connection close while eof indicates proper SSL shutdown. The patch you
made should provide enough clear indication to the author.

I'll work further on the non _file function conversions and send another
pull request later. My work project will shift in a few months time to let
me get back to getting full support for ConstBufferSequence obedient
objects for async_handshake working to make it more than just a proof of
concept.