Re: [libvirt] [libvirt-python v2 PATCH] Improve error output when use getTime with a nonzero flags.

2014-10-20 Thread Peter Krempa 
On 10/17/14 04:12, Luyao Huang wrote:
 When give a nonzero flags to getTime, c_retval will get -1 and goto cleanup.
 But py_retval still is NULL,so pass c_retval value to py_retval.
 This will make the output message more correct.
 
 error before use this patch:
 SystemError: error return without exception set
 
 after use the patch:
 libvirtError: unsupported flags (0x1) in function qemuDomainGetTime
 
 v1:
 https://www.redhat.com/archives/libvir-list/2014-October/msg00482.html
 
 Signed-off-by: Luyao Huang lhu...@redhat.com
 ---
  libvirt-override.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)
 
 diff --git a/libvirt-override.c b/libvirt-override.c
 index 9ba87eb..c779aa3 100644
 --- a/libvirt-override.c
 +++ b/libvirt-override.c
 @@ -7757,9 +7757,11 @@ libvirt_virDomainGetTime(PyObject *self 
 ATTRIBUTE_UNUSED, PyObject *args) {
  c_retval = virDomainGetTime(domain, seconds, nseconds, flags);
  LIBVIRT_END_ALLOW_THREADS;
  
 -if (c_retval  0)
 +if (c_retval  0){

Missing space before '{'

 + py_retval = libvirt_intWrap(c_retval);

Returning the return value from the C api is useless here. The function
returns a dict on success path thus on error you should return None
(VIR_PY_NONE).

  goto cleanup;
 -
 +}
 +   
  if (!(pyobj_seconds = libvirt_longlongWrap(seconds)) ||
  PyDict_SetItemString(dict, seconds, pyobj_seconds)  0)
  goto cleanup;
 

Peter



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

Re: [libvirt] [PATCH] docs: apps: Update references to virt-p2v and virt-v2v.

2014-10-20 Thread Peter Krempa 
On 10/18/14 16:21, Richard W.M. Jones wrote:
 These tools have been rewritten upstream, so you don't need to link to
 the old tools, link to the new ones and mention they are part of
 libguestfs.
 
 Also remove the link to Poor man's P2V.  There's no real reason to
 use that technique any longer since the rewritten tools are simple,
 fast and highly capable.
 ---
  docs/apps.html.in | 26 +++---
  1 file changed, 11 insertions(+), 15 deletions(-)
 

ACK,

Peter




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

Re: [libvirt] [PATCH] conf: tests: fix virDomainNetDefFormat for vhost-user in client mode

2014-10-20 Thread Peter Krempa 
On 10/17/14 18:45, Maxime Leroy wrote:
 The mode attribute is required for the source element of vhost-user. Thus
 virDomainNetDefFormat should always generate a xml with it, and
 not only when the mode is server.
 
 The commit fixes the issue. And it adds a vhostuser interface in 'client' mode
 to qemuxml2argv-net-vhostuser.(args|xml) to test this usecase.
 
 Signed-off-by: Maxime Leroy maxime.le...@6wind.com

I've reflowed your commit message to break a few long lines.


 ---
  src/conf/domain_conf.c | 5 +++--
  tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args | 7 +--
  tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.xml  | 7 ++-
  3 files changed, 14 insertions(+), 5 deletions(-)
 

and pushed your patch. Thanks for taking time to provide a test case.

Peter




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

Re: [libvirt] [PATCH] tests: fix incorrect caps for shmem-invalid-size, shmem-small-size

2014-10-20 Thread Peter Krempa 
On 10/17/14 18:05, Maxime Leroy wrote:
 VIR_TEST_DEBUG=2 ./qemuxml2argvtest generates the following output:
 
 409) QEMU XML-2-ARGV shmem-invalid-size
 ... Got expected error: unsupported configuration: ivshmem device is not \
supported with this QEMU binary
 OK
 410) QEMU XML-2-ARGV shmem-small-size
 ... Got expected error: unsupported configuration: ivshmem device is not \
 supported with this QEMU binary
 OK
 
 We should have:
 
 409) QEMU XML-2-ARGV shmem-invalid-size
 ... Got expected error: XML error: shmem size must be a power of two
 OK
 410) QEMU XML-2-ARGV shmem-small-size
 ... Got expected error: XML error: shmem size must be at least 1 MiB
 OK
 
 This commit fixes the issue by providing QEMU_CAPS_DEVICE_IVSHMEM caps
 for shmem-invalid-size, shmem-small-size test.
 
 Signed-off-by: Maxime Leroy maxime.le...@6wind.com
 ---
  tests/qemuxml2argvtest.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)
 

Nice catch! ACK and pushed.

Thanks

Peter




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

Re: [libvirt] spec, RFC: TLS support for NBD

2014-10-20 Thread Daniel P. Berrange 
On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote:
 On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote:
  Hi all,
  
  (added rjones from nbdkit fame -- hi there)
 
 [I'm happy to implement whatever you come up with, but I've added
 Florian Weimer to CC who is part of Red Hat's product security group]
 
  So I think the following would make sense to allow TLS in NBD.
  
  This would extend the newstyle negotiation by adding two options (i.e.,
  client requests), one server reply, and one server error as well as
  extend one existing reply, in the following manner:
  
  - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. The
former would be used to verify if the server will do TLS for a given
export:
  
C: NBD_OPT_PEEK_EXPORT
S: NBD_REP_SERVER, with an extra field after the export name
   containing flags that describe the export (R/O vs R/W state,
   whether TLS is allowed and/or required).

IMHO the server should never provide *any* information about the exported
volume(s) until the TLS layer has been fully setup. ie we shouldn't only
think about the actual block data transfers, we should protect the entire
NBD protocol even metadata related operations.

If the server indicates that TLS is allowed, the client may now issue
NBD_OPT_STARTTLS:
  
C: NBD_OPT_STARTTLS
S: NBD_REP_STARTTLS # or NBD_REP_ERR_POLICY, if unwilling
C: initiate TLS handshake
  
Once the TLS handshake has completed, negotiation should continue over
the secure channel. The client should initiate that by sending an
NBD_OPT_* message.
  
  - The server may reply to any and all negotiation request with
NBD_REP_ERR_TLS_REQD if it does not want to do anything without TLS.
However, if at least one export is supported without encryption, the
server must not in any case use this reply.
  
  There is no command to exit TLS again. I don't think that makes sense,
  but I could be persuaded otherwise with sound technical arguments.
  
  Thoughts?
  
  (full spec (with numbers etc) exists as an (uncommitted) diff to
  doc/proto.txt on my laptop, ...)

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

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

[libvirt] [libvirt-python PATCH] Add a type check for time in libvirt_virDomainSetTime

2014-10-20 Thread Luyao Huang 
When pass a number or other things to setTime,no error output,but set time to 0.
Add a type check and give a clear error messages:

TypeError: time must be dict

Signed-off-by: Luyao Huang lhu...@redhat.com
---
 libvirt-override.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libvirt-override.c b/libvirt-override.c
index 9ba87eb..05552a7 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7795,6 +7795,11 @@ libvirt_virDomainSetTime(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args) {
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
 
+if (!PyDict_Check(py_dict)) {
+PyErr_Format(PyExc_TypeError, time must be dict);
+return NULL;
+}
+
 py_dict_size = PyDict_Size(py_dict);
 
 if (py_dict_size == 2) {
-- 
1.8.3.1

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

[libvirt] Implement quorum support.

2014-10-20 Thread Matthias Gatto 
Hello,

I'm implementing quorum in libvirt.
I've try to follow this proposal:
http://www.redhat.com/archives/libvir-list/2014-May/msg00546.html
At this point I've done this:
- add all the field need by quorum in _virStorageSource
(nBackingStores, threshold).
- handle more than one backing store in virStorageSource by adding a
function virStorageSourcePushBackingStore
- add support of the quorum syntax for the xml parser.

so now i have to work on qemuBuildDriveStr

But I've a problem:
in qemu a child of a quorum is a BlockDriverState, where it's a
virStorageSource in libvirt, so a child in qemu contain more
information than a backingStore in libvirt(blockinfo, throttle...).

I think about several solution for this problem:
- I can handle quorum's child differently than backingStore and create
a virDomainDiskDefPtr childs field in virDomainDiskDef or
virStorageSource.
- I can change the virStorageSourcePtr backingStore, to
virDomainDiskDefPtr backingStore, and move the field to backingStore
to virDomainDiskDef.
-I can move all the field need by a quorum's child from
virStorageSource to virDomainDiskDef.

But i don't know which solution is the best for libvirt, how do you
think i can handle this problem ?

Best regards,
Matthias

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

Re: [libvirt] spec, RFC: TLS support for NBD

2014-10-20 Thread Stefan Hajnoczi 
On Mon, Oct 20, 2014 at 08:58:14AM +0100, Daniel P. Berrange wrote:
 On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote:
  On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote:
   Hi all,
   
   (added rjones from nbdkit fame -- hi there)
  
  [I'm happy to implement whatever you come up with, but I've added
  Florian Weimer to CC who is part of Red Hat's product security group]
  
   So I think the following would make sense to allow TLS in NBD.
   
   This would extend the newstyle negotiation by adding two options (i.e.,
   client requests), one server reply, and one server error as well as
   extend one existing reply, in the following manner:
   
   - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. The
 former would be used to verify if the server will do TLS for a given
 export:
   
 C: NBD_OPT_PEEK_EXPORT
 S: NBD_REP_SERVER, with an extra field after the export name
containing flags that describe the export (R/O vs R/W state,
whether TLS is allowed and/or required).
 
 IMHO the server should never provide *any* information about the exported
 volume(s) until the TLS layer has been fully setup. ie we shouldn't only
 think about the actual block data transfers, we should protect the entire
 NBD protocol even metadata related operations.

This makes sense.

TLS is about the transport, not about a particular NBD export.  The only
thing that should be communicated is STARTTLS.

Stefan


pgptWSav4pBPt.pgp
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 0/7] qemu: Introduce support for new the block_set_io_throttle parameters add in the version 1.7 of qemu.

2014-10-20 Thread Matthias Gatto 
On Tue, Oct 7, 2014 at 1:14 PM, Matthias Gatto
matthias.ga...@outscale.com wrote:
 This series of patches add support for bps_max, bps_rd_max, bps_wr_max,
 bps_max, bps_rd_max, bps_wr_max, and iops_size in the functions 
 qemuDomainSetBlockIoTune and qemuDomainGetBlockIoTune.
 The last patch add support for these parameters to the virsh blkdeviotune 
 command.

 v2: spellfix

 v3: Merge patch 1/9,2/9,5/9 together.
 Change the capability detection.(patch 2/7 and 3/7).
 Try to make the usage of QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX more 
 explicit(patch 3/7).

 v4: Rebase on HEAD.
 Update qemu_driver to comply with Pavel's patchs.(patch 3/6)
 Remove the qemu_monitor_text modification.(remove old patch 5/7)

 v5: Split patch 1/6 in two.
 Add documentation for the new xml options (patch 2/7)
 Change (void) to ATTRIBUTE_UNUSED (patch 4/7)
 Capability detection of supportMaxOptions move before usage of 
 supportMaxOptions

 Matthias Gatto (7):
   qemu: Add define for the news throttle options
   qemu: Modify the structure _virDomainBlockIoTuneInfo.
   qemu: Add the capability to detect if the qemu binary have the
 capability to use bps_max and friends
   qemu: Add bps_max and friends qemu driver
   qemu: Add bps_max and friends QMP suport
   qemu: add bps_max and friends to qemu command generation
   virsh: Add bps_max and friends to virsh

  docs/formatdomain.html.in|  25 
  docs/schemas/domaincommon.rng|  43 +++
  include/libvirt/libvirt.h.in | 110 
  src/conf/domain_conf.c   | 110 +++-
  src/conf/domain_conf.h   |   7 +
  src/qemu/qemu_capabilities.c |   2 +
  src/qemu/qemu_capabilities.h |   1 +
  src/qemu/qemu_command.c  |  57 +++-
  src/qemu/qemu_driver.c   | 186 
 ++-
  src/qemu/qemu_monitor.c  |  10 +-
  src/qemu/qemu_monitor.h  |   6 +-
  src/qemu/qemu_monitor_json.c |  64 +++--
  src/qemu/qemu_monitor_json.h |   6 +-
  tests/qemucapabilitiesdata/caps_2.1.1-1.caps |   1 +
  tests/qemumonitorjsontest.c  |   6 +-
  tools/virsh-domain.c | 119 +
  tools/virsh.pod  |  10 ++
  17 files changed, 730 insertions(+), 33 deletions(-)

 --
 1.8.3.1

ping

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

Re: [libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles

2014-10-20 Thread Stefan Bader 
On 19.10.2014 17:07, intrigeri wrote:
 Hi Stefan,
 
 Stefan Bader wrote (19 Oct 2014 11:07:40 GMT) :
 Yeah, I actually did but it felt a bit hackish but then I am told anything 
 looks
 a bit hackish when it involves autoconf. These are again against upstream
 libvirt mostly because the last touch timestamps always clash otherwise.
 
 Cool, I've tested this. I've imported these two patches in Debian's
 1.2.9-3 quilt series, made the build system use dh-autoreconf (the
 build system in the tarball wants aclocal 1.13, while Debian sid has
 1.14), and added a build-dep on libapparmor-dev to get the needed
 pkg-config file.
 
 Attempting to build the resulting source package in a clean sid chroot
 fails here:
 
   Making all in examples/apparmor
   make[3]: Entering directory 
 '/tmp/buildd/libvirt-1.2.9/debian/build/examples/apparmor'
   make[3]: Circular ../../config.h - ../../config.h dependency dropped.
   ./profile-preprocess ../../../../examples/apparmor/libvirt-qemu.in 
 libvirt-qemu
   ./profile-preprocess ../../../../examples/apparmor/libvirt-lxc.in 
 libvirt-lxc
   ./profile-preprocess 
 ../../../../examples/apparmor/usr.lib.libvirt.virt-aa-helper.in 
 usr.lib.libvirt.virt-aa-helper
   ./profile-preprocess ../../../../examples/apparmor/usr.sbin.libvirtd.in 
 usr.sbin.libvirtd
   make[3]: *** No rule to make target 'local-usr.sbin.libvirtd', needed by 
 'all-am'.  Stop.
   make[3]: *** Waiting for unfinished jobs
   /bin/bash: ./profile-preprocess: No such file or directory
   /bin/bash: ./profile-preprocess: No such file or directory
   Makefile:2068: recipe for target 'libvirt-qemu' failed
   make[3]: *** [libvirt-qemu] Error 127
   Makefile:2068: recipe for target 'libvirt-lxc' failed
   make[3]: *** [libvirt-lxc] Error 127
   /bin/bash: ./profile-preprocess: No such file or directory
   /bin/bash: ./profile-preprocess: No such file or directory
   Makefile:2068: recipe for target 'usr.lib.libvirt.virt-aa-helper' failed
   make[3]: *** [usr.lib.libvirt.virt-aa-helper] Error 127
   Makefile:2068: recipe for target 'usr.sbin.libvirtd' failed
   make[3]: *** [usr.sbin.libvirtd] Error 127
   make[3]: Leaving directory 
 '/tmp/buildd/libvirt-1.2.9/debian/build/examples/apparmor'
   Makefile:1979: recipe for target 'all-recursive' failed
   make[2]: *** [all-recursive] Error 1
   make[2]: Leaving directory '/tmp/buildd/libvirt-1.2.9/debian/build'
   Makefile:1877: recipe for target 'all' failed
   make[1]: *** [all] Error 2
   make[1]: Leaving directory '/tmp/buildd/libvirt-1.2.9/debian/build'
   dh_auto_build: make -j5 returned exit code 2
   debian/rules:126: recipe for target 'build' failed
   make: *** [build] Error 2
 
 Any hint?

Hm, partially this sounds like the preprocess script is not where it should be
and the other part looks like not finding any local-usr-sbin. Could likely be
that I need to do something better to make things work in place (as the upstream
libvirt instructions suggest) as well as with separate object tree (as it is in
Debian). I also saw something about circular dependency on config.h which
probably slipped my attention. For most of the problems I guess adding something
like $(srcdir) (need to look what this would be actually called) to the
pre-process scripts path as well as to the .in files..

-Stefan
 
 Cheers,
 




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

Re: [libvirt] [Qemu-devel] spec, RFC: TLS support for NBD

2014-10-20 Thread Markus Armbruster 
Stefan Hajnoczi stefa...@redhat.com writes:

 On Mon, Oct 20, 2014 at 08:58:14AM +0100, Daniel P. Berrange wrote:
 On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote:
  On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote:
   Hi all,
   
   (added rjones from nbdkit fame -- hi there)
  
  [I'm happy to implement whatever you come up with, but I've added
  Florian Weimer to CC who is part of Red Hat's product security group]
  
   So I think the following would make sense to allow TLS in NBD.
   
   This would extend the newstyle negotiation by adding two options (i.e.,
   client requests), one server reply, and one server error as well as
   extend one existing reply, in the following manner:
   
   - The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. The
 former would be used to verify if the server will do TLS for a given
 export:
   
 C: NBD_OPT_PEEK_EXPORT
 S: NBD_REP_SERVER, with an extra field after the export name
containing flags that describe the export (R/O vs R/W state,
whether TLS is allowed and/or required).
 
 IMHO the server should never provide *any* information about the exported
 volume(s) until the TLS layer has been fully setup. ie we shouldn't only
 think about the actual block data transfers, we should protect the entire
 NBD protocol even metadata related operations.

 This makes sense.

Seconded.

 TLS is about the transport, not about a particular NBD export.  The only
 thing that should be communicated is STARTTLS.

Furthermore, STARTTLS is vulnerable to active attacks: if you can get
between the peers, you can make them fall back to unencrypted silently.
How do you plan to guard against that?

See also https://www.agwa.name/blog/post/starttls_considered_harmful

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

Re: [libvirt] [Qemu-devel] spec, RFC: TLS support for NBD

2014-10-20 Thread Daniel P. Berrange 
On Mon, Oct 20, 2014 at 01:51:43PM +0200, Markus Armbruster wrote:
 Stefan Hajnoczi stefa...@redhat.com writes:
 
  On Mon, Oct 20, 2014 at 08:58:14AM +0100, Daniel P. Berrange wrote:
  On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote:
   On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote:
Hi all,

(added rjones from nbdkit fame -- hi there)
   
   [I'm happy to implement whatever you come up with, but I've added
   Florian Weimer to CC who is part of Red Hat's product security group]
   
So I think the following would make sense to allow TLS in NBD.

This would extend the newstyle negotiation by adding two options (i.e.,
client requests), one server reply, and one server error as well as
extend one existing reply, in the following manner:

- The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. 
The
  former would be used to verify if the server will do TLS for a given
  export:

  C: NBD_OPT_PEEK_EXPORT
  S: NBD_REP_SERVER, with an extra field after the export name
 containing flags that describe the export (R/O vs R/W state,
 whether TLS is allowed and/or required).
  
  IMHO the server should never provide *any* information about the exported
  volume(s) until the TLS layer has been fully setup. ie we shouldn't only
  think about the actual block data transfers, we should protect the entire
  NBD protocol even metadata related operations.
 
  This makes sense.
 
 Seconded.
 
  TLS is about the transport, not about a particular NBD export.  The only
  thing that should be communicated is STARTTLS.
 
 Furthermore, STARTTLS is vulnerable to active attacks: if you can get
 between the peers, you can make them fall back to unencrypted silently.
 How do you plan to guard against that?

Well the use of a STARTTLS message at a protocol level isn't vulnerable
per-se, rather it is the handling of it that matters. The key is what
happens if the server wants TLS and the client does not send a STARTTLS
message. If the server happily carries on with plain text that's bad. If
the server closes any connection that attempts to skip STARTTLS, that's
fine. Likewise if the client wants TLS and the server claims to not do
TLS, then the client should close the connection and not carry on. This
avoids the MITM downgrade problem.

So from the POV of QEMU / QEMU-NBD I'd expect us to have a CLI option
tls=on|off  and if the client / server are configured differently then
it would be a hard failure, never any negotiated fallback to plain text
if one requests TLS and the other doesn't.

If QEMU relies on the CLI option, then technically we do not need any
NBD protocol level changes at all. A standard TLS handshake could be
started the moment the TCP connection is established. Only once the
TLS handshake completes would the NBD protocol start running.

The real / main benefit of having a STARTTLS message would be to give
better error reporting for clients not attempting TLS. eg so they could
report a clear This server requires TLS error instead of just seeing
unintelligible data from the NBD server and no clue that it is a TLS
handshake.

This is how the VNC integration works at least. The VNC server advertizes
that it requires the TLS auth protocol extension. If the VNC client does
not support this, the server will drop the connection and the VNC client
can at least report to the user that the server requested use of TLS.

The key is that no data or metadata that is in any way related to remote
desktop (or NBD volume) is exchanged between server/client until after
the TLS auth protocol completes.

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

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

Re: [libvirt] [Qemu-devel] spec, RFC: TLS support for NBD

2014-10-20 Thread Richard W.M. Jones 
On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote:
 On 10/20/2014 01:51 PM, Markus Armbruster wrote:
 Furthermore, STARTTLS is vulnerable to active attacks: if you can get
 between the peers, you can make them fall back to unencrypted silently.
 How do you plan to guard against that?
 
 The usual way to deal with this is to use different syntax for
 TLS-enabled and non-TLS addresses (e.g., https:// and http://).
 With a TLS address, the client must enforce that only TLS-enabled
 connections are possible.  STARTTLS isn't the problem here, it's
 just an accident of history that many STARTTLS client
 implementations do not require a TLS handshake before proceeding.
 
 I cannot comment on whether the proposed STARTTLS command is at the
 correct stage of the NBD protocol.  If there is a protocol
 description for NBD, I can have a look.

Two actually :-)  Both are covered here:

http://sourceforge.net/p/nbd/code/ci/master/tree/doc/proto.txt

I believe that the proposed changes only cover the new style
protocol.

There's no common syntax for nbd URLs that I'm aware of.  At least,
both qemu  guestfish have nbd:... strings that they can parse, but
both have a completely different syntax.  But we could still have a
client-side indication (flag or nbds:..) to say that we want to force
TLS.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

Re: [libvirt] [libvirt-python v2 PATCH] Improve error output when use getTime with a nonzero flags.

2014-10-20 Thread Luyao Huang 
Thanks your help and useful messages.

This issue is so small and i just want to fix the ret = NULL with no exception.

So i want to make ret = -1 and make the SystemError: error return without 
exception set disappear.

Thanks,
Luyao Huang

- Original Message -
From: Peter Krempa pkre...@redhat.com
To: Luyao Huang lhu...@redhat.com, libvir-list@redhat.com
Sent: Monday, October 20, 2014 2:37:21 PM
Subject: Re: [libvirt] [libvirt-python v2 PATCH] Improve error output when use 
getTime with a nonzero flags.

On 10/17/14 04:12, Luyao Huang wrote:
 When give a nonzero flags to getTime, c_retval will get -1 and goto cleanup.
 But py_retval still is NULL,so pass c_retval value to py_retval.
 This will make the output message more correct.
 
 error before use this patch:
 SystemError: error return without exception set
 
 after use the patch:
 libvirtError: unsupported flags (0x1) in function qemuDomainGetTime
 
 v1:
 https://www.redhat.com/archives/libvir-list/2014-October/msg00482.html
 
 Signed-off-by: Luyao Huang lhu...@redhat.com
 ---
  libvirt-override.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)
 
 diff --git a/libvirt-override.c b/libvirt-override.c
 index 9ba87eb..c779aa3 100644
 --- a/libvirt-override.c
 +++ b/libvirt-override.c
 @@ -7757,9 +7757,11 @@ libvirt_virDomainGetTime(PyObject *self 
 ATTRIBUTE_UNUSED, PyObject *args) {
  c_retval = virDomainGetTime(domain, seconds, nseconds, flags);
  LIBVIRT_END_ALLOW_THREADS;
  
 -if (c_retval  0)
 +if (c_retval  0){

Missing space before '{'

 + py_retval = libvirt_intWrap(c_retval);

Returning the return value from the C api is useless here. The function
returns a dict on success path thus on error you should return None
(VIR_PY_NONE).

  goto cleanup;
 -
 +}
 +   
  if (!(pyobj_seconds = libvirt_longlongWrap(seconds)) ||
  PyDict_SetItemString(dict, seconds, pyobj_seconds)  0)
  goto cleanup;
 

Peter

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

Re: [libvirt] [libvirt-python v2 PATCH] Improve error output when use getTime with a nonzero flags.

2014-10-20 Thread Peter Krempa 
On 10/20/14 15:06, Luyao Huang wrote:
 Thanks your help and useful messages.
 
 This issue is so small and i just want to fix the ret = NULL with no 
 exception.

That's why I've suggested to return VIR_PY_NONE which should fix the
issue in a proper way.

 
 So i want to make ret = -1 and make the SystemError: error return without 
 exception set disappear.
 
 Thanks,
 Luyao Huang
 

Peter



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

[libvirt] [libvirt-python v3 PATCH] Improve error output when use getTime with a nonzero flags.

2014-10-20 Thread Luyao Huang 
When give a nonzero flags to getTime, c_retval will get -1 and goto cleanup.
But py_retval still is NULL,so set py_retval =  VIR_PY_NONE.
This will make the output message more correct.

error will disappear:
SystemError: error return without exception set

v2:
https://www.redhat.com/archives/libvir-list/2014-October/msg00497.html

Signed-off-by: Luyao Huang lhu...@redhat.com
---
 libvirt-override.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 9ba87eb..8690f4f 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7757,9 +7757,11 @@ libvirt_virDomainGetTime(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args) {
 c_retval = virDomainGetTime(domain, seconds, nseconds, flags);
 LIBVIRT_END_ALLOW_THREADS;
 
-if (c_retval  0)
+if (c_retval  0) {
+   py_retval = VIR_PY_NONE;
 goto cleanup;
-
+}
+   
 if (!(pyobj_seconds = libvirt_longlongWrap(seconds)) ||
 PyDict_SetItemString(dict, seconds, pyobj_seconds)  0)
 goto cleanup;
-- 
1.8.3.1

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

Re: [libvirt] [libvirt-python v3 PATCH] Improve error output when use getTime with a nonzero flags.

2014-10-20 Thread Pavel Hrdina 

On 10/20/2014 03:55 PM, Luyao Huang wrote:

When give a nonzero flags to getTime, c_retval will get -1 and goto cleanup.


Try to wrap the lines to 76 columns.


But py_retval still is NULL,so set py_retval =  VIR_PY_NONE.


s/NULL,so/NULL, so/


This will make the output message more correct.

error will disappear:
SystemError: error return without exception set

v2:
https://www.redhat.com/archives/libvir-list/2014-October/msg00497.html


This comments shouldn't be in the commit message but ...



Signed-off-by: Luyao Huang lhu...@redhat.com
---


... there.


  libvirt-override.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 9ba87eb..8690f4f 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7757,9 +7757,11 @@ libvirt_virDomainGetTime(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args) {
  c_retval = virDomainGetTime(domain, seconds, nseconds, flags);
  LIBVIRT_END_ALLOW_THREADS;

-if (c_retval  0)
+if (c_retval  0) {
+   py_retval = VIR_PY_NONE;


Use spaces instead of tabs.


  goto cleanup;
-
+}
+
  if (!(pyobj_seconds = libvirt_longlongWrap(seconds)) ||
  PyDict_SetItemString(dict, seconds, pyobj_seconds)  0)
  goto cleanup;



ACK to the logic, I'll update the patch and push it, thanks for the fix.

Pavel

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

Re: [libvirt] [Qemu-devel] spec, RFC: TLS support for NBD

2014-10-20 Thread Florian Weimer 

On 10/20/2014 01:51 PM, Markus Armbruster wrote:

Furthermore, STARTTLS is vulnerable to active attacks: if you can get
between the peers, you can make them fall back to unencrypted silently.
How do you plan to guard against that?


The usual way to deal with this is to use different syntax for 
TLS-enabled and non-TLS addresses (e.g., https:// and http://).  With a 
TLS address, the client must enforce that only TLS-enabled connections 
are possible.  STARTTLS isn't the problem here, it's just an accident of 
history that many STARTTLS client implementations do not require a TLS 
handshake before proceeding.


I cannot comment on whether the proposed STARTTLS command is at the 
correct stage of the NBD protocol.  If there is a protocol description 
for NBD, I can have a look.


--
Florian Weimer / Red Hat Product Security

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

Re: [libvirt] [PATCH] Fix cast errors with clang

2014-10-20 Thread Eric Blake 
On 10/18/2014 10:41 AM, Roman Bogorodskiy wrote:
 Build with clang fails with:
 
   CC   util/libvirt_util_la-virsocketaddr.lo
 util/virsocketaddr.c:904:17: error: cast from 'struct sockaddr *' to
 'struct sockaddr_in *' increases required alignment from 1 to 4
 [-Werror,-Wcast-align]
 inet4 = (struct sockaddr_in*) res-ai_addr;
 ^~
 util/virsocketaddr.c:909:17: error: cast from 'struct sockaddr *' to
 'struct sockaddr_in6 *' increases required alignment from 1 to 4
 [-Werror,-Wcast-align]
 inet6 = (struct sockaddr_in6*) res-ai_addr;
 ^~~
 2 errors generated.
 
 Fix by introducing a union of the appropriate sturcts.

s/sturcts/structs/

 ---
  src/util/virsocketaddr.c | 17 ++---
  1 file changed, 10 insertions(+), 7 deletions(-)
 
 diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
 index 5f54e68..162108c 100644
 --- a/src/util/virsocketaddr.c
 +++ b/src/util/virsocketaddr.c
 @@ -892,22 +892,25 @@ virSocketAddrIsNumericLocalhost(const char *addr)
  {
  struct addrinfo *res;
  struct in_addr tmp = { .s_addr = htonl(INADDR_LOOPBACK) };
 -struct sockaddr_in *inet4;
 -struct sockaddr_in6 *inet6;
 +union {
 +struct sockaddr *addr;
 +struct sockaddr_in *inet4;
 +struct sockaddr_in6 *inet6;
 +} sa;

Close, but not quite.  The POSIX solution for this is the
sockaddr_storage type, in sys/socket.h.  You shouldn't need to create
your own union, but instead reuse sockaddr_storage.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [Qemu-devel] spec, RFC: TLS support for NBDµ

2014-10-20 Thread Wouter Verhelst 
On Mon, Oct 20, 2014 at 01:51:43PM +0200, Markus Armbruster wrote:
 Stefan Hajnoczi stefa...@redhat.com writes:
 
  On Mon, Oct 20, 2014 at 08:58:14AM +0100, Daniel P. Berrange wrote:
  On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote:
   On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote:
Hi all,

(added rjones from nbdkit fame -- hi there)
   
   [I'm happy to implement whatever you come up with, but I've added
   Florian Weimer to CC who is part of Red Hat's product security group]
   
So I think the following would make sense to allow TLS in NBD.

This would extend the newstyle negotiation by adding two options (i.e.,
client requests), one server reply, and one server error as well as
extend one existing reply, in the following manner:

- The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. 
The
  former would be used to verify if the server will do TLS for a given
  export:

  C: NBD_OPT_PEEK_EXPORT
  S: NBD_REP_SERVER, with an extra field after the export name
 containing flags that describe the export (R/O vs R/W state,
 whether TLS is allowed and/or required).
  
  IMHO the server should never provide *any* information about the exported
  volume(s) until the TLS layer has been fully setup. ie we shouldn't only
  think about the actual block data transfers, we should protect the entire
  NBD protocol even metadata related operations.
 
  This makes sense.
 
 Seconded.

Mmm. I suppose the NBD_OPT_PEEK_EXPORT message could be defined so that
it is fine for an export which has the TLS required bit set to provide
differing information after TLS has been negotiated.

  TLS is about the transport, not about a particular NBD export.  The only
  thing that should be communicated is STARTTLS.
 
 Furthermore, STARTTLS is vulnerable to active attacks: if you can get
 between the peers, you can make them fall back to unencrypted silently.
 How do you plan to guard against that?

As I've said before in this discussion, encryption downgrade attacks are
not specific to STARTTLS; as soon as you have have an encrypted and an
unencrypted variant of a protocol, that becomes a problem. After all,
if an attacker can modify the communication so that STARTTLS is filtered
out of the communication, they can most likely also redirect all traffic
to a decrypting/encrypting proxy.

The only way to fix that is through userspace; make opportunistic TLS
(i.e., use it if available, but move on if not) difficult to achieve.

 See also https://www.agwa.name/blog/post/starttls_considered_harmful

A random blog post by an author who is speaking about STARTTLS in
general terms is not a good technical argument for why STARTTLS is a bad
idea in *this* specific case.

If I was defining a new protocol from scratch, I might dump the whole
thing in TLS to begin with. But that's just not the case, so I have to
deal with what exists already.

In addition, with the current state of affairs, it is *not possible* to
swap to an NBD device if you need to pipe its data through a separate
socket than the one you're handing to the kernel. The result of that is
that you can't do TLS on a device you want to swap to. This means we
need to continue to support a protocol that can do TLS for some exports,
and plain (unencrypted) traffic for other exports, *in the same running
nbd-server instance*.

I did add the NBD_REP_ERR_TLS_REQD message for a server which does not
export anything unencrypted, so that it can (after the initial few
exchanges) reply to anything except for STARTTLS with lalala, I'm not
listening until you encrypt things. However, unless it's fine to ditch
support for swapping to an NBD device (not an option from where I'm
standing), dropping support for unencrypted communication is not an
option.

-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26


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

Re: [libvirt] [Qemu-devel] spec, RFC: TLS support for NBD

2014-10-20 Thread Wouter Verhelst 
On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote:
 I cannot comment on whether the proposed STARTTLS command is at the correct
 stage of the NBD protocol.  If there is a protocol description for NBD, I
 can have a look.

To make this discussion in that regard a bit easier, I've committed the
proposed spec to a separate branch:

https://github.com/yoe/nbd/blob/tlsspec/doc/proto.txt

Thanks,

-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26

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