9:16 p.m.
I'm about to add an 'exportname' filter, and in the process, I noticed
a few shortcomings in our API. Time to fix those before the 1.22
release locks our API in stone. The existing 'tls-fallback' filter
already mentioned that we have situations where we want to expose a
different list before and after TLS negotiation on a tls=1 setup, but
.list_exports is executed before .open has set nbdkit_is_tls(). And
since a previous patch split out the .default_export functionality, it
is easiest to just repurpose the existing parameter to a new name.
However, it does adjust the signature for filters calling into the
next layer, as filters don't change whether TLS has been attempted.
This also fixes the cc plugin to expose .list_exports.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
docs/nbdkit-filter.pod | 10 ++++++--
docs/nbdkit-plugin.pod | 17 ++++++-------
filters/log/nbdkit-log-filter.pod | 2 +-
plugins/sh/nbdkit-sh-plugin.pod | 8 ++-----
include/nbdkit-filter.h | 3 +--
include/nbdkit-plugin.h | 2 +-
server/internal.h | 5 ++--
server/backend.c | 9 ++++---
server/filters.c | 6 ++---
server/plugins.c | 4 ++--
server/protocol-handshake-newstyle.c | 2 +-
plugins/cc/cc.c | 9 +++++++
plugins/sh/methods.c | 5 ++--
filters/log/log.c | 8 +++----
filters/tls-fallback/tls-fallback.c | 12 ++++++++--
tests/test-tls-fallback.sh | 36 ++++++++++++++++++++++++----
tests/test-layers-filter.c | 4 ++--
TODO | 6 -----
18 files changed, 93 insertions(+), 55 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 7d72d138..79c39f0d 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -340,12 +340,18 @@ an error message and return C<-1>.
=head2 C<.list_exports>
int (*list_exports) (nbdkit_next_list_exports *next, void *nxdata,
- int readonly, int default_only,
+ int readonly, int is_tls,
struct nbdkit_exports *exports);
This intercepts the plugin C<.list_exports> method and can be used to
filter which exports are advertised.
+The C<readonly> parameter matches what is passed to <.preconnect> and
+C<.open>, and may be changed by the filter when calling into the
+plugin. The C<is_tls> parameter informs the filter whether TLS
+negotiation has been completed by the client, but is not passed on to
+C<next> because it cannot be altered.
+
It is possible for filters to transform the exports list received back
from the layer below. Without error checking it would look like this:
@@ -357,7 +363,7 @@ from the layer below. Without error checking it would look like
this:
char *name, *desc;
exports2 = nbdkit_exports_new ();
- next_list_exports (nxdata, readonly, default_only, exports);
+ next_list_exports (nxdata, readonly, exports);
for (i = 0; i < nbdkit_exports_count (exports2); ++i) {
e = nbdkit_get_export (exports2, i);
name = adjust (e.name);
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 056b1450..9f5d6356 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -670,7 +670,7 @@ error message and return C<-1>.
=head2 C<.list_exports>
- int list_exports (int readonly, int default_only,
+ int list_exports (int readonly, int is_tls,
struct nbdkit_exports *exports);
This optional callback is called if the client tries to list the
@@ -688,11 +688,11 @@ not yet have a way to let the client advertise an intent to be
read-only even when the server allows writes, so this parameter may
not be as useful as it appears.
-If the C<default_only> flag is set then the client is querying for the
-name of the default export, and the plugin may optimize by adding only
-a single export to the returned list (the default export name, usually
-C<"">). The plugin can ignore this flag and return all exports if it
-wants.
+The C<is_tls> flag informs the plugin whether this listing was
+requested after the client has completed TLS negotiation. When
+running the server in a mode that permits but does not require TLS, be
+careful that any exports listed when C<is_tls> is C<false> do not leak
+unintended information.
The C<exports> parameter is an opaque object for collecting the list
of exports. Call C<nbdkit_add_export> as needed to add specific
@@ -783,8 +783,9 @@ server requires TLS authentication, then that has occurred as well.
But if the server is set up to have optional TLS authentication, you
may check C<nbdkit_is_tls> to learn whether the client has completed
TLS authentication. When running the server in a mode that permits
-but not requires TLS, be careful that you do not allow unauthenticated
-clients to cause a denial of service against authentication.
+but does not require TLS, be careful that you do not allow
+unauthenticated clients to cause a denial of service against
+authentication.
If there is an error, C<.open> should call C<nbdkit_error> with an
error message and return C<NULL>.
diff --git a/filters/log/nbdkit-log-filter.pod b/filters/log/nbdkit-log-filter.pod
index 721fc118..40bce76d 100644
--- a/filters/log/nbdkit-log-filter.pod
+++ b/filters/log/nbdkit-log-filter.pod
@@ -72,7 +72,7 @@ on the connection).
An example logging session of a client that requests an export list
before performing a single successful read is:
- 2020-08-06 02:07:23.080415 ListExports id=1 readonly=0 default_only=0 ...
+ 2020-08-06 02:07:23.080415 ListExports id=1 readonly=0 tls=0 ...
2020-08-06 02:07:23.080502 ...ListExports id=1 exports=[""] return=0
2020-08-06 02:07:23.080712 connection=1 Connect export='' tls=0 size=0x400
write=1 flush=1 rotational=0 trim=1 zero=2 fua=2 extents=1 cache=2 fast_zero=1
2020-08-06 02:07:23.080907 connection=1 Read id=1 offset=0x0 count=0x200 ...
diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index b827b658..c6e9c432 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -268,13 +268,9 @@ with status C<1>; unrecognized output is ignored.
=item C<list_exports>
- /path/to/script list_exports <readonly> <default_only>
+ /path/to/script list_exports <readonly> <tls>
-The C<readonly> parameter will be C<true> or C<false>. The
-C<default_only> parameter will be C<true> if the caller is only
-interested in the canonical name of the default export, or C<false> to
-get a full list of export names; the script may safely ignore this
-parameter and always provide a full list if desired.
+The C<readonly> and C<tls> parameters will be C<true> or
C<false>.
The first line of output informs nbdkit how to parse the rest of the
output, the remaining lines then supply the inputs of the C
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 2c5b36be..63458d59 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -66,7 +66,6 @@ typedef int nbdkit_next_get_ready (nbdkit_backend *nxdata);
typedef int nbdkit_next_after_fork (nbdkit_backend *nxdata);
typedef int nbdkit_next_preconnect (nbdkit_backend *nxdata, int readonly);
typedef int nbdkit_next_list_exports (nbdkit_backend *nxdata, int readonly,
- int ignored,
struct nbdkit_exports *exports);
typedef const char *nbdkit_next_default_export (nbdkit_backend *nxdata,
int readonly);
@@ -179,7 +178,7 @@ struct nbdkit_filter {
int (*preconnect) (nbdkit_next_preconnect *next, nbdkit_backend *nxdata,
int readonly);
int (*list_exports) (nbdkit_next_list_exports *next, nbdkit_backend *nxdata,
- int readonly, int default_only,
+ int readonly, int is_tls,
struct nbdkit_exports *exports);
const char * (*default_export) (nbdkit_next_default_export *next,
nbdkit_backend *nxdata,
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 28e83757..539e3f6e 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -140,7 +140,7 @@ struct nbdkit_plugin {
int (*get_ready) (void);
int (*after_fork) (void);
- int (*list_exports) (int readonly, int default_only,
+ int (*list_exports) (int readonly, int is_tls,
struct nbdkit_exports *exports);
const char * (*default_export) (int readonly, int is_tls);
};
diff --git a/server/internal.h b/server/internal.h
index f11f897e..fe485117 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -370,7 +370,7 @@ struct backend {
void (*get_ready) (struct backend *);
void (*after_fork) (struct backend *);
int (*preconnect) (struct backend *, int readonly);
- int (*list_exports) (struct backend *, int readonly, int ignored,
+ int (*list_exports) (struct backend *, int readonly, int is_tls,
struct nbdkit_exports *exports);
const char *(*default_export) (struct backend *, int readonly, int is_tls);
void *(*open) (struct backend *, int readonly, const char *exportname,
@@ -419,9 +419,8 @@ extern void backend_unload (struct backend *b, void (*unload) (void))
__attribute__((__nonnull__ (1)));
extern int backend_list_exports (struct backend *b, int readonly,
- int ignored,
struct nbdkit_exports *exports)
- __attribute__((__nonnull__ (1, 4)));
+ __attribute__((__nonnull__ (1, 3)));
extern const char *backend_default_export (struct backend *b, int readonly)
__attribute__((__nonnull__ (1)));
/* exportname is only valid for this call and almost certainly will be
diff --git a/server/backend.c b/server/backend.c
index 3e0a1d24..2023c40b 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -159,21 +159,20 @@ backend_unload (struct backend *b, void (*unload) (void))
}
int
-backend_list_exports (struct backend *b, int readonly, int default_only,
+backend_list_exports (struct backend *b, int readonly,
struct nbdkit_exports *exports)
{
GET_CONN;
struct handle *h = get_handle (conn, b->i);
size_t count;
- assert (!default_only); /* XXX Switch to is_tls... */
- controlpath_debug ("%s: list_exports readonly=%d default_only=%d",
- b->name, readonly, default_only);
+ controlpath_debug ("%s: list_exports readonly=%d tls=%d",
+ b->name, readonly, conn->using_tls);
assert (h->handle == NULL);
assert ((h->state & HANDLE_OPEN) == 0);
- if (b->list_exports (b, readonly, default_only, exports) == -1 ||
+ if (b->list_exports (b, readonly, conn->using_tls, exports) == -1 ||
exports_resolve_default (exports, b, readonly) == -1) {
controlpath_debug ("%s: list_exports failed", b->name);
return -1;
diff --git a/server/filters.c b/server/filters.c
index bb22be76..dd4417be 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -238,15 +238,15 @@ plugin_magic_config_key (struct backend *b)
}
static int
-filter_list_exports (struct backend *b, int readonly, int default_only,
+filter_list_exports (struct backend *b, int readonly, int is_tls,
struct nbdkit_exports *exports)
{
struct backend_filter *f = container_of (b, struct backend_filter, backend);
if (f->filter.list_exports)
return f->filter.list_exports (backend_list_exports, b->next,
- readonly, default_only, exports);
- return backend_list_exports (b->next, readonly, default_only, exports);
+ readonly, is_tls, exports);
+ return backend_list_exports (b->next, readonly, exports);
}
static const char *
diff --git a/server/plugins.c b/server/plugins.c
index 7425857e..8061b2e8 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -283,7 +283,7 @@ plugin_preconnect (struct backend *b, int readonly)
}
static int
-plugin_list_exports (struct backend *b, int readonly, int default_only,
+plugin_list_exports (struct backend *b, int readonly, int is_tls,
struct nbdkit_exports *exports)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
@@ -291,7 +291,7 @@ plugin_list_exports (struct backend *b, int readonly, int
default_only,
if (!p->plugin.list_exports)
return nbdkit_add_default_export (exports);
- return p->plugin.list_exports (readonly, default_only, exports);
+ return p->plugin.list_exports (readonly, is_tls, exports);
}
static const char *
diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c
index 1777219f..7a034360 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -88,7 +88,7 @@ send_newstyle_option_reply_exportnames (uint32_t option)
exps = nbdkit_exports_new ();
if (exps == NULL)
return send_newstyle_option_reply (option, NBD_REP_ERR_TOO_BIG);
- if (backend_list_exports (top, read_only, false, exps) == -1)
+ if (backend_list_exports (top, read_only, exps) == -1)
return send_newstyle_option_reply (option, NBD_REP_ERR_PLATFORM);
for (i = 0; i < nbdkit_exports_count (exps); i++) {
diff --git a/plugins/cc/cc.c b/plugins/cc/cc.c
index 1d688056..381d1f7e 100644
--- a/plugins/cc/cc.c
+++ b/plugins/cc/cc.c
@@ -348,6 +348,14 @@ cc_preconnect (int readonly)
return 0;
}
+static int
+cc_list_exports (int readonly, int is_tls, struct nbdkit_exports *exports)
+{
+ if (subplugin.list_exports)
+ return subplugin.list_exports (readonly, is_tls, exports);
+ return nbdkit_add_default_export (exports);
+}
+
static const char *
cc_default_export (int readonly, int is_tls)
{
@@ -571,6 +579,7 @@ static struct nbdkit_plugin plugin = {
.after_fork = cc_after_fork,
.preconnect = cc_preconnect,
+ .list_exports = cc_list_exports,
.default_export = cc_default_export,
.open = cc_open,
.close = cc_close,
diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
index 55d6d535..a0446fb2 100644
--- a/plugins/sh/methods.c
+++ b/plugins/sh/methods.c
@@ -303,13 +303,12 @@ parse_exports (const char *script,
}
int
-sh_list_exports (int readonly, int default_only,
- struct nbdkit_exports *exports)
+sh_list_exports (int readonly, int is_tls, struct nbdkit_exports *exports)
{
const char *method = "list_exports";
const char *script = get_script (method);
const char *args[] = { script, method, readonly ? "true" :
"false",
- default_only ? "true" : "false", NULL };
+ is_tls ? "true" : "false", NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
diff --git a/filters/log/log.c b/filters/log/log.c
index 4525f362..b6b967a3 100644
--- a/filters/log/log.c
+++ b/filters/log/log.c
@@ -247,16 +247,16 @@ output_return (struct handle *h, const char *act, uint64_t id, int
r, int *err)
/* List exports. */
static int
log_list_exports (nbdkit_next_list_exports *next, void *nxdata,
- int readonly, int default_only,
+ int readonly, int is_tls,
struct nbdkit_exports *exports)
{
static uint64_t id;
int r;
int err;
- output (NULL, "ListExports", ++id, "readonly=%d default_only=%d
...",
- readonly, default_only);
- r = next (nxdata, readonly, default_only, exports);
+ output (NULL, "ListExports", ++id, "readonly=%d tls=%d ...",
+ readonly, is_tls);
+ r = next (nxdata, readonly, exports);
if (r == -1) {
err = errno;
output_return (NULL, "...ListExports", id, r, &err);
diff --git a/filters/tls-fallback/tls-fallback.c b/filters/tls-fallback/tls-fallback.c
index 0fcc2bcf..c1e549df 100644
--- a/filters/tls-fallback/tls-fallback.c
+++ b/filters/tls-fallback/tls-fallback.c
@@ -74,7 +74,15 @@ tls_fallback_get_ready (nbdkit_next_get_ready *next, void *nxdata,
return next (nxdata);
}
-/* TODO: list_exports needs is_tls parameter */
+static int
+tls_fallback_list_exports (nbdkit_next_list_exports *next, void *nxdata,
+ int readonly, int is_tls,
+ struct nbdkit_exports *exports)
+{
+ if (!is_tls)
+ return nbdkit_add_export (exports, "", NULL);
+ return next (nxdata, readonly, exports);
+}
static const char *
tls_fallback_default_export (nbdkit_next_default_export *next, void *nxdata,
@@ -192,7 +200,7 @@ static struct nbdkit_filter filter = {
.config = tls_fallback_config,
.config_help = tls_fallback_config_help,
.get_ready = tls_fallback_get_ready,
- /* XXX .list_exports needs is_tls parameter */
+ .list_exports = tls_fallback_list_exports,
.default_export = tls_fallback_default_export,
.open = tls_fallback_open,
.get_size = tls_fallback_get_size,
diff --git a/tests/test-tls-fallback.sh b/tests/test-tls-fallback.sh
index 94449f31..fb232bf9 100755
--- a/tests/test-tls-fallback.sh
+++ b/tests/test-tls-fallback.sh
@@ -35,7 +35,7 @@ set -e
set -x
requires_plugin sh
-requires nbdsh -c 'exit (not h.supports_tls ())'
+requires nbdsh -c 'print (h.set_full_info)' -c 'exit (not h.supports_tls
())'
# Does the nbdkit binary support TLS?
if ! nbdkit --dump-config | grep -sq tls=yes; then
@@ -66,9 +66,12 @@ check () {
fi
}
case $1 in
- list_exports) echo NAMES; echo hello; echo world ;;
+ list_exports) check "$3"; echo INTERLEAVED
+ echo hello; echo world
+ echo world; echo tour ;;
default_export) check "$3"; echo hello ;;
open) check "$4"; echo $3 ;;
+ export_description) echo "=$2=" ;;
get_size) echo "$2" | wc -c ;;
pread) echo "$2" | dd skip=$4 count=$3 iflag=skip_bytes,count_bytes ;;
can_write | can_trim) exit 0 ;;
@@ -79,8 +82,19 @@ EOF
# Plaintext client sees only dummy volume
nbdsh -c '
import os
-h.set_export_name ("hello")
+
+def f (name, desc):
+ assert name == ""
+ assert desc == ""
+
+h.set_opt_mode (True)
+h.set_full_info (True)
h.connect_unix (os.environ["sock"])
+assert h.opt_list (f) == 1
+h.opt_info ()
+assert h.get_canonical_export_name() == ""
+h.set_export_name ("hello")
+h.opt_go ()
assert h.get_size () == 512
assert not h.can_trim()
assert h.pread (5, 0) == b"dummy"
@@ -89,11 +103,25 @@ assert h.pread (5, 0) == b"dummy"
# Encrypted client sees desired volumes
nbdsh -c '
import os
-h.set_export_name ("hello")
+
+def f (name, desc):
+ if name == "hello":
+ assert desc == "world"
+ elif name == "world":
+ assert desc == "tour"
+ else:
+ assert False
+
+h.set_opt_mode (True)
+h.set_full_info (True)
h.set_tls (nbd.TLS_REQUIRE)
h.set_tls_psk_file ("keys.psk")
h.set_tls_username ("qemu")
h.connect_unix (os.environ["sock"])
+assert h.opt_list (f) == 2
+h.opt_info ()
+assert h.get_canonical_export_name() == "hello"
+h.opt_go ()
assert h.get_size () == 6
assert h.can_trim ()
assert h.pread (5, 0) == b"hello"
diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c
index f6acb448..b3575963 100644
--- a/tests/test-layers-filter.c
+++ b/tests/test-layers-filter.c
@@ -108,11 +108,11 @@ test_layers_filter_preconnect (nbdkit_next_preconnect *next,
static int
test_layers_filter_list_exports (nbdkit_next_list_exports *next, void *nxdata,
- int readonly, int default_only,
+ int readonly, int is_tls,
struct nbdkit_exports *exports)
{
DEBUG_FUNCTION;
- return next (nxdata, readonly, default_only, exports);
+ return next (nxdata, readonly, exports);
}
static const char *
diff --git a/TODO b/TODO
index 9c32a94b..e4dd072f 100644
--- a/TODO
+++ b/TODO
@@ -71,12 +71,6 @@ General ideas for improvements
continue to keep their non-standard handshake while utilizing nbdkit
to prototype new behaviors in serving the kernel.
-* The current signature of .list_exports is awkward; it overloads the
- list response to NBD_OPT_LIST with the resolution of the default
- export name in .open, and it is missing a tls parameter. It is
- probably worth splitting out a new .default_export callback for the
- latter purpose, as well as fixing the signature for the former.
-
* At present, we ignore NBD_INFO_DESCRIPTION requests from a client
during NBD_OPT_GO. This may be easiest if we add a new
.get_description callback, called after .open alongside .get_size
--
2.28.0