Assigning to a boost::asio::ssl::context while asynchronous operations are using it

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
I cannot find any definitive documentation on whether it's safe to
move-assign to an ssl-context while it is used for asynchronous
operations, assuming of course the application is single-threaded.

In my tests I could not trigger any failures, but that may be pure luck
(or not lucky at all depending on point of view).

_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
move-assignment from what to where ?

The ssl context is context which tuned for incoming connections (if
its server side).
You can to have any number of ssl contexts for it (with ordered logic
of that work),
but I have just the one.

Without a code is hard to suggest what you doing, but the ssl context
is not for moving,
I have been storing it in server class, initialize it on start_server
method, and put it into session_start
for make connection with handshake etc (the ssl context forwarded for
each connection as reference).

If you need more detailed answer, you could to give more details of your case.

2019-09-25 20:32 GMT+05:00, Martijn Otto via Boost-users
<[hidden email]>:
> I cannot find any definitive documentation on whether it's safe to
> move-assign to an ssl-context while it is used for asynchronous
> operations, assuming of course the application is single-threaded.
>
> In my tests I could not trigger any failures, but that may be pure luck
> (or not lucky at all depending on point of view).
>
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
On Thu, 2019-09-26 at 04:27 +0500, Dmitrij V via Boost-users wrote:

> move-assignment from what to where ?
>
> The ssl context is context which tuned for incoming connections (if
> its server side).
> You can to have any number of ssl contexts for it (with ordered logic
> of that work),
> but I have just the one.
>
> Without a code is hard to suggest what you doing, but the ssl context
> is not for moving,
> I have been storing it in server class, initialize it on start_server
> method, and put it into session_start
> for make connection with handshake etc (the ssl context forwarded for
> each connection as reference).
>
> If you need more detailed answer, you could to give more details of
> your case.
>
> 2019-09-25 20:32 GMT+05:00, Martijn Otto via Boost-users
> <[hidden email]>:
> > I cannot find any definitive documentation on whether it's safe to
> > move-assign to an ssl-context while it is used for asynchronous
> > operations, assuming of course the application is single-threaded.
> >
> > In my tests I could not trigger any failures, but that may be pure
> > luck
> > (or not lucky at all depending on point of view).
> >
> _______________________________________________
> Boost-users mailing list
> [hidden email]
> https://lists.boost.org/mailman/listinfo.cgi/boost-users
OK, I'll do my best to explain it as clearly as I can.

In my main function I define an instance of an ssl context. This is
then provided by lvalue reference to a 'server' class, which uses it to
do a TLS handshake on each incoming connection.

Now I want to be able to load a new certificate without restarting the
process (minimizing downtime). For this I have implemented a signal
handler listening to SIGUSR1. This handler constructs a new ssl context
and then move-assigns it to the ssl context variable in the main
function.

Since the server class has a reference to this same object, it's also
going to see the changes performed from the signal handler.

The code is fully async, so there are no race-conditions in the classic
sense of the word. The server may, however, be somewhere in the middle
of performing a TLS handshake. I know that _after_ the handshake is
complete, the ssl context is no longer relevant, but I can imagine this
could break if the ssl context is move-assigned to when an asynchronous
tls handshake is in progress.

As I said, I could not manage to get this to fail in my tests, but
conceptually I can imagine this is wrong. I'd like to be sure about
this. If what I am doing is indeed not correct, I will need to use a
shared_ptr so the handshakes can complete with the old context. Since
this introduces overhead I'd like to do this only if really necessary.

_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
Yes, you have no problem for now just by pure luck.
In the destructor of the context freeing all related to SSL resources.

How you can avoid the problems in future:

as an one of soltions, in your loop thread (if you have it, like me
for example):

1) io_.stop();
2) while (io_.poll()) {}
3) HERE your code to assign the context (perhaps you will need to
rehandshake alived sessions)
4) io_.restart();
5) acceptor_.async_accept(...);

PS: In my past project I had very likely approach - restart server
without stopping the process app. I had reject all my beginnings and
forced clients to reconnect by losed the connections (there are many
bytes would needs to store in shared memory (related to session data))

--
regards
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
as an second solution is: to store the context in shared_ptr ...
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
I have wrote: ...without stopping the process app...

Sorry for my mistake: I had target: replacing the image of app with
execve call (with needs to save session's data in shared memory) -
that would was likely smart updater :)...
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
In reply to this post by Boost - Users mailing list
On Wed, Sep 25, 2019 at 11:53 PM Martijn Otto via Boost-users
<[hidden email]> wrote:
> If what I am doing is indeed not correct, I will need to use a
> shared_ptr so the handshakes can complete with the old context. Since
> this introduces overhead I'd like to do this only if really necessary.

If you are using SSL then you should not be concerned about the
overhead of using shared_ptr, it is not statistically significant
compared to the kernel I/O and SSL operations.

Regards
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
In reply to this post by Boost - Users mailing list
On Fri, 2019-09-27 at 06:30 +0500, Dmitrij V via Boost-users wrote:

> Yes, you have no problem for now just by pure luck.
> In the destructor of the context freeing all related to SSL
> resources.
>
> How you can avoid the problems in future:
>
> as an one of soltions, in your loop thread (if you have it, like me
> for example):
>
> 1) io_.stop();
> 2) while (io_.poll()) {}
> 3) HERE your code to assign the context (perhaps you will need to
> rehandshake alived sessions)
> 4) io_.restart();
> 5) acceptor_.async_accept(...);
>
> PS: In my past project I had very likely approach - restart server
> without stopping the process app. I had reject all my beginnings and
> forced clients to reconnect by losed the connections (there are many
> bytes would needs to store in shared memory (related to session
> data))
>
> --
> regards
> _______________________________________________
> Boost-users mailing list
> [hidden email]
> https://lists.boost.org/mailman/listinfo.cgi/boost-users
It makes sense for the destructor to indeed clean up resources. That's
what it's for. However, looking at the OpenSSL documentation, there are
three relevant functions:

SSL_CTX_new(), creating an SSL_CTX instance with refcount 1
SSL_CTX_up_ref(), increasing the refcount
SSL_CTX_free(), decreasing the refcount, freeing when reaching 0

Now, given that OpenSSL uses a reference-counted system for their TLS
contexts, it would make sense if boost::asio were to increment the
reference count when starting the handshake, and decrement it when this
is done, which could easily make what I'm doing safe.

However, looking at the source, it appears that's not what it's doing.
I believe it wouldn't be much work to rectify this. In ssl/stream.hpp,
we are already taking the underlying SSL_CTX object as a member and
store it in core_. I think it'd be trivial to add an SSL_CTX_up_ref()
to the constructor and an SSL_CTX_free() to the destructor. If we do
this, we avoid potential problems, like the context being freed while
the stream is active.

I could make a pull-request to implement these changes, if so desired.
Does this have any change of getting merged?


_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
Martijn Otto wrote:
> I could make a pull-request to implement these changes, if so desired.

+1, changes + documentation on its, please :)

> Does this have any change of getting merged?

Oh, that is not in my authority, but please, send the PR into
https://github.com/chriskohlhoff/asio too

--
regards
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
In reply to this post by Boost - Users mailing list
On 9/26/19 8:52 AM, Martijn Otto via Boost-users wrote:

> Now I want to be able to load a new certificate without restarting the
> process (minimizing downtime). For this I have implemented a signal
> handler listening to SIGUSR1. This handler constructs a new ssl context
> and then move-assigns it to the ssl context variable in the main
> function.

Please notice that only syscalls marked as async-signal-safe may be used
within a signal handler. Memory allocation is not async-signal-safe.
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
On Sun, 2019-09-29 at 12:57 +0200, Bjorn Reese via Boost-users wrote:

> On 9/26/19 8:52 AM, Martijn Otto via Boost-users wrote:
>
> > Now I want to be able to load a new certificate without restarting
> > the
> > process (minimizing downtime). For this I have implemented a signal
> > handler listening to SIGUSR1. This handler constructs a new ssl
> > context
> > and then move-assigns it to the ssl context variable in the main
> > function.
>
> Please notice that only syscalls marked as async-signal-safe may be
> used
> within a signal handler. Memory allocation is not async-signal-safe.
> _______________________________________________
> Boost-users mailing list
> [hidden email]
> https://lists.boost.org/mailman/listinfo.cgi/boost-users

That's interesting and good to know. I cannot find it anywhere in the
documentation for signal_set (
https://www.boost.org/doc/libs/1_71_0/doc/html/boost_asio/reference/signal_set.html
). My understanding has always been that this issue does not occur
using asio, since it will post() to the executor when a signal is
received, so the possibly unsafe code is not executed in the signal
handler the kernel sees.

Have I misunderstood this?

_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
In reply to this post by Boost - Users mailing list
On Sat, 2019-09-28 at 07:31 +0500, Dmitrij V via Boost-users wrote:

> Martijn Otto wrote:
> > I could make a pull-request to implement these changes, if so
> > desired.
>
> +1, changes + documentation on its, please :)
>
> > Does this have any change of getting merged?
>
> Oh, that is not in my authority, but please, send the PR into
> https://github.com/chriskohlhoff/asio too
>
> --
> regards
> _______________________________________________
> Boost-users mailing list
> [hidden email]
> https://lists.boost.org/mailman/listinfo.cgi/boost-users

After reading through the source code some more, it seems that no
changes are actually necessary, since my initial code was already safe.
I'll explain why.

The ssl::stream constructor, which does get a reference to the
ssl::context and extracts the underlying SSL_CTX but does not keep this
in a member. Instead, it forwards it to its core_ member, which is an
instance of detail::stream_core. detail::stream_core then forwards it
to its engine_ member, which is an instance of detail::engine.

detail::engine then uses it to call SSL_new. The documentation for
SSL_new does not explicitly mention it, but looking through the code I
can see it call SSL_CTX_up_ref(), which increases the reference count
on the underlying SSL_CTX, which means that even if the ssl_context
calls SSL_CTX_free(), the context will remain valid if there is an
ssl::stream using it.

_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
Dear Martijn Otto,
Please, if it is not hard for you: make PR with documentation the
behaviour for the context.

--
regards

2019-09-29 22:11 GMT+05:00, Martijn Otto via Boost-users
<[hidden email]>:

> On Sat, 2019-09-28 at 07:31 +0500, Dmitrij V via Boost-users wrote:
>> Martijn Otto wrote:
>> > I could make a pull-request to implement these changes, if so
>> > desired.
>>
>> +1, changes + documentation on its, please :)
>>
>> > Does this have any change of getting merged?
>>
>> Oh, that is not in my authority, but please, send the PR into
>> https://github.com/chriskohlhoff/asio too
>>
>> --
>> regards
>> _______________________________________________
>> Boost-users mailing list
>> [hidden email]
>> https://lists.boost.org/mailman/listinfo.cgi/boost-users
>
> After reading through the source code some more, it seems that no
> changes are actually necessary, since my initial code was already safe.
> I'll explain why.
>
> The ssl::stream constructor, which does get a reference to the
> ssl::context and extracts the underlying SSL_CTX but does not keep this
> in a member. Instead, it forwards it to its core_ member, which is an
> instance of detail::stream_core. detail::stream_core then forwards it
> to its engine_ member, which is an instance of detail::engine.
>
> detail::engine then uses it to call SSL_new. The documentation for
> SSL_new does not explicitly mention it, but looking through the code I
> can see it call SSL_CTX_up_ref(), which increases the reference count
> on the underlying SSL_CTX, which means that even if the ssl_context
> calls SSL_CTX_free(), the context will remain valid if there is an
> ssl::stream using it.
>
> _______________________________________________
> Boost-users mailing list
> [hidden email]
> https://lists.boost.org/mailman/listinfo.cgi/boost-users
>
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
Reply | Threaded
Open this post in threaded view
|

Re: Assigning to a boost::asio::ssl::context while asynchronous operations are using it

Boost - Users mailing list
In reply to this post by Boost - Users mailing list
On 9/29/19 6:39 PM, Martijn Otto via Boost-users wrote:

> Have I misunderstood this?

You are right. I had missed the fact that you installed your signal
handlers via Boost.Asio.
_______________________________________________
Boost-users mailing list
[hidden email]
https://lists.boost.org/mailman/listinfo.cgi/boost-users