[Openvpn-devel,1/2] Use key_state instead of multi for tls_send_payload parameter

Message ID 20230301135353.2811069-1-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,1/2] Use key_state instead of multi for tls_send_payload parameter | expand

Commit Message

Arne Schwabe March 1, 2023, 1:53 p.m. UTC
Currently, this function and other parts of OpenVPN assume that
multi->session[TM_ACTIVE].key[KS_PRIMARY] is always the right session
to send control message.

This assumption was only achieve through complicated session moving and
shuffling in our state machine in the past. The old logic basically also
always assumed that control messages are always for fully authenticated
clients. This assumption was never really true (see AUTH_FAILED message) but
has been broken even more by auth-pending. Cleaning up the state machine
transitions in 7dcde87b7a broke this assumption even more.

This change now allows to specify the key_state/TLS session that is used to
send the control message.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
 src/openvpn/forward.c | 5 ++++-
 src/openvpn/ssl.c     | 7 ++-----
 src/openvpn/ssl.h     | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)


Gert Doering March 20, 2023, 1:34 p.m. UTC | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

This one is fairly trivial refactoring - moving the "ks = get_key_scan()"
part out of tls_send_payload(), passing in "ks" instead of "multi".

Stared-at-code (straightforward), fed to GH for "maybe it is not?" and
local client side test.

Your patch has been applied to the master branch.

commit 06af538eb7bde36feb20ef63febb171c9607a5e6 (master)
commit 31279f71ab4124516fd0c2143f67a0c3f008ad20 (release/2.6)
Author: Arne Schwabe
Date:   Wed Mar 1 14:53:52 2023 +0100

     Use key_state instead of multi for tls_send_payload parameter

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20230301135353.2811069-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26319.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>

kind regards,

Gert Doering


diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 257c7c75c..9bb099097 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -372,8 +372,11 @@  send_control_channel_string_dowork(struct tls_multi *multi,
     struct gc_arena gc = gc_new();
     bool stat;
+    ASSERT(multi);
+    struct key_state *ks = get_key_scan(multi, 0);
     /* buffered cleartext write onto TLS control channel */
-    stat = tls_send_payload(multi, (uint8_t *) str, strlen(str) + 1);
+    stat = tls_send_payload(ks, (uint8_t *) str, strlen(str) + 1);
     msg(msglevel, "SENT CONTROL [%s]: '%s' (status=%d)",
         tls_common_name(multi, false),
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 016bdc57f..b84f23c62 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3988,18 +3988,15 @@  tls_post_encrypt(struct tls_multi *multi, struct buffer *buf)
-tls_send_payload(struct tls_multi *multi,
+tls_send_payload(struct key_state *ks,
                  const uint8_t *data,
                  int size)
-    struct key_state *ks;
     bool ret = false;
-    ASSERT(multi);
-    ks = get_key_scan(multi, 0);
+    ASSERT(ks);
     if (ks->state >= S_ACTIVE)
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index b0a2823fb..7ea13b920 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -414,7 +414,7 @@  void ssl_put_auth_challenge(const char *cr_str);
  * Send a payload over the TLS control channel
-bool tls_send_payload(struct tls_multi *multi,
+bool tls_send_payload(struct key_state *ks,
                       const uint8_t *data,
                       int size);