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

[...] >>> +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