Move protocol option negotiation from push_prepare to new function

This clean ups the code and removes the surprising side effects
of preparing a push reply to also select protocol options.

We also remember if we have seen a push request without async
push. This improves reaction time if deferred auth is involved
like managment interface deferred auth.  The other benefit is
removing a number of ifdefs.

NOTE: this patch breaks asynchronous authentication (via plugins
and possibly also via management interface).  The next commit will
fix this.  This is understood and hereby documented, but the two
individual commits are much cleaner without trying to fix it here
or squash both together.

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

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 885cf126f9e6dc44d2770fc9b52dad8b5e5c149f..5c4370a87d94a35c93a7d75b79b46fc5cb8a8578 100644 (file)
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1123,8 +1123,8 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
         }
 
         /*
-         * Drop non-TLS packet if client-connect script/plugin has not
-         * yet succeeded.
+         * Drop non-TLS packet if client-connect script/plugin and cipher selection
+         * has not yet succeeded.
          */
         if (c->c2.context_auth != CAS_SUCCEEDED)
         {

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 818700c2ad8302716185230ae257f6690173ac78..5db32cc99efe7918ae362b2ff931155ffb0192b5 100644 (file)
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -824,8 +824,8 @@ multi_create_instance(struct multi_context *m, const struct mroute_addr *real)
     mi->did_cid_hash = true;
 #endif
 
-#ifdef ENABLE_ASYNC_PUSH
     mi->context.c2.push_request_received = false;
+#ifdef ENABLE_ASYNC_PUSH
     mi->inotify_watch = -1;
 #endif
 
@@ -1772,6 +1772,78 @@ multi_client_connect_setenv(struct multi_context *m,
     gc_free(&gc);
 }
 
+/**
+ * Calculates the options that depend on the client capabilities
+ * based on local options and available peer info
+ * - choosen cipher
+ * - peer id
+ */
+static void
+multi_client_set_protocol_options(struct context *c)
+{
+
+    const char *optstr = NULL;
+    struct tls_multi *tls_multi = c->c2.tls_multi;
+    const char *const peer_info = tls_multi->peer_info;
+    struct options *o = &c->options;
+
+    /* Send peer-id if client supports it */
+    optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL;
+    if (optstr)
+    {
+        int proto = 0;
+        int r = sscanf(optstr, "IV_PROTO=%d", &proto);
+        if ((r == 1) && (proto >= 2))
+        {
+            tls_multi->use_peer_id = true;
+        }
+    }
+
+    /* Select cipher if client supports Negotiable Crypto Parameters */
+    if (o->ncp_enabled)
+    {
+        /* if we have already created our key, we cannot *change* our own
+         * cipher -> so log the fact and push the "what we have now" cipher
+         * (so the client is always told what we expect it to use)
+         */
+        const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
+        if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+        {
+            msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
+                 "server has already generated data channel keys, "
+                 "re-sending previously negotiated cipher '%s'",
+                 o->ciphername );
+        }
+        else
+        {
+            /*
+             * Push the first cipher from --ncp-ciphers to the client that
+             * the client announces to be supporting.
+             */
+            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
+                                                    peer_info,
+                                                    tls_multi->remote_ciphername,
+                                                    &o->gc);
+
+            if (push_cipher)
+            {
+                o->ciphername = push_cipher;
+            }
+            else
+            {
+                struct gc_arena gc = gc_new();
+                const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
+                msg(M_INFO, "PUSH: No common cipher between server and client."
+                    "Expect this connection not to work. "
+                    "Server ncp-ciphers: '%s', client supported ciphers '%s'",
+                    o->ncp_ciphers, peer_ciphers);
+                gc_free(&gc);
+            }
+        }
+    }
+}
+
+
 /*
  * Called as soon as the SSL/TLS connection authenticates.
  *
@@ -2074,13 +2146,14 @@ script_failed:
         /* set context-level authentication flag */
         mi->context.c2.context_auth = CAS_SUCCEEDED;
 
-#ifdef ENABLE_ASYNC_PUSH
-        /* authentication complete, send push reply */
+        /* authentication complete, calculate dynamic client specific options */
+        multi_client_set_protocol_options(&mi->context);
+
+        /* send push reply if ready*/
         if (mi->context.c2.push_request_received)
         {
             process_incoming_push_request(&mi->context);
         }
-#endif
     }
     else
     {

diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index 4609af3e39f91d0041d5f972822d118810469137..a13088527aedf69f7f306511d0ff0e85fff8dfdb 100644 (file)
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -432,9 +432,7 @@ struct context_2
 #if P2MP
 
     /* --ifconfig endpoints to be pushed to client */
-#ifdef ENABLE_ASYNC_PUSH
     bool push_request_received;
-#endif
     bool push_ifconfig_defined;
     time_t sent_push_reply_expiry;
     in_addr_t push_ifconfig_local;

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 7fec2a3485bca8bd83f35759f963b5c5ab5403f7..8d3aa3963bac8f825439b54626d6a7752646a31c 100644 (file)
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -437,7 +437,7 @@ prepare_auth_token_push_reply(struct tls_multi *tls_multi, struct gc_arena *gc,
 }
 
 /**
- * Prepare push options, based on local options and available peer info.
+ * Prepare push options, based on local options
  *
  * @param context       context structure storing data for VPN tunnel
  * @param gc            gc arena for allocating push options
@@ -449,9 +449,7 @@ bool
 prepare_push_reply(struct context *c, struct gc_arena *gc,
                    struct push_list *push_list)
 {
-    const char *optstr = NULL;
     struct tls_multi *tls_multi = c->c2.tls_multi;
-    const char *const peer_info = tls_multi->peer_info;
     struct options *o = &c->options;
 
     /* ipv6 */
@@ -480,18 +478,10 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
                                         0, gc));
     }
 
-    /* Send peer-id if client supports it */
-    optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL;
-    if (optstr)
+    if (tls_multi->use_peer_id)
     {
-        int proto = 0;
-        int r = sscanf(optstr, "IV_PROTO=%d", &proto);
-        if ((r == 1) && (proto >= 2))
-        {
-            push_option_fmt(gc, push_list, M_USAGE, "peer-id %d",
-                            tls_multi->peer_id);
-            tls_multi->use_peer_id = true;
-        }
+        push_option_fmt(gc, push_list, M_USAGE, "peer-id %d",
+                        tls_multi->peer_id);
     }
     /*
      * If server uses --auth-gen-token and we have an auth token
@@ -499,47 +489,11 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
      */
     prepare_auth_token_push_reply(tls_multi, gc, push_list);
 
-    /* Push cipher if client supports Negotiable Crypto Parameters */
-    if (o->ncp_enabled)
-    {
-        /* if we have already created our key, we cannot *change* our own
-         * cipher -> so log the fact and push the "what we have now" cipher
-         * (so the client is always told what we expect it to use)
-         */
-        const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
-        if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
-        {
-            msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
-                 "server has already generated data channel keys, "
-                 "re-sending previously negotiated cipher '%s'",
-                 o->ciphername );
-        }
-        else
-        {
-            /*
-             * Push the first cipher from --ncp-ciphers to the client that
-             * the client announces to be supporting.
-             */
-            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
-                                                    peer_info,
-                                                    tls_multi->remote_ciphername,
-                                                    &o->gc);
-
-            if (push_cipher)
-            {
-                o->ciphername = push_cipher;
-            }
-            else
-            {
-                const char *peer_ciphers = tls_peer_ncp_list(peer_info, gc);
-                msg(M_INFO, "PUSH: No common cipher between server and client."
-                    "Expect this connection not to work. "
-                    "Server ncp-ciphers: '%s', client supported ciphers '%s'",
-                    o->ncp_ciphers, peer_ciphers);
-            }
-        }
-        push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
-    }
+    /*
+     * Push the selected cipher, at this point the cipher has been
+     * already negotiated and been fixed
+     */
+    push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
 
     return true;
 }
@@ -794,9 +748,7 @@ process_incoming_push_request(struct context *c)
 {
     int ret = PUSH_MSG_ERROR;
 
-#ifdef ENABLE_ASYNC_PUSH
     c->c2.push_request_received = true;
-#endif
     if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED)
     {
         const char *client_reason = tls_client_reason(c->c2.tls_multi);