[libvirt] [RFC PATCH 4/4] qemu: migration: Forbid 'nbd' migration of non-shared storage if TLS is requested

Peter Krempa posted 4 patches 7 years ago
[libvirt] [RFC PATCH 4/4] qemu: migration: Forbid 'nbd' migration of non-shared storage if TLS is requested
Posted by Peter Krempa 7 years ago
Since libvirt is currently not able to setup the NBD migration stream
secured by TLS we should not allow such migration since data would be
transferred unencrypted.

This will break compatibility of TLS migration if non-shared storage is
requested but the security implications are more severe.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_migration.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 3b5ba4f0a1..24ef819738 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3352,6 +3352,15 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver,
     if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
                          QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
         if (mig->nbd) {
+            /* Currently libvirt does not support setting up of the NBD
+             * non-shared storage migration with TLS. As we need to honour the
+             * VIR_MIGRATE_TLS flag, we need to reject such migration. */
+            if (flags & VIR_MIGRATE_TLS) {
+                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                               _("NBD migration with TLS is not supported"));
+                goto error;
+            }
+
             /* This will update migrate_flags on success */
             if (qemuMigrationSrcDriveMirror(driver, vm, mig,
                                             spec->dest.host.name,
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 4/4] qemu: migration: Forbid 'nbd' migration of non-shared storage if TLS is requested
Posted by Kashyap Chamarthy 7 years ago
On Thu, Apr 26, 2018 at 04:51:49PM +0200, Peter Krempa wrote:
> Since libvirt is currently not able to setup the NBD migration stream
> secured by TLS we should not allow such migration since data would be
> transferred unencrypted.
> 
> This will break compatibility of TLS migration if non-shared storage is
> requested but the security implications are more severe.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_migration.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 3b5ba4f0a1..24ef819738 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3352,6 +3352,15 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver,
>      if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
>                           QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
>          if (mig->nbd) {
> +            /* Currently libvirt does not support setting up of the NBD
> +             * non-shared storage migration with TLS. As we need to honour the
> +             * VIR_MIGRATE_TLS flag, we need to reject such migration. */

You might want to reword the last sentence to be explicitly clear that:
"... reject such migration until TLS for NBD streams is implemented."

Or something like that.  Your choice.

>From what I understand, what you are saying is -- today if one sets
VIR_MIGRATE_TLS flag, then libvirt will use TLS for the migration stream
but not for the NBD stream via which non-shared disks will be migrated.
You are fixing that inconsistency.

> +            if (flags & VIR_MIGRATE_TLS) {
> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                               _("NBD migration with TLS is not supported"));
> +                goto error;
> +            }
> +
>              /* This will update migrate_flags on success */
>              if (qemuMigrationSrcDriveMirror(driver, vm, mig,
>                                              spec->dest.host.name,
> -- 
> 2.16.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
/kashyap

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 4/4] qemu: migration: Forbid 'nbd' migration of non-shared storage if TLS is requested
Posted by Daniel P. Berrangé 7 years ago
On Thu, Apr 26, 2018 at 04:51:49PM +0200, Peter Krempa wrote:
> Since libvirt is currently not able to setup the NBD migration stream
> secured by TLS we should not allow such migration since data would be
> transferred unencrypted.
> 
> This will break compatibility of TLS migration if non-shared storage is
> requested but the security implications are more severe.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_migration.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


IIUC, this doesn't actually require the 3 previous patches and can be
pushed on its own - we should push for this immediate release.

> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 3b5ba4f0a1..24ef819738 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3352,6 +3352,15 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver,
>      if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
>                           QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
>          if (mig->nbd) {
> +            /* Currently libvirt does not support setting up of the NBD
> +             * non-shared storage migration with TLS. As we need to honour the
> +             * VIR_MIGRATE_TLS flag, we need to reject such migration. */
> +            if (flags & VIR_MIGRATE_TLS) {
> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                               _("NBD migration with TLS is not supported"));
> +                goto error;
> +            }
> +
>              /* This will update migrate_flags on success */
>              if (qemuMigrationSrcDriveMirror(driver, vm, mig,
>                                              spec->dest.host.name,
> -- 
> 2.16.2
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 4/4] qemu: migration: Forbid 'nbd' migration of non-shared storage if TLS is requested
Posted by Peter Krempa 7 years ago
On Fri, Apr 27, 2018 at 10:55:56 +0100, Daniel Berrange wrote:
> On Thu, Apr 26, 2018 at 04:51:49PM +0200, Peter Krempa wrote:
> > Since libvirt is currently not able to setup the NBD migration stream
> > secured by TLS we should not allow such migration since data would be
> > transferred unencrypted.
> > 
> > This will break compatibility of TLS migration if non-shared storage is
> > requested but the security implications are more severe.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/qemu/qemu_migration.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Pushed now, thanks.

> IIUC, this doesn't actually require the 3 previous patches and can be
> pushed on its own - we should push for this immediate release.

The idea behind the other 3 patches was to actually implement the
destination side, so that we have both sides covered. If you enable TLS
for the NBD server it will not connect unless TLS is used. By using
this patch only, an older source libvirtd  will be able to migrate
even with newer destination libvirtd, since that will not require TLS
until those 3 patches will be pushed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 4/4] qemu: migration: Forbid 'nbd' migration of non-shared storage if TLS is requested
Posted by Daniel P. Berrangé 7 years ago
On Mon, Apr 30, 2018 at 10:42:24AM +0200, Peter Krempa wrote:
> On Fri, Apr 27, 2018 at 10:55:56 +0100, Daniel Berrange wrote:
> > On Thu, Apr 26, 2018 at 04:51:49PM +0200, Peter Krempa wrote:
> > > Since libvirt is currently not able to setup the NBD migration stream
> > > secured by TLS we should not allow such migration since data would be
> > > transferred unencrypted.
> > > 
> > > This will break compatibility of TLS migration if non-shared storage is
> > > requested but the security implications are more severe.
> > > 
> > > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > > ---
> > >  src/qemu/qemu_migration.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > 
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Pushed now, thanks.
> 
> > IIUC, this doesn't actually require the 3 previous patches and can be
> > pushed on its own - we should push for this immediate release.
> 
> The idea behind the other 3 patches was to actually implement the
> destination side, so that we have both sides covered. If you enable TLS
> for the NBD server it will not connect unless TLS is used. By using
> this patch only, an older source libvirtd  will be able to migrate
> even with newer destination libvirtd, since that will not require TLS
> until those 3 patches will be pushed.

Oh i see, nice trick.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 4/4] qemu: migration: Forbid 'nbd' migration of non-shared storage if TLS is requested
Posted by Peter Krempa 7 years ago
On Mon, Apr 30, 2018 at 10:08:05 +0100, Daniel Berrange wrote:
> On Mon, Apr 30, 2018 at 10:42:24AM +0200, Peter Krempa wrote:
> > On Fri, Apr 27, 2018 at 10:55:56 +0100, Daniel Berrange wrote:
> > > On Thu, Apr 26, 2018 at 04:51:49PM +0200, Peter Krempa wrote:
> > > > Since libvirt is currently not able to setup the NBD migration stream
> > > > secured by TLS we should not allow such migration since data would be
> > > > transferred unencrypted.
> > > > 
> > > > This will break compatibility of TLS migration if non-shared storage is
> > > > requested but the security implications are more severe.
> > > > 
> > > > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > > > ---
> > > >  src/qemu/qemu_migration.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > 
> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > Pushed now, thanks.
> > 
> > > IIUC, this doesn't actually require the 3 previous patches and can be
> > > pushed on its own - we should push for this immediate release.
> > 
> > The idea behind the other 3 patches was to actually implement the
> > destination side, so that we have both sides covered. If you enable TLS
> > for the NBD server it will not connect unless TLS is used. By using
> > this patch only, an older source libvirtd  will be able to migrate
> > even with newer destination libvirtd, since that will not require TLS
> > until those 3 patches will be pushed.
> 
> Oh i see, nice trick.

I've verified that everything works fine without TLS and with TLS if we
implement the transport properly and pushed these. This means that TLS
migration should for-now behave sanely.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list