[libvirt] [PATCH 38/38] qemu: domain: Add support for TLS for NBD with default TLS env

[Peter Krempa]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180531/0c4a4610/attachment-0001.sig>