[Openvpn-devel,v3,10/25] dco: periodically check and possibly rotate/delete keys

Message ID 20220802151604.2801-1-a@unstable.cc
State Accepted
Headers show
Series None | expand

Commit Message

Antonio Quartulli Aug. 2, 2022, 5:16 a.m. UTC
Data channel keys are periodically regenarated and installed in
ovpn-dco.
However, there is a certain moment when keys are rotated in order
to elect the new primary one.

Check the key status in userspace so that kernelspace can be informed as
well when rotations happen.

Signed-off-by: Antonio Quartulli <a@unstable.cc>
---

Changes from v2:
* reworked dco_update_keys():
** removed existing ASSERTs on keys and converted into something more
   logic and related to the flow
** fixed comment about userspace (use "we" for more clarity)
** add error code handling. The idea is to abort operations as soon as a
   failure is detected, so that the next iteration can make another
   attempt.

Changes from v1:
* added comments to ASSERT() in dco_update_keys()
---
 src/openvpn/dco.c     | 122 ++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/dco.h     |  14 +++++
 src/openvpn/forward.c |  19 +++++++
 3 files changed, 155 insertions(+)

Comments

Frank Lichtenheld Aug. 2, 2022, 11:10 p.m. UTC | #1
On Tue, Aug 02, 2022 at 05:16:04PM +0200, Antonio Quartulli wrote:
> Data channel keys are periodically regenarated and installed in
> ovpn-dco.
> However, there is a certain moment when keys are rotated in order
> to elect the new primary one.
> 
> Check the key status in userspace so that kernelspace can be informed as
> well when rotations happen.
> 
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
> 
> Changes from v2:
> * reworked dco_update_keys():
> ** removed existing ASSERTs on keys and converted into something more
>    logic and related to the flow
> ** fixed comment about userspace (use "we" for more clarity)
> ** add error code handling. The idea is to abort operations as soon as a
>    failure is detected, so that the next iteration can make another
>    attempt.

Looks to me like this addresses all my comments.

Regards,
Gert Doering Aug. 3, 2022, 4:30 a.m. UTC | #2
Acked-by: Gert Doering <gert@greenie.muc.de>

The review of that was a combined effort, mostly Frank on mail and
Antonio and I on IRC - but the v3 is now easier to understand, and
"should work nicely".

I have slightly extended the comment before "swapping primary and
secondary keys" to make a bit clearer how we end there (we discussed
that comment on IRC).

This said, I have only tested the "no DCO in kernel" cases, as the
rest is still missing functinality - and for "no DCO", this becomes
fairly much "no change", as the only new code otherwise is guarded
with "if (!dco_enabled(&c->options))".

Your patch has been applied to the master branch.

commit cd4ba9279d9c4cf13cdc3d8c49143c57eb0fe448
Author: Antonio Quartulli
Date:   Tue Aug 2 17:16:04 2022 +0200

     dco: periodically check and possibly rotate/delete keys

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220802151604.2801-1-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24785.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index b0dd922a..dfdf10ea 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -94,6 +94,128 @@  init_key_dco_bi(struct tls_multi *multi, struct key_state *ks,
                            ciphername);
 }
 
+/**
+ * Find a usable key that is not the primary (i.e. the secondary key)
+ *
+ * @param multi     The TLS struct to retrieve keys from
+ * @param primary   The primary key that should be skipped during the scan
+ *
+ * @return          The secondary key or NULL if none could be found
+ */
+static struct key_state *
+dco_get_secondary_key(struct tls_multi *multi, const struct key_state *primary)
+{
+    for (int i = 0; i < KEY_SCAN_SIZE; ++i)
+    {
+        struct key_state *ks = get_key_scan(multi, i);
+        struct key_ctx_bi *key = &ks->crypto_options.key_ctx_bi;
+
+        if (ks == primary)
+        {
+            continue;
+        }
+
+        if (ks->state >= S_GENERATED_KEYS && ks->authenticated == KS_AUTH_TRUE)
+        {
+            ASSERT(key->initialized);
+            return ks;
+        }
+    }
+
+    return NULL;
+}
+
+void
+dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
+{
+    msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->peer_id);
+
+    /* this function checks if keys have to be swapped or erased, therefore it
+     * can't do much if we don't have any key installed
+     */
+    if (multi->dco_keys_installed == 0)
+    {
+        return;
+    }
+
+    struct key_state *primary = tls_select_encryption_key(multi);
+    /* no primary key available -> no usable key exists, therefore we should
+     * tell DCO to simply wipe all keys
+     */
+    if (!primary)
+    {
+        msg(D_DCO, "No encryption key found. Purging data channel keys");
+
+        int ret = dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_PRIMARY);
+        if (ret < 0)
+        {
+            msg(D_DCO, "Cannot delete primary key during wipe: %s (%d)", strerror(-ret), ret);
+            return;
+        }
+
+        ret = dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY);
+        if (ret < 0)
+        {
+            msg(D_DCO, "Cannot delete secondary key during wipe: %s (%d)", strerror(-ret), ret);
+            return;
+        }
+
+        multi->dco_keys_installed = 0;
+        return;
+    }
+
+    /* if we have a primary key, it must have been installed already (keys
+     * are installed upon generation in the TLS code)
+     */
+    ASSERT(primary->dco_status != DCO_NOT_INSTALLED);
+
+    struct key_state *secondary = dco_get_secondary_key(multi, primary);
+    /* the current primary key was installed as secondary in DCO, this means
+     * we have promoted it and we should tell DCO to swap keys
+     */
+    if (primary->dco_status == DCO_INSTALLED_SECONDARY)
+    {
+        msg(D_DCO_DEBUG, "Swapping primary and secondary keys, now: id1=%d id2=%d",
+            primary->key_id, secondary ? secondary->key_id : -1);
+
+        int ret = dco_swap_keys(dco, multi->peer_id);
+        if (ret < 0)
+        {
+            msg(D_DCO, "Cannot swap keys: %s (%d)", strerror(-ret), ret);
+            return;
+        }
+
+        primary->dco_status = DCO_INSTALLED_PRIMARY;
+        if (secondary)
+        {
+            ASSERT(secondary->dco_status == DCO_INSTALLED_PRIMARY);
+            secondary->dco_status = DCO_INSTALLED_SECONDARY;
+        }
+    }
+
+    /* if we have no secondary key anymore, inform DCO about it */
+    if (!secondary && multi->dco_keys_installed == 2)
+    {
+        int ret = dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY);
+        if (ret < 0)
+        {
+            msg(D_DCO, "Cannot delete secondary key: %s (%d)", strerror(-ret), ret);
+            return;
+        }
+        multi->dco_keys_installed = 1;
+    }
+
+    /* all keys that are not installed are set to NOT installed */
+    for (int i = 0; i < KEY_SCAN_SIZE; ++i)
+    {
+        struct key_state *ks = get_key_scan(multi, i);
+        if (ks != primary && ks != secondary)
+        {
+            ks->dco_status = DCO_NOT_INSTALLED;
+        }
+    }
+}
+
 static bool
 dco_check_option_conflict_platform(int msglevel, const struct options *o)
 {
diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h
index 1692f5c3..b926e236 100644
--- a/src/openvpn/dco.h
+++ b/src/openvpn/dco.h
@@ -132,6 +132,14 @@  int init_key_dco_bi(struct tls_multi *multi, struct key_state *ks,
                     const struct key2 *key2, int key_direction,
                     const char *ciphername, bool server);
 
+/**
+ * Possibly swap or wipe keys from DCO
+ *
+ * @param dco           DCO device context
+ * @param multi         TLS multi instance
+ */
+void dco_update_keys(dco_context_t *dco, struct tls_multi *multi);
+
 #else /* if defined(ENABLE_DCO) */
 
 typedef void *dco_context_t;
@@ -192,5 +200,11 @@  init_key_dco_bi(struct tls_multi *multi, struct key_state *ks,
     return 0;
 }
 
+static inline void
+dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
+{
+    ASSERT(false);
+}
+
 #endif /* defined(ENABLE_DCO) */
 #endif /* ifndef DCO_H */
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 28f3c088..38d2683c 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -41,6 +41,7 @@ 
 #include "dhcp.h"
 #include "common.h"
 #include "ssl_verify.h"
+#include "dco.h"
 
 #include "memdbg.h"
 
@@ -140,6 +141,18 @@  context_reschedule_sec(struct context *c, int sec)
     }
 }
 
+void
+check_dco_key_status(struct context *c)
+{
+    /* DCO context is not yet initialised or enabled */
+    if (!dco_enabled(&c->options))
+    {
+        return;
+    }
+
+    dco_update_keys(&c->c1.tuntap->dco, c->c2.tls_multi);
+}
+
 /*
  * In TLS mode, let TLS level respond to any control-channel
  * packets which were received, or prepare any packets for
@@ -182,6 +195,12 @@  check_tls(struct context *c)
 
     interval_schedule_wakeup(&c->c2.tmp_int, &wakeup);
 
+    /* Our current code has no good hooks in the TLS machinery to update
+     * DCO keys. So we check the key status after the whole TLS machinery
+     * has been completed and potentially update them
+     */
+    check_dco_key_status(c);
+
     if (wakeup)
     {
         context_reschedule_sec(c, wakeup);