Allow changing cipher from a ccd file

PATCH Superseded Steffan Karger 3 participants 6 messages Jan 24, 2017 – Jul 21, 2020 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]...

Steffan Karger

author

2017-01-24 20:38 UTC #1 lore raw

PATCH 1/1 Allow changing cipher from a ccd file

As described in msg <[email protected]...> on [email protected], clients that are compiled with --disable-occ (included in --enable-small) won't send an options string. Without the options string, the 2.4 server doesn't know which cipher to use for poor man's NCP.

This patch allows working around that issue by allowing the 'cipher' directive to be used in --client-config-dir files. That way, a server admin can add ccd files to specify per-client which cipher to use.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/options.c | 2 +-
 src/openvpn/options.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

src/openvpn/options.c +1 -1

index 4b6d720..6f89616 100644
@@ -7536,7 +7536,7 @@ add_option(struct options *options,
     }
     else if (streq(p[0], "cipher") && p[1] && !p[2])
     {
-        VERIFY_PERMISSION(OPT_P_NCP);
+        VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
         options->ciphername = p[1];
     }
     else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])

src/openvpn/options.h +1 -1

index a14f2ab..f4f0226 100644
@@ -628,7 +628,7 @@ struct options
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
 #define OPT_P_PUSH            (1<<16)
-#define OPT_P_INSTANCE        (1<<17)
+#define OPT_P_INSTANCE        (1<<17) /**< Allow usage in ccd file */
 #define OPT_P_CONFIG          (1<<18)
 #define OPT_P_EXPLICIT_NOTIFY (1<<19)
 #define OPT_P_ECHO            (1<<20)

Signature · 1 line

2.7.4

Steffan Karger

author

2017-02-17 15:20 UTC #2 lore raw

PATCH v2 1/1 Allow changing cipher from a ccd file

Because the ccd files are read after where we would normally generate keys, this patch delays key generation for non-NCP p2mp servers until after reading the ccd file.

Trac: #845

Signed-off-by Steffan Karger <[email protected]...>

---
v2: postpone p2mp non-NCP key generation, such that setting cipher in
a ccd file for a non-NCP client actually works.

 src/openvpn/multi.c   | 14 ++++++++++++++
 src/openvpn/options.c |  2 +-
 src/openvpn/options.h |  2 +-
 src/openvpn/ssl.c     |  9 ++++-----
 4 files changed, 20 insertions(+), 7 deletions(-)

src/openvpn/multi.c +14 -0

index 56009b7..4c81e9a 100644
@@ -2086,6 +2086,20 @@ script_failed:
             mi->context.c2.context_auth = cc_succeeded_count ? CAS_PARTIAL : CAS_FAILED;
         }
 
+        /* Generate tunnel keys, unless IV_NCP >= 2 is negotiated. The first key
+         * generation is then postponed until after the pull/push, so we can
+         * process pushed cipher directives.
+         */
+        struct tls_session *session = &mi->context.c2.tls_multi->session[TM_ACTIVE];
+        struct key_state *ks = &session->key[KS_PRIMARY];
+        if (!session->opt->ncp_enabled && ks->authenticated
+            && !tls_session_update_crypto_params(session, &mi->context.options,
+                                                 &mi->context.c2.frame))
+        {
+            msg(D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+            cc_succeeded = false;
+        }
+
         /* set flag so we don't get called again */
         mi->connection_established_flag = true;

src/openvpn/options.c +1 -1

index dde1f48..0e6b393 100644
@@ -7536,7 +7536,7 @@ add_option(struct options *options,
     }
     else if (streq(p[0], "cipher") && p[1] && !p[2])
     {
-        VERIFY_PERMISSION(OPT_P_NCP);
+        VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
         options->ciphername = p[1];
     }
     else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])

src/openvpn/options.h +1 -1

index a14f2ab..f4f0226 100644
@@ -628,7 +628,7 @@ struct options
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
 #define OPT_P_PUSH            (1<<16)
-#define OPT_P_INSTANCE        (1<<17)
+#define OPT_P_INSTANCE        (1<<17) /**< Allow usage in ccd file */
 #define OPT_P_CONFIG          (1<<18)
 #define OPT_P_EXPLICIT_NOTIFY (1<<19)
 #define OPT_P_ECHO            (1<<20)

src/openvpn/ssl.c +4 -5

index 8c724cb..1479c77 100644
@@ -2401,12 +2401,11 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
     }
 
     /* Generate tunnel keys if we're a TLS server.
-     * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key
-     * generation is postponed until after the pull/push, so we can process pushed
-     * cipher directives.
+     * If we're a p2mp server, the first key generation is postponed so we can
+     * switch cipher during the connection setup phase.
      */
-    if (session->opt->server && !(session->opt->ncp_enabled
-                                  && session->opt->mode == MODE_SERVER && ks->key_id <= 0))
+    if (session->opt->server
+        && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0))
     {
         if (ks->authenticated)
         {

Signature · 1 line

2.7.4

Steffan Karger

2017-03-28 08:08 UTC #3 lore raw

Hi,

Quoted 90 lines · Steffan Karger

Because the ccd files are read after where we would normally generate keys, this patch delays key generation for non-NCP p2mp servers until after reading the ccd file.

Trac: #845

Signed-off-by Steffan Karger <[email protected]...>

---
v2: postpone p2mp non-NCP key generation, such that setting cipher in
a ccd file for a non-NCP client actually works.

 src/openvpn/multi.c   | 14 ++++++++++++++
 src/openvpn/options.c |  2 +-
 src/openvpn/options.h |  2 +-
 src/openvpn/ssl.c     |  9 ++++-----
 4 files changed, 20 insertions(+), 7 deletions(-)

src/openvpn/multi.c +14 -0

index 56009b7..4c81e9a 100644
@@ -2086,6 +2086,20 @@ script_failed:
             mi->context.c2.context_auth = cc_succeeded_count ? CAS_PARTIAL : CAS_FAILED;
         }
 
+        /* Generate tunnel keys, unless IV_NCP >= 2 is negotiated. The first key
+         * generation is then postponed until after the pull/push, so we can
+         * process pushed cipher directives.
+         */
+        struct tls_session *session = &mi->context.c2.tls_multi->session[TM_ACTIVE];
+        struct key_state *ks = &session->key[KS_PRIMARY];
+        if (!session->opt->ncp_enabled && ks->authenticated
+            && !tls_session_update_crypto_params(session, &mi->context.options,
+                                                 &mi->context.c2.frame))
+        {
+            msg(D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+            cc_succeeded = false;
+        }
+
         /* set flag so we don't get called again */
         mi->connection_established_flag = true;

src/openvpn/options.c +1 -1

index dde1f48..0e6b393 100644
@@ -7536,7 +7536,7 @@ add_option(struct options *options,
     }
     else if (streq(p[0], "cipher") && p[1] && !p[2])
     {
-        VERIFY_PERMISSION(OPT_P_NCP);
+        VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
         options->ciphername = p[1];
     }
     else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])

src/openvpn/options.h +1 -1

index a14f2ab..f4f0226 100644
@@ -628,7 +628,7 @@ struct options
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
 #define OPT_P_PUSH            (1<<16)
-#define OPT_P_INSTANCE        (1<<17)
+#define OPT_P_INSTANCE        (1<<17) /**< Allow usage in ccd file */
 #define OPT_P_CONFIG          (1<<18)
 #define OPT_P_EXPLICIT_NOTIFY (1<<19)
 #define OPT_P_ECHO            (1<<20)

src/openvpn/ssl.c +0 -3

index 8c724cb..1479c77 100644
@@ -2401,12 +2401,11 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
     }
 
     /* Generate tunnel keys if we're a TLS server.
-     * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key
-     * generation is postponed until after the pull/push, so we can process pushed
-     * cipher directives.

src/openvpn/ssl.c key_method_2_write(struct buffer *buf, struct tls_session *session) · line 2411

Quoted 10 lines

src/openvpn/ssl.c +4 -2

+     * If we're a p2mp server, the first key generation is postponed so we can
+     * switch cipher during the connection setup phase.
      */
-    if (session->opt->server && !(session->opt->ncp_enabled
-                                  && session->opt->mode == MODE_SERVER && ks->key_id <= 0))
+    if (session->opt->server
+        && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0))
     {
         if (ks->authenticated)
         {

This patch seems to work for the reporter from #845:
https://community.openvpn.net/openvpn/ticket/845#comment:5

-Steffan

(Yes, this is a shameless bump to trick people into reviewing this patch ;-) )

Steffan Karger

author

2018-03-08 19:47 UTC #4 lore raw

PATCH 1/1 Allow changing cipher from a ccd file

In reply to Steffan Karger

Because the ccd files are read after where we would normally generate keys, this patch delays key generation for non-NCP p2mp servers until after reading the ccd file.

Thanks to a sharp review comment from Gert, it turns out that multi_connections_establised() is actually a better place than incoming_push_message() to do the server-side NCP cipher selection and key generation. Doing it here removes the need for handling with multiple received push requests. Technically, the check for .authenticated before generating keys should even not be necessary, but I think it's good to leave it in as a double-check to prevent future mistakes.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: postpone p2mp non-NCP key generation, such that setting cipher in
a ccd file for a non-NCP client actually works.
v3: merge all server-side NCP key generation to the new place.

 src/openvpn/multi.c   | 45 +++++++++++++++++++++++++++++++++------------
 src/openvpn/options.c |  2 +-
 src/openvpn/options.h |  2 +-
 src/openvpn/push.c    | 22 ----------------------
 src/openvpn/ssl.c     |  9 ++++-----
 5 files changed, 39 insertions(+), 41 deletions(-)

src/openvpn/multi.c +33 ~~-12~~

index 6a72a1dc..77dd6724 100644
@@ -1786,6 +1786,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
     {
         struct gc_arena gc = gc_new();
         unsigned int option_types_found = 0;
+        struct options *options = &mi->context.options;
 
         const unsigned int option_permissions_mask =
             OPT_P_INSTANCE

@@ -1810,7 +1811,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
         generate_prefix(mi);
 
         /* delete instances of previous clients with same common-name */
-        if (!mi->context.options.duplicate_cn)
+        if (!options->duplicate_cn)
         {
             multi_delete_dup(m, mi);
         }

@@ -1822,11 +1823,11 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
          * Try to source a dynamic config file from the
          * --client-config-dir directory.
          */
-        if (mi->context.options.client_config_dir)
+        if (options->client_config_dir)
         {
             const char *ccd_file;
 
-            ccd_file = gen_path(mi->context.options.client_config_dir,
+            ccd_file = gen_path(options->client_config_dir,
                                 tls_common_name(mi->context.c2.tls_multi, false),
                                 &gc);

@@ -1842,7 +1843,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
             }
             else /* try default file */
             {
-                ccd_file = gen_path(mi->context.options.client_config_dir,
+                ccd_file = gen_path(options->client_config_dir,
                                     CCD_DEFAULT,
                                     &gc);

@@ -1876,7 +1877,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
         if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
         {
             struct argv argv = argv_new();
-            const char *dc_file = create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
+            const char *dc_file = create_temp_file(options->tmp_dir, "cc", &gc);
 
             if (!dc_file)
             {

@@ -1931,21 +1932,21 @@ script_depr_failed:
         /*
          * Run --client-connect script.
          */
-        if (mi->context.options.client_connect_script && cc_succeeded)
+        if (options->client_connect_script && cc_succeeded)
         {
             struct argv argv = argv_new();
             const char *dc_file = NULL;
 
             setenv_str(mi->context.c2.es, "script_type", "client-connect");
 
-            dc_file = create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
+            dc_file = create_temp_file(options->tmp_dir, "cc", &gc);
             if (!dc_file)
             {
                 cc_succeeded = false;
                 goto script_failed;
             }
 
-            argv_parse_cmd(&argv, mi->context.options.client_connect_script);
+            argv_parse_cmd(&argv, options->client_connect_script);
             argv_printf_cat(&argv, "%s", dc_file);
 
             if (openvpn_run_script(&argv, mi->context.c2.es, 0, "--client-connect"))

@@ -1983,13 +1984,33 @@ script_failed:
          * Check for "disable" directive in client-config-dir file
          * or config file generated by --client-connect script.
          */
-        if (mi->context.options.disable)
+        if (options->disable)
         {
             msg(D_MULTI_ERRORS, "MULTI: client has been rejected due to 'disable' directive");
             cc_succeeded = false;
             cc_succeeded_count = 0;
         }
 
+        /* Determine data channel cipher and generate data channel keys */
+        struct tls_multi *multi = mi->context.c2.tls_multi;
+        if (tls_peer_info_ncp_ver(multi->peer_info) >= 2 && options->ncp_enabled)
+        {
+            /* Select the first cipher from --ncp-ciphers for the data channel.
+             * TODO: actual negotiation, instead of server dictatorship. */
+            char *push_cipher = string_alloc(options->ncp_ciphers, &options->gc);
+            options->ciphername = strtok(push_cipher, ":");
+        }
+
+        struct tls_session *session = &multi->session[TM_ACTIVE];
+        if (!session->key[KS_PRIMARY].authenticated
+            || !tls_session_update_crypto_params(session, &mi->context.options,
+                                                 &mi->context.c2.frame))
+        {
+            msg(D_TLS_ERRORS, "TLS Error: updating crypto parameters failed");
+            cc_succeeded = false;
+            cc_succeeded_count = 0;
+        }
+
         if (cc_succeeded)
         {
             /*

@@ -2015,8 +2036,8 @@ script_failed:
                 msg(D_MULTI_ERRORS, "MULTI ERROR: primary virtual IP for %s (%s) violates tunnel network/netmask constraint (%s/%s)",
                     multi_instance_string(mi, false, &gc),
                     print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc),
-                    print_in_addr_t(mi->context.options.push_ifconfig_constraint_network, 0, &gc),
-                    print_in_addr_t(mi->context.options.push_ifconfig_constraint_netmask, 0, &gc));
+                    print_in_addr_t(options->push_ifconfig_constraint_network, 0, &gc),
+                    print_in_addr_t(options->push_ifconfig_constraint_netmask, 0, &gc));
             }
 
             /*

@@ -2053,7 +2074,7 @@ script_failed:
                  */
                 remove_iroutes_from_push_route_list(&mi->context.options);
             }
-            else if (mi->context.options.iroutes)
+            else if (options->iroutes)
             {
                 msg(D_MULTI_ERRORS, "MULTI: --iroute options rejected for %s -- iroute only works with tun-style tunnels",
                     multi_instance_string(mi, false, &gc));

src/openvpn/options.c +1 -1

index 587f73ad..5443b204 100644
@@ -7505,7 +7505,7 @@ add_option(struct options *options,
     }
     else if (streq(p[0], "cipher") && p[1] && !p[2])
     {
-        VERIFY_PERMISSION(OPT_P_NCP);
+        VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
         options->ciphername = p[1];
     }
     else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])

src/openvpn/options.h +1 -1

index 636bb8db..565b3697 100644
@@ -624,7 +624,7 @@ struct options
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
 #define OPT_P_PUSH            (1<<16)
-#define OPT_P_INSTANCE        (1<<17)
+#define OPT_P_INSTANCE        (1<<17) /**< Allow usage in ccd file */
 #define OPT_P_CONFIG          (1<<18)
 #define OPT_P_EXPLICIT_NOTIFY (1<<19)
 #define OPT_P_ECHO            (1<<20)

src/openvpn/push.c +0 ~~-22~~

index fee3eaab..d58d7119 100644
@@ -270,21 +270,6 @@ incoming_push_message(struct context *c, const struct buffer *buffer)
         }
         event_timeout_clear(&c->c2.push_request_interval);
     }
-    else if (status == PUSH_MSG_REQUEST)
-    {
-        if (c->options.mode == MODE_SERVER)
-        {
-            struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
-            /* Do not regenerate keys if client send a second push request */
-            if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
-                && !tls_session_update_crypto_params(session, &c->options,
-                                                     &c->c2.frame))
-            {
-                msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed");
-                goto error;
-            }
-        }
-    }
 
     goto cleanup;
 error:

@@ -384,13 +369,6 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
                  "re-sending previously negotiated cipher '%s'",
                  o->ciphername );
         }
-        else
-        {
-            /* Push the first cipher from --ncp-ciphers to the client.
-             * TODO: actual negotiation, instead of server dictatorship. */
-            char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
-            o->ciphername = strtok(push_cipher, ":");
-        }
         push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
     }
     else if (o->ncp_enabled)

src/openvpn/ssl.c +4 -5

index 21a29195..48de2e32 100644
@@ -2426,12 +2426,11 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
     }
 
     /* Generate tunnel keys if we're a TLS server.
-     * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key
-     * generation is postponed until after the pull/push, so we can process pushed
-     * cipher directives.
+     * If we're a p2mp server, the first key generation is postponed so we can
+     * switch cipher during the connection setup phase.
      */
-    if (session->opt->server && !(session->opt->ncp_enabled
-                                  && session->opt->mode == MODE_SERVER && ks->key_id <= 0))
+    if (session->opt->server
+        && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0))
     {
         if (ks->authenticated)
         {

Signature · 1 line

2.15.1

Steffan Karger

author

2018-08-12 08:51 UTC #5 lore raw

PATCH v4 1/1 Allow changing cipher from a ccd file

Because the ccd files are read after where we would normally generate keys, this patch delays key generation for non-NCP p2mp servers until after reading the ccd file.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: postpone p2mp non-NCP key generation, such that setting cipher in
a ccd file for a non-NCP client actually works.
v3: merge all server-side NCP key generation to the new place.
v4: rebase on current master

 src/openvpn/multi.c   | 55 +++++++++++++++++++++++++++++--------------
 src/openvpn/options.c |  2 +-
 src/openvpn/options.h |  2 +-
 src/openvpn/push.c    | 22 -----------------
 src/openvpn/ssl.c     |  9 ++++---
 5 files changed, 43 insertions(+), 47 deletions(-)

src/openvpn/multi.c +37 ~~-18~~

index db32500d..d7a95324 100644
@@ -1789,6 +1789,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
     {
         struct gc_arena gc = gc_new();
         unsigned int option_types_found = 0;
+        struct options *options = &mi->context.options;
 
         const unsigned int option_permissions_mask =
             OPT_P_INSTANCE

@@ -1813,7 +1814,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
         generate_prefix(mi);
 
         /* delete instances of previous clients with same common-name */
-        if (!mi->context.options.duplicate_cn)
+        if (!options->duplicate_cn)
         {
             multi_delete_dup(m, mi);
         }

@@ -1825,14 +1826,13 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
          * Try to source a dynamic config file from the
          * --client-config-dir directory.
          */
-        if (mi->context.options.client_config_dir)
+        if (options->client_config_dir)
         {
             const char *ccd_file;
 
-            ccd_file = platform_gen_path(mi->context.options.client_config_dir,
-                                         tls_common_name(mi->context.c2.tls_multi,
-                                                         false),
-                                         &gc);
+            ccd_file = platform_gen_path(options->client_config_dir,
+                                tls_common_name(mi->context.c2.tls_multi, false),
+                                &gc);
 
             /* try common-name file */
             if (platform_test_file(ccd_file))

@@ -1846,9 +1846,9 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
             }
             else /* try default file */
             {
-                ccd_file = platform_gen_path(mi->context.options.client_config_dir,
-                                             CCD_DEFAULT,
-                                             &gc);
+                ccd_file = platform_gen_path(options->client_config_dir,
+                                    CCD_DEFAULT,
+                                    &gc);
 
                 if (platform_test_file(ccd_file))
                 {

@@ -1880,7 +1880,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
         if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
         {
             struct argv argv = argv_new();
-            const char *dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
+            const char *dc_file = platform_create_temp_file(options->tmp_dir,
                                                             "cc", &gc);
 
             if (!dc_file)

@@ -1936,22 +1936,21 @@ script_depr_failed:
         /*
          * Run --client-connect script.
          */
-        if (mi->context.options.client_connect_script && cc_succeeded)
+        if (options->client_connect_script && cc_succeeded)
         {
             struct argv argv = argv_new();
             const char *dc_file = NULL;
 
             setenv_str(mi->context.c2.es, "script_type", "client-connect");
 
-            dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
-                                                "cc", &gc);
+            dc_file = platform_create_temp_file(options->tmp_dir, "cc", &gc);
             if (!dc_file)
             {
                 cc_succeeded = false;
                 goto script_failed;
             }
 
-            argv_parse_cmd(&argv, mi->context.options.client_connect_script);
+            argv_parse_cmd(&argv, options->client_connect_script);
             argv_printf_cat(&argv, "%s", dc_file);
 
             if (openvpn_run_script(&argv, mi->context.c2.es, 0, "--client-connect"))

@@ -1989,13 +1988,33 @@ script_failed:
          * Check for "disable" directive in client-config-dir file
          * or config file generated by --client-connect script.
          */
-        if (mi->context.options.disable)
+        if (options->disable)
         {
             msg(D_MULTI_ERRORS, "MULTI: client has been rejected due to 'disable' directive");
             cc_succeeded = false;
             cc_succeeded_count = 0;
         }
 
+        /* Determine data channel cipher and generate data channel keys */
+        struct tls_multi *multi = mi->context.c2.tls_multi;
+        if (tls_peer_info_ncp_ver(multi->peer_info) >= 2 && options->ncp_enabled)
+        {
+            /* Select the first cipher from --ncp-ciphers for the data channel.
+             * TODO: actual negotiation, instead of server dictatorship. */
+            char *push_cipher = string_alloc(options->ncp_ciphers, &options->gc);
+            options->ciphername = strtok(push_cipher, ":");
+        }
+
+        struct tls_session *session = &multi->session[TM_ACTIVE];
+        if (!session->key[KS_PRIMARY].authenticated
+            || !tls_session_update_crypto_params(session, &mi->context.options,
+                                                 &mi->context.c2.frame))
+        {
+            msg(D_TLS_ERRORS, "TLS Error: updating crypto parameters failed");
+            cc_succeeded = false;
+            cc_succeeded_count = 0;
+        }
+
         if (cc_succeeded)
         {
             /*

@@ -2021,8 +2040,8 @@ script_failed:
                 msg(D_MULTI_ERRORS, "MULTI ERROR: primary virtual IP for %s (%s) violates tunnel network/netmask constraint (%s/%s)",
                     multi_instance_string(mi, false, &gc),
                     print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc),
-                    print_in_addr_t(mi->context.options.push_ifconfig_constraint_network, 0, &gc),
-                    print_in_addr_t(mi->context.options.push_ifconfig_constraint_netmask, 0, &gc));
+                    print_in_addr_t(options->push_ifconfig_constraint_network, 0, &gc),
+                    print_in_addr_t(options->push_ifconfig_constraint_netmask, 0, &gc));
             }
 
             /*

@@ -2059,7 +2078,7 @@ script_failed:
                  */
                 remove_iroutes_from_push_route_list(&mi->context.options);
             }
-            else if (mi->context.options.iroutes)
+            else if (options->iroutes)
             {
                 msg(D_MULTI_ERRORS, "MULTI: --iroute options rejected for %s -- iroute only works with tun-style tunnels",
                     multi_instance_string(mi, false, &gc));

src/openvpn/options.c +1 -1

index 61fa9833..baf0b706 100644
@@ -7530,7 +7530,7 @@ add_option(struct options *options,
     }
     else if (streq(p[0], "cipher") && p[1] && !p[2])
     {
-        VERIFY_PERMISSION(OPT_P_NCP);
+        VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
         options->ciphername = p[1];
     }
     else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])

src/openvpn/options.h +1 -1

index acbd1087..67a9abea 100644
@@ -635,7 +635,7 @@ struct options
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
 #define OPT_P_PUSH            (1<<16)
-#define OPT_P_INSTANCE        (1<<17)
+#define OPT_P_INSTANCE        (1<<17) /**< Allow usage in ccd file */
 #define OPT_P_CONFIG          (1<<18)
 #define OPT_P_EXPLICIT_NOTIFY (1<<19)
 #define OPT_P_ECHO            (1<<20)

src/openvpn/push.c +0 ~~-22~~

index a7ec4dd6..45818010 100644
@@ -270,21 +270,6 @@ incoming_push_message(struct context *c, const struct buffer *buffer)
         }
         event_timeout_clear(&c->c2.push_request_interval);
     }
-    else if (status == PUSH_MSG_REQUEST)
-    {
-        if (c->options.mode == MODE_SERVER)
-        {
-            struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
-            /* Do not regenerate keys if client send a second push request */
-            if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
-                && !tls_session_update_crypto_params(session, &c->options,
-                                                     &c->c2.frame))
-            {
-                msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed");
-                goto error;
-            }
-        }
-    }
 
     goto cleanup;
 error:

@@ -385,13 +370,6 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
                  "re-sending previously negotiated cipher '%s'",
                  o->ciphername );
         }
-        else
-        {
-            /* Push the first cipher from --ncp-ciphers to the client.
-             * TODO: actual negotiation, instead of server dictatorship. */
-            char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
-            o->ciphername = strtok(push_cipher, ":");
-        }
         push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
     }
     else if (o->ncp_enabled)

src/openvpn/ssl.c +4 -5

index dcb54456..b78610d7 100644
@@ -2419,12 +2419,11 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
     }
 
     /* Generate tunnel keys if we're a TLS server.
-     * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key
-     * generation is postponed until after the pull/push, so we can process pushed
-     * cipher directives.
+     * If we're a p2mp server, the first key generation is postponed so we can
+     * switch cipher during the connection setup phase.
      */
-    if (session->opt->server && !(session->opt->ncp_enabled
-                                  && session->opt->mode == MODE_SERVER && ks->key_id <= 0))
+    if (session->opt->server
+        && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0))
     {
         if (ks->authenticated)
         {

Signature · 1 line

2.17.1

Gert Doering

2020-07-21 15:07 UTC #6 lore raw

Hi,

Quoted 5 lines · Steffan Karger

As described in msg <[email protected]...> on [email protected], clients that are compiled with --disable-occ (included in --enable-small) won't send an options string. Without the options string, the 2.4 server doesn't know which cipher to use for poor man's NCP.

Just for the record: we do not need this patch anymore, as Arne's magic refactoring work has made adding this functionality very trivial ("just permit cipher in ccd/ files") - and that one has been merged already.

I just found this one in patchwork and thought a bit of commenting would help transparency :-)

gert

Signature · 6 lines

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...