Re: [Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Paolo Bonzini 


On 19/10/2015 17:09, Daniel P. Berrange wrote:
> +
> +switch (secret->format) {
> +case QCRYPTO_SECRET_FORMAT_UTF8:
> +if (!g_utf8_validate(input, strlen(input), NULL)) {
> +error_setg(errp,
> +   "Data from secret %s is not valid UTF-8",
> +   secretid);
> +goto cleanup;
> +}
> +output = input;
> +input = NULL;
> +break;

Why validate secrets as UTF-8?  In other words why have "utf8" instead
of "binary" as a possible QCryptoSecretFormat?

Paolo

Re: [Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Daniel P. Berrange 
On Mon, Oct 19, 2015 at 05:18:56PM +0200, Paolo Bonzini wrote:
> 
> 
> On 19/10/2015 17:09, Daniel P. Berrange wrote:
> > +
> > +switch (secret->format) {
> > +case QCRYPTO_SECRET_FORMAT_UTF8:
> > +if (!g_utf8_validate(input, strlen(input), NULL)) {
> > +error_setg(errp,
> > +   "Data from secret %s is not valid UTF-8",
> > +   secretid);
> > +goto cleanup;
> > +}
> > +output = input;
> > +input = NULL;
> > +break;
> 
> Why validate secrets as UTF-8?  In other words why have "utf8" instead
> of "binary" as a possible QCryptoSecretFormat?

JSON doesn't accept arbitrary 8-bit binary data, so the alternative
'base64' is effectively providing binary data facility. Having to
use base64 for plain passwords is rather tedious though, so allowing
utf8 is a much more developer friendly approach for people using QEMU
without a mgmt tool like libvirt.

NB, this dual-format utf8-or-base64 approach matches the approach used
in QEMU guest agent for the guest-file-read/write commands for the same
reason.

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 :|

Re: [Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Daniel P. Berrange 
On Mon, Oct 19, 2015 at 05:40:08PM +0200, Paolo Bonzini wrote:
> 
> 
> On 19/10/2015 17:24, Daniel P. Berrange wrote:
> > JSON doesn't accept arbitrary 8-bit binary data, so the alternative
> > 'base64' is effectively providing binary data facility. Having to
> > use base64 for plain passwords is rather tedious though, so allowing
> > utf8 is a much more developer friendly approach for people using QEMU
> > without a mgmt tool like libvirt.
> > 
> > NB, this dual-format utf8-or-base64 approach matches the approach used
> > in QEMU guest agent for the guest-file-read/write commands for the same
> > reason.
> 
> The difference is that guest-file-read/write have the payload in JSON;
> for file-based secrets the payload is not JSON.

For non-file based secrets though, the payload *is* in the JSON,
and per the cover letter, I actually anticipate passing all
secrets inline in the JSON and only using the file backend for
loading the initial master key. This avoids the need to do
file handle passing and/or create lots of temporary files, when
hotplugging resources.

> So I think that "binary" (which is the default anyway) would fit all the
> usecases (direct over JSON, file-based, direct over command line).
> Direct over JSON would be limited to valid UTF-8, but that's just a
> limitation of the transport.

I don't think that's actually an acceptable limitation - I want the
inline data passing to be fully usable for non-UTF-8 data too.

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 :|

[Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Daniel P. Berrange 
Introduce a new QCryptoSecret object class which will be used
for providing passwords and keys to other objects which need
sensitive credentials.

The new object can provide secret values directly as properties,
or indirectly via a file. The latter includes support for file
descriptor passing syntax on UNIX platforms. Ordinarily passing
secret values directly as properties is insecure, since they
are visible in process listings, or in log files showing the
CLI args / QMP commands. It is possible to use AES-256-CBC to
encrypt the secret values though, in which case all that is
visible is the ciphertext.  For adhoc developer testing though,
it is fine to provide the secrets directly without encryption
so this is not explicitly forbidden.

The anticipated scenario is that libvirtd will create a random
master key per QEMU instance (eg /var/run/libvirt/qemu/$VMNAME.key)
and will use that key to encrypt all passwords it provides to
QEMU via '-object secret,'.  This avoids the need for libvirt
(or other mgmt apps) to worry about file descriptor passing.

It also makes life easier for people who are scripting the
management of QEMU, for whom FD passing is significantly more
complex.

Providing data inline (insecure, only for adhoc dev tetsing)

  $QEMU -object secret,id=sec0,data=letmein

Providing data indirectly

  echo -n "letmein" > mypasswd.txt
  $QEMU -object secret,id=sec0,file=mypasswd.txt

Providing binary data

  $QEMU -object secret,id=sec0,file=mykey.bin,format=base64

Providing data with encyption

  $QEMU -object secret,id=master0,file=mykey.bin,format=base64 \
-object secret,id=sec0,data=[base64 ciphertext],\
   keyid=master0,iv=[base64 IV],format=utf8

Note that 'format' here refers to the format of the decrypted
data, which is independant of the ciphertext which must always
be in base64.

More examples are shown in the updated docs.

Signed-off-by: Daniel P. Berrange 
---
 crypto/Makefile.objs   |   1 +
 crypto/secret.c| 513 +
 include/crypto/secret.h| 139 
 qapi/crypto.json   |  14 ++
 qemu-options.hx|  76 +++
 tests/.gitignore   |   1 +
 tests/Makefile |   2 +
 tests/test-crypto-secret.c | 440 ++
 8 files changed, 1186 insertions(+)
 create mode 100644 crypto/secret.c
 create mode 100644 include/crypto/secret.h
 create mode 100644 tests/test-crypto-secret.c

diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index b2a0e0b..a3135f1 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -7,6 +7,7 @@ crypto-obj-y += tlscreds.o
 crypto-obj-y += tlscredsanon.o
 crypto-obj-y += tlscredsx509.o
 crypto-obj-y += tlssession.o
+crypto-obj-y += secret.o
 
 # Let the userspace emulators avoid linking gnutls/etc
 crypto-aes-obj-y = aes.o
diff --git a/crypto/secret.c b/crypto/secret.c
new file mode 100644
index 000..e1ee4fa
--- /dev/null
+++ b/crypto/secret.c
@@ -0,0 +1,513 @@
+/*
+ * QEMU crypto secret support
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ *
+ */
+
+#include "crypto/secret.h"
+#include "crypto/cipher.h"
+#include "qom/object_interfaces.h"
+#include "trace.h"
+
+static void
+qcrypto_secret_prop_set_loaded(Object *obj,
+   bool value,
+   Error **errp)
+{
+QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+int fd;
+gchar *data = NULL;
+size_t offset = 0;
+size_t length = 0;
+
+if (value) {
+if (secret->file) {
+if (secret->data) {
+error_setg(errp,
+   "'file' and 'data' are mutually exclusive");
+return;
+}
+fd = qemu_open(secret->file, O_RDONLY);
+if (fd < 0) {
+error_setg_errno(errp, errno,
+ "Unable to open %s", secret->file);
+return;
+}
+while (length < (1024 * 1024)) { /* Limit secrets to 1 MB */
+if ((length - offset) < 1024) {
+length += 1024;
+data = g_renew(gchar, data, length);
+}
+ssize_t ret = read(fd, data + offset, length - offset);
+

Re: [Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Paolo Bonzini 


On 19/10/2015 17:24, Daniel P. Berrange wrote:
> JSON doesn't accept arbitrary 8-bit binary data, so the alternative
> 'base64' is effectively providing binary data facility. Having to
> use base64 for plain passwords is rather tedious though, so allowing
> utf8 is a much more developer friendly approach for people using QEMU
> without a mgmt tool like libvirt.
> 
> NB, this dual-format utf8-or-base64 approach matches the approach used
> in QEMU guest agent for the guest-file-read/write commands for the same
> reason.

The difference is that guest-file-read/write have the payload in JSON;
for file-based secrets the payload is not JSON.

So I think that "binary" (which is the default anyway) would fit all the
usecases (direct over JSON, file-based, direct over command line).
Direct over JSON would be limited to valid UTF-8, but that's just a
limitation of the transport.

Paolo

Re: [Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Paolo Bonzini 


On 19/10/2015 17:46, Daniel P. Berrange wrote:
>> > The difference is that guest-file-read/write have the payload in JSON;
>> > for file-based secrets the payload is not JSON.
> For non-file based secrets though, the payload *is* in the JSON,
> and per the cover letter, I actually anticipate passing all
> secrets inline in the JSON and only using the file backend for
> loading the initial master key. This avoids the need to do
> file handle passing and/or create lots of temporary files, when
> hotplugging resources.
> 
> > So I think that "binary" (which is the default anyway) would fit all the
> > usecases (direct over JSON, file-based, direct over command line).
> > Direct over JSON would be limited to valid UTF-8, but that's just a
> > limitation of the transport.
> 
> I don't think that's actually an acceptable limitation - I want the
> inline data passing to be fully usable for non-UTF-8 data too.

Of course, there's base64 for that.  On the other hand, I think there's
no reason for unencoded file-based data passing to be usable for UTF-8
data only.

Paolo

Re: [Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Daniel P. Berrange 
On Mon, Oct 19, 2015 at 06:12:53PM +0200, Paolo Bonzini wrote:
> 
> 
> On 19/10/2015 17:46, Daniel P. Berrange wrote:
> >> > The difference is that guest-file-read/write have the payload in JSON;
> >> > for file-based secrets the payload is not JSON.
> > For non-file based secrets though, the payload *is* in the JSON,
> > and per the cover letter, I actually anticipate passing all
> > secrets inline in the JSON and only using the file backend for
> > loading the initial master key. This avoids the need to do
> > file handle passing and/or create lots of temporary files, when
> > hotplugging resources.
> > 
> > > So I think that "binary" (which is the default anyway) would fit all the
> > > usecases (direct over JSON, file-based, direct over command line).
> > > Direct over JSON would be limited to valid UTF-8, but that's just a
> > > limitation of the transport.
> > 
> > I don't think that's actually an acceptable limitation - I want the
> > inline data passing to be fully usable for non-UTF-8 data too.
> 
> Of course, there's base64 for that.  On the other hand, I think there's
> no reason for unencoded file-based data passing to be usable for UTF-8
> data only.

Ok, so there's really two separate formats at play here.

The input format, eg the encoding of the data= value, or the contents
of the file, and the output format, which is that required by the consumer
inside QEMU. We convert between the two. eg you can provide data in base64
even if QEMU ultimately needs to use it in plain utf-8 format, or vica-verca.

IIUC, you're suggesting that for the input format, the data=XXX value
should allow a choice of utf8 or base64, while the external file could
just take raw or base64 data. That's easy enough to wire up - just add
a 3rd option to the format enum and make raw be the default for files.

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 :|

Re: [Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Paolo Bonzini 


On 19/10/2015 18:24, Daniel P. Berrange wrote:
> The input format, eg the encoding of the data= value, or the contents
> of the file, and the output format, which is that required by the consumer
> inside QEMU. We convert between the two. eg you can provide data in base64
> even if QEMU ultimately needs to use it in plain utf-8 format, or vica-verca.

Right.  In the end QCryptoSecret only needs to provide a raw output;
converting it to something else, and possibly applying restrictions such
as UTF-8, should depend on the user.  Of course the API can include
helper functions for common restrictions, but in general a "secret
storage" module is independent of them.

> IIUC, you're suggesting that for the input format, the data=XXX value
> should allow a choice of utf8 or base64, while the external file could
> just take raw or base64 data. That's easy enough to wire up - just add
> a 3rd option to the format enum and make raw be the default for files.

Almost.

I am also saying that the utf8 case for data=XXX actually should be raw,
because utf8 is just a limitation of JSON and not of the data=XXX
interface.  Non-UTF8 data=XXX would then be accepted for the -object
command line option.

Paolo

Re: [Qemu-block] [PATCH 01/17] crypto: add QCryptoSecret object class for password/key handling

2015-10-19 Thread Daniel P. Berrange 
On Mon, Oct 19, 2015 at 06:28:26PM +0200, Paolo Bonzini wrote:
> 
> 
> On 19/10/2015 18:24, Daniel P. Berrange wrote:
> > The input format, eg the encoding of the data= value, or the contents
> > of the file, and the output format, which is that required by the consumer
> > inside QEMU. We convert between the two. eg you can provide data in base64
> > even if QEMU ultimately needs to use it in plain utf-8 format, or 
> > vica-verca.
> 
> Right.  In the end QCryptoSecret only needs to provide a raw output;
> converting it to something else, and possibly applying restrictions such
> as UTF-8, should depend on the user.  Of course the API can include
> helper functions for common restrictions, but in general a "secret
> storage" module is independent of them.
> 
> > IIUC, you're suggesting that for the input format, the data=XXX value
> > should allow a choice of utf8 or base64, while the external file could
> > just take raw or base64 data. That's easy enough to wire up - just add
> > a 3rd option to the format enum and make raw be the default for files.
> 
> Almost.
> 
> I am also saying that the utf8 case for data=XXX actually should be raw,
> because utf8 is just a limitation of JSON and not of the data=XXX
> interface.  Non-UTF8 data=XXX would then be accepted for the -object
> command line option.

Ah ok, I see what you mean now

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 :|