[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Don't add SPICE TLS channels when TLS is disabled



On Tue, Feb 14, 2012 at 02:10:37PM -0700, Eric Blake wrote:
> Meta-question - if the XML requests secure, but TLS is disabled, should
> we instead be failing to start the domain with a complaint that we can't
> honor the XML?

Meta-non-answer, when a TLS port is set but TLS is disabled in the config
file, it's silently ignored:

if (driver->spiceTLS && def->graphics[0]->data.spice.tlsPort != -1)
    virBufferAsprintf(&opt, ",tls-port=%u", def->graphics[0]->data.spice.tlsPort);

so I just did the same for the secure channels. If we decide an error
should be returned, this can be done in a follow-up patch changing both
locations.

> 
> If the answer is that we should start the domain anyways, and there is
> just no security, because whoever disabled tls in qemu.conf knows what
> they are doing, then ACK.  If the answer is that we should error out on
> an impossible configuration,

My initial feeling was that we should error out. However, I don't think
it's realistic at this point:
* oVirt is already disabling TLS in qemu.conf if the admin says he does not
  want it during engine-setup, but oVirt still adds the TLS port/secure
  channels to its libvirt VMs even when TLS is disabled. They probably
  won't be that happy if libvirt starts erroring out here. Though we can
  also argue that it's a bug on their side, and that the VMs they start this
  way (with TLS XML bits but TLS disabled in the conf file) are not usable
  anyway because of these extra tls-channel options
* this will also be annoying if someone has setup a bunch of VMs with TLS
  parameters and then decides to change the TLS setting in qemu.conf, these
  VMs will suddenly fail to start until their conf is modified

Because of that, I think we should unfortunately just ignore these TLS bits
when TLS is disabled. We can log a warning though when we detect this. Once
again, regardless of what we decide to do, it's better done in a separate
patch.

Christophe

Attachment: pgpuHLAW2cF8F.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]