Re: [PATCH v3 4/9] tests: add migration tests of TLS with x509 credentials

[Eric Blake]

On Tue, Apr 26, 2022 at 05:00:43PM +0100, Daniel P. Berrangé wrote:
> This validates that we correctly handle migration success and failure
> scenarios when using TLS with x509 certificates. There are quite a few
> different scenarios that matter in relation to hostname validation.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  meson.build                  |   1 +
>  tests/qtest/meson.build      |   5 +
>  tests/qtest/migration-test.c | 382 ++++++++++++++++++++++++++++++++++-
>  3 files changed, 386 insertions(+), 2 deletions(-)
> 

> +
> +#ifdef CONFIG_TASN1
> +typedef struct {
> +    char *workdir;
> +    char *keyfile;
> +    char *cacert;
> +    char *servercert;
> +    char *serverkey;
> +    char *clientcert;
> +    char *clientkey;
> +} TestMigrateTLSX509Data;
> +
> +typedef struct {
> +    bool verifyclient;
> +    bool clientcert;
> +    bool hostileclient;
> +    bool authzclient;
> +    const char *certhostname;
> +    const char *certipaddr;
> +} TestMigrateTLSX509;
> +
> +static void *
> +test_migrate_tls_x509_start_common(QTestState *from,
> +                                   QTestState *to,
> +                                   TestMigrateTLSX509 *args)
> +{
> +    TestMigrateTLSX509Data *data = g_new0(TestMigrateTLSX509Data, 1);
> +    QDict *rsp;
> +
> +    data->workdir = g_strdup_printf("%s/tlscredsx5090", tmpfs);
> +    data->keyfile = g_strdup_printf("%s/key.pem", data->workdir);
> +
> +    data->cacert = g_strdup_printf("%s/ca-cert.pem", data->workdir);
> +    data->serverkey = g_strdup_printf("%s/server-key.pem", data->workdir);
> +    data->servercert = g_strdup_printf("%s/server-cert.pem", data->workdir);
> +    if (args->clientcert) {
> +        data->clientkey = g_strdup_printf("%s/client-key.pem", 
> data->workdir);
> +        data->clientcert = g_strdup_printf("%s/client-cert.pem", 
> data->workdir);
> +    }
> +
> +    mkdir(data->workdir, 0700);
> +
> +    test_tls_init(data->keyfile);
> +    g_assert(link(data->keyfile, data->serverkey) == 0);

Is relying on side effects inside a g_assert() wise?  But not the
first time our testsuite has done it, so I guess you're okay.

> +    if (args->clientcert) {
> +        g_assert(link(data->keyfile, data->clientkey) == 0);
> +    }
> +
> +    TLS_ROOT_REQ_SIMPLE(cacertreq, data->cacert);
> +    if (args->clientcert) {
> +        TLS_CERT_REQ_SIMPLE_CLIENT(servercertreq, cacertreq,
> +                                   args->hostileclient ?
> +                                   QCRYPTO_TLS_TEST_CLIENT_HOSTILE_NAME :
> +                                   QCRYPTO_TLS_TEST_CLIENT_NAME,
> +                                   data->clientcert);
> +    }
> +
> +    TLS_CERT_REQ_SIMPLE_SERVER(clientcertreq, cacertreq,
> +                               data->servercert,
> +                               args->certhostname,
> +                               args->certipaddr);

The new macros from 2/9 came in handy.


> +
> +/*
> + * The normal case: match server's cert hostname against
> + * whatever host we were telling QEMU to connect to (if any)
> + */
> +static void *
> +test_migrate_tls_x509_start_default_host(QTestState *from,
> +                                         QTestState *to)
> +{
> +    TestMigrateTLSX509 args = {
> +        .verifyclient = true,
> +        .clientcert = true,
> +        .certipaddr = "127.0.0.1"
> +    };
> +    return test_migrate_tls_x509_start_common(from, to, &args);
> +}
> +
> +/*
> + * The unusual case: the server's cert is different from
> + * the address we're telling QEMU to connect to (if any),
> + * so we must give QEMU an explicit hostname to validate
> + */
> +static void *
> +test_migrate_tls_x509_start_override_host(QTestState *from,
> +                                          QTestState *to)

Useful comments, and good coverage of a number of cases.

> +{
> +    TestMigrateTLSX509 args = {
> +        .verifyclient = true,
> +        .clientcert = true,
> +        .certhostname = "qemu.org",
> +    };
> +    return test_migrate_tls_x509_start_common(from, to, &args);
> +}
> +
> +/*
> + * The unusual case: the server's cert is different from
> + * the address we're telling QEMU to connect to, and so we
> + * expect the client to reject the server
> + */
> +static void *
> +test_migrate_tls_x509_start_mismatch_host(QTestState *from,
> +                                          QTestState *to)
> +{
> +    TestMigrateTLSX509 args = {
> +        .verifyclient = true,
> +        .clientcert = true,
> +        .certipaddr = "10.0.0.1",
> +    };
> +    return test_migrate_tls_x509_start_common(from, to, &args);
> +}
> +
> +static void *
> +test_migrate_tls_x509_start_friendly_client(QTestState *from,
> +                                            QTestState *to)
> +{
> +    TestMigrateTLSX509 args = {
> +        .verifyclient = true,
> +        .clientcert = true,
> +        .authzclient = true,
> +        .certipaddr = "127.0.0.1",
> +    };
> +    return test_migrate_tls_x509_start_common(from, to, &args);
> +}
> +
> +static void *
> +test_migrate_tls_x509_start_hostile_client(QTestState *from,
> +                                           QTestState *to)

Worth a comment on these two?

> @@ -1020,6 +1251,21 @@ static void test_precopy_unix_plain(void)
>      test_precopy_common(&args);
>  }
>  
> +static void test_precopy_unix_dirty_ring(void)
> +{
> +    g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> +    MigrateCommon args = {
> +        .start = {
> +            .use_dirty_ring = true,
> +        },
> +        .listen_uri = uri,
> +        .connect_uri = uri,
> +    };
> +
> +    test_precopy_common(&args);
> +}
> +

Looks like unintended code motion?  Should this be squashed into 3/9...

> +#ifdef CONFIG_GNUTLS
>  static void test_precopy_unix_tls_psk(void)

...especially since this is fixing the missing #ifdef I pointed out there?

>  {
>      g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> @@ -1033,19 +1279,38 @@ static void test_precopy_unix_tls_psk(void)
>      test_precopy_common(&args);
>  }
>  
> -static void test_precopy_unix_dirty_ring(void)
> +#ifdef CONFIG_TASN1
> +static void test_precopy_unix_tls_x509_default_host(void)
>  {
>      g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
>      MigrateCommon args = {
>          .start = {
> -            .use_dirty_ring = true,
> +            .hide_stderr = true,
>          },
> +        .connect_uri = uri,
>          .listen_uri = uri,
> +        .start_hook = test_migrate_tls_x509_start_default_host,
> +        .finish_hook = test_migrate_tls_x509_finish,
> +        .result = MIG_TEST_FAIL_DEST_QUIT_ERR,
> +    };

The code motion mixed in made this a bit harder to read.

> +
> +    test_precopy_common(&args);
> +}
> +
> +static void test_precopy_unix_tls_x509_override_host(void)
> +{
> +    g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> +    MigrateCommon args = {
>          .connect_uri = uri,
> +        .listen_uri = uri,
> +        .start_hook = test_migrate_tls_x509_start_override_host,
> +        .finish_hook = test_migrate_tls_x509_finish,
>      };
>  
>      test_precopy_common(&args);
>  }
> +#endif /* CONFIG_TASN1 */
> +#endif /* CONFIG_GNUTLS */
>  
>  #if 0
>  /* Currently upset on aarch64 TCG */
> @@ -1172,6 +1437,97 @@ static void test_precopy_tcp_tls_psk_mismatch(void)
>  
>      test_precopy_common(&args);
>  }
> +
> +#ifdef CONFIG_TASN1
> +static void test_precopy_tcp_tls_x509_default_host(void)

> +static void test_precopy_tcp_tls_x509_reject_anon_client(void)
> +{
> +    MigrateCommon args = {
> +        .start = {
> +            .hide_stderr = true,
> +        },
> +        .listen_uri = "tcp:127.0.0.1:0",
> +        .start_hook = test_migrate_tls_x509_start_reject_anon_client,
> +        .finish_hook = test_migrate_tls_x509_finish,
> +        .result = MIG_TEST_FAIL,
> +    };
> +
> +    test_precopy_common(&args);
> +}
> +#endif /* CONFIG_TASN1 */
>  #endif /* CONFIG_GNUTLS */
>  
>  static void *test_migrate_fd_start_hook(QTestState *from,
> @@ -1640,6 +1996,12 @@ int main(int argc, char **argv)
>  #ifdef CONFIG_GNUTLS
>      qtest_add_func("/migration/precopy/unix/tls/psk",
>                     test_precopy_unix_tls_psk);
> +#ifdef CONFIG_TASN1
> +    qtest_add_func("/migration/precopy/unix/tls/x509/default-host",
> +                   test_precopy_unix_tls_x509_default_host);
> +    qtest_add_func("/migration/precopy/unix/tls/x509/override-host",
> +                   test_precopy_unix_tls_x509_override_host);
> +#endif /* CONFIG_TASN1 */
>  #endif /* CONFIG_GNUTLS */
>  
>      qtest_add_func("/migration/precopy/tcp/plain", test_precopy_tcp_plain);
> @@ -1648,6 +2010,22 @@ int main(int argc, char **argv)
>                     test_precopy_tcp_tls_psk_match);
>      qtest_add_func("/migration/precopy/tcp/tls/psk/mismatch",
>                     test_precopy_tcp_tls_psk_mismatch);
> +#ifdef CONFIG_TASN1
> +    qtest_add_func("/migration/precopy/tcp/tls/x509/default-host",
> +                   test_precopy_tcp_tls_x509_default_host);
> +    qtest_add_func("/migration/precopy/tcp/tls/x509/override-host",
> +                   test_precopy_tcp_tls_x509_override_host);
> +    qtest_add_func("/migration/precopy/tcp/tls/x509/mismatch-host",
> +                   test_precopy_tcp_tls_x509_mismatch_host);
> +    qtest_add_func("/migration/precopy/tcp/tls/x509/friendly-client",
> +                   test_precopy_tcp_tls_x509_friendly_client);
> +    qtest_add_func("/migration/precopy/tcp/tls/x509/hostile-client",
> +                   test_precopy_tcp_tls_x509_hostile_client);
> +    qtest_add_func("/migration/precopy/tcp/tls/x509/allow-anon-client",
> +                   test_precopy_tcp_tls_x509_allow_anon_client);
> +    qtest_add_func("/migration/precopy/tcp/tls/x509/reject-anon-client",
> +                   test_precopy_tcp_tls_x509_reject_anon_client);
> +#endif /* CONFIG_TASN1 */
>  #endif /* CONFIG_GNUTLS */

Other than the potential mess with having to rebase the code motion of
test_precopy_unix_dirty_ring when fixing the #ifdef in 3, it looks
like these test additions are good.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org