On Tue, May 08, 2012 at 10:10:34PM +0300, Alon Levy wrote:
> On Tue, May 08, 2012 at 12:19:45PM -0600, Eric Blake wrote:
> > On 05/08/2012 11:42 AM, Alon Levy wrote:
> > > qemu's behavior in this case is to change the spice server behavior to
> > > require secure connection to any channel not otherwise specified as
> > > being in plaintext mode. libvirt doesn't currently allow requesting this
> > > (via plaintext-channel=<channel name>).
> > >
> > > RHBZ: 819499
> > >
> > > Signed-off-by: Alon Levy <alevy@xxxxxxxxxx>
> > > ---
> > > docs/formatdomain.html.in | 3 +++
> > > docs/schemas/domaincommon.rng | 9 +++++++++
> > > src/conf/domain_conf.c | 20 ++++++++++++++++++++
> > > src/conf/domain_conf.h | 1 +
> > > src/qemu/qemu_command.c | 13 +++++++++++++
> > > .../qemuxml2argv-graphics-spice.args | 2 +-
> > > .../qemuxml2argv-graphics-spice.xml | 2 +-
> > > 7 files changed, 48 insertions(+), 2 deletions(-)
> >
> > Definitely more code, but I think I like it better. In particular,
> >
> > >
> > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > > index 0525577..c0268b2 100644
> > > --- a/docs/formatdomain.html.in
> > > +++ b/docs/formatdomain.html.in
> > > @@ -2929,6 +2929,9 @@ qemu-kvm -net nic,model=? /dev/null
> > > <span class="since">Since 0.9.3</span>
> > > NB, this may not be supported by all hypervisors.
> > > <span class="since">"spice" since 0.8.6</span>.
> > > + The <code>defaultMode</code> attribute sets the default channel
> > > + security policy, valid values are <code>secure</code>,
> > > + <code>insecure</code> and the default <code>any</code>.
> >
> > I didn't realize that 'any' was different! Having a defaultMode call it
> > out makes sense now, given this matrix:
> >
> > insecure any secure
> > tls available plaintext tls tls
> > tls disabled plaintext plaintext error
> >
> > And it also makes sense to the qemu command line - you can only specify
> > plaintext or tls to lock a channel to a particular mode; if you omit the
> > specification, then the mode defaults to the best available (according
> > to whether tls is available).
>
> Your table is correct and a good summary. But to be clear the "any"
> value is not a valid value to put in qemu's command line, it's the
> internal default state (actually it's oring of the two valid channel
> modes, insecure and secure). But it behaves exactly like your table
> explains, i.e. not setting tls-channel=default or
> plaintext-channel=default results in the middle column, setting
> tls-channel=default in the right, and plaintext-channel=default in the
> left. You could have expanded the table because it's also possible to
> start spice with only tls-port set, and no port, then you would get the
> reverse of the right column:
>
> insecure any secure
> both plaintext tls tls
This would actually be:
both plaintext whichever-the-client-first-connects-to
tls
> only clear plaintext plaintext error
> only tls error plaintext tls
Oops, should read:
only tls error tls tls
>
> >
> > Missing a <span>since</span> notation; I've added that.
> >
> > > @@ -12124,6 +12140,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
> > > virBufferEscapeString(buf, " keymap='%s'",
> > > def->data.spice.keymap);
> > >
> > > + if (def->data.spice.defaultMode != VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY)
> > > + virBufferAsprintf(buf, " defaultMode='%s'",
> > > + virDomainGraphicsSpiceChannelModeTypeToString(def->data.spice.defaultMode));
> >
> > This means we don't output the user's explicit defaultMode='any'. It's
> > easy enough to fix by making the enum have four values instead of three
> > (the fourth value, 'default', is value 0 and not valid in XML, as the
> > placeholder for no attribute present, but otherwise behaves like 'any').
> > Then again, you matched the style of the individual channels, so I
> > won't bother changing it.
> >
> > > +++ b/src/conf/domain_conf.h
> > > @@ -1233,6 +1233,7 @@ struct _virDomainGraphicsDef {
> > > virDomainGraphicsAuthDef auth;
> > > unsigned int autoport :1;
> > > int channels[VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST];
> > > + int defaultMode;
> >
> > I like to add a comment to the enum values that are valid in this field.
> >
> > ACK; I squashed this in, then pushed:
>
> Looks good.
>
> >
> > diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
> > index 816af41..1478832 100644
> > --- i/docs/formatdomain.html.in
> > +++ w/docs/formatdomain.html.in
> > @@ -2931,7 +2931,11 @@ qemu-kvm -net nic,model=? /dev/null
> > <span class="since">"spice" since 0.8.6</span>.
> > The <code>defaultMode</code> attribute sets the default
> > channel
> > security policy, valid values are <code>secure</code>,
> > - <code>insecure</code> and the default <code>any</code>.
> > + <code>insecure</code> and the default <code>any</code>
> > + (which is secure if possible, but falls back to insecure
> > + rather than erroring out if no secure path is
> > + available). <span class="since">"defaultMode" since
> > + 0.9.12</span>.
> > </p>
> > <p>
> > When SPICE has both a normal and TLS secured TCP port
> > diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h
> > index 895ddc4..00178e1 100644
> > --- i/src/conf/domain_conf.h
> > +++ w/src/conf/domain_conf.h
> > @@ -1233,7 +1233,7 @@ struct _virDomainGraphicsDef {
> > virDomainGraphicsAuthDef auth;
> > unsigned int autoport :1;
> > int channels[VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST];
> > - int defaultMode;
> > + int defaultMode; /* enum virDomainGraphicsSpiceChannelMode */
> > int image;
> > int jpeg;
> > int zlib;
> >
> > --
> > Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> >
>
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list