Libguestfs - Sep 2019 - Re: [PATCH nbdkit 1/4] Add reflection plugin.

This series is based on my blog posting here:
https://rwmj.wordpress.com/2019/09/13/nbdkit-supports-exportnames/

It depends on the fix for realloc:
https://www.redhat.com/archives/libguestfs/2019-September/thread.html#00103

This series adds a fun plugin, and also an semi-related feature I've
long thought to be desirable.  You can consider patches 1 & 4, and
patches 2 & 3 as forming standalone patch series (but they do depend
on each other).

The fun plugin is a reflection plugin which "reflects" client
information back to the client.  We have a few fun plugins like this
(hello there, nbdkit-full-plugin) and normally I would push these
without review, but in this particular case there's a specific danger
that sending back data under control of the client might lead to a
security problem.  I _believe_ I have avoided that pitfall, but my
belief isn't as good as having experts review it :-)

The new feature is nbdkit_peer_name() which returns the sockaddr of
the peer.  It's essentially a wrapper around getpeername(2).  This
would allow several features to be implemented in future:

 - Plugins could accept or reject connections based on IP address.

 - Plugins could change content based on client.  (The fourth patch in
   the series is a PoC of this implemented in the new reflection
   plugin.)  Be cautious about combining this feature with multi-conn
   as it's not obviously always safe to do.

 - Some filters could usefully modify their behaviour based on client
   address: The TODO file currently notes that the rate filter could
   be changed to limit traffic based on client IP.

Rich.

The source for the easter egg example is not included, but it is:

        org 0x7c00
        ;  clear screen
        mov ah,0
        mov al,3
        int 0x10
        ; print string
        mov ah,0x13
        mov bl,0xa
        mov al,1
        mov cx,len
        mov dh,0
        mov dl,0
        mov bp,hello
        int 0x10
        hlt
hello:  db "*** Hello from nbdkit! ***",0xd,0xa
len:    equ $-hello
        ; pad to end of sector
        times 510-($-$$) db 0
        ; boot signature
        db 0x55,0xaa
---
 configure.ac                                  |   2 +
 docs/nbdkit-plugin.pod                        |   2 +
 plugins/data/nbdkit-data-plugin.pod           |   1 +
 plugins/memory/nbdkit-memory-plugin.pod       |   3 +-
 plugins/reflection/Makefile.am                |  64 +++++
 .../reflection/nbdkit-reflection-plugin.pod   | 104 +++++++
 plugins/reflection/reflection.c               | 265 ++++++++++++++++++
 tests/Makefile.am                             |   8 +
 tests/test-reflection-base64.sh               |  98 +++++++
 tests/test-reflection-raw.sh                  |  68 +++++
 10 files changed, 614 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 4af403c..8261a7f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -846,6 +846,7 @@ non_lang_plugins="\
         partitioning \
         pattern \
         random \
+        reflection \
         split \
         ssh \
         streaming \
@@ -922,6 +923,7 @@ AC_CONFIG_FILES([Makefile
                  plugins/perl/Makefile
                  plugins/python/Makefile
                  plugins/random/Makefile
+                 plugins/reflection/Makefile
                  plugins/ruby/Makefile
                  plugins/rust/Cargo.toml
                  plugins/rust/Makefile
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index e465410..39fa643 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -389,6 +389,8 @@ client data, be cautious when parsing it.>
 
 On error, C<nbdkit_error> is called and the call returns C<NULL>.
 
+See also L<nbdkit-reflection-plugin(1)>.
+
 =head1 CALLBACKS
 
 =head2 C<.name>
diff --git a/plugins/data/nbdkit-data-plugin.pod
b/plugins/data/nbdkit-data-plugin.pod
index b0957ad..fd342b4 100644
--- a/plugins/data/nbdkit-data-plugin.pod
+++ b/plugins/data/nbdkit-data-plugin.pod
@@ -240,6 +240,7 @@ L<nbdkit-null-plugin(1)>,
 L<nbdkit-partitioning-plugin(1)>,
 L<nbdkit-pattern-plugin(1)>,
 L<nbdkit-random-plugin(1)>,
+L<nbdkit-reflection-plugin(1)>,
 L<nbdkit-zero-plugin(1)>,
 L<https://github.com/libguestfs/nbdkit/blob/master/plugins/data/disk2data.pl>,
 L<https://en.wikipedia.org/wiki/Base64>.
diff --git a/plugins/memory/nbdkit-memory-plugin.pod
b/plugins/memory/nbdkit-memory-plugin.pod
index 76824d6..4503651 100644
--- a/plugins/memory/nbdkit-memory-plugin.pod
+++ b/plugins/memory/nbdkit-memory-plugin.pod
@@ -86,7 +86,8 @@ L<nbdkit(1)>,
 L<nbdkit-plugin(3)>,
 L<nbdkit-loop(1)>,
 L<nbdkit-data-plugin(1)>,
-L<nbdkit-file-plugin(1)>.
+L<nbdkit-file-plugin(1)>,
+L<nbdkit-reflection-plugin(1)>.
 
 =head1 AUTHORS
 
diff --git a/plugins/reflection/Makefile.am b/plugins/reflection/Makefile.am
new file mode 100644
index 0000000..40aa786
--- /dev/null
+++ b/plugins/reflection/Makefile.am
@@ -0,0 +1,64 @@
+# nbdkit
+# Copyright (C) 2017-2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+include $(top_srcdir)/common-rules.mk
+
+EXTRA_DIST = nbdkit-reflection-plugin.pod
+
+plugin_LTLIBRARIES = nbdkit-reflection-plugin.la
+
+nbdkit_reflection_plugin_la_SOURCES = \
+	reflection.c \
+	$(top_srcdir)/include/nbdkit-plugin.h \
+	$(NULL)
+
+nbdkit_reflection_plugin_la_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	$(NULL)
+nbdkit_reflection_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
+nbdkit_reflection_plugin_la_LDFLAGS = \
+	-module -avoid-version -shared \
+	-Wl,--version-script=$(top_srcdir)/plugins/plugins.syms \
+	$(NULL)
+nbdkit_reflection_plugin_la_LIBADD = \
+	$(NULL)
+
+if HAVE_POD
+
+man_MANS = nbdkit-reflection-plugin.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-reflection-plugin.1: nbdkit-reflection-plugin.pod
+	$(PODWRAPPER) --section=1 --man $@ \
+	    --html $(top_builddir)/html/$@.html \
+	    $<
+
+endif HAVE_POD
diff --git a/plugins/reflection/nbdkit-reflection-plugin.pod
b/plugins/reflection/nbdkit-reflection-plugin.pod
new file mode 100644
index 0000000..1b260b6
--- /dev/null
+++ b/plugins/reflection/nbdkit-reflection-plugin.pod
@@ -0,0 +1,104 @@
+=head1 NAME
+
+nbdkit-reflection-plugin - reflect export name back to the client
+
+=head1 SYNOPSIS
+
+ nbdkit reflection [mode=]exportname|base64exportname
+
+=head1 DESCRIPTION
+
+C<nbdkit-reflection-plugin> is a test plugin which reflects
+information sent by the client back to the client.
+
+In its default mode (C<mode=exportname>) it converts the export name
+passed from the client into a disk image.  C<mode=base64exportname> is
+similar except the client must base64-encode the data in the export
+name, allowing arbitrary binary data to be sent (see L</EXAMPLES>
+below to make this clearer).
+
+The plugin only supports read-only access.  To make the disk writable,
+add L<nbdkit-cow-filter(1)> on top.
+
+=head1 EXAMPLES
+
+Create a reflection disk.  By setting the export name to
C<"hello">
+when we open it, a virtual disk of only 5 bytes containing these
+characters is created.  We then display the contents:
+
+ $ nbdkit reflection mode=exportname
+ $ nbdsh -u 'nbd://localhost/hello' -c - <<'EOF'
+ size = h.get_size()
+ print("size = %d" % size)
+ buf = h.pread(size, 0)
+ print("buf = %r" % buf)
+ EOF
+
+ size = 5
+ buf = b"hello"
+
+By running a reflection plugin, you can pass whole bootable VMs on the
+qemu command line:
+
+ $ nbdkit reflection mode=base64exportname
+ $ qemu-system-x86_64 \
+   -drive
'snapshot=on,file.driver=nbd,file.host=localhost,file.port=10809,file.export+
tACwA80QtBOzCrABuRwAtgCyAL0ZfM0Q9CoqKiBIZWxsbyBmcm9tIG5iZGtp
+ dCEgKioqDQoAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
+ AAAAAAAAAAAAAAAAAAAAVao+ '
+
+=head1 PARAMETERS
+
+=over 4
+
+=item [B<mode=>]B<base64exportname>
+
+Reflect the export name passed by the client, assuming the client
+string is base64 encoded.
+
+This mode is only supported if nbdkit was compiled with GnuTLS E<ge>
+3.6.0.  You can find out by checking if:
+
+ $ nbdkit reflection --dump-plugin
+
+contains:
+
+ reflection_base64=yes
+
+=item [B<mode=>]B<exportname>
+
+Reflect the raw export name passed by the client.  Note the export
+name cannot contain ASCII NUL characters.
+
+This is the default mode.
+
+C<mode=> is a magic config key and may be omitted in most cases.
+See L<nbdkit(1)/Magic parameters>.
+
+=back
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-plugin(3)>,
+L<nbdkit-cow-filter(1)>,
+L<nbdkit-data-plugin(1)>,
+L<nbdkit-memory-plugin(1)/Preloading small amounts of data>.
+
+=head1 AUTHORS
+
+Richard W.M. Jones
+
+=head1 COPYRIGHT
+
+Copyright (C) 2019 Red Hat Inc.
diff --git a/plugins/reflection/reflection.c b/plugins/reflection/reflection.c
new file mode 100644
index 0000000..74f7169
--- /dev/null
+++ b/plugins/reflection/reflection.c
@@ -0,0 +1,265 @@
+/* nbdkit
+ * Copyright (C) 2017-2019 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#if defined(HAVE_GNUTLS) && defined(HAVE_GNUTLS_BASE64_DECODE2)
+#include <gnutls/gnutls.h>
+#define HAVE_BASE64 1
+#endif
+
+#define NBDKIT_API_VERSION 2
+
+#include <nbdkit-plugin.h>
+
+/* The mode. */
+enum mode {
+  MODE_EXPORTNAME,
+  MODE_BASE64EXPORTNAME,
+};
+static enum mode mode = MODE_EXPORTNAME;
+
+static int
+reflection_config (const char *key, const char *value)
+{
+  if (strcmp (key, "mode") == 0) {
+    if (strcasecmp (value, "exportname") == 0) {
+      mode = MODE_EXPORTNAME;
+    }
+    else if (strcasecmp (value, "base64exportname") == 0) {
+#ifdef HAVE_BASE64
+      mode = MODE_BASE64EXPORTNAME;
+#else
+      nbdkit_error ("the plugin was compiled without base64
support");
+      return -1;
+#endif
+    }
+    else {
+      nbdkit_error ("unknown mode: '%s'", value);
+      return -1;
+    }
+  }
+  else {
+    nbdkit_error ("unknown parameter '%s'", key);
+    return -1;
+  }
+
+  return 0;
+}
+
+#define reflection_config_help \
+  "mode=MODE    Plugin mode."
+
+/* Provide a way to detect if the base64 feature is supported. */
+static void
+reflection_dump_plugin (void)
+{
+#ifdef HAVE_BASE64
+  printf ("reflection_base64=yes\n");
+#endif
+}
+
+/* Per-connection handle. */
+struct handle {
+  void *data;                   /* Block device data. */
+  size_t len;                   /* Length of data in bytes. */
+};
+
+static int
+decode_base64 (const char *data, size_t len, struct handle *ret)
+{
+#ifdef HAVE_BASE64
+  gnutls_datum_t in, out;
+  int err;
+
+  /* For unclear reasons gnutls_base64_decode2 won't handle an empty
+   * string, even though base64("") == "".
+   * https://tools.ietf.org/html/rfc4648#section-10
+   * https://gitlab.com/gnutls/gnutls/issues/834
+   * So we have to special-case it.
+   */
+  if (len == 0) {
+    ret->data = NULL;
+    ret->len = 0;
+    return 0;
+  }
+
+  in.data = (unsigned char *) data;
+  in.size = len;
+  err = gnutls_base64_decode2 (&in, &out);
+  if (err != GNUTLS_E_SUCCESS) {
+    nbdkit_error ("base64: %s", gnutls_strerror (err));
+    /* We don't have to free out.data.  I verified that it is freed on
+     * the error path of gnutls_base64_decode2.
+     */
+    return -1;
+  }
+
+  ret->data = out.data;         /* caller frees, eventually */
+  ret->len = out.size;
+  return 0;
+#else
+  nbdkit_error ("the plugin was compiled without base64 support");
+  return -1;
+#endif
+}
+
+/* Create the per-connection handle.
+ *
+ * This is a rather unusual plugin because it has to parse data sent
+ * by the client.  For security reasons, be careful about:
+ *
+ * - Returning more data than is sent by the client.
+ *
+ * - Inputs that result in unbounded output.
+ *
+ * - Inputs that could hang, crash or exploit the server.
+ */
+static void *
+reflection_open (int readonly)
+{
+  const char *export_name;
+  size_t export_name_len;
+  struct handle *h;
+
+  h = malloc (sizeof *h);
+  if (h == NULL) {
+    nbdkit_error ("malloc: %m");
+    return NULL;
+  }
+
+  switch (mode) {
+  case MODE_EXPORTNAME:
+  case MODE_BASE64EXPORTNAME:
+    export_name = nbdkit_export_name ();
+    if (export_name == NULL) {
+      free (h);
+      return NULL;
+    }
+    export_name_len = strlen (export_name);
+
+    if (mode == MODE_EXPORTNAME) {
+      h->len = export_name_len;
+      h->data = strdup (export_name);
+      if (h->data == NULL) {
+        nbdkit_error ("strdup: %m");
+        free (h);
+        return NULL;
+      }
+      return h;
+    }
+    else /* mode == MODE_BASE64EXPORTNAME */ {
+      if (decode_base64 (export_name, export_name_len, h) == -1) {
+        free (h);
+        return NULL;
+      }
+      return h;
+    }
+
+  default:
+    abort ();
+  }
+}
+
+/* Close the per-connection handle. */
+static void
+reflection_close (void *handle)
+{
+  struct handle *h = handle;
+
+  free (h->data);
+  free (h);
+}
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+/* Get the disk size. */
+static int64_t
+reflection_get_size (void *handle)
+{
+  struct handle *h = handle;
+
+  return (int64_t) h->len;
+}
+
+/* Read-only plugin so multi-conn is safe. */
+static int
+reflection_can_multi_conn (void *handle)
+{
+  return 1;
+}
+
+/* Cache. */
+static int
+reflection_can_cache (void *handle)
+{
+  /* Everything is already in memory, returning this without
+   * implementing .cache lets nbdkit do the correct no-op.
+   */
+  return NBDKIT_CACHE_NATIVE;
+}
+
+/* Read data. */
+static int
+reflection_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
+                  uint32_t flags)
+{
+  struct handle *h = handle;
+
+  memcpy (buf, h->data + offset, count);
+  return 0;
+}
+
+static struct nbdkit_plugin plugin = {
+  .name              = "reflection",
+  .version           = PACKAGE_VERSION,
+  .config            = reflection_config,
+  .config_help       = reflection_config_help,
+  .dump_plugin       = reflection_dump_plugin,
+  .magic_config_key  = "mode",
+  .open              = reflection_open,
+  .close             = reflection_close,
+  .get_size          = reflection_get_size,
+  .can_multi_conn    = reflection_can_multi_conn,
+  .can_cache         = reflection_can_cache,
+  .pread             = reflection_pread,
+  /* In this plugin, errno is preserved properly along error return
+   * paths from failed system calls.
+   */
+  .errno_is_preserved = 1,
+};
+
+NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9eec75e..69d5d5e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -111,6 +111,8 @@ EXTRA_DIST = \
 	test-rate-dynamic.sh \
 	test.rb \
 	test-readahead-copy.sh \
+	test-reflection-base64.sh \
+	test-reflection-raw.sh \
 	test-shutdown.sh \
 	test-ssh.sh \
 	test.tcl \
@@ -602,6 +604,12 @@ test_random_CPPFLAGS = -I $(top_srcdir)/common/include
 test_random_CFLAGS = $(WARNINGS_CFLAGS) $(LIBGUESTFS_CFLAGS)
 test_random_LDADD = libtest.la $(LIBGUESTFS_LIBS)
 
+# reflection plugin test.
+TESTS += \
+	test-reflection-base64.sh \
+	test-reflection-raw.sh \
+	$(NULL)
+
 # split files plugin test.
 check_DATA += split1 split2 split3
 CLEANFILES += split1 split2 split3
diff --git a/tests/test-reflection-base64.sh b/tests/test-reflection-base64.sh
new file mode 100755
index 0000000..cf21bf0
--- /dev/null
+++ b/tests/test-reflection-base64.sh
@@ -0,0 +1,98 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Test the relection plugin with base64-encoded export name.
+
+source ./functions.sh
+set -e
+set -x
+
+requires nbdsh --version
+# XXX This needs to test $PYTHON somehow.
+#requires python -c 'import base64'
+
+# Test if mode=base64exportname is supported in this build.
+if ! nbdkit reflection --dump-plugin | grep -sq
"reflection_base64=yes"; then
+    echo "$0: mode=base64exportname is not supported in this build"
+    exit 77
+fi
+
+sock=`mktemp -u`
+files="reflection-base64.out reflection-base64.pid $sock"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Run nbdkit.
+start_nbdkit -P reflection-base64.pid -U $sock \
+       reflection mode=base64exportname
+
+export e sock
+for e in "" "test" "テスト" \
+        
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+do
+    nbdsh -c '
+import os
+import base64
+
+e = os.environ["e"]
+b = base64.b64encode(e.encode("utf-8")).decode("utf-8")
+print ("e = %r, b = %r" % (e,b))
+h.set_export_name (b)
+h.connect_unix (os.environ["sock"])
+
+size = h.get_size ()
+assert size == len (e.encode("utf-8"))
+
+# Zero-sized reads are not defined in the NBD protocol.
+if size > 0:
+   buf = h.pread (size, 0)
+   assert buf == e.encode("utf-8")
+'
+done
+
+# Test that it fails if the caller passes in non-base64 data.  The
+# server drops the connection in this case so it's not very graceful
+# but we should at least get an nbd.Error and not something else.
+nbdsh -c '
+import os
+import sys
+
+h.set_export_name ("xyz")
+try:
+    h.connect_unix (os.environ["sock"])
+    # This should not happen.
+    sys.exit (1)
+except nbd.Error as ex:
+    sys.exit (0)
+# This should not happen.
+sys.exit (1)
+'
diff --git a/tests/test-reflection-raw.sh b/tests/test-reflection-raw.sh
new file mode 100755
index 0000000..675e316
--- /dev/null
+++ b/tests/test-reflection-raw.sh
@@ -0,0 +1,68 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Test the relection plugin with raw export name.
+
+source ./functions.sh
+set -e
+set -x
+
+requires nbdsh --version
+
+sock=`mktemp -u`
+files="reflection-raw.out reflection-raw.pid $sock"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Run nbdkit.
+start_nbdkit -P reflection-raw.pid -U $sock reflection
+
+for e in "" "test" "テスト" \
+        
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+do
+    export e sock
+    nbdsh -c '
+import os
+
+e = os.environ["e"]
+h.set_export_name (e)
+h.connect_unix (os.environ["sock"])
+
+size = h.get_size ()
+assert size == len (e.encode("utf-8"))
+
+# Zero-sized reads are not defined in the NBD protocol.
+if size > 0:
+   buf = h.pread (size, 0)
+   assert buf == e.encode("utf-8")
+'
+done
-- 
2.23.0

We are going to change <nbdkit-common.h> so it includes
<sys/socket.h>.  However because this exports connect(2) we need to
rename globals called ‘connect’ to avoid a conflict.
---
 plugins/guestfs/guestfs-plugin.c | 8 ++++----
 plugins/libvirt/libvirt-plugin.c | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/plugins/guestfs/guestfs-plugin.c b/plugins/guestfs/guestfs-plugin.c
index c8af987..a958f41 100644
--- a/plugins/guestfs/guestfs-plugin.c
+++ b/plugins/guestfs/guestfs-plugin.c
@@ -48,7 +48,7 @@
 #include "cleanup.h"
 
 /* Configuration. */
-static const char *connect = NULL; /* libvirt URI */
+static const char *libvirt_uri = NULL; /* libvirt URI */
 static const char *export = NULL;  /* export device or file */
 static const char *format = NULL;  /* format parameter */
 static int trace = 0, debug = 0;
@@ -87,7 +87,7 @@ plugin_guestfs_config (const char *key, const char *value)
     }
   }
   else if (strcmp (key, "connect") == 0) {
-    connect = value;
+    libvirt_uri = value;
   }
   else if (strcmp (key, "export") == 0) {
     export = value;
@@ -355,9 +355,9 @@ add_disks (guestfs_h *g, int readonly, struct drive *drives)
       GUESTFS_ADD_DOMAIN_ALLOWUUID_BITMASK;
     domain_optargs.readonly = readonly;
     domain_optargs.allowuuid = 1;
-    if (connect) {
+    if (libvirt_uri) {
       domain_optargs.bitmask |= GUESTFS_ADD_DOMAIN_LIBVIRTURI_BITMASK;
-      domain_optargs.libvirturi = connect;
+      domain_optargs.libvirturi = libvirt_uri;
     }
     if (guestfs_add_domain_argv (g, drives->value, &domain_optargs) ==
-1) {
       GERROR (g, "domain %s", drives->value);
diff --git a/plugins/libvirt/libvirt-plugin.c b/plugins/libvirt/libvirt-plugin.c
index 71cac42..5fdaef3 100644
--- a/plugins/libvirt/libvirt-plugin.c
+++ b/plugins/libvirt/libvirt-plugin.c
@@ -55,7 +55,7 @@
 #include <nbdkit-plugin.h>
 
 /* Configuration. */
-static const char *connect = NULL;
+static const char *libvirt_uri = NULL;
 static const char *domain = NULL;
 static const char *disk = NULL;
 
@@ -63,7 +63,7 @@ static int
 virt_config (const char *key, const char *value)
 {
   if (strcmp (key, "connect") == 0) {
-    connect = value;
+    libvirt_uri = value;
   }
   else if (strcmp (key, "domain") == 0) {
     domain = value;
@@ -119,7 +119,7 @@ virt_open (int readonly)
   }
 
   /* Connect to libvirt. */
-  h->conn = virConnectOpen (connect);
+  h->conn = virConnectOpen (libvirt_uri);
   if (!h->conn) {
     nbdkit_error ("virConnectOpen failed, see earlier error
messages");
     goto err1;
-- 
2.23.0

Works essentially just like calling getpeername(2), because that's how
it is implemented.
---
 TODO                    |  6 ++++++
 docs/nbdkit-plugin.pod  | 23 +++++++++++++++++++++++
 include/nbdkit-common.h |  2 ++
 server/nbdkit.syms      |  1 +
 server/public.c         | 21 +++++++++++++++++++++
 5 files changed, 53 insertions(+)

diff --git a/TODO b/TODO
index 04def3c..d2cf0ae 100644
--- a/TODO
+++ b/TODO
@@ -77,6 +77,12 @@ General ideas for improvements
   name(s) that a plugin might want to support.  Probably we should
   deprecate the -e option entirely since it does nothing useful.
 
+* Add plugin "connect" method.  This would be called on a connection
+  before handshaking or TLS negotiation, and could be used (with
+  nbdkit_peer_name) to accept or reject connections based on IP
+  address, rather like a poor man's TCP wrappers.  See also commit
+  c05686f9577f.
+
 Suggestions for plugins
 -----------------------
 
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 39fa643..70d2d64 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -391,6 +391,29 @@ On error, C<nbdkit_error> is called and the call
returns C<NULL>.
 
 See also L<nbdkit-reflection-plugin(1)>.
 
+=head1 PEER NAME
+
+It is possible to get the address of the client when you are running
+in any connected callback.
+
+=head2 C<nbdkit_peer_name>
+
+ int nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen);
+
+Return the peer (client) address, if available.  The C<addr> and
+C<addrlen> parameters behave like L<getpeername(2)>.  In particular
+you must initialize C<addrlen> with the size of the buffer pointed to
+by C<addr>, and if C<addr> is not large enough then the address
will
+be truncated.
+
+In some cases this is not available or the address returned will be
+meaningless (eg. if there is a proxy between the client and nbdkit).
+This call uses thread-local magic so no parameter is required to
+specify the current connection.
+
+On success this returns C<0>.  On error, C<nbdkit_error> is called
and
+this call returns C<-1>.
+
 =head1 CALLBACKS
 
 =head2 C<.name>
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index acf0abd..aac63fb 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -40,6 +40,7 @@
 #include <stdarg.h>
 #include <stdint.h>
 #include <errno.h>
+#include <sys/socket.h>
 
 #include <nbdkit-version.h>
 
@@ -87,6 +88,7 @@ extern int nbdkit_read_password (const char *value, char
**password);
 extern char *nbdkit_realpath (const char *path);
 extern int nbdkit_nanosleep (unsigned sec, unsigned nsec);
 extern const char *nbdkit_export_name (void);
+extern int nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen);
 
 struct nbdkit_extents;
 extern int nbdkit_add_extent (struct nbdkit_extents *,
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index 1fb1315..d792a5f 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -50,6 +50,7 @@
     nbdkit_nanosleep;
     nbdkit_parse_bool;
     nbdkit_parse_size;
+    nbdkit_peer_name;
     nbdkit_read_password;
     nbdkit_realpath;
     nbdkit_set_error;
diff --git a/server/public.c b/server/public.c
index 96ab353..c8b06a6 100644
--- a/server/public.c
+++ b/server/public.c
@@ -49,6 +49,7 @@
 #include <errno.h>
 #include <poll.h>
 #include <signal.h>
+#include <sys/socket.h>
 
 #include "get-current-dir-name.h"
 
@@ -392,3 +393,23 @@ nbdkit_export_name (void)
 
   return conn->exportname;
 }
+
+int
+nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen)
+{
+  struct connection *conn = threadlocal_get_conn ();
+  int s;
+
+  if (!conn) {
+    nbdkit_error ("no connection in this thread");
+    return -1;
+  }
+
+  s = conn->sockin;
+  if (s == -1) {
+    nbdkit_error ("socket not open");
+    return -1;
+  }
+
+  return getpeername (s, addr, addrlen);
+}
-- 
2.23.0

---
 .../reflection/nbdkit-reflection-plugin.pod   | 23 ++++-
 plugins/reflection/reflection.c               | 88 +++++++++++++++++++
 tests/Makefile.am                             |  2 +
 tests/test-reflection-address.sh              | 63 +++++++++++++
 4 files changed, 174 insertions(+), 2 deletions(-)

diff --git a/plugins/reflection/nbdkit-reflection-plugin.pod
b/plugins/reflection/nbdkit-reflection-plugin.pod
index 1b260b6..7f52c58 100644
--- a/plugins/reflection/nbdkit-reflection-plugin.pod
+++ b/plugins/reflection/nbdkit-reflection-plugin.pod
@@ -1,10 +1,10 @@
 =head1 NAME
 
-nbdkit-reflection-plugin - reflect export name back to the client
+nbdkit-reflection-plugin - reflect client info back to the client
 
 =head1 SYNOPSIS
 
- nbdkit reflection [mode=]exportname|base64exportname
+ nbdkit reflection [mode=]exportname|base64exportname|address
 
 =head1 DESCRIPTION
 
@@ -17,6 +17,9 @@ similar except the client must base64-encode the data in the
export
 name, allowing arbitrary binary data to be sent (see L</EXAMPLES>
 below to make this clearer).
 
+C<mode=address> creates a disk which contains the client's IP address
+and port number as a string.
+
 The plugin only supports read-only access.  To make the disk writable,
 add L<nbdkit-cow-filter(1)> on top.
 
@@ -57,10 +60,26 @@ qemu command line:
  AAAAAAAAAAAAAAAAAAAAVao  '
 
+Another use for the reflection plugin is to send back the client's IP
+address:
+
+ $ nbdkit reflection mode=address
+ $ nbdsh -u 'nbd://localhost' -c 'print(h.pread(h.get_size(),
0))'
+
+which will print something like:
+
+ b'[::1]:58912'
+
 =head1 PARAMETERS
 
 =over 4
 
+=item [B<mode=>]B<address>
+
+Reflect the client's IP address and client port number as a string in
+the usual format.  For Unix sockets this sets the disk to the string
+C<"unix"> to avoid leaking host paths.
+
 =item [B<mode=>]B<base64exportname>
 
 Reflect the export name passed by the client, assuming the client
diff --git a/plugins/reflection/reflection.c b/plugins/reflection/reflection.c
index 74f7169..a0d7c60 100644
--- a/plugins/reflection/reflection.c
+++ b/plugins/reflection/reflection.c
@@ -35,6 +35,9 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
 
 #if defined(HAVE_GNUTLS) && defined(HAVE_GNUTLS_BASE64_DECODE2)
 #include <gnutls/gnutls.h>
@@ -49,6 +52,7 @@
 enum mode {
   MODE_EXPORTNAME,
   MODE_BASE64EXPORTNAME,
+  MODE_ADDRESS,
 };
 static enum mode mode = MODE_EXPORTNAME;
 
@@ -67,6 +71,9 @@ reflection_config (const char *key, const char *value)
       return -1;
 #endif
     }
+    else if (strcasecmp (value, "address") == 0) {
+      mode = MODE_ADDRESS;
+    }
     else {
       nbdkit_error ("unknown mode: '%s'", value);
       return -1;
@@ -137,6 +144,74 @@ decode_base64 (const char *data, size_t len, struct handle
*ret)
 #endif
 }
 
+static int
+handle_address (struct sockaddr *sa, socklen_t addrlen,
+                struct handle *ret)
+{
+  struct sockaddr_in *addr = (struct sockaddr_in *) sa;
+  struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *) sa;
+  union {
+    char straddr[INET_ADDRSTRLEN];
+    char straddr6[INET6_ADDRSTRLEN];
+  } u;
+  int r;
+  char *str;
+
+  switch (addr->sin_family) {
+  case AF_INET:
+    if (inet_ntop (AF_INET, &addr->sin_addr,
+                   u.straddr, sizeof u.straddr) == NULL) {
+      nbdkit_error ("inet_ntop: %m");
+      return -1;
+    }
+    r = asprintf (&str, "%s:%d", u.straddr, ntohs
(addr->sin_port));
+    if (r == -1) {
+      nbdkit_error ("asprintf: %m");
+      return -1;
+    }
+    ret->len = r;
+    ret->data = str;
+    return 0;
+
+  case AF_INET6:
+    if (inet_ntop (AF_INET6, &addr6->sin6_addr,
+                   u.straddr6, sizeof u.straddr6) == NULL) {
+      nbdkit_error ("inet_ntop: %m");
+      return -1;
+    }
+    r = asprintf (&str, "[%s]:%d", u.straddr6, ntohs
(addr6->sin6_port));
+    if (r == -1) {
+      nbdkit_error ("asprintf: %m");
+      return -1;
+    }
+    ret->len = r;
+    ret->data = str;
+    return 0;
+
+  case AF_UNIX:
+    /* We don't want to expose the socket path because it's a host
+     * filesystem name.  The client might not really be running on the
+     * same machine (eg. it using a proxy).  However it doesn't even
+     * matter because getpeername(2) on Linux returns a zero length
+     * sun_path in this case!
+     */
+    str = strdup ("unix");
+    if (str == NULL) {
+      nbdkit_error ("strdup: %m");
+      return -1;
+    }
+    ret->len = strlen (str);
+    ret->data = str;
+    return 0;
+
+  default:
+    nbdkit_debug ("unsupported socket family %d",
addr->sin_family);
+    ret->data = NULL;
+    ret->len = 0;
+    return 0;
+  }
+}
+
 /* Create the per-connection handle.
  *
  * This is a rather unusual plugin because it has to parse data sent
@@ -147,12 +222,16 @@ decode_base64 (const char *data, size_t len, struct handle
*ret)
  * - Inputs that result in unbounded output.
  *
  * - Inputs that could hang, crash or exploit the server.
+ *
+ * - Leaking host information (eg. paths).
  */
 static void *
 reflection_open (int readonly)
 {
   const char *export_name;
   size_t export_name_len;
+  struct sockaddr_storage addr;
+  socklen_t addrlen;
   struct handle *h;
 
   h = malloc (sizeof *h);
@@ -189,6 +268,15 @@ reflection_open (int readonly)
       return h;
     }
 
+  case MODE_ADDRESS:
+    addrlen = sizeof addr;
+    if (nbdkit_peer_name ((struct sockaddr *) &addr, &addrlen) == -1 ||
+        handle_address ((struct sockaddr *) &addr, addrlen, h) == -1) {
+      free (h);
+      return NULL;
+    }
+    return h;
+
   default:
     abort ();
   }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 69d5d5e..1b1e05b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -111,6 +111,7 @@ EXTRA_DIST = \
 	test-rate-dynamic.sh \
 	test.rb \
 	test-readahead-copy.sh \
+	test-reflection-address.sh \
 	test-reflection-base64.sh \
 	test-reflection-raw.sh \
 	test-shutdown.sh \
@@ -606,6 +607,7 @@ test_random_LDADD = libtest.la $(LIBGUESTFS_LIBS)
 
 # reflection plugin test.
 TESTS += \
+	test-reflection-address.sh \
 	test-reflection-base64.sh \
 	test-reflection-raw.sh \
 	$(NULL)
diff --git a/tests/test-reflection-address.sh b/tests/test-reflection-address.sh
new file mode 100755
index 0000000..68a4e8e
--- /dev/null
+++ b/tests/test-reflection-address.sh
@@ -0,0 +1,63 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Test the relection plugin with mode=address.
+
+source ./functions.sh
+set -e
+set -x
+
+requires nbdsh --version
+
+sock=`mktemp -u`
+files="reflection-address.out reflection-address.pid $sock"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Run nbdkit.
+start_nbdkit -P reflection-address.pid -U $sock \
+       reflection mode=address
+
+export sock
+nbdsh -c - <<'EOF'
+import os
+import re
+
+h.connect_unix (os.environ["sock"])
+
+size = h.get_size ()
+assert size > 0
+
+buf = h.pread (size, 0)
+print ("buf = %r" % buf)
+assert buf == b'unix'
+EOF
-- 
2.23.0

On Sun, Sep 15, 2019 at 03:55:41PM +0100, Richard W.M. Jones
wrote:
>  - Plugins could change content based on client.  (The fourth patch in
>    the series is a PoC of this implemented in the new reflection
>    plugin.)  Be cautious about combining this feature with multi-conn
>    as it's not obviously always safe to do.
Given this commit, I guess we should squash in the following to the
4th patch:

diff --git a/plugins/reflection/reflection.c b/plugins/reflection/reflection.c
index a0d7c60..f765557 100644
--- a/plugins/reflection/reflection.c
+++ b/plugins/reflection/reflection.c
@@ -303,11 +303,22 @@ reflection_get_size (void *handle)
   return (int64_t) h->len;
 }
 
-/* Read-only plugin so multi-conn is safe. */
 static int
 reflection_can_multi_conn (void *handle)
 {
-  return 1;
+  switch (mode) {
+    /* Safe for exportname modes since clients should only request
+     * multi-conn with the same export name.
+     */
+  case MODE_EXPORTNAME:
+  case MODE_BASE64EXPORTNAME:
+    return 1;
+    /* Unsafe for mode=address because all multi-conn connections
+     * won't necessarily originate from the same client address.
+     */
+  case MODE_ADDRESS:
+    return 0;
+  }
 }
 
 /* Cache. */


Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Sun, Sep 15, 2019 at 03:55:41PM +0100, Richard W.M. Jones
wrote:
>This series is based on my blog posting here:
>https://rwmj.wordpress.com/2019/09/13/nbdkit-supports-exportnames/
>
>It depends on the fix for realloc:
>https://www.redhat.com/archives/libguestfs/2019-September/thread.html#00103
>
>This series adds a fun plugin, and also an semi-related feature I've
>long thought to be desirable.  You can consider patches 1 & 4, and
>patches 2 & 3 as forming standalone patch series (but they do depend
>on each other).
>
>The fun plugin is a reflection plugin which "reflects" client
>information back to the client.  We have a few fun plugins like this
>(hello there, nbdkit-full-plugin) and normally I would push these
>without review, but in this particular case there's a specific danger
>that sending back data under control of the client might lead to a
>security problem.  I _believe_ I have avoided that pitfall, but my
>belief isn't as good as having experts review it :-)
>
Even though I am not part of that group you mentioned I had a look at it and I
could not find any issue with that; you are keeping the size returned by the
base64 decoder and other than that it is all data which should be safe as far as
I understand it.
>The new feature is nbdkit_peer_name() which returns the sockaddr of
>the peer.  It's essentially a wrapper around getpeername(2).  This
It's a pity there is no information for UNIX sockets.  Would it make sense
to at
least try to get the PID (or even a command name) using at least:

  getsockopt(..., SOL_SOCKET, SO_PEERCRED, ...)

at least where SO_PEERCRED is defined?

It looks good to me.
>would allow several features to be implemented in future:
>
> - Plugins could accept or reject connections based on IP address.
>
> - Plugins could change content based on client.  (The fourth patch in
>   the series is a PoC of this implemented in the new reflection
>   plugin.)  Be cautious about combining this feature with multi-conn
>   as it's not obviously always safe to do.
>
> - Some filters could usefully modify their behaviour based on client
>   address: The TODO file currently notes that the rate filter could
>   be changed to limit traffic based on client IP.
>
>Rich.
>
>
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs

On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> The source for the easter egg example is not included, but it is:
> 
>         org 0x7c00
>         ;  clear screen
>         mov ah,0
>         mov al,3
>         int 0x10
>         ; print string
>         mov ah,0x13
>         mov bl,0xa
>         mov al,1
>         mov cx,len
>         mov dh,0
>         mov dl,0
>         mov bp,hello
>         int 0x10
>         hlt
> hello:  db "*** Hello from nbdkit! ***",0xd,0xa
> len:    equ $-hello
>         ; pad to end of sector
>         times 510-($-$$) db 0
>         ; boot signature
>         db 0x55,0xaa
> ---
> +++ b/plugins/reflection/nbdkit-reflection-plugin.pod
> @@ -0,0 +1,104 @@
> +=head1 NAME
> +
> +nbdkit-reflection-plugin - reflect export name back to the client
> +
> +=head1 SYNOPSIS
> +
> + nbdkit reflection [mode=]exportname|base64exportname
> +
> +=head1 DESCRIPTION
> +
> +C<nbdkit-reflection-plugin> is a test plugin which reflects
> +information sent by the client back to the client.
> +
> +In its default mode (C<mode=exportname>) it converts the export name
> +passed from the client into a disk image.  C<mode=base64exportname>
is
> +similar except the client must base64-encode the data in the export
> +name, allowing arbitrary binary data to be sent (see L</EXAMPLES>
> +below to make this clearer).
Is it worth noting that the NBD protocol imposes a 4k limit on the
export name, which would limit things to about a 3k disk image when
using base64?  (It looks like nbdkit does not currently enforce that
limit -- maybe it should, but as a separate patch -- but even if we
don't change that, you're still limited to finding a client willing to
send an export name larger than the protocol permits)
> +
> +The plugin only supports read-only access.  To make the disk writable,
> +add L<nbdkit-cow-filter(1)> on top.
> +
> +=head1 EXAMPLES
> +
> +Create a reflection disk.  By setting the export name to
C<"hello">
> +when we open it, a virtual disk of only 5 bytes containing these
> +characters is created.  We then display the contents:
> +
> + $ nbdkit reflection mode=exportname
> + $ nbdsh -u 'nbd://localhost/hello' -c - <<'EOF'
> + size = h.get_size()
> + print("size = %d" % size)
> + buf = h.pread(size, 0)
> + print("buf = %r" % buf)
> + EOF
This leaves nbdkit running as a background daemon after the fact.  Is it
worth implementing a new 'nbdkit --one-shot' flag that causes nbdkit to
exit after the first client disconnects?  I also wonder if we want to
add a way to differentiate between SIGINT (terminate nbdkit as soon as
possible) vs. SIGHUP (no new clients permitted, but keep nbdkit up and
running for as long as current clients stay connected) [it would have to
be a new command line option, as existing users are expecting SIGHUP and
SIGINT to behave identically, and the idea is somewhat at odds with our
other idea in TODO of having the v3 protocol have strongly-typed plugin
parameters with a way to send SIGHUP as a way to do a live reload of
parameters backed by a file]

> +=head1 SEE ALSO
> +
> +L<nbdkit(1)>,
> +L<nbdkit-plugin(3)>,
> +L<nbdkit-cow-filter(1)>,
> +L<nbdkit-data-plugin(1)>,
> +L<nbdkit-memory-plugin(1)/Preloading small amounts of data>.
Except that nbdkit-memory-plugin points only to nbdkit-data-plugin, so
is this link helping?
> +++ b/plugins/reflection/reflection.c
> +/* The mode. */
> +enum mode {
> +  MODE_EXPORTNAME,
> +  MODE_BASE64EXPORTNAME,
> +};
> +static enum mode mode = MODE_EXPORTNAME;
Explicit assignment to a 0 value, instead of relying on .bss; but I'm
okay with it (it's not as obvious as =0 or =false).
> +
> +static int
> +reflection_config (const char *key, const char *value)
> +{
> +  if (strcmp (key, "mode") == 0) {
> +    if (strcasecmp (value, "exportname") == 0) {
Do we want to be nice and allow 'export-name' as an [undocumented]
alternative spelling?
> +      mode = MODE_EXPORTNAME;
> +    }
> +    else if (strcasecmp (value, "base64exportname") == 0) {
similarly for 'base64-export-name'

> +#define reflection_config_help \
> +  "mode=MODE    Plugin mode."
> +
Worth listing the valid values of MODE, or the fact that this parameter
is optional because it defaults to exportname?

> +/* Create the per-connection handle.
> + *
> + * This is a rather unusual plugin because it has to parse data sent
> + * by the client.  For security reasons, be careful about:
> + *
> + * - Returning more data than is sent by the client.
> + *
> + * - Inputs that result in unbounded output.
> + *
> + * - Inputs that could hang, crash or exploit the server.
> + */
Nice reminder. I agree that we're okay for now, but as we add new modes
in the future, it will remind us to think about it.

> +/* Read-only plugin so multi-conn is safe. */
> +static int
> +reflection_can_multi_conn (void *handle)
> +{
> +  return 1;
Correct for now.  I also see your followup to patch 4 that changes this
once IP addresses are exposed.

> +++ b/tests/test-reflection-base64.sh
> +
> +requires nbdsh --version
> +# XXX This needs to test $PYTHON somehow.
> +#requires python -c 'import base64'
Not just any python, but the version of python hard-coded as what will
run nbdsh (which may be different than the $PYTHON that nbdkit was
compiled with).  I'm not sure what to suggest, other than just:

requires nbdsh -c 'import base64'

which could, in fact, be a single test (no need to test if nbdsh
--version works if nbdsh -c ... works).

> +
> +export e sock
> +for e in "" "test" "テスト" \
Worth also testing '-n' or '\n' (two strings that caused
problems with
the sh plugin implementation) (here, and in the plaintext version)?
> +
> +# Test that it fails if the caller passes in non-base64 data.  The
> +# server drops the connection in this case so it's not very graceful
> +# but we should at least get an nbd.Error and not something else.
> +nbdsh -c '
> +import os
> +import sys
> +
> +h.set_export_name ("xyz")
> +try:
> +    h.connect_unix (os.environ["sock"])
> +    # This should not happen.
> +    sys.exit (1)
> +except nbd.Error as ex:
> +    sys.exit (0)
> +# This should not happen.
> +sys.exit (1)
The test looks good.  (Yeah, figuring out how to make it more graceful
in the future might be nice, but that's not this patch's problem.
I'm
thinking that if a plugin's .open() fails, nbdkit could reply with
NBD_REP_ERR_UNKNOWN or NBD_REP_ERR_INVALID, and then wait for the client
to disconnect, rather than the current hard hangup initiated by the server)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> We are going to change <nbdkit-common.h> so it includes
> <sys/socket.h>.  However because this exports connect(2) we need to
> rename globals called ‘connect’ to avoid a conflict.
> ---
>  plugins/guestfs/guestfs-plugin.c | 8 ++++----
>  plugins/libvirt/libvirt-plugin.c | 6 +++---
>  2 files changed, 7 insertions(+), 7 deletions(-)
Mechanical.  ACK

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> Works essentially just like calling getpeername(2), because that's how
> it is implemented.
> ---
>  TODO                    |  6 ++++++
>  docs/nbdkit-plugin.pod  | 23 +++++++++++++++++++++++
>  include/nbdkit-common.h |  2 ++
>  server/nbdkit.syms      |  1 +
>  server/public.c         | 21 +++++++++++++++++++++
>  5 files changed, 53 insertions(+)
> 
> diff --git a/TODO b/TODO
> index 04def3c..d2cf0ae 100644
> --- a/TODO
> +++ b/TODO
> @@ -77,6 +77,12 @@ General ideas for improvements
>    name(s) that a plugin might want to support.  Probably we should
>    deprecate the -e option entirely since it does nothing useful.
>  
> +* Add plugin "connect" method.  This would be called on a
connection
> +  before handshaking or TLS negotiation, and could be used (with
> +  nbdkit_peer_name) to accept or reject connections based on IP
> +  address, rather like a poor man's TCP wrappers.  See also commit
> +  c05686f9577f.
Yes, you now have more justification for why a .connect would be a
useful callback (and we would document that the plugin is responsible
for NOT sticking a lot of code into .connect, so that it does not become
an amplification attack).
> +int
> +nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen)
> +{
> +  struct connection *conn = threadlocal_get_conn ();
> +  int s;
> +
> +  if (!conn) {
> +    nbdkit_error ("no connection in this thread");
> +    return -1;
> +  }
> +
> +  s = conn->sockin;
> +  if (s == -1) {
> +    nbdkit_error ("socket not open");
> +    return -1;
> +  }
> +
> +  return getpeername (s, addr, addrlen);
You need to call nbdkit_error() if getpeername() returns -1

Otherwise, looks reasonable.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/15/19 10:04 AM, Richard W.M. Jones wrote:
> On Sun, Sep 15, 2019 at 03:55:41PM +0100, Richard W.M. Jones wrote:
>>  - Plugins could change content based on client.  (The fourth patch in
>>    the series is a PoC of this implemented in the new reflection
>>    plugin.)  Be cautious about combining this feature with multi-conn
>>    as it's not obviously always safe to do.
> 
> Given this commit, I guess we should squash in the following to the
> 4th patch:
> 
> diff --git a/plugins/reflection/reflection.c
b/plugins/reflection/reflection.c
> index a0d7c60..f765557 100644
> --- a/plugins/reflection/reflection.c
> +++ b/plugins/reflection/reflection.c
> @@ -303,11 +303,22 @@ reflection_get_size (void *handle)
>    return (int64_t) h->len;
>  }
>  
> -/* Read-only plugin so multi-conn is safe. */
>  static int
>  reflection_can_multi_conn (void *handle)
>  {
> -  return 1;
> +  switch (mode) {
> +    /* Safe for exportname modes since clients should only request
> +     * multi-conn with the same export name.
> +     */
> +  case MODE_EXPORTNAME:
> +  case MODE_BASE64EXPORTNAME:
> +    return 1;
> +    /* Unsafe for mode=address because all multi-conn connections
> +     * won't necessarily originate from the same client address.
> +     */
> +  case MODE_ADDRESS:
> +    return 0;
Correct - any two simultaneous clients over TCP will necessarily have
different content even if they have requested the same export name, so
you do need this patch squashed in.  Unix sockets (currently) get the
same content, but it's not worth trying to distinguish TCP vs. Unix
sockets here.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> ---
Short commit message; at a minimum, I'd probably at least mention that
we thought about potential security issues, and didn't see how it could
be abused.
>  .../reflection/nbdkit-reflection-plugin.pod   | 23 ++++-
>  plugins/reflection/reflection.c               | 88 +++++++++++++++++++
>  tests/Makefile.am                             |  2 +
>  tests/test-reflection-address.sh              | 63 +++++++++++++
>  4 files changed, 174 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/reflection/nbdkit-reflection-plugin.pod
b/plugins/reflection/nbdkit-reflection-plugin.pod
> index 1b260b6..7f52c58 100644
> --- a/plugins/reflection/nbdkit-reflection-plugin.pod
> +++ b/plugins/reflection/nbdkit-reflection-plugin.pod
> @@ -1,10 +1,10 @@
>  =head1 NAME
>  
> -nbdkit-reflection-plugin - reflect export name back to the client
> +nbdkit-reflection-plugin - reflect client info back to the client
Could perhaps squash this hunk into patch 1, but it's also fine here.
> +Another use for the reflection plugin is to send back the client's IP
> +address:
> +
> + $ nbdkit reflection mode=address
> + $ nbdsh -u 'nbd://localhost' -c 'print(h.pread(h.get_size(),
0))'
> +
> +which will print something like:
> +
> + b'[::1]:58912'
Do we want a mode that attempts to do DNS lookup to convert an address
back to a name, so that this could result in b'localhost:58912'?

> +
> +  case AF_UNIX:
> +    /* We don't want to expose the socket path because it's a host
> +     * filesystem name.  The client might not really be running on the
> +     * same machine (eg. it using a proxy).  However it doesn't even
missing 'is'

> +++ b/tests/test-reflection-address.sh
> @@ -0,0 +1,63 @@
> +# Test the relection plugin with mode=address.
reflection (hmm, I missed that typo in patch 1, where this was copied from)
> +# Run nbdkit.
> +start_nbdkit -P reflection-address.pid -U $sock \
> +       reflection mode=address
Is it worth also trying to test a TCP socket (although we have to worry
about finding a free port for the server, as well as heavily filtering
the result as it might be [::1] or 127.0.0.1, and most certainly will
have a different client port number per test run.

But overall, I think this is a useful thing to add to the plugin, and
I'm not seeing any security holes in letting the client know the
client's IP address as seen by the server.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> The fun plugin is a reflection plugin which "reflects" client
> information back to the client.  We have a few fun plugins like this
> (hello there, nbdkit-full-plugin) and normally I would push these
> without review, but in this particular case there's a specific danger
> that sending back data under control of the client might lead to a
> security problem.  I _believe_ I have avoided that pitfall, but my
> belief isn't as good as having experts review it :-)
> 
A thought for another potentially fun mode: what if the reflection 
plugin reports back connection uptime?  The disk would be a fixed-width 
size (maybe 8 bytes: uint32_t for seconds, uint32_t for nanoseconds), 
where the contents are always changing to record the delta in time 
between any given request and when the connection was first created (in 
this mode, multi-conn and caching contents is actually quite wrong). 
Back-to-back commands can in turn be used to measure connection latencies.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Fri, Sep 27, 2019 at 10:22:10PM -0500, Eric Blake
wrote:
> On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> 
> >The fun plugin is a reflection plugin which "reflects" client
> >information back to the client.  We have a few fun plugins like this
> >(hello there, nbdkit-full-plugin) and normally I would push these
> >without review, but in this particular case there's a specific
danger
> >that sending back data under control of the client might lead to a
> >security problem.  I _believe_ I have avoided that pitfall, but my
> >belief isn't as good as having experts review it :-)
> >
> 
> A thought for another potentially fun mode: what if the reflection
> plugin reports back connection uptime?  The disk would be a
> fixed-width size (maybe 8 bytes: uint32_t for seconds, uint32_t for
> nanoseconds), where the contents are always changing to record the
> delta in time between any given request and when the connection was
> first created (in this mode, multi-conn and caching contents is
> actually quite wrong). Back-to-back commands can in turn be used to
> measure connection latencies.
Yes good idea - I sent a patch.  It needs a test which I can get
around to later.

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