Re: [libvirt] [PATCH v5 4/4] qemu/rbd: improve rbd device specification

2011-11-16 Thread Daniel P. Berrange 
On Tue, Nov 15, 2011 at 05:05:27PM -0700, Eric Blake wrote:
 On 10/31/2011 07:29 PM, Josh Durgin wrote:
  From: Sage Weil s...@newdream.net
  +if (sec) {
  +char *base64 = NULL;
  +
  +secret = (char *)conn-secretDriver-getValue(sec, 
  secret_size, 0,
  +  
  VIR_SECRET_GET_VALUE_INTERNAL_CALL);
  +if (secret == NULL) {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +_(could not get the value of the secret 
  for username %s),
  +disk-auth.username);
  +goto error;
  +}
  +/* qemu/librbd wants it base64 encoded */
  +base64_encode_alloc(secret, secret_size, base64);
  +if (!base64) {
  +virReportOOMError();
  +goto error;
  +}
  +virBufferEscape(opt, :, :key=%s:auth_supported=cephx none,
  +base64);
  +VIR_FREE(base64);
 
 The command line that we pass to qemu gets logged.  But what happens if
 the secret was marked as ephemeral - could we be violating the premise
 of not exposing passwords to too broad an audience?  Or are we already
 safe in that the log entries created by virCommand can only be exposed
 to users that already can get at the secret information by other means?
 
 Maybe this means we should we be adding capabilities into virCommand to
 prevent the logging of the actual secret (whether base64-encoded or
 otherwise), and instead log an alternate string?  That is, should
 virCommand be tracking parallel argv arrays; the real array passed to
 exec() but never logged, and the alternate array (normally matching the
 real one, but which can differ in this particular case of passing an
 argument that contains a password)?

The passing of secrets on the command line is just a temporary hack
we're doing to prove the overall handling of passwords for Ceph. The
real plan is to set them via  monitor command in QEMU, but we're just
waiting for some QEMU work before changing libvirt todo that.


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] [PATCH v5 4/4] qemu/rbd: improve rbd device specification

2011-11-16 Thread Eric Blake 
On 11/15/2011 06:37 PM, Josh Durgin wrote:
 The command line that we pass to qemu gets logged.  But what happens if
 the secret was marked as ephemeral - could we be violating the premise
 of not exposing passwords to too broad an audience?  Or are we already
 safe in that the log entries created by virCommand can only be exposed
 to users that already can get at the secret information by other means?
 
 The secret can be read from the command line of the running process,
 which is even less secure than the log. I'm working on passing the
 secret via the qemu monitor instead of the command line, which will
 avoid both issues.
 
 Maybe this means we should we be adding capabilities into virCommand to
 prevent the logging of the actual secret (whether base64-encoded or
 otherwise), and instead log an alternate string?  That is, should
 virCommand be tracking parallel argv arrays; the real array passed to
 exec() but never logged, and the alternate array (normally matching the
 real one, but which can differ in this particular case of passing an
 argument that contains a password)?

Given your arguments (that ps can read argv of qemu, even if we hid it
from libvirt logs, and that we will be moving to a monitor command as
soon as qemu supports one), I see no reason to hack up virCommand to
support alternate log output.

-- 
Eric Blake   ebl...@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] [PATCH v5 4/4] qemu/rbd: improve rbd device specification

2011-11-15 Thread Eric Blake 
On 10/31/2011 07:29 PM, Josh Durgin wrote:
 From: Sage Weil s...@newdream.net

Sorry for letting my review of this slip 2 weeks.

 
 This improves the support for qemu rbd devices by adding support for a few
 key features (e.g., authentication) and cleaning up the way in which
 rbd configuration options are passed to qemu.
 
 An auth member of the disk source xml specifies how librbd should
 authenticate. The username attribute is the Ceph/RBD user to authenticate as.
 The usage or uuid attributes specify which secret to use. Usage is an
 arbitrary identifier local to libvirt.
 
 The old RBD support relied on setting an environment variable to
 communicate information to qemu/librbd.  Instead, pass those options
 explicitly to qemu.  Update the qemu argument parsing and tests
 accordingly.
 
 Signed-off-by: Sage Weil s...@newdream.net
 Signed-off-by: Josh Durgin josh.dur...@dreamhost.com
 ---
 
 Changes since v4:
 * fixes memory management issues
 * keep older rbd command line parsing and test case
 * check qemuAddRBDHost return values
 * use more efficient virBuffer functions

Looks like you got all my review points.

ACK and pushed, although I do have some questions that may deserve
followup patches:

 +static int
 +qemuBuildRBDString(virConnectPtr conn,
 +   virDomainDiskDefPtr disk,
 +   virBufferPtr opt)
 +{
 +int i, ret = 0;
 +virSecretPtr sec = NULL;
 +char *secret = NULL;
 +size_t secret_size;
 +
 +virBufferAsprintf(opt, rbd:%s, disk-src);
 +if (disk-auth.username) {
 +virBufferEscape(opt, :, :id=%s, disk-auth.username);

This results in ambiguous output if disk-auth.username can end in a
single backslash (since then, you would have \: when combined with the
next part of the option, making it look like the next :mon_host=
option is instead a continuation of the :id= username).  Should we be
escaping backslash as well as colon?  Or should virBufferEscape be
taught to always escape backslash in addition to whatever characters
were passed in to its 'toescape' argument?

 +if (sec) {
 +char *base64 = NULL;
 +
 +secret = (char *)conn-secretDriver-getValue(sec, secret_size, 
 0,
 +  
 VIR_SECRET_GET_VALUE_INTERNAL_CALL);
 +if (secret == NULL) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +_(could not get the value of the secret for 
 username %s),
 +disk-auth.username);
 +goto error;
 +}
 +/* qemu/librbd wants it base64 encoded */
 +base64_encode_alloc(secret, secret_size, base64);
 +if (!base64) {
 +virReportOOMError();
 +goto error;
 +}
 +virBufferEscape(opt, :, :key=%s:auth_supported=cephx none,
 +base64);
 +VIR_FREE(base64);

The command line that we pass to qemu gets logged.  But what happens if
the secret was marked as ephemeral - could we be violating the premise
of not exposing passwords to too broad an audience?  Or are we already
safe in that the log entries created by virCommand can only be exposed
to users that already can get at the secret information by other means?

Maybe this means we should we be adding capabilities into virCommand to
prevent the logging of the actual secret (whether base64-encoded or
otherwise), and instead log an alternate string?  That is, should
virCommand be tracking parallel argv arrays; the real array passed to
exec() but never logged, and the alternate array (normally matching the
real one, but which can differ in this particular case of passing an
argument that contains a password)?

-- 
Eric Blake   ebl...@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] [PATCH v5 4/4] qemu/rbd: improve rbd device specification

2011-11-15 Thread Josh Durgin 

On 11/15/2011 04:05 PM, Eric Blake wrote:

On 10/31/2011 07:29 PM, Josh Durgin wrote:

From: Sage Weils...@newdream.net


Sorry for letting my review of this slip 2 weeks.



This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.

Anauth  member of the disk source xml specifies how librbd should
authenticate. The username attribute is the Ceph/RBD user to authenticate as.
The usage or uuid attributes specify which secret to use. Usage is an
arbitrary identifier local to libvirt.

The old RBD support relied on setting an environment variable to
communicate information to qemu/librbd.  Instead, pass those options
explicitly to qemu.  Update the qemu argument parsing and tests
accordingly.

Signed-off-by: Sage Weils...@newdream.net
Signed-off-by: Josh Durginjosh.dur...@dreamhost.com
---

Changes since v4:
* fixes memory management issues
* keep older rbd command line parsing and test case
* check qemuAddRBDHost return values
* use more efficient virBuffer functions


Looks like you got all my review points.

ACK and pushed, although I do have some questions that may deserve
followup patches:


+static int
+qemuBuildRBDString(virConnectPtr conn,
+   virDomainDiskDefPtr disk,
+   virBufferPtr opt)
+{
+int i, ret = 0;
+virSecretPtr sec = NULL;
+char *secret = NULL;
+size_t secret_size;
+
+virBufferAsprintf(opt, rbd:%s, disk-src);
+if (disk-auth.username) {
+virBufferEscape(opt, :, :id=%s, disk-auth.username);


This results in ambiguous output if disk-auth.username can end in a
single backslash (since then, you would have \: when combined with the
next part of the option, making it look like the next :mon_host=
option is instead a continuation of the :id= username).  Should we be
escaping backslash as well as colon?  Or should virBufferEscape be
taught to always escape backslash in addition to whatever characters
were passed in to its 'toescape' argument?


Escaping backslashes wouldn't hurt, but these usernames aren't expected 
to have backslashes in them (they're genericNames in the xml schema).





+if (sec) {
+char *base64 = NULL;
+
+secret = (char *)conn-secretDriver-getValue(sec,secret_size, 0,
+  
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+if (secret == NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(could not get the value of the secret for 
username %s),
+disk-auth.username);
+goto error;
+}
+/* qemu/librbd wants it base64 encoded */
+base64_encode_alloc(secret, secret_size,base64);
+if (!base64) {
+virReportOOMError();
+goto error;
+}
+virBufferEscape(opt, :, :key=%s:auth_supported=cephx none,
+base64);
+VIR_FREE(base64);


The command line that we pass to qemu gets logged.  But what happens if
the secret was marked as ephemeral - could we be violating the premise
of not exposing passwords to too broad an audience?  Or are we already
safe in that the log entries created by virCommand can only be exposed
to users that already can get at the secret information by other means?


The secret can be read from the command line of the running process, 
which is even less secure than the log. I'm working on passing the 
secret via the qemu monitor instead of the command line, which will 
avoid both issues.



Maybe this means we should we be adding capabilities into virCommand to
prevent the logging of the actual secret (whether base64-encoded or
otherwise), and instead log an alternate string?  That is, should
virCommand be tracking parallel argv arrays; the real array passed to
exec() but never logged, and the alternate array (normally matching the
real one, but which can differ in this particular case of passing an
argument that contains a password)?



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

[libvirt] [PATCH v5 4/4] qemu/rbd: improve rbd device specification

2011-10-31 Thread Josh Durgin 
From: Sage Weil s...@newdream.net

This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.

An auth member of the disk source xml specifies how librbd should
authenticate. The username attribute is the Ceph/RBD user to authenticate as.
The usage or uuid attributes specify which secret to use. Usage is an
arbitrary identifier local to libvirt.

The old RBD support relied on setting an environment variable to
communicate information to qemu/librbd.  Instead, pass those options
explicitly to qemu.  Update the qemu argument parsing and tests
accordingly.

Signed-off-by: Sage Weil s...@newdream.net
Signed-off-by: Josh Durgin josh.dur...@dreamhost.com
---

Changes since v4:
* fixes memory management issues
* keep older rbd command line parsing and test case
* check qemuAddRBDHost return values
* use more efficient virBuffer functions

 src/qemu/qemu_command.c|  356 ++--
 tests/qemuargv2xmltest.c   |2 +
 .../qemuxml2argv-disk-drive-network-rbd-auth.args  |   10 +
 .../qemuxml2argv-disk-drive-network-rbd-auth.xml   |   37 ++
 ...muxml2argv-disk-drive-network-rbd-ceph-env.args |6 +
 ...emuxml2argv-disk-drive-network-rbd-ceph-env.xml |   34 ++
 .../qemuxml2argv-disk-drive-network-rbd.args   |7 +-
 tests/qemuxml2argvtest.c   |   58 
 8 files changed, 406 insertions(+), 104 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dc92fa3..55859e2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -38,6 +38,7 @@
 #include domain_audit.h
 #include domain_conf.h
 #include network/bridge_driver.h
+#include base64.h
 
 #include sys/utsname.h
 #include sys/stat.h
@@ -1495,6 +1496,189 @@ qemuSafeSerialParamValue(const char *value)
 return 0;
 }
 
+static int
+qemuBuildRBDString(virConnectPtr conn,
+   virDomainDiskDefPtr disk,
+   virBufferPtr opt)
+{
+int i, ret = 0;
+virSecretPtr sec = NULL;
+char *secret = NULL;
+size_t secret_size;
+
+virBufferAsprintf(opt, rbd:%s, disk-src);
+if (disk-auth.username) {
+virBufferEscape(opt, :, :id=%s, disk-auth.username);
+/* look up secret */
+switch (disk-auth.secretType) {
+case VIR_DOMAIN_DISK_SECRET_TYPE_UUID:
+sec = virSecretLookupByUUID(conn,
+disk-auth.secret.uuid);
+break;
+case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE:
+sec = virSecretLookupByUsage(conn,
+ VIR_SECRET_USAGE_TYPE_CEPH,
+ disk-auth.secret.usage);
+break;
+}
+
+if (sec) {
+char *base64 = NULL;
+
+secret = (char *)conn-secretDriver-getValue(sec, secret_size, 0,
+  
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+if (secret == NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(could not get the value of the secret for 
username %s),
+disk-auth.username);
+goto error;
+}
+/* qemu/librbd wants it base64 encoded */
+base64_encode_alloc(secret, secret_size, base64);
+if (!base64) {
+virReportOOMError();
+goto error;
+}
+virBufferEscape(opt, :, :key=%s:auth_supported=cephx none,
+base64);
+VIR_FREE(base64);
+} else {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(rbd username '%s' specified but secret not 
found),
+disk-auth.username);
+goto error;
+}
+}
+
+if (disk-nhosts  0) {
+virBufferAddLit(opt, :mon_host=);
+for (i = 0; i  disk-nhosts; ++i) {
+if (i) {
+virBufferAddLit(opt, \\;);
+}
+if (disk-hosts[i].port) {
+virBufferAsprintf(opt, %s\\:%s,
+  disk-hosts[i].name,
+  disk-hosts[i].port);
+} else {
+virBufferAsprintf(opt, %s, disk-hosts[i].name);
+}
+}
+}
+
+cleanup:
+VIR_FREE(secret);
+if (sec)
+virUnrefSecret(sec);
+
+return ret;
+
+error:
+ret = -1;
+goto