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é <berra...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
