diff mbox series

[Openvpn-devel,v2,1/9] Implement optional cipher in --data-ciphers prefixed with ?

Message ID 20211206005740.3072232-1-arne@rfc2549.org
State Changes Requested
Headers
Series [Openvpn-devel,v2,1/9] Implement optional cipher in --data-ciphers prefixed with ? |

Commit Message

Arne Schwabe Dec. 5, 2021, 1:57 p.m. UTC 
  This allows to use the same configuration multiple platforms/ssl libraries
and include optional algorithms that are not available on all platforms

For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to
emulate the default behaviour of OpenVPN 2.6.

Patch v2: fix error_found reset by optional cipher, fix typo in Changes.rst

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl_ncp.c               | 8 ++++++--
 tests/unit_tests/openvpn/test_ncp.c | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Gert Doering Dec. 5, 2021, 8:06 p.m. UTC | #1 
Hi,

On Mon, Dec 06, 2021 at 01:57:40AM +0100, Arne Schwabe wrote:
> This allows to use the same configuration multiple platforms/ssl libraries
> and include optional algorithms that are not available on all platforms
> 
> For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to
> emulate the default behaviour of OpenVPN 2.6.
> 
> Patch v2: fix error_found reset by optional cipher, fix typo in Changes.rst
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
[..]
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index 2f1df1c52..2561daab2 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -134,7 +134,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>          {
>              const char* optstr = optional ? "optional ": "";
>              msg(M_WARN, "Unsupported %scipher in --data-ciphers: %s", optstr, token);
> -            error_found = !optional;
> +            error_found = error_found || !optional;
>          }

This looks good now, but the rest of the patch got merge-conflicted
to death, I'm afraid.  The actual "optional" part got lost (token++)...

>          else
>          {
> @@ -448,6 +448,10 @@ p2p_ncp_set_options(struct tls_multi *multi, struct tls_session *session)
>  
>          }
>      }
> +    if (iv_proto_peer & IV_PROTO_RENOG_DYN_TLSCRYPT)
> +    {
> +        session->opt->crypto_flags |= CO_USE_DYN_TLSCRYPT_RENOG;
> +    }
>  #endif
>  }

... and this is unrelated...

> @@ -491,4 +495,4 @@ p2p_mode_ncp(struct tls_multi *multi, struct tls_session *session)
>          multi->use_peer_id, multi->peer_id, common_cipher);
>  
>      gc_free(&gc);
> -}
> \ No newline at end of file
> +}

As is this.

> diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
> index faf09a36c..6702133ad 100644
> --- a/tests/unit_tests/openvpn/test_ncp.c
> +++ b/tests/unit_tests/openvpn/test_ncp.c
> @@ -95,6 +95,9 @@ test_check_ncp_ciphers_list(void **state)
>      /* All unsupported should still yield an empty list */
>      assert_ptr_equal(mutate_ncp_cipher_list("?kugelfisch:?grasshopper", &gc), NULL);
>  
> +    /* If the last is optional, previous invalid ciphers should be ignored */
> +    assert_ptr_equal(mutate_ncp_cipher_list("Vollbit:Littlebit:AES-256-CBC:BF-CBC:?nixbit", &gc), NULL);
> +
>      /* For testing that with OpenSSL 1.1.0+ that also accepts ciphers in
>       * a different spelling the normalised cipher output is the same */
>      bool have_chacha_mixed_case = cipher_kt_get("ChaCha20-Poly1305");

This one also seems to be "an extra change on top of 1/9 v1" (grasshopper),
not "1/9 v2".

gert
diff mbox series

Patch

diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 2f1df1c52..2561daab2 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -134,7 +134,7 @@  mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
         {
             const char* optstr = optional ? "optional ": "";
             msg(M_WARN, "Unsupported %scipher in --data-ciphers: %s", optstr, token);
-            error_found = !optional;
+            error_found = error_found || !optional;
         }
         else
         {
@@ -448,6 +448,10 @@  p2p_ncp_set_options(struct tls_multi *multi, struct tls_session *session)
 
         }
     }
+    if (iv_proto_peer & IV_PROTO_RENOG_DYN_TLSCRYPT)
+    {
+        session->opt->crypto_flags |= CO_USE_DYN_TLSCRYPT_RENOG;
+    }
 #endif
 }
 
@@ -491,4 +495,4 @@  p2p_mode_ncp(struct tls_multi *multi, struct tls_session *session)
         multi->use_peer_id, multi->peer_id, common_cipher);
 
     gc_free(&gc);
-}
\ No newline at end of file
+}
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index faf09a36c..6702133ad 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -95,6 +95,9 @@  test_check_ncp_ciphers_list(void **state)
     /* All unsupported should still yield an empty list */
     assert_ptr_equal(mutate_ncp_cipher_list("?kugelfisch:?grasshopper", &gc), NULL);
 
+    /* If the last is optional, previous invalid ciphers should be ignored */
+    assert_ptr_equal(mutate_ncp_cipher_list("Vollbit:Littlebit:AES-256-CBC:BF-CBC:?nixbit", &gc), NULL);
+
     /* For testing that with OpenSSL 1.1.0+ that also accepts ciphers in
      * a different spelling the normalised cipher output is the same */
     bool have_chacha_mixed_case = cipher_kt_get("ChaCha20-Poly1305");