tls-crypt: introduce tls_crypt_kt()

Because cipher_ctx_block_size() is a static function we now need to include tls_crypt.c from the tests, rather than tls_crypt.h.

Reduces code duplication: less lines, same functionality.

Because cipher_ctx_block_size() is a static function we now need to include tls_crypt.c from the tests, rather than tls_crypt.h.

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

---
 src/openvpn/tls_crypt.c                   | 30 ++++++++++++++++++------------
 tests/unit_tests/openvpn/Makefile.am      |  3 +--
 tests/unit_tests/openvpn/test_tls_crypt.c | 18 ++----------------
 3 files changed, 21 insertions(+), 30 deletions(-)

src/openvpn/tls_crypt.c +18 -12

index e13bb4e..c8d26c7 100644
@@ -35,19 +35,9 @@
 
 #include "tls_crypt.h"
 
-int
-tls_crypt_buf_overhead(void)
+static struct key_type
+tls_crypt_kt(void)
 {
-    return packet_id_size(true) + TLS_CRYPT_TAG_SIZE + TLS_CRYPT_BLOCK_SIZE;
-}
-
-void
-tls_crypt_init_key(struct key_ctx_bi *key, const char *key_file,
-                   const char *key_inline, bool tls_server)
-{
-    const int key_direction = tls_server ?
-                              KEY_DIRECTION_NORMAL : KEY_DIRECTION_INVERSE;
-
     struct key_type kt;
     kt.cipher = cipher_kt_get("AES-256-CTR");
     kt.digest = md_kt_get("SHA256");

@@ -64,6 +54,22 @@ tls_crypt_init_key(struct key_ctx_bi *key, const char *key_file,
     kt.cipher_length = cipher_kt_key_size(kt.cipher);
     kt.hmac_length = md_kt_size(kt.digest);
 
+    return kt;
+}
+
+int
+tls_crypt_buf_overhead(void)
+{
+    return packet_id_size(true) + TLS_CRYPT_TAG_SIZE + TLS_CRYPT_BLOCK_SIZE;
+}
+
+void
+tls_crypt_init_key(struct key_ctx_bi *key, const char *key_file,
+                   const char *key_inline, bool tls_server)
+{
+    const int key_direction = tls_server ?
+                              KEY_DIRECTION_NORMAL : KEY_DIRECTION_INVERSE;
+    struct key_type kt = tls_crypt_kt();
     crypto_read_openvpn_key(&kt, key, key_file, key_inline, key_direction,
                             "Control Channel Encryption", "tls-crypt");
 }

tests/unit_tests/openvpn/Makefile.am +1 -2

index 3bd382c..7b44f42 100644
@@ -54,5 +54,4 @@ tls_crypt_testdriver_SOURCES = test_tls_crypt.c mock_msg.c \
 	$(openvpn_srcdir)/crypto_openssl.c \
 	$(openvpn_srcdir)/otime.c \
 	$(openvpn_srcdir)/packet_id.c \
-	$(openvpn_srcdir)/platform.c \
-	$(openvpn_srcdir)/tls_crypt.c
+	$(openvpn_srcdir)/platform.c

tests/unit_tests/openvpn/test_tls_crypt.c +1 -9

index 9b82035..0a58a7d 100644
@@ -39,7 +39,7 @@
 #include <setjmp.h>
 #include <cmocka.h>
 
-#include "tls_crypt.h"
+#include "tls_crypt.c"
 
 #include "mock_msg.h"
 

@@ -60,23 +60,9 @@ setup(void **state) {
     struct test_context *ctx = calloc(1, sizeof(*ctx));
     *state = ctx;
 
-    ctx->kt.cipher = cipher_kt_get("AES-256-CTR");
-    ctx->kt.digest = md_kt_get("SHA256");
-    if (!ctx->kt.cipher)
-    {
-        printf("No AES-256-CTR support, skipping test.\n");
-        return 0;
-    }
-    if (!ctx->kt.digest)

Quoted 21 lines · Steffan Karger

tests/unit_tests/openvpn/test_tls_crypt.c +0 -15

@@ -60,23 +60,9 @@ setup(void **state) {
     struct test_context *ctx = calloc(1, sizeof(*ctx));
     *state = ctx;
 
-    ctx->kt.cipher = cipher_kt_get("AES-256-CTR");
-    ctx->kt.digest = md_kt_get("SHA256");
-    if (!ctx->kt.cipher)
-    {
-        printf("No AES-256-CTR support, skipping test.\n");
-        return 0;
-    }
-    if (!ctx->kt.digest)
-    {
-        printf("No HMAC-SHA256 support, skipping test.\n");
-        return 0;
-    }
-    ctx->kt.cipher_length = cipher_kt_key_size(ctx->kt.cipher);
-    ctx->kt.hmac_length = md_kt_size(ctx->kt.digest);
-
     struct key key = { 0 };
 

tests/unit_tests/openvpn/test_tls_crypt.c setup(void **state) { · line 65

Quoted 1 line

tests/unit_tests/openvpn/test_tls_crypt.c +1 -0

+    ctx->kt = tls_crypt_kt();

Now we don't have the NULL checks on ctx->kt.digest and ctx->kt.cipher anymore.
I understand this there is no variable involved as we statically search for "AES-256-CTR" and "SHA256", however, shouldn't we at least assert for them being not NULL ? Somebody may mess up the main code and the unitest should properly detect the wrong condition instead of segfaulting.

Reduces code duplication (and prepares for tls-crypt-v2, which needs the same functionality at more places).

Because tls_crypt_kt() is a static function we now need to include tls_crypt.c from the tests, rather than tls_crypt.h.

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

---
v2: improve error handling

 src/openvpn/tls_crypt.c                   | 40 ++++++++++++++++++++-----------
 tests/unit_tests/openvpn/Makefile.am      |  3 +--
 tests/unit_tests/openvpn/test_tls_crypt.c | 20 ++++------------
 3 files changed, 32 insertions(+), 31 deletions(-)

src/openvpn/tls_crypt.c +15 -14

index e13bb4e..403060d 100644
@@ -35,35 +35,47 @@
 
 #include "tls_crypt.h"
 
-int
-tls_crypt_buf_overhead(void)
-{
-    return packet_id_size(true) + TLS_CRYPT_TAG_SIZE + TLS_CRYPT_BLOCK_SIZE;
-}
-
-void
-tls_crypt_init_key(struct key_ctx_bi *key, const char *key_file,
-                   const char *key_inline, bool tls_server)
+static struct key_type
+tls_crypt_kt(void)
 {
-    const int key_direction = tls_server ?
-                              KEY_DIRECTION_NORMAL : KEY_DIRECTION_INVERSE;
-
     struct key_type kt;
     kt.cipher = cipher_kt_get("AES-256-CTR");
     kt.digest = md_kt_get("SHA256");
 
     if (!kt.cipher)
     {
-        msg(M_FATAL, "ERROR: --tls-crypt requires AES-256-CTR support.");
+        msg(M_WARN, "ERROR: --tls-crypt requires AES-256-CTR support.");
+        return (struct key_type) { 0 };
     }
     if (!kt.digest)
     {
-        msg(M_FATAL, "ERROR: --tls-crypt requires HMAC-SHA-256 support.");
+        msg(M_WARN, "ERROR: --tls-crypt requires HMAC-SHA-256 support.");
+        return (struct key_type) { 0 };
     }
 
     kt.cipher_length = cipher_kt_key_size(kt.cipher);
     kt.hmac_length = md_kt_size(kt.digest);
 
+    return kt;
+}
+
+int
+tls_crypt_buf_overhead(void)
+{
+    return packet_id_size(true) + TLS_CRYPT_TAG_SIZE + TLS_CRYPT_BLOCK_SIZE;
+}
+

src/openvpn/tls_crypt.c +1 -0

+    }
     crypto_read_openvpn_key(&kt, key, key_file, key_inline, key_direction,
                             "Control Channel Encryption", "tls-crypt");
 }

tests/unit_tests/openvpn/Makefile.am +1 -2

index 3bd382c..7b44f42 100644
@@ -54,5 +54,4 @@ tls_crypt_testdriver_SOURCES = test_tls_crypt.c mock_msg.c \
 	$(openvpn_srcdir)/crypto_openssl.c \
 	$(openvpn_srcdir)/otime.c \
 	$(openvpn_srcdir)/packet_id.c \
-	$(openvpn_srcdir)/platform.c \
-	$(openvpn_srcdir)/tls_crypt.c
+	$(openvpn_srcdir)/platform.c

tests/unit_tests/openvpn/test_tls_crypt.c +5 -10

index 9b82035..0a6a08f 100644
@@ -39,7 +39,7 @@
 #include <setjmp.h>
 #include <cmocka.h>
 
-#include "tls_crypt.h"
+#include "tls_crypt.c"
 
 #include "mock_msg.h"
 

@@ -60,23 +60,13 @@ setup(void **state) {
     struct test_context *ctx = calloc(1, sizeof(*ctx));
     *state = ctx;
 
-    ctx->kt.cipher = cipher_kt_get("AES-256-CTR");
-    ctx->kt.digest = md_kt_get("SHA256");
-    if (!ctx->kt.cipher)
-    {
-        printf("No AES-256-CTR support, skipping test.\n");
-        return 0;
-    }
-    if (!ctx->kt.digest)
+    struct key key = { 0 };
+
+    ctx->kt = tls_crypt_kt();
+    if (!ctx->kt.cipher || !ctx->kt.digest)
     {
-        printf("No HMAC-SHA256 support, skipping test.\n");