[M] Change in openvpn[master]: Do not differentiate TLS server and client context initialisation

Attention is currently required from: plaisthos.

Hello flichtenheld, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1728?usp=email

to look at the new patch set (#5).

The change is no longer submittable: checks~ChecksSubmitRule is unsatisfied now.


Change subject: Do not differentiate TLS server and client context initialisation
......................................................................

Do not differentiate TLS server and client context initialisation

OpenSSL has the quite curious way of allowing to create contexts
that allow only server or only client. This creates extra
complications when we want to use both server and client SSL
objects and does not seem to have any advantages.

We later explicitly tell OpenSSL to initialise the SSL objects
to be a server or client object in key_state_ssl_init via
SSL_set_accept_state or SSL_set_connect_state. If this is
mismatched we end up getting an error from OpenSSL ("called a
function you should not call") that ends up calling a function
that is not defined in that TLS_method.

Looking into the OpenSSL source (IMPLEMENT_tls_meth_func) the main
difference between the methods is whether they have a proper
accept/connect or have the ssl_undefined_function that triggers the
"called a function you should not call".

Our mbed TLS code basically does give the SSL context any personality
of client or server until we are in the same area in which the OpenSSL
code calls SSL_set_accept_state/SSL_set_connect_sate.

This also modifies the mbedTLS to use the decision to pick client
and server in that path as the OpenSSL code path.

Change-Id: Iaf1f3475e4f27a920c028cd73b1a2497953583d0
Signed-off-by: Arne Schwabe <[email protected]>
---
M src/openvpn/ssl.c
M src/openvpn/ssl_backend.h
M src/openvpn/ssl_mbedtls.c
M src/openvpn/ssl_mbedtls.h
M src/openvpn/ssl_openssl.c
M tests/unit_tests/openvpn/test_ssl.c
6 files changed, 17 insertions(+), 65 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/28/1728/5

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 9f0052e..22b3f48 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -525,18 +525,11 @@
     struct tls_root_ctx *new_ctx;
     ALLOC_OBJ_CLEAR(new_ctx, struct tls_root_ctx);
 
-    if (options->tls_server)
-    {
-        tls_ctx_server_new(new_ctx);
+    tls_ctx_new(new_ctx);
 
-        if (options->dh_file)
-        {
-            tls_ctx_load_dh_params(new_ctx, options->dh_file, options->dh_file_inline);
-        }
-    }
-    else /* if client */
+    if (options->tls_server && options->dh_file)
     {
-        tls_ctx_client_new(new_ctx);
+        tls_ctx_load_dh_params(new_ctx, options->dh_file, options->dh_file_inline);
     }
 
     /* Restrict allowed certificate crypto algorithms */
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index 816fb9c..a6d57f2 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -117,18 +117,11 @@
 int tls_version_max(void);
 
 /**
- * Initialise a library-specific TLS context for a server.
+ * Initialise a library-specific TLS context.
  *
  * @param ctx           TLS context to initialise
  */
-void tls_ctx_server_new(struct tls_root_ctx *ctx);
-
-/**
- * Initialises a library-specific TLS context for a client.
- *
- * @param ctx           TLS context to initialise
- */
-void tls_ctx_client_new(struct tls_root_ctx *ctx);
+void tls_ctx_new(struct tls_root_ctx *ctx);
 
 /**
  * Frees the library-specific TLSv1 context
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 07caf08..6d7ac04 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -88,7 +88,7 @@
 }
 
 void
-tls_ctx_server_new(struct tls_root_ctx *ctx)
+tls_ctx_new(struct tls_root_ctx *ctx)
 {
     ASSERT(NULL != ctx);
     CLEAR(*ctx);
@@ -99,24 +99,9 @@
 
     ALLOC_OBJ_CLEAR(ctx->ca_chain, mbedtls_x509_crt);
 
-    ctx->endpoint = MBEDTLS_SSL_IS_SERVER;
     ctx->initialised = true;
 }
 
-void
-tls_ctx_client_new(struct tls_root_ctx *ctx)
-{
-    ASSERT(NULL != ctx);
-    CLEAR(*ctx);
-
-#if MBEDTLS_VERSION_NUMBER < 0x04000000
-    ALLOC_OBJ_CLEAR(ctx->dhm_ctx, mbedtls_dhm_context);
-#endif
-    ALLOC_OBJ_CLEAR(ctx->ca_chain, mbedtls_x509_crt);
-
-    ctx->endpoint = MBEDTLS_SSL_IS_CLIENT;
-    ctx->initialised = true;
-}
 
 void
 tls_ctx_free(struct tls_root_ctx *ctx)
@@ -1141,7 +1126,8 @@
     /* Initialise SSL config */
     ALLOC_OBJ_CLEAR(ks_ssl->ssl_config, mbedtls_ssl_config);
     mbedtls_ssl_config_init(ks_ssl->ssl_config);
-    mbedtls_ssl_config_defaults(ks_ssl->ssl_config, ssl_ctx->endpoint, MBEDTLS_SSL_TRANSPORT_STREAM,
+    int endpoint = is_server ? MBEDTLS_SSL_IS_SERVER : MBEDTLS_SSL_IS_CLIENT;
+    mbedtls_ssl_config_defaults(ks_ssl->ssl_config, endpoint, MBEDTLS_SSL_TRANSPORT_STREAM,
                                 MBEDTLS_SSL_PRESET_DEFAULT);
 #ifdef MBEDTLS_DEBUG_C
     /* We only want to have mbed TLS generate debug level logging when we would
@@ -1530,7 +1516,7 @@
     struct tls_root_ctx tls_ctx;
     const int *ciphers = mbedtls_ssl_list_ciphersuites();
 
-    tls_ctx_server_new(&tls_ctx);
+    tls_ctx_new(&tls_ctx);
     tls_ctx_set_cert_profile(&tls_ctx, tls_cert_profile);
     tls_ctx_restrict_ciphers(&tls_ctx, cipher_list);
 
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index 6b678b2..32d4f60 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -114,8 +114,6 @@
 {
     bool initialised; /**< True if the context has been initialised */
 
-    int endpoint;     /**< Whether or not this is a server or a client */
-
 #if MBEDTLS_VERSION_NUMBER < 0x04000000
     mbedtls_dhm_context *dhm_ctx;          /**< Diffie-Helmann-Merkle context */
 #endif
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index ef99b22..c5de165 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -100,37 +100,19 @@
 }
 
 void
-tls_ctx_server_new(struct tls_root_ctx *ctx)
+tls_ctx_new(struct tls_root_ctx *ctx)
 {
     ASSERT(NULL != ctx);
 
-    ctx->ctx = SSL_CTX_new_ex(tls_libctx, NULL, SSLv23_server_method());
+    ctx->ctx = SSL_CTX_new_ex(tls_libctx, NULL, TLS_method());
 
     if (ctx->ctx == NULL)
     {
-        crypto_msg(M_FATAL, "SSL_CTX_new SSLv23_server_method");
+        crypto_msg(M_FATAL, "SSL_CTX_new TLS_method");
     }
     if (ERR_peek_error() != 0)
     {
-        crypto_msg(M_WARN, "Warning: TLS server context initialisation "
-                           "has warnings.");
-    }
-}
-
-void
-tls_ctx_client_new(struct tls_root_ctx *ctx)
-{
-    ASSERT(NULL != ctx);
-
-    ctx->ctx = SSL_CTX_new_ex(tls_libctx, NULL, SSLv23_client_method());
-
-    if (ctx->ctx == NULL)
-    {
-        crypto_msg(M_FATAL, "SSL_CTX_new SSLv23_client_method");
-    }
-    if (ERR_peek_error() != 0)
-    {
-        crypto_msg(M_WARN, "Warning: TLS client context initialisation "
+        crypto_msg(M_WARN, "Warning: TLS context initialisation "
                            "has warnings.");
     }
 }
diff --git a/tests/unit_tests/openvpn/test_ssl.c b/tests/unit_tests/openvpn/test_ssl.c
index d473d67..54d52a0 100644
--- a/tests/unit_tests/openvpn/test_ssl.c
+++ b/tests/unit_tests/openvpn/test_ssl.c
@@ -175,7 +175,7 @@
     struct gc_arena gc = gc_new();
 
     struct tls_root_ctx ctx = { 0 };
-    tls_ctx_client_new(&ctx);
+    tls_ctx_new(&ctx);
     tls_ctx_load_cert_file(&ctx, unittest_cert, true);
 
     openvpn_x509_cert_t *cert = NULL;
@@ -208,13 +208,13 @@
     /* test loading of inlined cert and key.
      * loading the key also checks that it matches the loaded certificate
      */
-    tls_ctx_client_new(&ctx);
+    tls_ctx_new(&ctx);
     tls_ctx_load_cert_file(&ctx, unittest_cert, true);
     assert_int_equal(tls_ctx_load_priv_file(&ctx, unittest_key, true), 0);
     tls_ctx_free(&ctx);
 
     /* test loading of cert and key from file */
-    tls_ctx_client_new(&ctx);
+    tls_ctx_new(&ctx);
     tls_ctx_load_cert_file(&ctx, global_state.certfile, false);
     assert_int_equal(tls_ctx_load_priv_file(&ctx, global_state.keyfile, false), 0);
     tls_ctx_free(&ctx);
@@ -252,7 +252,7 @@
     string_mod(BSTR(&keyuri), CC_ANY, CC_BACKSLASH, '/');
 #endif /* _WIN32 */
 
-    tls_ctx_client_new(&ctx);
+    tls_ctx_new(&ctx);
     tls_ctx_load_cert_file(&ctx, BSTR(&certuri), false);
     assert_int_equal(tls_ctx_load_priv_file(&ctx, BSTR(&keyuri), false), 0);
     tls_ctx_free(&ctx);

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1728?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iaf1f3475e4f27a920c028cd73b1a2497953583d0
Gerrit-Change-Number: 1728
Gerrit-PatchSet: 5
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel