diff mbox series

[Openvpn-devel,v2,2/3] Use dedicated multi->dco_peer_id for DCO instead of multi->peer_id

Message ID 20221124230025.3205108-1-arne@rfc2549.org
State Changes Requested
Headers show
Series None | expand

Commit Message

Arne Schwabe Nov. 24, 2022, 11 p.m. UTC 
The lifetime and state machine of multi->peer_id does not exactly the
lifetime/state of DCO. This is especially for p2p NCP where a reconnection
can change the peer id. Also use this new field with value -1 to mean
not installed, replacing the dco_peer_added field.

Patch v2: fix one comparison checking for 0 instead of -1

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/dco.c        | 24 ++++++++++++------------
 src/openvpn/forward.c    |  2 +-
 src/openvpn/init.c       |  4 ++--
 src/openvpn/multi.c      |  8 ++++----
 src/openvpn/ssl.c        |  1 +
 src/openvpn/ssl_common.h |  9 ++++++++-
 6 files changed, 28 insertions(+), 20 deletions(-)

Comments

Gert Doering Nov. 25, 2022, 9:17 a.m. UTC | #1 
Hi,

On Fri, Nov 25, 2022 at 12:00:25AM +0100, Arne Schwabe wrote:
> The lifetime and state machine of multi->peer_id does not exactly the
> lifetime/state of DCO. This is especially for p2p NCP where a reconnection
> can change the peer id. Also use this new field with value -1 to mean
> not installed, replacing the dco_peer_added field.
> 
> Patch v2: fix one comparison checking for 0 instead of -1

This looks good, but fails the server test - after the first few
client connects it seems to get confused on peer installation, and dies
miserably

Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: MULTI: primary virtual IPv6 for freebsd-14-amd64/194.97.140.5:13776: fd00:abcd:220:2::1002
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: dco_new_peer: netlink reports error (-6): Object exists: No such file or directory (errno=2)
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: dco_new_peer: failed to send netlink message: File exists (-17)
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: Cannot add peer to DCO: File exists (-17)
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: dco_set_peer: netlink reports object not found, ovpn-dco unloaded?
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: dco_set_peer: failed to send netlink message: No such file or directory (-2)
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: Cannot set parameters for DCO peer (id=4294967295): No such file or directory

   >>> now that id= hints at "something has not been initialized properly"
   >>> as that is "(unsiged int) -1"

Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: Data Channel: using negotiated cipher 'AES-256-GCM'
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: Data Channel MTU parms [ mss_fix:1380 max_frag:0 tun_mtu:1500 tun_max_mtu:1600 headroom:136 payload:1768 tailroom:562 ET:0 ]
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: dco_new_key: netlink reports object not found, ovpn-dco unloaded?
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: dco_new_key: failed to send netlink message: No such file or directory (-2)
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: Impossible to install key material in DCO: No such file or directory
Nov 25 09:51:53 ubuntu2004 tun-udp-p2mp[641000]: Exiting due to fatal error


(This is plain p2mp udp TLS server, with t_client client connecting to
it - same client cert, but new connection, new source ip/port - there
is a second client connected for client-to-client testing)

gert
diff mbox series

Patch

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index f190d038b..d83f24fef 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -55,7 +55,7 @@  dco_install_key(struct tls_multi *multi, struct key_state *ks,
                 const char *ciphername)
 
 {
-    msg(D_DCO_DEBUG, "%s: peer_id=%d keyid=%d", __func__, multi->peer_id,
+    msg(D_DCO_DEBUG, "%s: peer_id=%d keyid=%d", __func__, multi->dco_peer_id,
         ks->key_id);
 
     /* Install a key in the PRIMARY slot only when no other key exist.
@@ -69,7 +69,7 @@  dco_install_key(struct tls_multi *multi, struct key_state *ks,
         slot = OVPN_KEY_SLOT_SECONDARY;
     }
 
-    int ret = dco_new_key(multi->dco, multi->peer_id, ks->key_id, slot,
+    int ret = dco_new_key(multi->dco, multi->dco_peer_id, ks->key_id, slot,
                           encrypt_key, encrypt_iv,
                           decrypt_key, decrypt_iv,
                           ciphername);
@@ -133,7 +133,7 @@  dco_get_secondary_key(struct tls_multi *multi, const struct key_state *primary)
 void
 dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
 {
-    msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->peer_id);
+    msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->dco_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
@@ -151,14 +151,14 @@  dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
     {
         msg(D_DCO, "No encryption key found. Purging data channel keys");
 
-        int ret = dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_PRIMARY);
+        int ret = dco_del_key(dco, multi->dco_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);
+        ret = dco_del_key(dco, multi->dco_peer_id, OVPN_KEY_SLOT_SECONDARY);
         if (ret < 0)
         {
             msg(D_DCO, "Cannot delete secondary key during wipe: %s (%d)", strerror(-ret), ret);
@@ -184,7 +184,7 @@  dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
         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);
+        int ret = dco_swap_keys(dco, multi->dco_peer_id);
         if (ret < 0)
         {
             msg(D_DCO, "Cannot swap keys: %s (%d)", strerror(-ret), ret);
@@ -202,7 +202,7 @@  dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
     /* 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);
+        int ret = dco_del_key(dco, multi->dco_peer_id, OVPN_KEY_SLOT_SECONDARY);
         if (ret < 0)
         {
             msg(D_DCO, "Cannot delete secondary key: %s (%d)", strerror(-ret), ret);
@@ -465,7 +465,7 @@  dco_p2p_add_new_peer(struct context *c)
         return ret;
     }
 
-    c->c2.tls_multi->dco_peer_added = true;
+    c->c2.tls_multi->dco_peer_id = multi->peer_id;
     c->c2.link_socket->info.lsa->actual.dco_installed = true;
 
     return 0;
@@ -479,10 +479,10 @@  dco_remove_peer(struct context *c)
         return;
     }
 
-    if (c->c1.tuntap && c->c2.tls_multi && c->c2.tls_multi->dco_peer_added)
+    if (c->c1.tuntap && c->c2.tls_multi && c->c2.tls_multi->dco_peer_id == -1)
     {
-        dco_del_peer(&c->c1.tuntap->dco, c->c2.tls_multi->peer_id);
-        c->c2.tls_multi->dco_peer_added = false;
+        dco_del_peer(&c->c1.tuntap->dco, c->c2.tls_multi->dco_peer_id);
+        c->c2.tls_multi->dco_peer_id = -1;
     }
 }
 
@@ -585,7 +585,7 @@  dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi)
         return ret;
     }
 
-    c->c2.tls_multi->dco_peer_added = true;
+    c->c2.tls_multi->dco_peer_id = peer_id;
 
     if (c->mode == CM_CHILD_TCP)
     {
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 622be8411..8aef82606 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1731,7 +1731,7 @@  process_outgoing_link(struct context *c)
                 if (should_use_dco_socket(c->c2.to_link_addr))
                 {
                     size = dco_do_write(&c->c1.tuntap->dco,
-                                        c->c2.tls_multi->peer_id,
+                                        c->c2.tls_multi->dco_peer_id,
                                         &c->c2.to_link);
                 }
                 else
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 1b30c8f0a..0e4769775 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2151,14 +2151,14 @@  do_deferred_options_part2(struct context *c)
         && (c->options.ping_send_timeout || c->c2.frame.mss_fix))
     {
         int ret = dco_set_peer(&c->c1.tuntap->dco,
-                               c->c2.tls_multi->peer_id,
+                               c->c2.tls_multi->dco_peer_id,
                                c->options.ping_send_timeout,
                                c->options.ping_rec_timeout,
                                c->c2.frame.mss_fix);
         if (ret < 0)
         {
             msg(D_DCO, "Cannot set parameters for DCO peer (id=%u): %s",
-                c->c2.tls_multi->peer_id, strerror(-ret));
+                c->c2.tls_multi->dco_peer_id, strerror(-ret));
             return false;
         }
     }
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index b9b087e01..bd823e81f 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2448,14 +2448,14 @@  multi_client_connect_late_setup(struct multi_context *m,
             if (mi->context.options.ping_send_timeout || mi->context.c2.frame.mss_fix)
             {
                 int ret = dco_set_peer(&mi->context.c1.tuntap->dco,
-                                       mi->context.c2.tls_multi->peer_id,
+                                       mi->context.c2.tls_multi->dco_peer_id,
                                        mi->context.options.ping_send_timeout,
                                        mi->context.options.ping_rec_timeout,
                                        mi->context.c2.frame.mss_fix);
                 if (ret < 0)
                 {
                     msg(D_DCO, "Cannot set parameters for DCO peer (id=%u): %s",
-                        mi->context.c2.tls_multi->peer_id, strerror(-ret));
+                        mi->context.c2.tls_multi->dco_peer_id, strerror(-ret));
                     mi->context.c2.tls_multi->multi_state = CAS_FAILED;
                 }
             }
@@ -3226,8 +3226,8 @@  process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi,
     }
 
     /* When kernel already deleted the peer, the socket is no longer
-     * installed and we don't need to cleanup the state in the kernel */
-    mi->context.c2.tls_multi->dco_peer_added = false;
+     * installed, and we do not need to clean up the state in the kernel */
+    mi->context.c2.tls_multi->dco_peer_id = -1;
     mi->context.sig->signal_text = reason;
     multi_signal_instance(m, mi, SIGTERM);
 }
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index a73a2b714..818100c23 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1315,6 +1315,7 @@  tls_multi_init(struct tls_options *tls_options)
 
     /* get command line derived options */
     ret->opt = *tls_options;
+    ret->dco_peer_id = -1;
 
     return ret;
 }
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 24d40ab83..e967970dd 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -661,7 +661,14 @@  struct tls_multi
     /* Only used when DCO is used to remember how many keys we installed
      * for this session */
     int dco_keys_installed;
-    bool dco_peer_added;
+    /**
+     * This is the handle that DCO uses to identify this session with the
+     * kernel.
+     *
+     * We keep this separate as the normal peer_id can change during
+     * p2p NCP and we need to track the id that is really used.
+     */
+    int dco_peer_id;
 
     dco_context_t *dco;
 };