qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 35/39] tests/qtest: migration-test: Skip running some TLS


From: Bin Meng
Subject: Re: [PATCH v2 35/39] tests/qtest: migration-test: Skip running some TLS cases for win32
Date: Wed, 28 Sep 2022 14:03:28 +0800

Hi Daniel,

On Tue, Sep 27, 2022 at 11:40 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Sep 22, 2022 at 07:54:05PM +0800, Bin Meng wrote:
> > On Thu, Sep 22, 2022 at 6:39 PM Daniel P. Berrangé <berrange@redhat.com> 
> > wrote:
> > >
> > > On Thu, Sep 22, 2022 at 10:47:26AM +0800, Bin Meng wrote:
> > > > On Thu, Sep 22, 2022 at 1:23 AM Daniel P. Berrangé 
> > > > <berrange@redhat.com> wrote:
> > > > >
> > > > > On Wed, Sep 21, 2022 at 05:51:33PM +0100, Dr. David Alan Gilbert 
> > > > > wrote:
> > > > > > * Bin Meng (bmeng.cn@gmail.com) wrote:
> > > > > > > From: Bin Meng <bin.meng@windriver.com>
> > > > > > >
> > > > > > > Some migration test cases use TLS to communicate, but they fail on
> > > > > > > Windows with the following error messages:
> > > > > > >
> > > > > > >   qemu-system-x86_64: TLS handshake failed: Insufficient 
> > > > > > > credentials for that request.
> > > > > > >   qemu-system-x86_64: TLS handshake failed: Error in the pull 
> > > > > > > function.
> > > > > > >   query-migrate shows failed migration: TLS handshake failed: 
> > > > > > > Error in the pull function.
> > > > > > >
> > > > > > > Disable them temporarily.
> > > > > > >
> > > > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > > > > ---
> > > > > > > I am not familar with the gnutls and simply enabling the gnutls 
> > > > > > > debug
> > > > > > > output does not give me an immedidate hint on why it's failing on
> > > > > > > Windows. Disable these cases for now until someone or maintainers
> > > > > > > who may want to test this on Windows.
> > > > > >
> > > > > > Copying in Dan Berrange, he's our expert on weird TLS failures.
> > > > >
> > > > > Seems to match this:
> > > > >
> > > > >    https://gnutls.org/faq.html#key-usage-violation2
> > > > >
> > > > > which suggests we have a configuration mis-match.
> > > > >
> > > > > I'm surprised to see you are only needing to disable the TLS PSK 
> > > > > tests,
> > > > > not the TLS x509 tests.
> > > >
> > > > The TLS x509 qtests all passed.
> > > >
> > > > >
> > > > > I'd like to know if tests/unit/test-crypto-tlssession passes.
> > > >
> > > > These unit tests currently are not built on Windows as they simply
> > > > don't build due to usage of socketpair().
> > >
> > > Doh, yes, that's rather annoying, as debugging this problem in the
> > > unit tests would be easier than in qtests.
> > >
> > > > > If so, it might suggest we are missing 'priority: NORMAL' property
> > > > > when configuring TLS creds for the migration test.
> > > >
> > > > I did the following changes but the error is still the same:
> > >
> > > >
> > > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > > > index dbee9b528a..c1e3f11873 100644
> > > > --- a/tests/qtest/migration-test.c
> > > > +++ b/tests/qtest/migration-test.c
> > > > @@ -788,7 +788,8 @@ test_migrate_tls_psk_start_common(QTestState *from,
> > > > " 'id': 'tlscredspsk0',"
> > > > " 'endpoint': 'client',"
> > > > " 'dir': %s,"
> > > > - " 'username': 'qemu'} }",
> > > > + " 'username': 'qemu',"
> > > > + " 'priority': 'NORMAL'} }",
> > > > data->workdir);
> > > > qobject_unref(rsp);
> > > > @@ -797,7 +798,8 @@ test_migrate_tls_psk_start_common(QTestState *from,
> > > > " 'arguments': { 'qom-type': 'tls-creds-psk',"
> > > > " 'id': 'tlscredspsk0',"
> > > > " 'endpoint': 'server',"
> > > > - " 'dir': %s } }",
> > > > + " 'dir': %s,"
> > > > + " 'priority': 'NORMAL'} }",
> > > > mismatch ? data->workdiralt : data->workdir);
> > > > qobject_unref(rsp);
> > > >
> > > > I am not sure whether I did the right changes.
> > >
> > >
> > > That ought to have been sufficient, if priority strings were the
> > > problem.
> > >
> > >
> > > I think we'd need the debug output from gnutls - could you edit 
> > > crypto/init.c
> > > and uncomment the '#define DEBUG_GNUTLS' line near the top.
> > >
> > > If you can post the output you get from a single migration-test test case
> > > involving PSK, it might be enough to diagnose why gnutls is failing.
> > >
> >
> > Here is the output:
> >
> > # Start of tls tests
> > # starting QEMU: ./qemu-system-x86_64 -qtest
> > unix:D:\msys64\tmp/qtest-18480.sock -qtest-log nul -chardev
> > socket,path=D:\msys64\tmp/qtest-18480.qmp,id=char0 -mon
> > chardev=char0,mode=control -display none -accel kvm -accel t
> > cg -name source,debug-threads=on -m 150M -serial
> > file:D:\msys64\tmp\migration-test-A5WJS1/src_serial -drive
> > file=D:\msys64\tmp\migration-test-A5WJS1/bootsect,format=raw -accel
> > qtest
> > qemu: thread naming not supported on this host
> > # starting QEMU: ./qemu-system-x86_64 -qtest
> > unix:D:\msys64\tmp/qtest-18480.sock -qtest-log nul -chardev
> > socket,path=D:\msys64\tmp/qtest-18480.qmp,id=char0 -mon
> > chardev=char0,mode=control -display none -accel kvm -accel t
> > cg -name target,debug-threads=on -m 150M -serial
> > file:D:\msys64\tmp\migration-test-A5WJS1/dest_serial -incoming
> > unix:D:\msys64\tmp\migration-test-A5WJS1/migsocket -drive
> > file=D:\msys64\tmp\migration-test-A5WJS1/bootsect,f
> > ormat=raw -accel qtest
>
> Comparing to running the same test on my machine.....
>
> > 4: EXT[0000015bb1dd2c50]: Sending extension Supported Versions/43 (9 bytes)
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Post Handshake Auth/49)
> > for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Safe
> > Renegotiation/65281) for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Sending extension Safe Renegotiation/65281 (1 
> > bytes)
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Server Name
> > Indication/0) for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Cookie/44) for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Early Data/42) for 'client 
> > hello'
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (PSK Key Exchange
> > Modes/45) for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Sending extension PSK Key Exchange Modes/45 (3 
> > bytes)
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Record Size Limit/28)
> > for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Sending extension Record Size Limit/28 (2 bytes)
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Maximum Record Size/1)
> > for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Compress
> > Certificate/27) for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (ClientHello Padding/21)
> > for 'client hello'
> > 4: EXT[0000015bb1dd2c50]: Preparing extension (Pre Shared Key/41) for
> > 'client hello'
>
> Right here is missing two items:
>
>   4: EXT[0x55bd0c660d30]: sent PSK identity 'qemu' (0)
>   4: EXT[0x55bd0c660d30]: Sending extension Pre Shared Key/41 (47 bytes)
>
> So it appears the client is not sendnig the PSK credentials
>
> > 4: HSK[0000015bb1dd2c50]: CLIENT HELLO was queued [343 bytes]
> > 5: REC[0000015bb1dd2c50]: Preparing Packet Handshake(22) with length:
> > 343 and min pad: 0
> > 9: ENC[0000015bb1dd2c50]: cipher: NULL, MAC: MAC-NULL, Epoch: 0
> > 5: REC[0000015bb1dd2c50]: Sent Packet[1] Handshake(22) in epoch 0 and
> > length: 348
>
> I believe there are probably two issues - first we're igonring the
> return value of gnutls_psk_set_client_credentials() and I have a feeling
> that is reporting an error.

Indeed gnutls_psk_set_client_credentials() fails with "Error in
parsing." message.

> Second, when we write the PSK credentials out
> to disk, we're not using binary mode, so I think UNIX line endings are
> getting turned into  DOS line endings, and when we later load the PSK
> credentials there's a stray \r present  that probably breaks
> gnutls_psk_set_client_credentials.

I think that's what happened.

>
> Could you try this patch and see if it makes the PSK tests work for
> migration-test:

Yes, this patch fixed the TLS test cases in the migration-test on Windows!

Thank you very much for the help!

Would you mind sending the patches on your own, or you want me to
include them in the next version of this series?

>
> diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
> index a4f9891274..546cad1c5a 100644
> --- a/crypto/tlscredspsk.c
> +++ b/crypto/tlscredspsk.c
> @@ -109,7 +109,12 @@ qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds,
>              goto cleanup;
>          }
>
> -        gnutls_psk_set_server_credentials_file(creds->data.server, pskfile);
> +        ret = gnutls_psk_set_server_credentials_file(creds->data.server, 
> pskfile);
> +        if (ret < 0) {
> +            error_setg(errp, "Cannot set PSK server credentials: %s",
> +                       gnutls_strerror(ret));
> +            goto cleanup;
> +        }
>          gnutls_psk_set_server_dh_params(creds->data.server,
>                                          creds->parent_obj.dh_params);
>      } else {
> @@ -135,8 +140,13 @@ qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds,
>              goto cleanup;
>          }
>
> -        gnutls_psk_set_client_credentials(creds->data.client,
> -                                          username, &key, 
> GNUTLS_PSK_KEY_HEX);
> +        ret = gnutls_psk_set_client_credentials(creds->data.client,
> +                                                username, &key, 
> GNUTLS_PSK_KEY_HEX);
> +        if (ret < 0) {
> +            error_setg(errp, "Cannot set PSK client credentials: %s",
> +                       gnutls_strerror(ret));
> +            goto cleanup;
> +        }
>      }
>
>      rv = 0;
> diff --git a/tests/unit/crypto-tls-psk-helpers.c 
> b/tests/unit/crypto-tls-psk-helpers.c
> index 511e08cc9c..c6cc740772 100644
> --- a/tests/unit/crypto-tls-psk-helpers.c
> +++ b/tests/unit/crypto-tls-psk-helpers.c
> @@ -27,15 +27,14 @@
>  static void
>  test_tls_psk_init_common(const char *pskfile, const char *user, const char 
> *key)
>  {
> -    FILE *fp;
> +    g_autoptr(GError) gerr = NULL;
> +    g_autofree char *line = g_strdup_printf("%s:%s\n", user, key);
>
> -    fp = fopen(pskfile, "w");
> -    if (fp == NULL) {
> -        g_critical("Failed to create pskfile %s: %s", pskfile, 
> strerror(errno));
> +    g_file_set_contents(pskfile, line, strlen(line), &gerr);
> +    if (gerr != NULL) {
> +        g_critical("Failed to create pskfile %s: %s", pskfile, 
> gerr->message);
>          abort();
>      }
> -    fprintf(fp, "%s:%s\n", user, key);
> -    fclose(fp);
>  }
>
>  void test_tls_psk_init(const char *pskfile)
>

Regards,
Bin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]