| Message ID | 20200810143707.5834-14-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | OpenVPN refactoring | expand |
Acked-by: Gert Doering <gert@greenie.muc.de> Server-side and client-side tested. Not sure if I understand all possible implications of S_NORMAL_OP, but indeed it is not *used* anywere, except in ">= S_ACTIVE". The flow of "at which point in time we set must_negotiate = 0" changes a bit - the old code would do it "when it expired AND we're in >= S_ACTIVE", while the new code would do it "right when setting S_ACTIVE" - which is the only place where S_ACTIVE is set, so it would always catch said condition. This should be totally fine. Your patch has been applied to the master branch. commit c13d20fae3961ba67de3c4c85c75ebd1ac802b26 Author: Arne Schwabe Date: Mon Aug 10 16:37:03 2020 +0200 Remove S_OP_NORMAL key state. Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20200810143707.5834-14-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20674.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
On 11-08-2020 10:45, Gert Doering wrote: > Acked-by: Gert Doering <gert@greenie.muc.de> > > Server-side and client-side tested. > > Not sure if I understand all possible implications of S_NORMAL_OP, > but indeed it is not *used* anywere, except in ">= S_ACTIVE". > > The flow of "at which point in time we set must_negotiate = 0" > changes a bit - the old code would do it "when it expired AND > we're in >= S_ACTIVE", while the new code would do it "right > when setting S_ACTIVE" - which is the only place where S_ACTIVE > is set, so it would always catch said condition. This should be > totally fine. > > Your patch has been applied to the master branch. FWIW: I agree, I don't think we need S_NORMAL_OP. Less code, more better. -Steffan
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index a43ee985..0d54c9ed 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -755,9 +755,6 @@ state_name(int state) case S_ACTIVE: return "S_ACTIVE"; - case S_NORMAL_OP: - return "S_NORMAL_OP"; - case S_ERROR: return "S_ERROR"; @@ -2705,21 +2702,12 @@ tls_process(struct tls_multi *multi, } /* Are we timed out on receive? */ - if (now >= ks->must_negotiate) + if (now >= ks->must_negotiate && ks->state < S_ACTIVE) { - if (ks->state < S_ACTIVE) - { - msg(D_TLS_ERRORS, - "TLS Error: TLS key negotiation failed to occur within %d seconds (check your network connectivity)", - session->opt->handshake_window); - goto error; - } - else /* assume that ks->state == S_ACTIVE */ - { - dmsg(D_TLS_DEBUG_MED, "STATE S_NORMAL_OP"); - ks->state = S_NORMAL_OP; - ks->must_negotiate = 0; - } + msg(D_TLS_ERRORS, + "TLS Error: TLS key negotiation failed to occur within %d seconds (check your network connectivity)", + session->opt->handshake_window); + goto error; } /* Wait for Initial Handshake ACK */ @@ -2759,6 +2747,8 @@ tls_process(struct tls_multi *multi, } state_change = true; ks->state = S_ACTIVE; + /* Cancel negotiation timeout */ + ks->must_negotiate = 0; INCR_SUCCESS; /* Set outgoing address for data channel packets */ diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 9f777750..96897e48 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -64,8 +64,7 @@ * material. * -# \c S_GOT_KEY, have received remote part of \c key_source2 random * material. - * -# \c S_ACTIVE, normal operation during remaining handshake window. - * -# \c S_NORMAL_OP, normal operation. + * -# \c S_ACTIVE, normal operation * * Servers follow the same order, except for \c S_SENT_KEY and \c * S_GOT_KEY being reversed, because the server first receives the @@ -94,9 +93,9 @@ * immediately after negotiation has * completed while still within the * handshake window. */ -/* ready to exchange data channel packets */ -#define S_NORMAL_OP 7 /**< Normal operational \c key_state - * state. */ +/* Note that earlier versions also had a S_OP_NORMAL state that was + * virtually identical with S_ACTIVE and the code still assumes everything + * >= S_ACTIVE to be fully operational */ /** @} name Control channel negotiation states */ /** @} addtogroup control_processor */
The key state is virtually identical S_ACTIVE and we only did the state state transition form S_ACTIVE to S_OP_NORMAL at the point where we normally would have timed out the TLS negotiation. This is a very useful to have and indeed we never that information. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/ssl.c | 24 +++++++----------------- src/openvpn/ssl_common.h | 9 ++++----- 2 files changed, 11 insertions(+), 22 deletions(-)