[Openvpn-devel,v4,11/25] dco: split option parsing routines

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

Commit Message

Antonio Quartulli Aug. 2, 2022, 11:50 p.m. UTC
DCO will try to install keys upon generating them, however, this happens
when parsing pushed cipher options (due to NCP).

For this reason we need to postpone parsing pushed cipher options to *after*
the tunnel interface has been opened, otherwise we would have no DCO netdev
object to operate on.

At the same time we split the parsing code, so that we can ensure that
the NEW_PEER call can happen after the received peer-id has been parsed
(it is required by all DCO API calls).

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

Changes from v3:
* call do_deferred_options_part2() only if !pulled_options. This is the
  same condition that triggered the call to do_deferred_options() in the
  first place, therefore part2 must follow the same logic.
* add extra comment in do_up to explain "step2" after open_tun()

Changes from v2:
* rename finish_options() to do_deferred_options_part2()
* add comments to explain why this new function is required
* remove invocation in multi.c: we already perform key generation as
  last step

Changes from v1:
* removed error message in case of failure of finish_options(). The
  latter already warns the user about the failure - no need to print
  another generic message.
---
 src/openvpn/init.c | 81 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 25 deletions(-)

Comments

Gert Doering Aug. 3, 2022, 5:56 a.m. UTC | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

Subjected to the full set of server side tests that so nicely crashed
11/25 v2 :-) (passed) and to client Linux/FreeBSD t_client tests including
P2P NCP (passes as well).

The code looks good, and more correct as well.  Thanks for all the
comments and for renaming the function - if it helps my brain, future
contributors might benefit from it as well.

Just for reference: do_deferred_p2p_ncp() is all "part2" type stuff,
so it gets moved as a whole, not split.

NOTE: there is a small change in behaviour, which I'm not sure if 
problematic or not.  The "part2" behaviour was previously encapsulated
in

 if (c->options.pull)
 {
    ...
 }

and is no longer checking this in "_part2()".

This makes (as far as I can see) a difference if and only if a client
*without* --pull (-> c->options.pull) will receive a PUSH_REPLY from
a server that is behaving different from what we do - we only send
PUSH_REPLY in reply to a PUSH_REQUEST, or to IV_PROTO=IV_PROTO_REQUEST_PUSH
(and that is only happening if --pull / --client is set on the client).

In the server instance - which is called from multi.c - this would
make a difference *if* we called _part2() from multi.c - but we do not(!),
as that code path calls multi_client_generate_tls_keys(), which does
the tls_session_update_crypto_params() for us...  so, server would
not call it before (!options.pull) and doesn't call it now (no call
to _part2()) -> no change here.


Your patch has been applied to the master branch.

commit 6b9f4d71d859c27a9a71699aa899c9a2c5c3b680
Author: Antonio Quartulli
Date:   Wed Aug 3 11:50:12 2022 +0200

     dco: split option parsing routines

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 338d797b..de8faeb4 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2078,6 +2078,37 @@  options_hash_changed_or_zero(const struct sha256_digest *a,
            || !memcmp(a, &zero, sizeof(struct sha256_digest));
 }
 
+/**
+ * This function is expected to be invoked after open_tun() was performed.
+ *
+ * This kind of behaviour is required by DCO, because the following operations
+ * can be done only after the DCO device was created and the new peer was
+ * properly added.
+ */
+static bool
+do_deferred_options_part2(struct context *c)
+{
+    struct frame *frame_fragment = NULL;
+#ifdef ENABLE_FRAGMENT
+    if (c->options.ce.fragment)
+    {
+        frame_fragment = &c->c2.frame_fragment;
+    }
+#endif
+
+    struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
+    if (!tls_session_update_crypto_params(c->c2.tls_multi, session,
+                                          &c->options, &c->c2.frame,
+                                          frame_fragment,
+                                          get_link_socket_info(c)))
+    {
+        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
+        return false;
+    }
+
+    return true;
+}
+
 bool
 do_up(struct context *c, bool pulled_options, unsigned int option_types_found)
 {
@@ -2093,14 +2124,6 @@  do_up(struct context *c, bool pulled_options, unsigned int option_types_found)
                 return false;
             }
         }
-        else if (c->mode == MODE_POINT_TO_POINT)
-        {
-            if (!do_deferred_p2p_ncp(c))
-            {
-                msg(D_TLS_ERRORS, "ERROR: Failed to apply P2P negotiated protocol options");
-                return false;
-            }
-        }
 
         /* if --up-delay specified, open tun, do ifconfig, and run up script now */
         if (c->options.up_delay || PULL_DEFINED(&c->options))
@@ -2127,6 +2150,31 @@  do_up(struct context *c, bool pulled_options, unsigned int option_types_found)
             }
         }
 
+        /* do_deferred_options_part2() and do_deferred_p2p_ncp() *must* be
+         * invoked after open_tun().
+         * This is required by DCO because we must have created the interface
+         * and added the peer before we can fiddle with the keys or any other
+         * data channel per-peer setting.
+         */
+        if (pulled_options)
+        {
+            if (!do_deferred_options_part2(c))
+            {
+                return false;
+            }
+        }
+        else
+        {
+            if (c->mode == MODE_POINT_TO_POINT)
+            {
+                if (!do_deferred_p2p_ncp(c))
+                {
+                    msg(D_TLS_ERRORS, "ERROR: Failed to apply P2P negotiated protocol options");
+                    return false;
+                }
+            }
+        }
+
         if (c->c2.did_open_tun)
         {
             c->c1.pulled_options_digest_save = c->c2.pulled_options_digest;
@@ -2332,23 +2380,6 @@  do_deferred_options(struct context *c, const unsigned int found)
         {
             return false;
         }
-        struct frame *frame_fragment = NULL;
-#ifdef ENABLE_FRAGMENT
-        if (c->options.ce.fragment)
-        {
-            frame_fragment = &c->c2.frame_fragment;
-        }
-#endif
-
-        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
-        if (!tls_session_update_crypto_params(c->c2.tls_multi, session,
-                                              &c->options, &c->c2.frame,
-                                              frame_fragment,
-                                              get_link_socket_info(c)))
-        {
-            msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
-            return false;
-        }
     }
 
     return true;