Multiple fixes and improvements to TLS tests

Calling qcrypto_init ensures that all relevant initialization is done. In particular this honours the debugging settings and thread settings.

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 tests/crypto-tls-x509-helpers.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

tests/crypto-tls-x509-helpers.c +0 -0

index 173d4e28fb..70df68f5df 100644

The test-vmstate test is a bit chatty because it triggers various expected failure scenarios and the code in question uses error_report instead of accepting 'Error **errp' parameters. To silence this test the stubs for error_vprintf() were changed to send errors via g_test_message() instead of stderr:

commit 28017e010ddf6849cfa830e898da3e44e6610952
Author: Paolo Bonzini <[email protected]>
Date: Mon Oct 24 18:31:03 2016 +0200

tests: send error_report to test log

Implement error_vprintf to send the output of error_report to the test log. This silences test-vmstate.

Signed-off-by Paolo Bonzini <[email protected]>

Message-Id: <[email protected]>

Unfortunately this change has global impact across the entire test suite and means that when tests fail for unexpected reasons, the message is not displayed on stderr. eg when using &error_abort in a call the test merely prints

Unexpected error in qcrypto_tls_session_check_certificate() at crypto/tlssession.c:280:

and the actual error message is hidden, making it impossible to diagnose the failure. This is especially problematic in CI or build systems where it isn't possible to easily pass the --debug-log flag to tests and re-run with the test log visible.

This change makes the previous big hammer much more nuanced, providing a flag in the stub error_vprintf() that can used on a per-test basis to silence the errors. Only the test-vmstate silences errors initially.

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 stubs/error-printf.c | 5 ++++-
 tests/test-vmstate.c | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

stubs/error-printf.c +0 -0

index ac6b92aa69..2199d79d28 100644

stubs/error-printf.c +3 -1

+
 void error_vprintf(const char *fmt, va_list ap)
 {
-    if (g_test_initialized() && !g_test_subprocess()) {
+    if (g_test_initialized() && !g_test_subprocess() &&
+        getenv("QTEST_SILENT_ERRORS")) {
         char *msg = g_strdup_vprintf(fmt, ap);
         g_test_message("%s", msg);
         g_free(msg);

tests/test-vmstate.c +0 -0

index 087844b6c8..42923bb1df 100644

Hi Daniel,

Quoted 48 lines · Daniel P. Berrangé

The test-vmstate test is a bit chatty because it triggers various expected failure scenarios and the code in question uses error_report instead of accepting 'Error **errp' parameters. To silence this test the stubs for error_vprintf() were changed to send errors via g_test_message() instead of stderr:

commit 28017e010ddf6849cfa830e898da3e44e6610952
Author: Paolo Bonzini <[email protected]>
Date: Mon Oct 24 18:31:03 2016 +0200

tests: send error_report to test log

Implement error_vprintf to send the output of error_report to the test log. This silences test-vmstate.

Signed-off-by Paolo Bonzini <[email protected]>

Message-Id: <[email protected]>

Unfortunately this change has global impact across the entire test suite and means that when tests fail for unexpected reasons, the message is not displayed on stderr. eg when using &error_abort in a call the test merely prints

Unexpected error in qcrypto_tls_session_check_certificate() at crypto/tlssession.c:280:

and the actual error message is hidden, making it impossible to diagnose the failure. This is especially problematic in CI or build systems where it isn't possible to easily pass the --debug-log flag to tests and re-run with the test log visible.

This change makes the previous big hammer much more nuanced, providing a flag in the stub error_vprintf() that can used on a per-test basis to silence the errors. Only the test-vmstate silences errors initially.

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 stubs/error-printf.c | 5 ++++-
 tests/test-vmstate.c | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

stubs/error-printf.c +0 -0

index ac6b92aa69..2199d79d28 100644
@@ -2,9 +2,12 @@
 #include "qemu-common.h"
 #include "qemu/error-report.h"
 

stubs/error-printf.c line 5

Quoted 1 line

stubs/error-printf.c +1 -0

+bool silence_test_errors;

This is not used.

Quoted 13 lines

stubs/error-printf.c +3 -1

+
 void error_vprintf(const char *fmt, va_list ap)
 {
-    if (g_test_initialized() && !g_test_subprocess()) {
+    if (g_test_initialized() && !g_test_subprocess() &&
+        getenv("QTEST_SILENT_ERRORS")) {
         char *msg = g_strdup_vprintf(fmt, ap);
         g_test_message("%s", msg);
         g_free(msg);

tests/test-vmstate.c +0 -1

index 087844b6c8..42923bb1df 100644
--- a/tests/test-vmstate.c

tests/test-vmstate.c line 35

Quoted 6 lines

+++ b/tests/test-vmstate.c

tests/test-vmstate.c +1 -0

@@ -32,6 +32,7 @@
 #include "../migration/qemu-file-channel.h"
 #include "../migration/savevm.h"
 #include "qemu/coroutine.h"
+#include "qemu/error-report.h"

Why? This doesn't seem necessary, neither related to this patch.

Quoted 8 lines

tests/test-vmstate.c +0 -0

 #include "io/channel-file.h"
 
 static char temp_file[] = "/tmp/vmst.test.XXXXXX";

@@ -859,6 +860,8 @@ int main(int argc, char **argv)
 
     module_call_init(MODULE_INIT_QOM);
 

tests/test-vmstate.c int main(int argc, char **argv) · line 867

Quoted 6 lines

tests/test-vmstate.c +2 -0

+    setenv("QTEST_SILENT_ERRORS", "1", 1);
+
     g_test_init(&argc, &argv, NULL);
     g_test_add_func("/vmstate/simple/primitive", test_simple_primitive);
     g_test_add_func("/vmstate/versioned/load/v1", test_load_v1);

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Tested-by Philippe Mathieu-Daudé <[email protected]>

Most of the TLS related tests are passing an in a "Error" object to methods that are expected to fail, but then ignoring any error that is set and instead asserting on a return value. This means that when an error is unexpectedly raised, no information about it is printed out, making failures hard to diagnose. Changing these tests to pass in &error_abort will make unexpected failures print messages to stderr.

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 tests/test-crypto-tlscredsx509.c | 11 +----
 tests/test-crypto-tlssession.c   | 76 ++++++++++++--------------------
 tests/test-io-channel-tls.c      | 24 ++++------
 3 files changed, 39 insertions(+), 72 deletions(-)

tests/test-crypto-tlscredsx509.c +2 -9

index af2f80e89c..30f9ac4bbf 100644
@@ -54,7 +54,7 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
         "sanity-check", "yes",
         NULL);
 
-    if (*errp) {
+    if (!creds) {
         return NULL;
     }
     return QCRYPTO_TLS_CREDS(creds);

@@ -74,7 +74,6 @@ static void test_tls_creds(const void *opaque)
     struct QCryptoTLSCredsTestData *data =
         (struct QCryptoTLSCredsTestData *)opaque;
     QCryptoTLSCreds *creds;
-    Error *err = NULL;
 
 #define CERT_DIR "tests/test-crypto-tlscredsx509-certs/"
     mkdir(CERT_DIR, 0700);

@@ -113,17 +112,11 @@ static void test_tls_creds(const void *opaque)
          QCRYPTO_TLS_CREDS_ENDPOINT_SERVER :
          QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT),
         CERT_DIR,
-        &err);
+        data->expectFail ? NULL : &error_abort);
 
     if (data->expectFail) {
-        error_free(err);
         g_assert(creds == NULL);
     } else {
-        if (err) {
-            g_printerr("Failed to generate creds: %s\n",
-                       error_get_pretty(err));
-            error_free(err);
-        }
         g_assert(creds != NULL);
     }
 

tests/test-crypto-tlssession.c +28 -48

index 7bd811796e..fd9acf9067 100644
@@ -52,28 +52,21 @@ static ssize_t testRead(char *buf, size_t len, void *opaque)
 
 static QCryptoTLSCreds *test_tls_creds_psk_create(
     QCryptoTLSCredsEndpoint endpoint,
-    const char *dir,
-    Error **errp)
+    const char *dir)
 {
-    Error *err = NULL;
     Object *parent = object_get_objects_root();
     Object *creds = object_new_with_props(
         TYPE_QCRYPTO_TLS_CREDS_PSK,
         parent,
         (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
          "testtlscredsserver" : "testtlscredsclient"),
-        &err,
+        &error_abort,
         "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
                      "server" : "client"),
         "dir", dir,
         "priority", "NORMAL",
         NULL
         );
-
-    if (err) {
-        error_propagate(errp, err);
-        return NULL;
-    }
     return QCRYPTO_TLS_CREDS(creds);
 }
 

@@ -87,7 +80,6 @@ static void test_crypto_tls_session_psk(void)
     int channel[2];
     bool clientShake = false;
     bool serverShake = false;
-    Error *err = NULL;
     int ret;
 
     /* We'll use this for our fake client-server connection */

@@ -104,25 +96,23 @@ static void test_crypto_tls_session_psk(void)
 
     clientCreds = test_tls_creds_psk_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
-        WORKDIR,
-        &err);
+        WORKDIR);
     g_assert(clientCreds != NULL);
 
     serverCreds = test_tls_creds_psk_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
-        WORKDIR,
-        &err);
+        WORKDIR);
     g_assert(serverCreds != NULL);
 
     /* Now the real part of the test, setup the sessions */
     clientSess = qcrypto_tls_session_new(
         clientCreds, NULL, NULL,
-        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &err);
+        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &error_abort);
+    g_assert(clientSess != NULL);
+
     serverSess = qcrypto_tls_session_new(
         serverCreds, NULL, NULL,
-        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &err);
-
-    g_assert(clientSess != NULL);
+        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &error_abort);
     g_assert(serverSess != NULL);
 
     /* For handshake to work, we need to set the I/O callbacks

@@ -145,7 +135,7 @@ static void test_crypto_tls_session_psk(void)
         int rv;
         if (!serverShake) {
             rv = qcrypto_tls_session_handshake(serverSess,
-                                               &err);
+                                               &error_abort);
             g_assert(rv >= 0);
             if (qcrypto_tls_session_get_handshake_status(serverSess) ==
                 QCRYPTO_TLS_HANDSHAKE_COMPLETE) {

@@ -154,7 +144,7 @@ static void test_crypto_tls_session_psk(void)
         }
         if (!clientShake) {
             rv = qcrypto_tls_session_handshake(clientSess,
-                                               &err);
+                                               &error_abort);
             g_assert(rv >= 0);
             if (qcrypto_tls_session_get_handshake_status(clientSess) ==
                 QCRYPTO_TLS_HANDSHAKE_COMPLETE) {

@@ -165,8 +155,10 @@ static void test_crypto_tls_session_psk(void)
 
 
     /* Finally make sure the server & client validation is successful. */
-    g_assert(qcrypto_tls_session_check_credentials(serverSess, &err) == 0);
-    g_assert(qcrypto_tls_session_check_credentials(clientSess, &err) == 0);
+    g_assert(qcrypto_tls_session_check_credentials(serverSess,
+                                                   &error_abort) == 0);
+    g_assert(qcrypto_tls_session_check_credentials(clientSess,
+                                                   &error_abort) == 0);
 
     object_unparent(OBJECT(serverCreds));
     object_unparent(OBJECT(clientCreds));

@@ -192,17 +184,15 @@ struct QCryptoTLSSessionTestData {
 
 static QCryptoTLSCreds *test_tls_creds_x509_create(
     QCryptoTLSCredsEndpoint endpoint,
-    const char *certdir,
-    Error **errp)
+    const char *certdir)
 {
-    Error *err = NULL;
     Object *parent = object_get_objects_root();
     Object *creds = object_new_with_props(
         TYPE_QCRYPTO_TLS_CREDS_X509,
         parent,
         (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
          "testtlscredsserver" : "testtlscredsclient"),
-        &err,
+        &error_abort,
         "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
                      "server" : "client"),
         "dir", certdir,

@@ -217,11 +207,6 @@ static QCryptoTLSCreds *test_tls_creds_x509_create(
         "sanity-check", "no",
         NULL
         );
-
-    if (err) {
-        error_propagate(errp, err);
-        return NULL;
-    }
     return QCRYPTO_TLS_CREDS(creds);
 }
 

@@ -249,7 +234,6 @@ static void test_crypto_tls_session_x509(const void *opaque)
     int channel[2];
     bool clientShake = false;
     bool serverShake = false;
-    Error *err = NULL;
     int ret;
 
     /* We'll use this for our fake client-server connection */

@@ -293,14 +277,12 @@ static void test_crypto_tls_session_x509(const void *opaque)
 
     clientCreds = test_tls_creds_x509_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
-        CLIENT_CERT_DIR,
-        &err);
+        CLIENT_CERT_DIR);
     g_assert(clientCreds != NULL);
 
     serverCreds = test_tls_creds_x509_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
-        SERVER_CERT_DIR,
-        &err);
+        SERVER_CERT_DIR);
     g_assert(serverCreds != NULL);
 
     acl = qemu_acl_init("tlssessionacl");

@@ -314,13 +296,13 @@ static void test_crypto_tls_session_x509(const void *opaque)
     /* Now the real part of the test, setup the sessions */
     clientSess = qcrypto_tls_session_new(
         clientCreds, data->hostname, NULL,
-        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &err);
+        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &error_abort);
+    g_assert(clientSess != NULL);
+
     serverSess = qcrypto_tls_session_new(
         serverCreds, NULL,
         data->wildcards ? "tlssessionacl" : NULL,
-        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &err);
-
-    g_assert(clientSess != NULL);
+        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &error_abort);
     g_assert(serverSess != NULL);
 
     /* For handshake to work, we need to set the I/O callbacks

@@ -343,7 +325,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
         int rv;
         if (!serverShake) {
             rv = qcrypto_tls_session_handshake(serverSess,
-                                               &err);
+                                               &error_abort);
             g_assert(rv >= 0);
             if (qcrypto_tls_session_get_handshake_status(serverSess) ==
                 QCRYPTO_TLS_HANDSHAKE_COMPLETE) {

@@ -352,7 +334,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
         }
         if (!clientShake) {
             rv = qcrypto_tls_session_handshake(clientSess,
-                                               &err);
+                                               &error_abort);
             g_assert(rv >= 0);
             if (qcrypto_tls_session_get_handshake_status(clientSess) ==
                 QCRYPTO_TLS_HANDSHAKE_COMPLETE) {

@@ -365,10 +347,9 @@ static void test_crypto_tls_session_x509(const void *opaque)
     /* Finally make sure the server validation does what
      * we were expecting
      */
-    if (qcrypto_tls_session_check_credentials(serverSess, &err) < 0) {
+    if (qcrypto_tls_session_check_credentials(
+            serverSess, data->expectServerFail ? NULL : &error_abort) < 0) {
         g_assert(data->expectServerFail);
-        error_free(err);
-        err = NULL;
     } else {
         g_assert(!data->expectServerFail);
     }

@@ -376,10 +357,9 @@ static void test_crypto_tls_session_x509(const void *opaque)
     /*
      * And the same for the client validation check
      */
-    if (qcrypto_tls_session_check_credentials(clientSess, &err) < 0) {
+    if (qcrypto_tls_session_check_credentials(
+            clientSess, data->expectClientFail ? NULL : &error_abort) < 0) {
         g_assert(data->expectClientFail);
-        error_free(err);
-        err = NULL;
     } else {
         g_assert(!data->expectClientFail);
     }

tests/test-io-channel-tls.c +9 -15

index bb88ee870f..4900c6d433 100644
@@ -30,6 +30,7 @@
 #include "crypto/init.h"
 #include "crypto/tlscredsx509.h"
 #include "qemu/acl.h"
+#include "qapi/error.h"
 #include "qom/object_interfaces.h"
 
 #ifdef QCRYPTO_HAVE_TLS_TEST_SUPPORT

@@ -64,8 +65,7 @@ static void test_tls_handshake_done(QIOTask *task,
 
 
 static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
-                                              const char *certdir,
-                                              Error **errp)
+                                              const char *certdir)
 {
     Object *parent = object_get_objects_root();
     Object *creds = object_new_with_props(

@@ -73,7 +73,7 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
         parent,
         (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
          "testtlscredsserver" : "testtlscredsclient"),
-        errp,
+        &error_abort,
         "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
                      "server" : "client"),
         "dir", certdir,

@@ -89,9 +89,6 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
         NULL
         );
 
-    if (*errp) {
-        return NULL;
-    }
     return QCRYPTO_TLS_CREDS(creds);
 }
 

@@ -121,7 +118,6 @@ static void test_io_channel_tls(const void *opaque)
     int channel[2];
     struct QIOChannelTLSHandshakeData clientHandshake = { false, false };
     struct QIOChannelTLSHandshakeData serverHandshake = { false, false };
-    Error *err = NULL;
     QIOChannelTest *test;
     GMainContext *mainloop;
 

@@ -157,14 +153,12 @@ static void test_io_channel_tls(const void *opaque)
 
     clientCreds = test_tls_creds_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
-        CLIENT_CERT_DIR,
-        &err);
+        CLIENT_CERT_DIR);
     g_assert(clientCreds != NULL);
 
     serverCreds = test_tls_creds_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
-        SERVER_CERT_DIR,
-        &err);
+        SERVER_CERT_DIR);
     g_assert(serverCreds != NULL);
 
     acl = qemu_acl_init("channeltlsacl");

@@ -176,10 +170,10 @@ static void test_io_channel_tls(const void *opaque)
     }
 
     clientChanSock = qio_channel_socket_new_fd(
-        channel[0], &err);
+        channel[0], &error_abort);
     g_assert(clientChanSock != NULL);
     serverChanSock = qio_channel_socket_new_fd(
-        channel[1], &err);
+        channel[1], &error_abort);
     g_assert(serverChanSock != NULL);
 
     /*

@@ -193,12 +187,12 @@ static void test_io_channel_tls(const void *opaque)
     /* Now the real part of the test, setup the sessions */
     clientChanTLS = qio_channel_tls_new_client(
         QIO_CHANNEL(clientChanSock), clientCreds,
-        data->hostname, &err);
+        data->hostname, &error_abort);
     g_assert(clientChanTLS != NULL);
 
     serverChanTLS = qio_channel_tls_new_server(
         QIO_CHANNEL(serverChanSock), serverCreds,
-        "channeltlsacl", &err);
+        "channeltlsacl", &error_abort);
     g_assert(serverChanTLS != NULL);
 
     qio_channel_tls_handshake(clientChanTLS,

When gnutls negotiates TLS 1.3 instead of 1.2, the order of messages sent by the handshake changes. This exposed a logic bug in the test suite which caused us to wait for the server to see handshake completion, but not wait for the client to see completion. The result was the client didn't receive the certificate for verification and the test failed.

This is exposed in Fedora 29 rawhide which has just enabled TLS 1.3 in its GNUTLS builds.

Signed-off-by Daniel P. Berrangé <[email protected]>

---
  tests/test-crypto-tlssession.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)