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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
