[libvirt] [PATCH v8 1/5] domain: Add optional 'tls' attribute for TCP chardev

[John Ferlan]

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