[Openvpn-devel,1/3] Change options->data_channel_use_ekm to flags

Message ID 20210408140229.31824-2-arne@rfc2549.org
State Accepted
Headers show
Series
  • P2P NCP support patch set
Related show

Commit Message

Arne Schwabe April 8, 2021, 2:02 p.m.
Instead maintaining two different representation of the data channel
options in struct options and struct tls_options, use the same
flags variable that tls_options uses.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/multi.c   | 5 ++++-
 src/openvpn/options.c | 4 ++--
 src/openvpn/options.h | 4 ++--
 src/openvpn/push.c    | 2 +-
 src/openvpn/ssl.c     | 6 ++----
 5 files changed, 11 insertions(+), 10 deletions(-)

Comments

Antonio Quartulli April 15, 2021, 10:27 p.m. | #1
Hi Arne,

On 08/04/2021 16:02, Arne Schwabe wrote:
> Instead maintaining two different representation of the data channel
> options in struct options and struct tls_options, use the same
> flags variable that tls_options uses.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Thia patch looks good, but I Was wondering: why converting this flag to
a bitmask? are you planning to add more bits to
o->data_channel_crypto_flags?

Or is it just to simplify the conversion to crypto_flags?

Cheers,
Arne Schwabe April 16, 2021, 11:01 a.m. | #2
Am 16.04.21 um 00:27 schrieb Antonio Quartulli:
> Hi Arne,
> 
> On 08/04/2021 16:02, Arne Schwabe wrote:
>> Instead maintaining two different representation of the data channel
>> options in struct options and struct tls_options, use the same
>> flags variable that tls_options uses.
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> 
> Thia patch looks good, but I Was wondering: why converting this flag to
> a bitmask? are you planning to add more bits to
> o->data_channel_crypto_flags?
> 
> Or is it just to simplify the conversion to crypto_flags?
> 

Kinda both. I have a patch that extends the IV to be xor'ed instead of
just counting upwards like TLS 1.3 is doing that but I currently keeping
that back as it might not be worth doing that.

Arne
Antonio Quartulli April 20, 2021, 4:14 p.m. | #3
Hi,

On 08/04/2021 16:02, Arne Schwabe wrote:
> Instead maintaining two different representation of the data channel
> options in struct options and struct tls_options, use the same
> flags variable that tls_options uses.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

It's a simple restyling, converting this flag to bitfield.
I don't think there is much gain, but allows adding more flags in the
future with little effort (and little chance to introduce bugs).

Passes my basic test (with the ekm being properly activated).

GitLab CI compile rig is happy too.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering April 27, 2021, 2:24 p.m. | #4
Lightly stared at code and ran client-side tests that actually used
EKM... which meant "upgrading the server the t_client tests talk to"
(and all passes).

Your patch has been applied to the master branch.

commit 9c625f4a6633de05d030884cac779cb41a5060e1
Author: Arne Schwabe
Date:   Thu Apr 8 16:02:26 2021 +0200

     Change options->data_channel_use_ekm to flags

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index d5f34c349..e6eb34bfb 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1783,7 +1783,10 @@  multi_client_set_protocol_options(struct context *c)
     }
 
 #ifdef HAVE_EXPORT_KEYING_MATERIAL
-    o->data_channel_use_ekm = (proto & IV_PROTO_TLS_KEY_EXPORT);
+    if (proto & IV_PROTO_TLS_KEY_EXPORT)
+    {
+        o->data_channel_crypto_flags |= CO_USE_TLS_KEY_MATERIAL_EXPORT;
+    }
 #endif
 
     /* Select cipher if client supports Negotiable Crypto Parameters */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 9e61b1e05..24d722fd5 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3651,7 +3651,7 @@  pre_pull_restore(struct options *o, struct gc_arena *gc)
 
     o->push_continuation = 0;
     o->push_option_types_found = 0;
-    o->data_channel_use_ekm = false;
+    o->data_channel_crypto_flags = 0;
 }
 
 /**
@@ -7949,7 +7949,7 @@  add_option(struct options *options,
 #ifdef HAVE_EXPORT_KEYING_MATERIAL
         if (streq(p[1], "tls-ekm"))
         {
-            options->data_channel_use_ekm = true;
+            options->data_channel_crypto_flags |= CO_USE_TLS_KEY_MATERIAL_EXPORT;
         }
         else
 #endif
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 65e5ffccf..b80cd3d1b 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -651,8 +651,8 @@  struct options
      * to the routing tables that would move packets into the tunnel. */
     bool allow_recursive_routing;
 
-    /* Use RFC5705 key export to generate data channel keys */
-    bool data_channel_use_ekm;
+    /* data channel crypto flags set by push/pull. Reuses the CO_* crypto_flags */
+    unsigned int data_channel_crypto_flags;
 };
 
 #define streq(x, y) (!strcmp((x), (y)))
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 15a9141ee..2e92d8ee2 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -595,7 +595,7 @@  prepare_push_reply(struct context *c, struct gc_arena *gc,
     {
         push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
     }
-    if (o->data_channel_use_ekm)
+    if (o->data_channel_crypto_flags & CO_USE_TLS_KEY_MATERIAL_EXPORT)
     {
         push_option_fmt(gc, push_list, M_USAGE, "key-derivation tls-ekm");
     }
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index d8662d000..5d65c3da5 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1860,10 +1860,8 @@  tls_session_update_crypto_params(struct tls_session *session,
         return false;
     }
 
-    if (options->data_channel_use_ekm)
-    {
-        session->opt->crypto_flags |= CO_USE_TLS_KEY_MATERIAL_EXPORT;
-    }
+    /* Import crypto settings that might be set by pull/push */
+    session->opt->crypto_flags |= options->data_channel_crypto_flags;
 
     if (strcmp(options->ciphername, session->opt->config_ciphername))
     {