On 10/15/2016 09:37 AM, Pavel Hrdina wrote:
On Fri, Oct 14, 2016 at 03:49:13PM -0400, John Ferlan wrote:
On 10/14/2016 11:30 AM, Pavel Hrdina wrote:
On Fri, Oct 14, 2016 at 10:58:12AM -0400, John Ferlan wrote:
[...]
UGH... mea culpa - in my mind the TLS hadn't been added to the chardev yet and that was the rest of this series... Of course the rest of this series is adding the passphrase for the environment (<sigh>).
If I could have got that review earlier, then this situation wouldn't be a problem (see in my mind it wasn't).
If a domain is already running, then encryption is occurring and this setting has no effect except for hotplug.
If we were using encryption in 2.3.0... stopped our domain... installed 2.4.0... started our domain, then yes, I see/agree.... Frustrating since there's really no "simple" way to determine this.
> > The correct solution is: > > - if chardev_tls is set to 1 and tls attribute is not configured in the XML > the default after starting the domain should be tls=yes
So IOW:
+ if (cfg->chardevTLS && + dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) {
changes to
+ if (cfg->chardevTLS && + dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) {
or (haveTLS == YES || haveTLS == ABSENT)
This of course is essentially the logic from v6 which said disableTLS on a case by case basis.... All we have now is the positive form...
So for both qemuBuildChrChardevStr and qemuDomainAttachChrDevice I'd have a similar change. Does that suffice and do you need to see the change?
I think that we should reflect the state in live XML if the attribute exists.
Add a code to qemuProcessPrepareDomain() that will set dev->data.tcp.haveTLS to config value if dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_ABSENT and in qemuBuildChrChardevStr() you can just check if dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES. This will also ensure that the live XML contains the 'tls' attribute. But make sure, that migration works properly for all combinations.
I don't see the advantage... Altering the running domain would involve
The advantage is in case when the chardevTLS is set but the offline XML doesn't have 'tls' attribute configured. If the domain is started than it's perfectly clear from the live XML that the tls is enabled or not.
Maybe I didn't explain it well enough... I don't see the advantage of modifying qemuProcessPrepareDomain or qemuProcessAttach to try and determine whether we should "enable" this property. I see no need to introspect to inspect a setting in order to then make some assumption whether the property should be enabled or not. We'll get ourselves in trouble doing so. Too much complexity, too much danger for making a bad assumption or decision.
I believe I already agreed that the concept of this patch was wrong given that it's enabling a feature that could already be enabled. Like I pointed out - my mindset was geared toward what the code was in September before we had a release with the host wide changes incorporated... The initial 'disableTLS' was written right after the code was pushed and rewritten to tls='yes|no' model after review. In any case, I think it does point out a reason/need to be sure patches are considered in a timely manner! Not any one person's fault - it just something to be more diligent about. I could have pinged a little harder too! With v9, the concept of enabling a feature that's already enabled is no longer the methodology used. Rather it's disable a feature on a case by case basis, by choice. Still what's written below shouldn't be lost because I think it's very valuable for the next person that falls into the same trap needing to do adjust a feature that could already be 'in use'. I would hope that something could be added to the hacking page or a wiki page for things that need to be considered...
There is a small complexity but noting danger and nothing slick
- in qemuProcessPrepareDomain: if (dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_ABSENT) { dev->data.tcp.haveTLS = cfg->chardevTLS; dev->data.tcp.tlsFromConfig = true; }
- in qemuProcessAttach: the detection of TCP chardev is broken and doesn't even work, but if someone fix it than it's also simple
if (opt && strstr(opt, "tls-creds")) dev->data.tcp.haveTLS = true;
FWIW: A quick look in qemuProcessAttach doesn't have 'opt'... But then again isn't this for the qemu-attach processing, not the same path used when a new libvirtd starts and finds the currently running domains. My brain hurts every time I dig into that code.
NOTE: this domain was executed outside of libvirt so it was not based on config
- in virDomainChrSourceDefParseXML: add code to parse "fromConfig" if (flags & VIR_DOMAIN_DEF_PARSE_STATUS)
- in virDomainChrSourceDefFormat: if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT && !(flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE && def->data.tcp.tlsFromConfig)) virBufferAsprintf(buf, " tls='%s'", virTristateBoolTypeToString(def->data.tcp.haveTLS));
if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS) virBufferAsprintf(buf, " fromConfig='%d'", def->data.tcp.tlsFromConfig);
I can see that someone may not like this approach, but this is how it works in libvirt. It's our mess that we introduce a feature and in next release we introduce an attribute to control/reflect that feature via XML.
Like or dislike, it's not exactly what "simply" comes to mind when adding a new attribute... There is a bit of complexity and knowledge perhaps gained from having done this before that makes it easier the second, third, fourth, etc. time around... Hence why I suggest a wiki/hacking entry to describe what needs to be considered.
Try to look at it from user POV, there is a config option that can enable TLS encryption for all domains and all chardevs, but there is also 'tls' attribute that can be used to disable TLS encryption if it's set in qemu.conf or enable TLS encryption if only proper certificates are set in qemu.conf. So if someone looks at the live XML, if there is an 'tls' attribute, it's clear, but if the attribute is missing, I would have to remember/check some different place to see whether TLS encryption is configured or not. I would not like this and I would asking a question why libvirt cannot reflect this case in live XML?
I get it - again my POV wasn't from the perspective that a release could already have TLS enabled for a domain... Do you think this still applies given v9 logic to add the disable attribute? User sets chardev_tls = 1 in qemu.conf... User is happy. One day user realizes that I have this domain that I don't want that. User finds the tls flag for their <serial type='tcp'> <source ...> chardev. User sets attribute tls='no' on that chardev and is happy again. Is there a live XML concern with tls='no' ('yes' and 'absent' follow the host setting).
[...]
Finally, yeah migration is the final nail in the coffin for this. How does one migrate from 2.4 to 2.3 if 2.3 doesn't have this attribute we just quietly set to YES for them?
Well if the attribute is set to YES and the chardevTLS is set to 1 or if the attribute is set to NO and chardevTLS is set to 0 we can safely remove the attribute from migratable XML, because it's on user to ensure that the configuration is properly set on both sides, for other cases that migration should not be allowed because whether the encryption is enabled or not could be changed.
I think avoiding touching the migration XML is far more important than trying to figure out all the possible permutations in order to come up with some slick way to modify XML on the fly.
I don't agree and we already do a lot of stuff to create a proper migratable XML that is backward compatible if no new feature was used. Unfortunately the 'tls' attribute isn't a new feature, the feature already exists in previous libvirt release and therefore we have to handle it properly to not break migration.
Again, in the mindset of tls='no' has to be set - is there a concern still? One concern I can think of is if hostA is using TLS via qemu.conf, we migrate the domain to hostB which hasn't configure TLS - now what? Something to dig into on Monday... John