[Openvpn-devel,v3,17/18] xkey-provider: Add a test for generic key load and signature

Message ID 20211214165928.30676-18-selva.nair@gmail.com
State Superseded
Headers show
Series
  • External key provider for use with OpenSSL 3
Related show

Commit Message

Selva Nair Dec. 14, 2021, 4:59 p.m.
From: Selva Nair <selva.nair@gmail.com>

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 configure.ac                             |   2 -
 tests/unit_tests/openvpn/Makefile.am     |   4 -
 tests/unit_tests/openvpn/test_provider.c | 112 +++++++++++++++++++++--
 3 files changed, 105 insertions(+), 13 deletions(-)

Comments

Arne Schwabe Jan. 20, 2022, 11:22 a.m. | #1
Am 14.12.21 um 17:59 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
>   configure.ac                             |   2 -
>   tests/unit_tests/openvpn/Makefile.am     |   4 -
>   tests/unit_tests/openvpn/test_provider.c | 112 +++++++++++++++++++++--
>   3 files changed, 105 insertions(+), 13 deletions(-)

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Jan. 20, 2022, 2:51 p.m. | #2
Hi,

On Tue, Dec 14, 2021 at 11:59:27AM -0500, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>

Is it OK if I squash 16+17 together?  I dislike the "history churn"
of modifying configure.ac and Makefile.am in 16 just to remove 
the AM_CONDITIONAL bits again in 17...

gert
Selva Nair Jan. 20, 2022, 3:21 p.m. | #3
Hi

On Thu, Jan 20, 2022 at 9:51 AM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Tue, Dec 14, 2021 at 11:59:27AM -0500, selva.nair@gmail.com wrote:
> > From: Selva Nair <selva.nair@gmail.com>
> >
> > Signed-off-by: Selva Nair <selva.nair@gmail.com>
>
> Is it OK if I squash 16+17 together?  I dislike the "history churn"
> of modifying configure.ac and Makefile.am in 16 just to remove
> the AM_CONDITIONAL bits again in 17...
>

Yeah, a previous version had checking for OpenSSL version in configure.ac
and the AM_CONDITIONAL made sense only in that case.  I can send a
new 16/18 or please do squash 16 with 17.

Thanks,

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 20, 2022 at 9:51 AM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Tue, Dec 14, 2021 at 11:59:27AM -0500, <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a> wrote:<br>
&gt; From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
&gt; <br>
&gt; Signed-off-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
<br>
Is it OK if I squash 16+17 together?  I dislike the &quot;history churn&quot;<br>
of modifying <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a> and Makefile.am in 16 just to remove <br>
the AM_CONDITIONAL bits again in 17...<br></blockquote><div><br></div><div>Yeah, a previous version had checking for OpenSSL version in <a href="http://configure.ac">configure.ac</a></div><div>and the AM_CONDITIONAL made sense only in that case.  I can send a</div><div>new 16/18 or please do squash 16 with 17.</div><div><br></div><div>Thanks,</div><div><br></div><div>Selva</div></div></div>
Gert Doering Jan. 20, 2022, 3:44 p.m. | #4
Hi,

On Thu, Jan 20, 2022 at 10:21:19AM -0500, Selva Nair wrote:
> Yeah, a previous version had checking for OpenSSL version in configure.ac
> and the AM_CONDITIONAL made sense only in that case.  I can send a
> new 16/18 or please do squash 16 with 17.

If you could send a new "16+17 v4" that would make my life easier
in not having to think about joint commit messages :-)

thanks,

gert

Patch

diff --git a/configure.ac b/configure.ac
index c446f631..e0f9c332 100644
--- a/configure.ac
+++ b/configure.ac
@@ -766,8 +766,6 @@  PKG_CHECK_MODULES(
 	[]
 )
 
-AM_CONDITIONAL([HAVE_XKEY_PROVIDER], [false])
-
 if test "${with_crypto_library}" = "openssl"; then
 	AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
 	AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am
index 96b670ae..6b5c94ab 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -11,9 +11,7 @@  if HAVE_LD_WRAP_SUPPORT
 test_binaries += tls_crypt_testdriver
 endif
 
-if HAVE_XKEY_PROVIDER
 test_binaries += provider_testdriver
-endif
 
 TESTS = $(test_binaries)
 check_PROGRAMS = $(test_binaries)
@@ -99,7 +97,6 @@  networking_testdriver_SOURCES = test_networking.c mock_msg.c \
 	$(openvpn_srcdir)/platform.c
 endif
 
-if HAVE_XKEY_PROVIDER
 provider_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
 	$(OPTIONAL_CRYPTO_CFLAGS)
@@ -113,7 +110,6 @@  provider_testdriver_SOURCES = test_provider.c mock_msg.c \
 	$(openvpn_srcdir)/base64.c \
 	mock_get_random.c \
 	$(openvpn_srcdir)/platform.c
-endif
 
 auth_token_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
diff --git a/tests/unit_tests/openvpn/test_provider.c b/tests/unit_tests/openvpn/test_provider.c
index dcf39019..0182b3b4 100644
--- a/tests/unit_tests/openvpn/test_provider.c
+++ b/tests/unit_tests/openvpn/test_provider.c
@@ -29,6 +29,10 @@ 
 #endif
 
 #include "syshead.h"
+#include "manage.h"
+#include "xkey_common.h"
+
+#ifdef HAVE_XKEY_PROVIDER
 
 #include <setjmp.h>
 #include <cmocka.h>
@@ -37,9 +41,6 @@ 
 #include <openssl/core_names.h>
 #include <openssl/evp.h>
 
-#include "manage.h"
-#include "xkey_common.h"
-
 struct management *management; /* global */
 static int mgmt_callback_called;
 
@@ -91,11 +92,11 @@  static const char *test_digest_b64 = "dzhlAB6WSMZXC67At5b5Zk1f0Lfb8zq/Asx4YYMgIO
  * --- the smallest size of the actual signature with the above
  * keys.
  */
-const uint8_t good_sig[] =
+static const uint8_t good_sig[] =
    {0xd8, 0xa7, 0xd9, 0x81, 0xd8, 0xaa, 0xd8, 0xad, 0x20, 0xd9, 0x8a, 0xd8,
     0xa7, 0x20, 0xd8, 0xb3, 0xd9, 0x85, 0xd8, 0xb3, 0xd9, 0x85, 0x0};
 
-const char *good_sig_b64 = "2KfZgdiq2K0g2YrYpyDYs9mF2LPZhQA=";
+static const char *good_sig_b64 = "2KfZgdiq2K0g2YrYpyDYs9mF2LPZhQA=";
 
 static EVP_PKEY *
 load_pubkey(const char *pem)
@@ -155,10 +156,16 @@  management_query_pk_sig(struct management *man, const char *b64_data,
     if (strstr(algorithm, "data=message"))
     {
          expected_tbs = test_msg_b64;
+         assert_non_null(strstr(algorithm, "hashalg=SHA256"));
     }
-
     assert_string_equal(b64_data, expected_tbs);
 
+    /* We test using ECDSA or PSS with saltlen = digest */
+    if (!strstr(algorithm, "ECDSA"))
+    {
+        assert_non_null(strstr(algorithm, "RSA_PKCS1_PSS_PADDING,hashalg=SHA256,saltlen=digest"));
+    }
+
     /* Return a predefined string as sig so that the caller
      * can confirm that this callback was exercised.
      */
@@ -230,7 +237,6 @@  digest_sign(EVP_PKEY *pkey)
         goto done;
     }
 
-
     /* sign with sig = NULL to get required siglen */
     assert_int_equal(EVP_DigestSign(mctx, sig, &siglen, (uint8_t*)test_msg, strlen(test_msg)), 1);
     assert_true(siglen > 0);
@@ -288,6 +294,90 @@  again:
     }
 }
 
+/* helpers for testing generic key load and sign */
+static int xkey_free_called;
+static int xkey_sign_called;
+static void
+xkey_free(void *handle)
+{
+    xkey_free_called = 1;
+    /* We use a dummy string as handle -- check its value */
+    assert_string_equal(handle, "xkey_handle");
+}
+
+static int
+xkey_sign(void *handle, unsigned char *sig, size_t *siglen,
+          const unsigned char *tbs, size_t tbslen, XKEY_SIGALG s)
+{
+    if (!sig)
+    {
+        *siglen = 256; /* some arbitrary size */
+        return 1;
+    }
+
+    xkey_sign_called = 1; /* called with non-null sig */
+
+    if (!strcmp(s.op, "DigestSign"))
+    {
+        assert_memory_equal(tbs, test_msg, strlen(test_msg));
+    }
+    else
+    {
+        assert_memory_equal(tbs, test_digest, sizeof(test_digest));
+    }
+
+    /* For the test use sha256 and PSS padding for RSA */
+    assert_int_equal(OBJ_sn2nid(s.mdname), NID_sha256);
+    if (!strcmp(s.keytype, "RSA"))
+    {
+        assert_string_equal(s.padmode, "pss"); /* we use PSS for the test */
+    }
+    else if (strcmp(s.keytype, "EC"))
+    {
+        fail_msg("Unknown keytype: %s", s.keytype);
+    }
+
+    /* return a predefined string as sig */
+    memcpy(sig, good_sig, min_int(sizeof(good_sig), *siglen));
+
+    return 1;
+}
+
+/* Load a key as a generic key and check its sign op gets
+ * called for signature.
+ */
+static void
+xkey_provider_test_generic_sign_cb(void **state)
+{
+    EVP_PKEY *pubkey;
+    const char *dummy = "xkey_handle"; /* a dummy handle for the external key */
+
+    for (size_t i = 0; i < _countof(pubkeys); i++)
+    {
+        pubkey = load_pubkey(pubkeys[i]);
+        assert_true(pubkey != NULL);
+
+        EVP_PKEY *privkey = xkey_load_generic_key(NULL, (void*)dummy, pubkey, xkey_sign, xkey_free);
+        assert_true(privkey != NULL);
+
+        xkey_sign_called = 0;
+        xkey_free_called = 0;
+        uint8_t *sig = digest_sign(privkey);
+        assert_non_null(sig);
+
+        /* check callback for signature got exercised */
+        assert_int_equal(xkey_sign_called, 1);
+        assert_memory_equal(sig, good_sig, sizeof(good_sig));
+        test_free(sig);
+
+        EVP_PKEY_free(pubkey);
+        EVP_PKEY_free(privkey);
+
+        /* check key's free-op got called */
+        assert_int_equal(xkey_free_called, 1);
+    }
+}
+
 int
 main(void)
 {
@@ -296,6 +386,7 @@  main(void)
     const struct CMUnitTest tests[] = {
         cmocka_unit_test(xkey_provider_test_fetch),
         cmocka_unit_test(xkey_provider_test_mgmt_sign_cb),
+        cmocka_unit_test(xkey_provider_test_generic_sign_cb),
     };
 
     int ret = cmocka_run_group_tests_name("xkey provider tests", tests, NULL, NULL);
@@ -303,3 +394,10 @@  main(void)
     uninit_test();
     return ret;
 }
+#else
+int
+main(void)
+{
+    return 0;
+}
+#endif /* HAVE_XKEY_PROVIDER */