Use the default TLS env if TLS is required for NBD. The rest of the
implementation is rather simple since all pieces were in place.

Note that separate configuration knobs in qemu.conf can be added later
if it's desired to configure them.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 docs/schemas/domaincommon.rng                      |  5 ++++
 src/qemu/qemu_command.c                            |  5 ++++
 src/qemu/qemu_domain.c                             | 33 ++++++++++++++++++++--
 .../disk-drive-network-tlsx509.args                |  9 +++++-
 .../disk-drive-network-tlsx509.xml                 |  8 ++++++
 tests/qemuxml2argvtest.c                           |  2 +-
 .../disk-drive-network-tlsx509.xml                 |  8 ++++++
 7 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 703a1bb6f8..ce2d1e91e0 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1706,6 +1706,11 @@
       <optional>
         <attribute name="name"/>
       </optional>
+      <optional>
+        <attribute name="tls">
+          <ref name="virYesNo"/>
+        </attribute>
+      </optional>
       <ref name="diskSourceNetworkHost"/>
       <optional>
         <ref name="encryption"/>
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 134e1a3a20..07fa35c6b3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1392,6 +1392,11 @@ qemuDiskSourceNeedsProps(virStorageSourcePtr src,
         virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET))
         return true;

+    if (actualType == VIR_STORAGE_TYPE_NETWORK &&
+        src->protocol == VIR_STORAGE_NET_PROTOCOL_NBD &&
+        src->haveTLS == VIR_TRISTATE_BOOL_YES)
+        return true;
+
     return false;
 }

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e329cdf958..db7884a9a1 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9937,6 +9937,29 @@ qemuProcessPrepareStorageSourceTlsVxhs(virStorageSourcePtr src,
 }


+static int
+qemuProcessPrepareStorageSourceTlsNbd(virStorageSourcePtr src,
+                                      virQEMUDriverConfigPtr cfg,
+                                      virQEMUCapsPtr qemuCaps)
+{
+    /* XXX: for NBD we don't have the qemu.conf knobs for private TLS env */
+    if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
+        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("this qemu does not support TLS transport for nbd"));
+            return -1;
+        }
+
+        if (VIR_STRDUP(src->tlsCertdir, cfg->defaultTLSx509certdir) < 0)
+            return -1;
+
+        src->tlsVerify = true;
+    }
+
+    return 0;
+}
+
+
 /* qemuProcessPrepareStorageSourceTLS:
  * @source: source for a disk
  * @cfg: driver configuration
@@ -9951,7 +9974,8 @@ qemuProcessPrepareStorageSourceTlsVxhs(virStorageSourcePtr src,
 static int
 qemuDomainPrepareStorageSourceTLS(virStorageSourcePtr src,
                                   virQEMUDriverConfigPtr cfg,
-                                  const char *parentAlias)
+                                  const char *parentAlias,
+                                  virQEMUCapsPtr qemuCaps)
 {
     if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK)
         return 0;
@@ -9963,6 +9987,10 @@ qemuDomainPrepareStorageSourceTLS(virStorageSourcePtr src,
         break;

     case VIR_STORAGE_NET_PROTOCOL_NBD:
+        if (qemuProcessPrepareStorageSourceTlsNbd(src, cfg, qemuCaps) < 0)
+            return -1;
+        break;
+
     case VIR_STORAGE_NET_PROTOCOL_RBD:
     case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
     case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
@@ -12505,7 +12533,8 @@ qemuDomainPrepareDiskSourceLegacy(virDomainDiskDefPtr disk,
     if (qemuDomainPrepareStorageSourcePR(disk->src, priv, disk->info.alias) < 0)
         return -1;

-    if (qemuDomainPrepareStorageSourceTLS(disk->src, cfg, disk->info.alias) < 0)
+    if (qemuDomainPrepareStorageSourceTLS(disk->src, cfg, disk->info.alias,
+                                          priv->qemuCaps) < 0)
         return -1;

     return 0;
diff --git a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
index 91d3a8a70a..970b8a32a6 100644
--- a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
+++ b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
@@ -43,4 +43,11 @@ id=virtio-disk1 \
 file.server.host=192.168.0.3,file.server.port=9999,format=raw,if=none,\
 id=drive-virtio-disk2,serial=eb90327c-8302-4725-9e1b-4e85ed4dc252,cache=none \
 -device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk2,\
-id=virtio-disk2
+id=virtio-disk2 \
+-object tls-creds-x509,id=objvirtio-disk3_tls0,dir=/etc/pki/qemu,\
+endpoint=client,verify-peer=yes \
+-drive file.driver=nbd,file.server.type=inet,file.server.host=example.com,\
+file.server.port=1234,file.tls-creds=objvirtio-disk3_tls0,format=raw,if=none,\
+id=drive-virtio-disk3,cache=none \
+-device virtio-blk-pci,bus=pci.0,addr=0x7,drive=drive-virtio-disk3,\
+id=virtio-disk3
diff --git a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.xml b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.xml
index a66e81f065..9f6f298b54 100644
--- a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.xml
+++ b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.xml
@@ -41,6 +41,14 @@
       <serial>eb90327c-8302-4725-9e1b-4e85ed4dc252</serial>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
     </disk>
+    <disk type='network' device='disk'>
+      <driver name='qemu' type='raw' cache='none'/>
+      <source protocol='nbd' tls='yes'>
+        <host name='example.com' port='1234'/>
+      </source>
+      <target dev='vdd' bus='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
+    </disk>
     <controller type='usb' index='0'/>
     <controller type='pci' index='0' model='pci-root'/>
     <input type='mouse' bus='ps2'/>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a67d95f471..53b8b31a46 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1051,7 +1051,7 @@ mymain(void)
     DO_TEST("disk-drive-network-vxhs", QEMU_CAPS_VXHS);
     driver.config->vxhsTLS = 1;
     DO_TEST("disk-drive-network-tlsx509", QEMU_CAPS_VXHS,
-            QEMU_CAPS_OBJECT_TLS_CREDS_X509);
+            QEMU_CAPS_OBJECT_TLS_CREDS_X509, QEMU_CAPS_NBD_TLS);
     driver.config->vxhsTLS = 0;
     VIR_FREE(driver.config->vxhsTLSx509certdir);
     DO_TEST("disk-drive-no-boot",
diff --git a/tests/qemuxml2xmloutdata/disk-drive-network-tlsx509.xml b/tests/qemuxml2xmloutdata/disk-drive-network-tlsx509.xml
index 7053affd17..a9b8d32646 100644
--- a/tests/qemuxml2xmloutdata/disk-drive-network-tlsx509.xml
+++ b/tests/qemuxml2xmloutdata/disk-drive-network-tlsx509.xml
@@ -41,6 +41,14 @@
       <serial>eb90327c-8302-4725-9e1b-4e85ed4dc252</serial>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
     </disk>
+    <disk type='network' device='disk'>
+      <driver name='qemu' type='raw' cache='none'/>
+      <source protocol='nbd' tls='yes'>
+        <host name='example.com' port='1234'/>
+      </source>
+      <target dev='vdd' bus='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
+    </disk>
     <controller type='usb' index='0'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
     </controller>
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, May 30, 2018 at 02:41:34PM +0200, Peter Krempa wrote:
>Use the default TLS env if TLS is required for NBD. The rest of the
>implementation is rather simple since all pieces were in place.
>
>Note that separate configuration knobs in qemu.conf can be added later
>if it's desired to configure them.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> docs/schemas/domaincommon.rng                      |  5 ++++
> src/qemu/qemu_command.c                            |  5 ++++
> src/qemu/qemu_domain.c                             | 33 ++++++++++++++++++++--
> .../disk-drive-network-tlsx509.args                |  9 +++++-
> .../disk-drive-network-tlsx509.xml                 |  8 ++++++
> tests/qemuxml2argvtest.c                           |  2 +-
> .../disk-drive-network-tlsx509.xml                 |  8 ++++++
> 7 files changed, 66 insertions(+), 4 deletions(-)
>

>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index e329cdf958..db7884a9a1 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -9937,6 +9937,29 @@ qemuProcessPrepareStorageSourceTlsVxhs(virStorageSourcePtr src,
> }
>
>
>+static int
>+qemuProcessPrepareStorageSourceTlsNbd(virStorageSourcePtr src,

Please, TLSNBD.

>+                                      virQEMUDriverConfigPtr cfg,
>+                                      virQEMUCapsPtr qemuCaps)
>+{
>+    /* XXX: for NBD we don't have the qemu.conf knobs for private TLS env */
>+    if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
>+        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
>+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                           _("this qemu does not support TLS transport for nbd"));

NBD

>+            return -1;
>+        }
>+
>+        if (VIR_STRDUP(src->tlsCertdir, cfg->defaultTLSx509certdir) < 0)
>+            return -1;
>+
>+        src->tlsVerify = true;
>+    }
>+
>+    return 0;
>+}
>+
>+

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/30/2018 08:41 AM, Peter Krempa wrote:
> Use the default TLS env if TLS is required for NBD. The rest of the
> implementation is rather simple since all pieces were in place.
> 
> Note that separate configuration knobs in qemu.conf can be added later
> if it's desired to configure them.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  docs/schemas/domaincommon.rng                      |  5 ++++
>  src/qemu/qemu_command.c                            |  5 ++++
>  src/qemu/qemu_domain.c                             | 33 ++++++++++++++++++++--
>  .../disk-drive-network-tlsx509.args                |  9 +++++-
>  .../disk-drive-network-tlsx509.xml                 |  8 ++++++
>  tests/qemuxml2argvtest.c                           |  2 +-
>  .../disk-drive-network-tlsx509.xml                 |  8 ++++++
>  7 files changed, 66 insertions(+), 4 deletions(-)
> 

[...]

> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e329cdf958..db7884a9a1 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -9937,6 +9937,29 @@ qemuProcessPrepareStorageSourceTlsVxhs(virStorageSourcePtr src,
>  }
> 
> 
> +static int
> +qemuProcessPrepareStorageSourceTlsNbd(virStorageSourcePtr src,
> +                                      virQEMUDriverConfigPtr cfg,
> +                                      virQEMUCapsPtr qemuCaps)
> +{
> +    /* XXX: for NBD we don't have the qemu.conf knobs for private TLS env */

I believe the thought was to use the migrate ones and not default. That
way we could modify the qemu.conf to note that the migrate environment
would be used for NBD as it made no sense to have/use separate envs.

> +    if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("this qemu does not support TLS transport for nbd"));
> +            return -1;
> +        }
> +
> +        if (VIR_STRDUP(src->tlsCertdir, cfg->defaultTLSx509certdir) < 0)
> +            return -1;
> +
> +        src->tlsVerify = true;

I think this is problematic for the default environment w/r/t since the
right certs won't be present...

John


[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, May 30, 2018 at 18:55:34 -0400, John Ferlan wrote:
> 
> 
> On 05/30/2018 08:41 AM, Peter Krempa wrote:
> > Use the default TLS env if TLS is required for NBD. The rest of the
> > implementation is rather simple since all pieces were in place.
> > 
> > Note that separate configuration knobs in qemu.conf can be added later
> > if it's desired to configure them.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  docs/schemas/domaincommon.rng                      |  5 ++++
> >  src/qemu/qemu_command.c                            |  5 ++++
> >  src/qemu/qemu_domain.c                             | 33 ++++++++++++++++++++--
> >  .../disk-drive-network-tlsx509.args                |  9 +++++-
> >  .../disk-drive-network-tlsx509.xml                 |  8 ++++++
> >  tests/qemuxml2argvtest.c                           |  2 +-
> >  .../disk-drive-network-tlsx509.xml                 |  8 ++++++
> >  7 files changed, 66 insertions(+), 4 deletions(-)
> > 
> 
> [...]
> 
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index e329cdf958..db7884a9a1 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -9937,6 +9937,29 @@ qemuProcessPrepareStorageSourceTlsVxhs(virStorageSourcePtr src,
> >  }
> > 
> > 
> > +static int
> > +qemuProcessPrepareStorageSourceTlsNbd(virStorageSourcePtr src,
> > +                                      virQEMUDriverConfigPtr cfg,
> > +                                      virQEMUCapsPtr qemuCaps)
> > +{
> > +    /* XXX: for NBD we don't have the qemu.conf knobs for private TLS env */
> 
> I believe the thought was to use the migrate ones and not default. That
> way we could modify the qemu.conf to note that the migrate environment
> would be used for NBD as it made no sense to have/use separate envs.

No. The migration environment shall be used only for NBD when migrating
disks. This is already the case by the way.

For accessing regular disks we should use the default one or a specific
one (e.g. as we do have for vxhs) if that will ever be added.

The separate environment might be wanted in the future if somebody wants
to have separate certificates for it, but it's not strictly required and
can easily be retrofitted into this optional way.

> > +    if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
> > +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("this qemu does not support TLS transport for nbd"));
> > +            return -1;
> > +        }
> > +
> > +        if (VIR_STRDUP(src->tlsCertdir, cfg->defaultTLSx509certdir) < 0)
> > +            return -1;
> > +
> > +        src->tlsVerify = true;
> 
> I think this is problematic for the default environment w/r/t since the
> right certs won't be present...

Please elaborate on this. I didn't quite get what you meant.

> 
> John
> 
> 
> [...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

>>> +qemuProcessPrepareStorageSourceTlsNbd(virStorageSourcePtr src,
>>> +                                      virQEMUDriverConfigPtr cfg,
>>> +                                      virQEMUCapsPtr qemuCaps)
>>> +{
>>> +    /* XXX: for NBD we don't have the qemu.conf knobs for private TLS env */
>>
>> I believe the thought was to use the migrate ones and not default. That
>> way we could modify the qemu.conf to note that the migrate environment
>> would be used for NBD as it made no sense to have/use separate envs.
> 
> No. The migration environment shall be used only for NBD when migrating
> disks. This is already the case by the way.
> 
> For accessing regular disks we should use the default one or a specific
> one (e.g. as we do have for vxhs) if that will ever be added.
> 
> The separate environment might be wanted in the future if somebody wants
> to have separate certificates for it, but it's not strictly required and
> can easily be retrofitted into this optional way.
> 

And how would anyone really know this? Why was this decision was made in
favor of creating an NBD specific set of values. Ironically not a
shortcut we've used/allowed for when adding TLS to chardev, migrate, or
vxhs.


>>> +    if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
>>> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                           _("this qemu does not support TLS transport for nbd"));
>>> +            return -1;
>>> +        }
>>> +
>>> +        if (VIR_STRDUP(src->tlsCertdir, cfg->defaultTLSx509certdir) < 0)
>>> +            return -1;
>>> +
>>> +        src->tlsVerify = true;
>>
>> I think this is problematic for the default environment w/r/t since the
>> right certs won't be present...
> 
> Please elaborate on this. I didn't quite get what you meant.
> 

tlsVerify is what's used for the verifypeer - in order for it to be
useful, then the default environment would need:

#  client-cert.pem - the client certificate signed with the ca-cert.pem
#  client-key.pem - the client private key

if the default environment doesn't have those, then blindly setting this
will cause a TLS failure if the default environment doesn't have those
files.

Since you're using cfg->defaultTLSx509certdir, then this should use
cfg->defaultTLSx509verify.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, May 31, 2018 at 07:34:14 -0400, John Ferlan wrote:
> [...]
> 
> >>> +qemuProcessPrepareStorageSourceTlsNbd(virStorageSourcePtr src,
> >>> +                                      virQEMUDriverConfigPtr cfg,
> >>> +                                      virQEMUCapsPtr qemuCaps)
> >>> +{
> >>> +    /* XXX: for NBD we don't have the qemu.conf knobs for private TLS env */
> >>
> >> I believe the thought was to use the migrate ones and not default. That
> >> way we could modify the qemu.conf to note that the migrate environment
> >> would be used for NBD as it made no sense to have/use separate envs.
> > 
> > No. The migration environment shall be used only for NBD when migrating
> > disks. This is already the case by the way.
> > 
> > For accessing regular disks we should use the default one or a specific
> > one (e.g. as we do have for vxhs) if that will ever be added.
> > 
> > The separate environment might be wanted in the future if somebody wants
> > to have separate certificates for it, but it's not strictly required and
> > can easily be retrofitted into this optional way.
> > 
> 
> And how would anyone really know this? Why was this decision was made in
> favor of creating an NBD specific set of values. Ironically not a
> shortcut we've used/allowed for when adding TLS to chardev, migrate, or
> vxhs.

Well, this patch is mostly a simple implementation of TLS which allows
to use the default environment. Adding all the bloat necessary to setup
custom environment was not really a focus of this series.

I can move out this patch into hold if you think that the private
environment is necessary right from the beginning since adding TLS for
NBD wasn't really the main focus.

> 
> 
> >>> +    if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
> >>> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
> >>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >>> +                           _("this qemu does not support TLS transport for nbd"));
> >>> +            return -1;
> >>> +        }
> >>> +
> >>> +        if (VIR_STRDUP(src->tlsCertdir, cfg->defaultTLSx509certdir) < 0)
> >>> +            return -1;
> >>> +
> >>> +        src->tlsVerify = true;
> >>
> >> I think this is problematic for the default environment w/r/t since the
> >> right certs won't be present...
> > 
> > Please elaborate on this. I didn't quite get what you meant.
> > 
> 
> tlsVerify is what's used for the verifypeer - in order for it to be
> useful, then the default environment would need:
> 
> #  client-cert.pem - the client certificate signed with the ca-cert.pem
> #  client-key.pem - the client private key

These are required for verifying that the client is allowed to contact
the server ...

> if the default environment doesn't have those, then blindly setting this
> will cause a TLS failure if the default environment doesn't have those
> files.

No, that's not how it works. Both NBD and VXHS are 'clients' so they
always need to verify the server. [1]

> Since you're using cfg->defaultTLSx509certdir, then this should use
> cfg->defaultTLSx509verify.

In fact, tlsVerify for disks is pointless as long as we don't have
server-mode disks.

I'll actually try to remove that variable for now.

> 
> John
> 

[1] https://security.libvirt.org/2017/0002.html
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list