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

2016-10-18 Thread John Ferlan 


On 10/18/2016 07:21 AM, Daniel P. Berrange wrote:
> On Tue, Oct 18, 2016 at 06:59:57AM -0400, John Ferlan wrote:
>>
>>
>> On 10/18/2016 02:27 AM, Pavel Hrdina wrote:
>> [...]
>>

 "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."
>>>
>>> Don't forget this part of the same review:
>>>
>>> "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"
>>>
>>> That also means to override chardev_tls = "0" by "tls='yes'".
>>
>> If the default cfg behaviour is "1", then that tells us "someone" has
>> set up the TLS environment and thus the domain/chardev override would be
>> "no".
>>
>> If the default cfg behaviour is "0", then that means we cannot guarantee
>> the environment necessary has been set up and we cannot allow the
>> domain/chardev setting to enable TLS.
> 
> We have two separate tasks at the host level
> 
>  - Setup of TLS certificates (ie put the PEM files in the right places)
>  - Global default for use of TLS by chardevs
> 
> We only have a config option in qemu.conf for the latter. ie if
> chardev_tls=1, this is implicitly saying that TLS certs are deployed
> in right place.  IIUC, you're saying that if chardev_tls=0, then we
> should interpret that to meant TLS certs are *not* deployed.
> 

If someone has gone through the trouble of setting up their environment
because they found in qemu.conf support was added, why would they set
chardev_tls=0 afterwards? IOW: Are we assuming something or are we over
engineering things?

Why would someone set up and define chardevTLSx509certdir and then have
chardevTLS=0? Maybe, they believe this will disable TLS for every
domain. After all, why would disabling a global setting allow some
localized setting override that.

Just because there's a "default" environment, why should we assume that
because they added "tls='yes'" into their domain that that is the
environment they want?

Maybe they do have a chardev specific environment, but also decided to
comment it out when they commented out chardev_tls=1? So we're assuming
that by finding that "tls='yes'" in the domain that the domain should
use that environment? How can we be so sure that's what's desired?

This is the problem with two triggers - which one do you pull and which
one has a bullet in the chamber? We're having a hard enough time
agreeing on the implementation. Now the customer has to figure out all
the permutations and possibilities.

Personally, if I disable a global variable/value, then I wouldn't expect
some other definition to override that. For example, if I set my SELinux
to be "permissive" or "disabled", then I'm probably not very happy if
something ignores that and decides to be "enforcing".

> Pavel is saying that if chardev_tls=0 in qemu.conf, and tls=1 in the
> XML, then we should assume that TLS certs *are* deployed on the host
> in the right place. I think this is not unreasonable - we can easily
> check to see if the certs exist on disk in this case.

FWIW: Remember how qemu_conf.c sets things up...

During qemuStateInitialize, we call virQEMUDriverConfigNew which will
set a default directory path for "cfg->defaultTLSx509certdir" and then
will use that as a default value for vnc, spice, and chardev. There is
no check "if" that directory structure exists.

Once the defaults are set up, then if "chardev_tls_x509_cert_dir" is
defined, it will override the default (similar for others).

So chardevTLSx509certdir always has some default value, but is that the
certificate environment we want to use for chardev's? How do we know?

> 
> IOW, I agree that we should have a tri-state at the XML level
> 
>  - no tls attribute in XML - honour  chardev_tls from qemu.conf
>  - tls=1 in XML, then turn on TLS
>  - tls=0 in XML, then don't use TLS
> 

So if we get this far, then a qemu_domain.c function, such as

bool
qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg,
   const virDomainChrSourceDef *dev)
{
if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO)
return true;
if (!cfg->chardevTLS && dev->data.tcp.haveTLS ==
VIR_TRISTATE_BOOL_YES &&
virFileExists(cfg->chardevTLSx509certdir))
return true;
return false;
}

where I suppose we could also compare chardevTLSx509certdir and
defaultTLSx509certdir... Still how do we know that by merely having the
environment there and "tls='yes'" in the domain chardev config that this
is what's desired.  It might well be, but it may also be that the
clearing of the global chardev_tls was meant to disable regardless of
what the domain setting was.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-10-18 Thread Daniel P. Berrange 
On Tue, Oct 18, 2016 at 06:59:57AM -0400, John Ferlan wrote:
> 
> 
> On 10/18/2016 02:27 AM, Pavel Hrdina wrote:
> [...]
> 
> >>
> >> "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."
> > 
> > Don't forget this part of the same review:
> > 
> > "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"
> > 
> > That also means to override chardev_tls = "0" by "tls='yes'".
> 
> If the default cfg behaviour is "1", then that tells us "someone" has
> set up the TLS environment and thus the domain/chardev override would be
> "no".
> 
> If the default cfg behaviour is "0", then that means we cannot guarantee
> the environment necessary has been set up and we cannot allow the
> domain/chardev setting to enable TLS.

We have two separate tasks at the host level

 - Setup of TLS certificates (ie put the PEM files in the right places)
 - Global default for use of TLS by chardevs

We only have a config option in qemu.conf for the latter. ie if
chardev_tls=1, this is implicitly saying that TLS certs are deployed
in right place.  IIUC, you're saying that if chardev_tls=0, then we
should interpret that to meant TLS certs are *not* deployed.

Pavel is saying that if chardev_tls=0 in qemu.conf, and tls=1 in the
XML, then we should assume that TLS certs *are* deployed on the host
in the right place. I think this is not unreasonable - we can easily
check to see if the certs exist on disk in this case.

IOW, I agree that we should have a tri-state at the XML level

 - no tls attribute in XML - honour  chardev_tls from qemu.conf
 - tls=1 in XML, then turn on TLS
 - tls=0 in XML, then don't use TLS

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-10-18 Thread Pavel Hrdina 
On Tue, Oct 18, 2016 at 06:59:57AM -0400, John Ferlan wrote:
> 
> 
> On 10/18/2016 02:27 AM, Pavel Hrdina wrote:
> [...]
> 
> >>
> >> "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."
> > 
> > Don't forget this part of the same review:
> > 
> > "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"
> > 
> > That also means to override chardev_tls = "0" by "tls='yes'".
> 
> If the default cfg behaviour is "1", then that tells us "someone" has
> set up the TLS environment and thus the domain/chardev override would be
> "no".
> 
> If the default cfg behaviour is "0", then that means we cannot guarantee
> the environment necessary has been set up and we cannot allow the
> domain/chardev setting to enable TLS. 

Likewise someone can modify only qemu.conf without preparing certificates and
the domain would fail to start.  Libvirt cannot guarantee that the environment
is configured in any case.  So if someone doesn't setup the environment and
uses "tls='yes'" libvirt will print sane error message from qemu that the
certificate files doesn't exist.

Pavel


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-10-18 Thread John Ferlan 


On 10/18/2016 02:27 AM, Pavel Hrdina wrote:
[...]

>>
>> "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."
> 
> Don't forget this part of the same review:
> 
> "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"
> 
> That also means to override chardev_tls = "0" by "tls='yes'".

If the default cfg behaviour is "1", then that tells us "someone" has
set up the TLS environment and thus the domain/chardev override would be
"no".

If the default cfg behaviour is "0", then that means we cannot guarantee
the environment necessary has been set up and we cannot allow the
domain/chardev setting to enable TLS.



John


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-10-18 Thread Pavel Hrdina 
On Mon, Oct 17, 2016 at 11:24:58AM -0400, John Ferlan wrote:
> 
> 
> On 10/17/2016 10:37 AM, Pavel Hrdina wrote:
> > On Mon, Oct 17, 2016 at 09:54:46AM -0400, John Ferlan wrote:
> >>
> >>
> >> On 10/17/2016 04:09 AM, Pavel Hrdina wrote:
> >>> On Fri, Oct 14, 2016 at 04:23:04PM -0400, John Ferlan wrote:
>  Add an optional "tls='yes|no'" attribute for a TCP chardev for the
>  express purpose to disable setting up TLS for the specific chardev in
>  the event the qemu.conf settings have enabled hypervisor wide TLS for
>  serial TCP chardevs.
> 
>  Signed-off-by: John Ferlan 
>  ---
>   docs/formatdomain.html.in  | 21 +
>   docs/schemas/domaincommon.rng  |  5 +++
>   src/conf/domain_conf.c | 22 +-
>   src/conf/domain_conf.h |  1 +
>   src/qemu/qemu_command.c|  3 +-
>   src/qemu/qemu_hotplug.c|  3 +-
>   ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +
>   ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 
>  ++
>   .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml|  2 +-
>   tests/qemuxml2argvtest.c   |  3 ++
>   ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml |  1 +
>   .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml  |  2 +-
>   tests/qemuxml2xmltest.c|  1 +
>   13 files changed, 139 insertions(+), 5 deletions(-)
>   create mode 100644 
>  tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
>   create mode 100644 
>  tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
>   create mode 12 
>  tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
> 
>  diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>  index 9051178..6145d65 100644
>  --- a/docs/formatdomain.html.in
>  +++ b/docs/formatdomain.html.in
>  @@ -6204,6 +6204,27 @@ qemu-kvm -net nic,model=? /dev/null
> /devices
> ...
>   
>  +
>  +  Since 2.4.0, the optional attribute
>  +  tls can be used to control whether a serial chardev
>  +  TCP communication channel would utilize a hypervisor configured
>  +  TLS X.509 certificate environment in order to encrypt the data
>  +  channel. For the QEMU hypervisor usage of TLS is controlled by the
>  +  chardev_tls setting in file /etc/libvirt/qemu.conf. 
>  If
>  +  enabled, then by setting this attribute to "no" will disable usage
>  +  of the TLS environment for this particular serial TCP data 
>  channel.
>  +
> >>>
> >>> Previous discussion:
> >>>
> >>> http://www.redhat.com/archives/libvir-list/2016-October/msg00734.html
> >>>
> >>> So now there is no functional issue if we agree that this design is what 
> >>> we
> >>> want.  I personally thing that there could be also use-case where you 
> >>> want to
> >>> configure certificates in qemu.conf and use 'tls' attribute to explicitly
> >>> enable TLS encryption only for some VMs and only for some chardev and 
> >>> this is
> >>> not currently possible whit this design.  Now user can enable the TLS 
> >>> encryption
> >>> for every chardev for every domain and explicitly disable for some 
> >>> chardevs.
> >>>
> >>> This would require to add all the extra code from that discussion to 
> >>> handle
> >>> migration properly and that's why I've started the discussion.
> >>>
> >>
> >> w/r/t: design - re-read what I pulled from Dan's v5 review:
> >>
> >> http://www.redhat.com/archives/libvir-list/2016-October/msg00698.html
> >>
> >> While forcing to set "tls='yes'" is certainly a mechanism that could be
> >> used and adding extra code is possible, is that something that we want
> >> to require of everyone that's using TLS chardev's? If you go through the
> >> trouble of configuring for your host, then how would you feel about
> >> having to update all your domains (for those that have more than 1 or 2)
> >> to set the property in order to be able to use it? Again, I really don't
> > 
> > I have a feeling that we are having trouble to understand each other.  I'm 
> > not
> > saying that you will have to set "tls='yes'" for every domain and every 
> > chardev.
> > Forcing to set "tls='no'" for every domain and every chardevs if you want 
> > to use
> > TLS only for one domain is not a good mechanism.
> 
> Clearly ;-)  Setting "tls='yes'" wasn't my implication either. The way
> this patch is written 'YES' or 'ABSENT' have the same meaning to take
> the host default.
> 
> So "forcing" setting of "tls='no'"
> > 
> > What I would like to achieve is this:
> > 
> > 1. Currently there is already existing "chardev_tls" config option that 
> > allows
> > you

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

2016-10-17 Thread John Ferlan 


On 10/17/2016 10:37 AM, Pavel Hrdina wrote:
> On Mon, Oct 17, 2016 at 09:54:46AM -0400, John Ferlan wrote:
>>
>>
>> On 10/17/2016 04:09 AM, Pavel Hrdina wrote:
>>> On Fri, Oct 14, 2016 at 04:23:04PM -0400, John Ferlan wrote:
 Add an optional "tls='yes|no'" attribute for a TCP chardev for the
 express purpose to disable setting up TLS for the specific chardev in
 the event the qemu.conf settings have enabled hypervisor wide TLS for
 serial TCP chardevs.

 Signed-off-by: John Ferlan 
 ---
  docs/formatdomain.html.in  | 21 +
  docs/schemas/domaincommon.rng  |  5 +++
  src/conf/domain_conf.c | 22 +-
  src/conf/domain_conf.h |  1 +
  src/qemu/qemu_command.c|  3 +-
  src/qemu/qemu_hotplug.c|  3 +-
  ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +
  ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 
 ++
  .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml|  2 +-
  tests/qemuxml2argvtest.c   |  3 ++
  ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml |  1 +
  .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml  |  2 +-
  tests/qemuxml2xmltest.c|  1 +
  13 files changed, 139 insertions(+), 5 deletions(-)
  create mode 100644 
 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
  create mode 100644 
 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
  create mode 12 
 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml

 diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
 index 9051178..6145d65 100644
 --- a/docs/formatdomain.html.in
 +++ b/docs/formatdomain.html.in
 @@ -6204,6 +6204,27 @@ qemu-kvm -net nic,model=? /dev/null
/devices
...
  
 +
 +  Since 2.4.0, the optional attribute
 +  tls can be used to control whether a serial chardev
 +  TCP communication channel would utilize a hypervisor configured
 +  TLS X.509 certificate environment in order to encrypt the data
 +  channel. For the QEMU hypervisor usage of TLS is controlled by the
 +  chardev_tls setting in file /etc/libvirt/qemu.conf. If
 +  enabled, then by setting this attribute to "no" will disable usage
 +  of the TLS environment for this particular serial TCP data channel.
 +
>>>
>>> Previous discussion:
>>>
>>> http://www.redhat.com/archives/libvir-list/2016-October/msg00734.html
>>>
>>> So now there is no functional issue if we agree that this design is what we
>>> want.  I personally thing that there could be also use-case where you want 
>>> to
>>> configure certificates in qemu.conf and use 'tls' attribute to explicitly
>>> enable TLS encryption only for some VMs and only for some chardev and this 
>>> is
>>> not currently possible whit this design.  Now user can enable the TLS 
>>> encryption
>>> for every chardev for every domain and explicitly disable for some chardevs.
>>>
>>> This would require to add all the extra code from that discussion to handle
>>> migration properly and that's why I've started the discussion.
>>>
>>
>> w/r/t: design - re-read what I pulled from Dan's v5 review:
>>
>> http://www.redhat.com/archives/libvir-list/2016-October/msg00698.html
>>
>> While forcing to set "tls='yes'" is certainly a mechanism that could be
>> used and adding extra code is possible, is that something that we want
>> to require of everyone that's using TLS chardev's? If you go through the
>> trouble of configuring for your host, then how would you feel about
>> having to update all your domains (for those that have more than 1 or 2)
>> to set the property in order to be able to use it? Again, I really don't
> 
> I have a feeling that we are having trouble to understand each other.  I'm not
> saying that you will have to set "tls='yes'" for every domain and every 
> chardev.
> Forcing to set "tls='no'" for every domain and every chardevs if you want to 
> use
> TLS only for one domain is not a good mechanism.

Clearly ;-)  Setting "tls='yes'" wasn't my implication either. The way
this patch is written 'YES' or 'ABSENT' have the same meaning to take
the host default.

So "forcing" setting of "tls='no'"
> 
> What I would like to achieve is this:
> 
> 1. Currently there is already existing "chardev_tls" config option that allows
> you to enable/disable TLS for all domain and all chardevs
> 
> 2. This patch improves the current functionality by adding an option to
> explicitly disable TLS for some chardves by using "tls='no'" if "chardev_tls"
> is set to "1".
> 
> 3. My additional proposal is to add another improvement to current 
>

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

2016-10-17 Thread Pavel Hrdina 
On Mon, Oct 17, 2016 at 09:54:46AM -0400, John Ferlan wrote:
> 
> 
> On 10/17/2016 04:09 AM, Pavel Hrdina wrote:
> > On Fri, Oct 14, 2016 at 04:23:04PM -0400, John Ferlan wrote:
> >> Add an optional "tls='yes|no'" attribute for a TCP chardev for the
> >> express purpose to disable setting up TLS for the specific chardev in
> >> the event the qemu.conf settings have enabled hypervisor wide TLS for
> >> serial TCP chardevs.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  docs/formatdomain.html.in  | 21 +
> >>  docs/schemas/domaincommon.rng  |  5 +++
> >>  src/conf/domain_conf.c | 22 +-
> >>  src/conf/domain_conf.h |  1 +
> >>  src/qemu/qemu_command.c|  3 +-
> >>  src/qemu/qemu_hotplug.c|  3 +-
> >>  ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +
> >>  ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 
> >> ++
> >>  .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml|  2 +-
> >>  tests/qemuxml2argvtest.c   |  3 ++
> >>  ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml |  1 +
> >>  .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml  |  2 +-
> >>  tests/qemuxml2xmltest.c|  1 +
> >>  13 files changed, 139 insertions(+), 5 deletions(-)
> >>  create mode 100644 
> >> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
> >>  create mode 100644 
> >> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
> >>  create mode 12 
> >> tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
> >>
> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> >> index 9051178..6145d65 100644
> >> --- a/docs/formatdomain.html.in
> >> +++ b/docs/formatdomain.html.in
> >> @@ -6204,6 +6204,27 @@ qemu-kvm -net nic,model=? /dev/null
> >>/devices
> >>...
> >>  
> >> +
> >> +  Since 2.4.0, the optional attribute
> >> +  tls can be used to control whether a serial chardev
> >> +  TCP communication channel would utilize a hypervisor configured
> >> +  TLS X.509 certificate environment in order to encrypt the data
> >> +  channel. For the QEMU hypervisor usage of TLS is controlled by the
> >> +  chardev_tls setting in file /etc/libvirt/qemu.conf. If
> >> +  enabled, then by setting this attribute to "no" will disable usage
> >> +  of the TLS environment for this particular serial TCP data channel.
> >> +
> > 
> > Previous discussion:
> > 
> > http://www.redhat.com/archives/libvir-list/2016-October/msg00734.html
> > 
> > So now there is no functional issue if we agree that this design is what we
> > want.  I personally thing that there could be also use-case where you want 
> > to
> > configure certificates in qemu.conf and use 'tls' attribute to explicitly
> > enable TLS encryption only for some VMs and only for some chardev and this 
> > is
> > not currently possible whit this design.  Now user can enable the TLS 
> > encryption
> > for every chardev for every domain and explicitly disable for some chardevs.
> > 
> > This would require to add all the extra code from that discussion to handle
> > migration properly and that's why I've started the discussion.
> > 
> 
> w/r/t: design - re-read what I pulled from Dan's v5 review:
> 
> http://www.redhat.com/archives/libvir-list/2016-October/msg00698.html
> 
> While forcing to set "tls='yes'" is certainly a mechanism that could be
> used and adding extra code is possible, is that something that we want
> to require of everyone that's using TLS chardev's? If you go through the
> trouble of configuring for your host, then how would you feel about
> having to update all your domains (for those that have more than 1 or 2)
> to set the property in order to be able to use it? Again, I really don't

I have a feeling that we are having trouble to understand each other.  I'm not
saying that you will have to set "tls='yes'" for every domain and every chardev.
Forcing to set "tls='no'" for every domain and every chardevs if you want to use
TLS only for one domain is not a good mechanism.

What I would like to achieve is this:

1. Currently there is already existing "chardev_tls" config option that allows
you to enable/disable TLS for all domain and all chardevs

2. This patch improves the current functionality by adding an option to
explicitly disable TLS for some chardves by using "tls='no'" if "chardev_tls"
is set to "1".

3. My additional proposal is to add another improvement to current functionality
by allowing to explicitly enable TLS for some chardevs by using "tls='yes'" if
"chardev_tls" is set to "0".  And I can see the use-case, you have a shared host
between several people and only one of them would like to use TLS encryption for
his domain and not affect other users so he

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

2016-10-17 Thread John Ferlan 


On 10/17/2016 04:09 AM, Pavel Hrdina wrote:
> On Fri, Oct 14, 2016 at 04:23:04PM -0400, John Ferlan wrote:
>> Add an optional "tls='yes|no'" attribute for a TCP chardev for the
>> express purpose to disable setting up TLS for the specific chardev in
>> the event the qemu.conf settings have enabled hypervisor wide TLS for
>> serial TCP chardevs.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  docs/formatdomain.html.in  | 21 +
>>  docs/schemas/domaincommon.rng  |  5 +++
>>  src/conf/domain_conf.c | 22 +-
>>  src/conf/domain_conf.h |  1 +
>>  src/qemu/qemu_command.c|  3 +-
>>  src/qemu/qemu_hotplug.c|  3 +-
>>  ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +
>>  ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 
>> ++
>>  .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml|  2 +-
>>  tests/qemuxml2argvtest.c   |  3 ++
>>  ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml |  1 +
>>  .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml  |  2 +-
>>  tests/qemuxml2xmltest.c|  1 +
>>  13 files changed, 139 insertions(+), 5 deletions(-)
>>  create mode 100644 
>> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
>>  create mode 100644 
>> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
>>  create mode 12 
>> tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 9051178..6145d65 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -6204,6 +6204,27 @@ qemu-kvm -net nic,model=? /dev/null
>>/devices
>>...
>>  
>> +
>> +  Since 2.4.0, the optional attribute
>> +  tls can be used to control whether a serial chardev
>> +  TCP communication channel would utilize a hypervisor configured
>> +  TLS X.509 certificate environment in order to encrypt the data
>> +  channel. For the QEMU hypervisor usage of TLS is controlled by the
>> +  chardev_tls setting in file /etc/libvirt/qemu.conf. If
>> +  enabled, then by setting this attribute to "no" will disable usage
>> +  of the TLS environment for this particular serial TCP data channel.
>> +
> 
> Previous discussion:
> 
> http://www.redhat.com/archives/libvir-list/2016-October/msg00734.html
> 
> So now there is no functional issue if we agree that this design is what we
> want.  I personally thing that there could be also use-case where you want to
> configure certificates in qemu.conf and use 'tls' attribute to explicitly
> enable TLS encryption only for some VMs and only for some chardev and this is
> not currently possible whit this design.  Now user can enable the TLS 
> encryption
> for every chardev for every domain and explicitly disable for some chardevs.
> 
> This would require to add all the extra code from that discussion to handle
> migration properly and that's why I've started the discussion.
> 

w/r/t: design - re-read what I pulled from Dan's v5 review:

http://www.redhat.com/archives/libvir-list/2016-October/msg00698.html

While forcing to set "tls='yes'" is certainly a mechanism that could be
used and adding extra code is possible, is that something that we want
to require of everyone that's using TLS chardev's? If you go through the
trouble of configuring for your host, then how would you feel about
having to update all your domains (for those that have more than 1 or 2)
to set the property in order to be able to use it? Again, I really don't
think it's a good idea to be mucking around with changing XML of running
domains, adding extra checks in the migration code, altering the
format/parse code to check flags, and/or anything else that may come up.
The less we do that the better - introduces less risk for making or
missing assumptions.

For comparison spice uses an attribute mechanism "tlsPort={-1|0|port}"
in order to use TLS, but it also checks cfg->spiceTLS. Alternatively,
vnc will just take the cfg->vncTLS to decide whether to use or not
(e.g., there's no way to avoid).

So one of the existing mechanisms forces you to define something to use
TLS configured for the host and the other doesn't. Spice has the
additional configuration option to determine specific channels by name
that would use the secure port.

This patch can still be considered "outside" of the subsequent 4 in this
series...

>> +
>> +  ...
>> +  devices
>> +serial type="tcp"
>> +  source mode='connect' host="127.0.0.1" service="" 
>> tls="yes"/
>> +  protocol type="raw"/
>> +  target port="0"/
>> +/serial
>> +  /devices
>> +  ...
>> +
>>  UDP network console
>>  
>>  
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng

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

2016-10-17 Thread Pavel Hrdina 
On Fri, Oct 14, 2016 at 04:23:04PM -0400, John Ferlan wrote:
> Add an optional "tls='yes|no'" attribute for a TCP chardev for the
> express purpose to disable setting up TLS for the specific chardev in
> the event the qemu.conf settings have enabled hypervisor wide TLS for
> serial TCP chardevs.
> 
> Signed-off-by: John Ferlan 
> ---
>  docs/formatdomain.html.in  | 21 +
>  docs/schemas/domaincommon.rng  |  5 +++
>  src/conf/domain_conf.c | 22 +-
>  src/conf/domain_conf.h |  1 +
>  src/qemu/qemu_command.c|  3 +-
>  src/qemu/qemu_hotplug.c|  3 +-
>  ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +
>  ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 
> ++
>  .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml|  2 +-
>  tests/qemuxml2argvtest.c   |  3 ++
>  ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml |  1 +
>  .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml  |  2 +-
>  tests/qemuxml2xmltest.c|  1 +
>  13 files changed, 139 insertions(+), 5 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
>  create mode 12 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 9051178..6145d65 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6204,6 +6204,27 @@ qemu-kvm -net nic,model=? /dev/null
>/devices
>...
>  
> +
> +  Since 2.4.0, the optional attribute
> +  tls can be used to control whether a serial chardev
> +  TCP communication channel would utilize a hypervisor configured
> +  TLS X.509 certificate environment in order to encrypt the data
> +  channel. For the QEMU hypervisor usage of TLS is controlled by the
> +  chardev_tls setting in file /etc/libvirt/qemu.conf. If
> +  enabled, then by setting this attribute to "no" will disable usage
> +  of the TLS environment for this particular serial TCP data channel.
> +

Previous discussion:

http://www.redhat.com/archives/libvir-list/2016-October/msg00734.html

So now there is no functional issue if we agree that this design is what we
want.  I personally thing that there could be also use-case where you want to
configure certificates in qemu.conf and use 'tls' attribute to explicitly
enable TLS encryption only for some VMs and only for some chardev and this is
not currently possible whit this design.  Now user can enable the TLS encryption
for every chardev for every domain and explicitly disable for some chardevs.

This would require to add all the extra code from that discussion to handle
migration properly and that's why I've started the discussion.

> +
> +  ...
> +  devices
> +serial type="tcp"
> +  source mode='connect' host="127.0.0.1" service="" 
> tls="yes"/
> +  protocol type="raw"/
> +  target port="0"/
> +/serial
> +  /devices
> +  ...
> +
>  UDP network console
>  
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 3106510..e6741bb 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3453,6 +3453,11 @@
>  
>
>  
> +
> +  
> +
> +  
> +
>  
>
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 93b34e0..9062544 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1997,6 +1997,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
>  
>  if (VIR_STRDUP(dest->data.tcp.service, src->data.tcp.service) < 0)
>  return -1;
> +
> +dest->data.tcp.haveTLS = src->data.tcp.haveTLS;
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_UNIX:
> @@ -10038,6 +10040,7 @@ 
> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>  char *master = NULL;
>  char *slave = NULL;
>  char *append = NULL;
> +char *haveTLS = NULL;
>  int remaining = 0;
>  
>  while (cur != NULL) {
> @@ -10045,6 +10048,8 @@ 
> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>  if (xmlStrEqual(cur->name, BAD_CAST "source")) {
>  if (!mode)
>  mode = virXMLPropString(cur, "mode");
> +if (!haveTLS)
> +haveTLS = virXMLPropString(cur, "tls");
>  
>  switch ((virDomainChrType) def->type) {
>  case VIR_DOMAIN_CHR_TYPE_FILE:
> @@ -10221,6 +10226,15 @@ 
> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>

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

2016-10-14 Thread John Ferlan 
Add an optional "tls='yes|no'" attribute for a TCP chardev for the
express purpose to disable setting up TLS for the specific chardev in
the event the qemu.conf settings have enabled hypervisor wide TLS for
serial TCP chardevs.

Signed-off-by: John Ferlan 
---
 docs/formatdomain.html.in  | 21 +
 docs/schemas/domaincommon.rng  |  5 +++
 src/conf/domain_conf.c | 22 +-
 src/conf/domain_conf.h |  1 +
 src/qemu/qemu_command.c|  3 +-
 src/qemu/qemu_hotplug.c|  3 +-
 ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +
 ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++
 .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml|  2 +-
 tests/qemuxml2argvtest.c   |  3 ++
 ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml |  1 +
 .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml  |  2 +-
 tests/qemuxml2xmltest.c|  1 +
 13 files changed, 139 insertions(+), 5 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
 create mode 12 
tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 9051178..6145d65 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6204,6 +6204,27 @@ qemu-kvm -net nic,model=? /dev/null
   /devices
   ...
 
+
+  Since 2.4.0, the optional attribute
+  tls can be used to control whether a serial chardev
+  TCP communication channel would utilize a hypervisor configured
+  TLS X.509 certificate environment in order to encrypt the data
+  channel. For the QEMU hypervisor usage of TLS is controlled by the
+  chardev_tls setting in file /etc/libvirt/qemu.conf. If
+  enabled, then by setting this attribute to "no" will disable usage
+  of the TLS environment for this particular serial TCP data channel.
+
+
+  ...
+  devices
+serial type="tcp"
+  source mode='connect' host="127.0.0.1" service="" tls="yes"/
+  protocol type="raw"/
+  target port="0"/
+/serial
+  /devices
+  ...
+
 UDP network console
 
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3106510..e6741bb 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3453,6 +3453,11 @@
 
   
 
+
+  
+
+  
+
 
   
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 93b34e0..9062544 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1997,6 +1997,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
 
 if (VIR_STRDUP(dest->data.tcp.service, src->data.tcp.service) < 0)
 return -1;
+
+dest->data.tcp.haveTLS = src->data.tcp.haveTLS;
 break;
 
 case VIR_DOMAIN_CHR_TYPE_UNIX:
@@ -10038,6 +10040,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr 
def,
 char *master = NULL;
 char *slave = NULL;
 char *append = NULL;
+char *haveTLS = NULL;
 int remaining = 0;
 
 while (cur != NULL) {
@@ -10045,6 +10048,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr 
def,
 if (xmlStrEqual(cur->name, BAD_CAST "source")) {
 if (!mode)
 mode = virXMLPropString(cur, "mode");
+if (!haveTLS)
+haveTLS = virXMLPropString(cur, "tls");
 
 switch ((virDomainChrType) def->type) {
 case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -10221,6 +10226,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr 
def,
 def->data.tcp.listen = true;
 }
 
+if (haveTLS &&
+(def->data.tcp.haveTLS =
+ virTristateBoolTypeFromString(haveTLS)) <= 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("unknown chardev 'tls' setting '%s'"),
+   haveTLS);
+goto error;
+}
+
 if (!protocol)
 def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW;
 else if ((def->data.tcp.protocol =
@@ -10305,6 +10319,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr 
def,
 VIR_FREE(append);
 VIR_FREE(logappend);
 VIR_FREE(logfile);
+VIR_FREE(haveTLS);
 
 return remaining;
 
@@ -21453,7 +21468,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
 virBufferAsprintf(buf, "data.tcp.listen ? "bind" : "connect");
 virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host);
-virBufferEscapeString(buf, "service='%s'/>\n",