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 went back to the v5 review and reconsidered Dan's review points:
1. "As default behaviour I think it is desirable that we can turn TLS on
for every VM at once - I tend to view it as a host network integration
task, rather than a VM configuration task. Same rationale that we use
for TLS wth VNC/SPICE."
2. "There's no reason we can't have a tri-state TLS flag against the
chardev in the XML too, to override the default behaviour of
cfg->chardevTLS"
>From which I generated a v6 to "Add an optional "disableTLS='yes'"
option"... For which Dan points out:
"Can we just call this "tls=yes|no" - negative attributes (disableFOO)
are kind of ugly IMHO, as they imply that the default status is
enabled, which is not really the case - the default is hypervisor
specific and undefined."
So, now there's a v7 which adds "tls='yes|no'", where the default is no
and OK quite rightfully in hindsight is not what was originally
requested. That's left unchanged into this v8 until you call it out.
So where does that leave me - well the "tls='yes|no'" property still
should exist. What changes is that rather than "enforcing" that it gets
set to "yes" in order to make something work that worked before without
doing so, the default should be that whatever is in qemu.conf takes
priority unless someone adds "tls='no'" to the chardev entry.
That's what I'll change things to and document in a soon to be posted v9.
>> messing in qemuProcessAttach and setting the attribute for the domain if
>> chardevTLS is set *BUT* how do we know without a bit of introspection if
>> that domain was started with TLS and is using TLS? (IOW: looking at the
>> running domain for tls-creds-x509). Additionally, someone could have
>
> Well, we do that introspection and we must do it to properly detect what has
> been used to start that domain. The domain XML should reflect the current state
> as closely as possible.
>
>> changed the chardevTLS setting in qemu.conf after the domain was
>> initially started and thus it wouldn't have it, so setting it blindly
>
> Yes, someone can change the chardevTLS after the domain was started and it won't
> break anything. If the domain is destroyed and started again, everything works,
> if the domain is saved and started again it will start with TLS encryption
> enabled. If someone removes certificates from qemu.conf and completely disable
> TLS encryption and the domain was saved with TLS encryption configured it's
> desirable that it should error out while restoring that domain.
>
>> would probably have disastrous results.
>>
>> Altering a domain definition during qemuProcessPrepareDomain (e.g.
>> domain startup time) still has no way of determining if that domain had
>> ever been started using TLS if it's not in the XML.
>
> I don't understand why we need to know if the domain had ever been started using
> TLS?
>
I assumed that's what you were asking... The qemuProcessPrepareDomain is
called in the qemuProcessStart path. So that to me says there was
concern over domain startup.
>> 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.
John
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list