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