[Qemu-devel] [PATCH 0/2] block: Introduce "json-file:" protocol

[Qemu-devel] [PATCH 0/2] block: Introduce "json-file:" protocol

[Fam Zheng]

This would be a safer channel when we want to provide block options that
contain sensitive information, such as fields for authentication.



Fam Zheng (2):
  block: Add "json-file:" pseudo protocol
  iotests: Add tests for "json-file:" pseudo protocol

 block.c                    | 58 +++++++++++++++++++++++++++++++++++++++++-----
 tests/qemu-iotests/089     | 27 +++++++++++++++++----
 tests/qemu-iotests/089.out | 15 ++++++++++++
 3 files changed, 90 insertions(+), 10 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] block: Add "json-file:" pseudo protocol

[Fam Zheng]

When opening the image, "json-file:/path/to/json/file" has the same as
"json-file:$(cat /path/to/json/file)". The advantage is that sensitive
information that would be exposed in /proc/$PID/cmdline or log files,
is no longer visible this way.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index e9f40dc..785f317 100644
--- a/block.c
+++ b/block.c
@@ -962,10 +962,6 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
 {
     QObject *options_obj;
     QDict *options;
-    int ret;
-
-    ret = strstart(filename, "json:", &filename);
-    assert(ret);
 
     options_obj = qobject_from_json(filename);
     if (!options_obj) {
@@ -985,6 +981,36 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
     return options;
 }
 
+static char *read_json_file(const char *filename, Error **errp)
+{
+    BlockDriverState *file = NULL;
+    QDict *options;
+    int r;
+    int64_t sectors;
+    char *buf;
+
+    options = qdict_new();
+    qdict_put(options, "driver",
+              qstring_from_str("raw"));
+    r = bdrv_open(&file, filename, NULL, options, 0, errp);
+    if (r) {
+        return NULL;
+    }
+    sectors = DIV_ROUND_UP(bdrv_getlength(file), BDRV_SECTOR_SIZE);
+    if (sectors > (1024 * 1024 / BDRV_SECTOR_SIZE)) {
+        error_setg(errp, "JSON file is too large");
+        return NULL;
+    }
+    buf = g_malloc0(sectors << BDRV_SECTOR_BITS);
+    r = bdrv_read(file, 0, (uint8_t *)buf, sectors);
+    if (r) {
+        error_setg(errp, "Failed to read JSON file");
+        g_free(buf);
+        return NULL;
+    }
+    return buf;
+}
+
 /*
  * Fills in default options for opening images and converts the legacy
  * filename/flags pair to option QDict entries.
@@ -1002,8 +1028,28 @@ static int bdrv_fill_options(QDict **options, const char **pfilename,
     Error *local_err = NULL;
 
     /* Parse json: pseudo-protocol */
-    if (filename && g_str_has_prefix(filename, "json:")) {
-        QDict *json_options = parse_json_filename(filename, &local_err);
+    if (filename &&
+        (g_str_has_prefix(filename, "json:") ||
+         g_str_has_prefix(filename, "json-file:"))) {
+        char *json_str;
+        QDict *json_options;
+        int ret;
+
+        if (g_str_has_prefix(filename, "json-file:")) {
+            strstart(filename, "json-file:", &filename);
+            json_str = read_json_file(filename, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return -EIO;
+            }
+        } else {
+            ret = strstart(filename, "json:", &filename);
+            assert(ret);
+
+            json_str = g_strdup(filename);
+        }
+        json_options = parse_json_filename(json_str, &local_err);
+        g_free(json_str);
         if (local_err) {
             error_propagate(errp, local_err);
             return -EINVAL;
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] iotests: Add tests for "json-file:" pseudo protocol

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/089     | 27 +++++++++++++++++++++++----
 tests/qemu-iotests/089.out | 15 +++++++++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
index 3e0038d..6115563 100755
--- a/tests/qemu-iotests/089
+++ b/tests/qemu-iotests/089
@@ -67,9 +67,7 @@ $QEMU_IO -c 'write -P 42 0 512' -c 'write -P 23 512 512' \
 
 $QEMU_IMG convert -f raw -O $IMGFMT "$TEST_IMG.base" "$TEST_IMG"
 
-$QEMU_IO_PROG --cache $CACHEMODE \
-         -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
-         -c 'read -P 66 1024 512' "json:{
+img_opts="{
     \"driver\": \"$IMGFMT\",
     \"file\": {
         \"driver\": \"$IMGFMT\",
@@ -77,7 +75,15 @@ $QEMU_IO_PROG --cache $CACHEMODE \
             \"filename\": \"$TEST_IMG\"
         }
     }
-}" | _filter_qemu_io
+}"
+
+echo "$img_opts" > $TEST_IMG.json
+
+for f in "json:$img_opts" "json-file:$TEST_IMG.json"; do
+    $QEMU_IO_PROG --cache $CACHEMODE \
+             -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
+             -c 'read -P 66 1024 512' "$f" | _filter_qemu_io
+done
 
 # This should fail (see test 072)
 $QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
@@ -126,6 +132,19 @@ $QEMU_IO -c "open -o driver=qcow2 json:{\"file.filename\":\"$TEST_IMG\"}" \
 $QEMU_IO -c "open -o driver=qcow2 json:{\"driver\":\"raw\",\"file.filename\":\"$TEST_IMG\"}" \
          -c "info" 2>&1 | _filter_testdir | _filter_imgfmt
 
+echo
+echo "=== Test invalid json-file: ==="
+echo
+
+echo "This is not a valid json file#^*)@" > $TEST_IMG.json
+TEST_IMG="json-file:$TEST_IMG.json" _img_info
+
+echo
+echo "=== Test large json-file: ==="
+echo
+
+$QEMU_IMG create $TEST_IMG.json 1G
+TEST_IMG="json-file:$TEST_IMG.json" _img_info
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
index 5b541a3..6847f60 100644
--- a/tests/qemu-iotests/089.out
+++ b/tests/qemu-iotests/089.out
@@ -15,6 +15,12 @@ read 512/512 bytes at offset 512
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 512/512 bytes at offset 1024
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 1024
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Pattern verification failed at offset 0, 512 bytes
 read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -53,4 +59,13 @@ Format specific information:
     lazy refcounts: false
     refcount bits: 16
     corrupt: false
+
+=== Test invalid json-file: ===
+
+qemu-img: Could not open 'json-TEST_DIR/t.IMGFMT.json': Could not parse the JSON options
+
+=== Test large json-file: ===
+
+Formatting '/home/fam/build/last/tests/qemu-iotests/scratch/t.qcow2.json', fmt=raw size=1073741824
+qemu-img: Could not open 'json-TEST_DIR/t.IMGFMT.json': JSON file is too large
 *** done
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/2] block: Introduce "json-file:" protocol

[Daniel P. Berrange]

On Tue, Nov 03, 2015 at 03:01:16PM +0800, Fam Zheng wrote:
> This would be a safer channel when we want to provide block options that
> contain sensitive information, such as fields for authentication.

If passing of security sensitive data is the motivation for this,
then I don't think we want it really. This approach merely avoids
the config being visible in the process listing, which is really just
one problem. When people file bugs they are going to need to provide
the block device config, and that still going to contain sensitive
info and thus leak. Similarly apps generating block config like libvirt
are still going to be logging the block config they generate, which is
again going to leak secrets. Finally, we need the ability to pass
security sensitive data to QEMU in many other areas besides block devices
so need a more general mechanism

I've proposed a way to provide secrets to QEMU in a way that is
usable across all QEMU backends, that I think is a much better
approach because it totally decouples the sensitive data from
the rest of the config data, rather than having it inline

  https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04365.html

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-devel] [PATCH 0/2] block: Introduce "json-file:" protocol

[Fam Zheng]

On Tue, 11/03 07:35, Daniel P. Berrange wrote:
> On Tue, Nov 03, 2015 at 03:01:16PM +0800, Fam Zheng wrote:
> > This would be a safer channel when we want to provide block options that
> > contain sensitive information, such as fields for authentication.
> 
> If passing of security sensitive data is the motivation for this,
> then I don't think we want it really. This approach merely avoids
> the config being visible in the process listing, which is really just
> one problem. When people file bugs they are going to need to provide
> the block device config, and that still going to contain sensitive
> info and thus leak. Similarly apps generating block config like libvirt
> are still going to be logging the block config they generate, which is
> again going to leak secrets. Finally, we need the ability to pass
> security sensitive data to QEMU in many other areas besides block devices
> so need a more general mechanism
> 
> I've proposed a way to provide secrets to QEMU in a way that is
> usable across all QEMU backends, that I think is a much better
> approach because it totally decouples the sensitive data from
> the rest of the config data, rather than having it inline
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04365.html
> 

Apparently I've overlooked your post, thanks for pointing out. I think your
solution covers everything this series has to do.

Fam