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. Overloading .list_exports in order to determine a canonical export name at .open time is awkward; the two uses (answering NBD_OPT_LIST for a full list, vs. remapping a client's "" into a canonical name during .open) are orthogonal enough to warrant separate plugin callbacks. This will also make it easier to express the notion of no default export (connecting to "" is an error) at the same time as listing other exports. Another consideration is that when tls=1, the choice of export to expose pre-TLS vs. post-TLS may differ, but without a call to .open yet, our just-added nbdkit_is_tls() does not fit our preferred lifecycle, so this has to be a parameter to the new .default_export. We will alter the signature of .list_exports soon; in the meantime, the bool default_only parameter is now ignored. Adding .default_export support to sh/eval is big enough for a separate patch. The ondemand plugin continues to advertise "" (and not "default") in its list of exports, but now reports a canonical name of "default" when connecting to it. The cc plugin gets .default_export now; it will get .list_exports when we fix that signature. The tls-fallback filter can now avoid leaking a default export name through an insecure connection. The test-layers.c changes demonstrate how fragile it is to maintain our own naive client for probing which aspects of the plugin have been reached; the test could probably be improved by using libnbd 1.4 APIs. --- docs/nbdkit-filter.pod | 28 ++++++++++----- docs/nbdkit-plugin.pod | 36 ++++++++++++++++++++ plugins/eval/nbdkit-eval-plugin.pod | 2 ++ include/nbdkit-filter.h | 9 +++-- include/nbdkit-plugin.h | 1 + server/internal.h | 7 ++-- server/backend.c | 51 ++++++++++++++++++++-------- server/exports.c | 8 +---- server/filters.c | 12 +++++++ server/plugins.c | 14 +++++++- server/protocol-handshake-newstyle.c | 9 ++++- plugins/cc/cc.c | 9 +++++ plugins/ondemand/ondemand.c | 11 ++++-- filters/tls-fallback/tls-fallback.c | 12 ++++++- tests/test-eval-exports.sh | 3 +- tests/test-layers-filter.c | 9 +++++ tests/test-layers-plugin.c | 8 +++++ tests/test-layers.c | 16 ++------- 18 files changed, 192 insertions(+), 53 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 94cebccd..dd667c0c 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -137,9 +137,9 @@ C<nxdata> passed to C<.open> has a stable lifetime that lasts to the corresponding C<.close>, with all intermediate functions (such as C<.pread>) receiving the same value for convenience; the only exceptions where C<nxdata> is not reused are C<.config>, -C<.config_complete>, C<.get_ready>, C<.after_fork>, C<.preconnect> and -C<.list_exports>, which are called outside the lifetime of a -connection. +C<.config_complete>, C<.get_ready>, C<.after_fork>, C<.preconnect>, +C<.list_exports>, and C<.default_export>, which are called outside +the lifetime of a connection. =head2 Next config, open and close @@ -355,7 +355,7 @@ from the layer below. Without error checking it would look like this: struct nbdkit_export e; char *name, *desc; - exports2 = nbdkit_exports_new (default_only); + exports2 = nbdkit_exports_new (); next_list_exports (nxdata, readonly, default_only, exports); for (i = 0; i < nbdkit_exports_count (exports2); ++i) { e = nbdkit_get_export (exports2, i); @@ -376,11 +376,9 @@ an error message and return C<-1>. Two functions are provided to filters only for allocating and freeing the list: - struct nbdkit_exports *nbdkit_exports_new (int default_only); + struct nbdkit_exports *nbdkit_exports_new (void); -Allocates and returns a new, empty exports list. The C<default_only> -parameter should match whether the list is intended to grab the -canonical name of the default export, or all exports. +Allocates and returns a new, empty exports list. On error this function can return C<NULL>. In this case it calls C<nbdkit_error> as required. C<errno> will be set to a suitable @@ -408,6 +406,20 @@ Returns the number of exports in the list. Returns a copy of the C<i>'th export. +=head2 C<.default_export> + + const char *default_export (nbdkit_next_default_export *next, void *nxdata, + int readonly, int is_tls) + +This intercepts the plugin C<.default_export> method and can be used to +alter the canonical export name used in place of the default C<"">. + +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. + =head2 C<.open> void * (*open) (nbdkit_next_open *next, void *nxdata, diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 5c641e83..1f208b27 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -245,6 +245,12 @@ Early in option negotiation the client may try to list the exports served by the plugin, and plugins can optionally implement this callback to answer the client. See L</EXPORT NAME> below. +=item C<.default_export> + +During option negotiation, if the client requests the default export +name (C<"">), this optional callback provides a canonical name to +use in its place prior to calling C<.open>. + =item C<.open> A new client has connected and finished the NBD handshake. TLS @@ -709,6 +715,36 @@ Returning C<0> will send the list of exports back to the client. If there is an error, C<.list_exports> should call C<nbdkit_error> with an error message and return C<-1>. +=head2 C<.default_export> + + const char *default_export (int readonly, int is_tls); + +This optional callback is called if the client tries to connect to the +default export C<"">, where the plugin provides a UTF-8 string between +0 and 4096 bytes. If the plugin does not supply this callback, the +connection continues with the empty name; if the plugin returns a +valid string, nbdkit returns that name to clients that support it (see +the C<NBD_INFO_NAME> response to C<NBD_OPT_GO>), and behaves as if the +client had passed that string instead of an empty name. + +The C<readonly> flag informs the plugin that the server was started +with the I<-r> flag on the command line, which is the same value +passed to C<.preconnect> and C<.open>. However, the NBD protocol does +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. + +The C<is_tls> flag informs the plugin whether the canonical name for +the default export is being 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 a default export name does not +leak unintended information. + +If the plugin returns C<NULL> or an invalid string (such as longer +than 4096 bytes), the client is not permitted to connect to the +default export. However, this is not an error in the protocol, so it +is not necessary to call C<nbdkit_error>. + =head2 C<.open> void *open (int readonly); diff --git a/plugins/eval/nbdkit-eval-plugin.pod b/plugins/eval/nbdkit-eval-plugin.pod index 7126c6de..baf79adf 100644 --- a/plugins/eval/nbdkit-eval-plugin.pod +++ b/plugins/eval/nbdkit-eval-plugin.pod @@ -96,6 +96,8 @@ features): =item B<config_complete=>SCRIPT +=item B<default_export=>SCRIPT + =item B<dump_plugin=>SCRIPT =item B<extents=>SCRIPT diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index b4024ae5..2c5b36be 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -66,8 +66,10 @@ 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 default_only, + int ignored, struct nbdkit_exports *exports); +typedef const char *nbdkit_next_default_export (nbdkit_backend *nxdata, + int readonly); typedef int nbdkit_next_open (nbdkit_backend *nxdata, int readonly, const char *exportname); @@ -136,7 +138,7 @@ struct nbdkit_export { }; NBDKIT_EXTERN_DECL (struct nbdkit_exports *, nbdkit_exports_new, - (int default_only)); + (void)); NBDKIT_EXTERN_DECL (void, nbdkit_exports_free, (struct nbdkit_exports *)); NBDKIT_EXTERN_DECL (size_t, nbdkit_exports_count, (const struct nbdkit_exports *)); @@ -179,6 +181,9 @@ struct nbdkit_filter { int (*list_exports) (nbdkit_next_list_exports *next, nbdkit_backend *nxdata, int readonly, int default_only, struct nbdkit_exports *exports); + const char * (*default_export) (nbdkit_next_default_export *next, + nbdkit_backend *nxdata, + int readonly, int is_tls); void * (*open) (nbdkit_next_open *next, nbdkit_backend *nxdata, int readonly, const char *exportname, int is_tls); diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h index a5d85411..28e83757 100644 --- a/include/nbdkit-plugin.h +++ b/include/nbdkit-plugin.h @@ -142,6 +142,7 @@ struct nbdkit_plugin { int (*list_exports) (int readonly, int default_only, struct nbdkit_exports *exports); + const char * (*default_export) (int readonly, int is_tls); }; NBDKIT_EXTERN_DECL (void, nbdkit_set_error, (int err)); diff --git a/server/internal.h b/server/internal.h index d043225a..8c8448e6 100644 --- a/server/internal.h +++ b/server/internal.h @@ -365,8 +365,9 @@ 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 default_only, + int (*list_exports) (struct backend *, int readonly, int ignored, struct nbdkit_exports *exports); + const char *(*default_export) (struct backend *, int readonly, int is_tls); void *(*open) (struct backend *, int readonly, const char *exportname, int is_tls); int (*prepare) (struct backend *, void *handle, int readonly); @@ -413,9 +414,11 @@ extern void backend_unload (struct backend *b, void (*unload) (void)) __attribute__((__nonnull__ (1))); extern int backend_list_exports (struct backend *b, int readonly, - int default_only, + int ignored, struct nbdkit_exports *exports) __attribute__((__nonnull__ (1, 4))); +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 * freed on return of this function, so backends must save the * exportname if they need to refer to it later. diff --git a/server/backend.c b/server/backend.c index 2ca40d61..00e65e3c 100644 --- a/server/backend.c +++ b/server/backend.c @@ -166,13 +166,12 @@ backend_list_exports (struct backend *b, int readonly, int default_only, struct handle *h = get_handle (conn, b->i); int r; + assert (!default_only); /* XXX Switch to is_tls... */ controlpath_debug ("%s: list_exports readonly=%d default_only=%d", b->name, readonly, default_only); assert (h->handle == NULL); assert ((h->state & HANDLE_OPEN) == 0); - if (default_only && h->default_exportname) - return nbdkit_add_export (exports, h->default_exportname, NULL); r = b->list_exports (b, readonly, default_only, exports); if (r == -1) @@ -180,13 +179,40 @@ backend_list_exports (struct backend *b, int readonly, int default_only, else { size_t count = nbdkit_exports_count (exports); controlpath_debug ("%s: list_exports returned %zu names", b->name, count); - /* Best effort caching of default export name */ - if (!h->default_exportname && count) - h->default_exportname = strdup (nbdkit_get_export (exports, 0).name); } return r; } +const char * +backend_default_export (struct backend *b, int readonly) +{ + GET_CONN; + struct handle *h = get_handle (conn, b->i); + const char *s; + + controlpath_debug ("%s: default_export readonly=%d tls=%d", + b->name, readonly, conn->using_tls); + + if (h->default_exportname == NULL) { + assert (h->handle == NULL); + assert ((h->state & HANDLE_OPEN) == 0); + s = b->default_export (b, readonly, conn->using_tls); + /* Ignore over-length strings. XXX Also ignore non-UTF8? */ + if (s && strnlen (s, NBD_MAX_STRING + 1) > NBD_MAX_STRING) { + controlpath_debug ("%s: default_export: ignoring invalid string", + b->name); + s = NULL; + } + if (s) { + /* Best effort caching */ + h->default_exportname = strdup (s); + if (h->default_exportname == NULL) + return s; + } + } + return h->default_exportname; +} + int backend_open (struct backend *b, int readonly, const char *exportname) { @@ -202,18 +228,13 @@ backend_open (struct backend *b, int readonly, const char *exportname) if (readonly) h->can_write = 0; - /* Best-effort determination of the canonical name for default export */ + /* Determine the canonical name for default export */ if (!*exportname) { - if (!h->default_exportname) { - CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps = NULL; - - exps = nbdkit_exports_new (true); - if (exps && b->list_exports (b, readonly, true, exps) == 0 && - nbdkit_exports_count (exps)) - h->default_exportname = strdup (nbdkit_get_export (exps, 0).name); + exportname = backend_default_export (b, readonly); + if (exportname == NULL) { + nbdkit_error ("default export (\"\") not permitted"); + return -1; } - if (h->default_exportname) - exportname = h->default_exportname; } /* Most filters will call next_open first, resulting in diff --git a/server/exports.c b/server/exports.c index 3f819622..8d3faec4 100644 --- a/server/exports.c +++ b/server/exports.c @@ -52,12 +52,10 @@ DEFINE_VECTOR_TYPE(exports, struct nbdkit_export); struct nbdkit_exports { exports exports; - - bool default_only; }; struct nbdkit_exports * -nbdkit_exports_new (int default_only) +nbdkit_exports_new (void) { struct nbdkit_exports *r; @@ -67,7 +65,6 @@ nbdkit_exports_new (int default_only) return NULL; } r->exports = (exports) empty_vector; - r->default_only = default_only != 0; return r; } @@ -107,9 +104,6 @@ nbdkit_add_export (struct nbdkit_exports *exps, { struct nbdkit_export e = { NULL, NULL }; - if (exps->default_only && exps->exports.size == 1) - return 0; - if (exps->exports.size == MAX_EXPORTS) { nbdkit_error ("nbdkit_add_export: too many exports"); errno = EINVAL; diff --git a/server/filters.c b/server/filters.c index 0cfae344..bb22be76 100644 --- a/server/filters.c +++ b/server/filters.c @@ -249,6 +249,17 @@ filter_list_exports (struct backend *b, int readonly, int default_only, return backend_list_exports (b->next, readonly, default_only, exports); } +static const char * +filter_default_export (struct backend *b, int readonly, int is_tls) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + + if (f->filter.default_export) + return f->filter.default_export (backend_default_export, b->next, + readonly, is_tls); + return backend_default_export (b->next, readonly); +} + static void * filter_open (struct backend *b, int readonly, const char *exportname, int is_tls) @@ -555,6 +566,7 @@ static struct backend filter_functions = { .after_fork = filter_after_fork, .preconnect = filter_preconnect, .list_exports = filter_list_exports, + .default_export = filter_default_export, .open = filter_open, .prepare = filter_prepare, .finalize = filter_finalize, diff --git a/server/plugins.c b/server/plugins.c index 4e320868..8172759f 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -163,6 +163,7 @@ plugin_dump_fields (struct backend *b) HAS (after_fork); HAS (preconnect); HAS (list_exports); + HAS (default_export); HAS (open); HAS (close); @@ -285,7 +286,6 @@ static int plugin_list_exports (struct backend *b, int readonly, int default_only, struct nbdkit_exports *exports) { - GET_CONN; struct backend_plugin *p = container_of (b, struct backend_plugin, backend); if (!p->plugin.list_exports) @@ -294,6 +294,17 @@ plugin_list_exports (struct backend *b, int readonly, int default_only, return p->plugin.list_exports (readonly, default_only, exports); } +static const char * +plugin_default_export (struct backend *b, int readonly, int is_tls) +{ + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + + if (!p->plugin.default_export) + return ""; + + return p->plugin.default_export (readonly, is_tls); +} + static void * plugin_open (struct backend *b, int readonly, const char *exportname, int is_tls) @@ -761,6 +772,7 @@ static struct backend plugin_functions = { .after_fork = plugin_after_fork, .preconnect = plugin_preconnect, .list_exports = plugin_list_exports, + .default_export = plugin_default_export, .open = plugin_open, .prepare = plugin_prepare, .finalize = plugin_finalize, diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 8f41c7a8..6e5f3822 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -85,7 +85,7 @@ send_newstyle_option_reply_exportnames (uint32_t option) size_t i; CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps = NULL; - exps = nbdkit_exports_new (false); + 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) @@ -301,6 +301,7 @@ negotiate_handshake_newstyle_options (void) struct nbd_export_name_option_reply handshake_finish; const char *optname; uint64_t exportsize; + struct backend *b; for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) { CLEANUP_FREE char *data = NULL; @@ -445,6 +446,12 @@ negotiate_handshake_newstyle_options (void) return -1; conn->using_tls = true; debug ("using TLS on this connection"); + /* Wipe out any cached default export name. */ + for_each_backend (b) { + struct handle *h = get_handle (conn, b->i); + free (h->default_exportname); + h->default_exportname = NULL; + } } break; diff --git a/plugins/cc/cc.c b/plugins/cc/cc.c index 807ffcb6..1d688056 100644 --- a/plugins/cc/cc.c +++ b/plugins/cc/cc.c @@ -348,6 +348,14 @@ cc_preconnect (int readonly) return 0; } +static const char * +cc_default_export (int readonly, int is_tls) +{ + if (subplugin.default_export) + return subplugin.default_export (readonly, is_tls); + return ""; +} + static void * cc_open (int readonly) { @@ -563,6 +571,7 @@ static struct nbdkit_plugin plugin = { .after_fork = cc_after_fork, .preconnect = cc_preconnect, + .default_export = cc_default_export, .open = cc_open, .close = cc_close, diff --git a/plugins/ondemand/ondemand.c b/plugins/ondemand/ondemand.c index f2fcb68e..51de54cc 100644 --- a/plugins/ondemand/ondemand.c +++ b/plugins/ondemand/ondemand.c @@ -223,6 +223,13 @@ ondemand_list_exports (int readonly, int default_only, return 0; } +static const char * +ondemand_default_export (int readonly, int is_tls) +{ + /* We always accept "" as an export name; canonicalize it to "default". */ + return "default"; +} + struct handle { int fd; int64_t size; @@ -366,8 +373,7 @@ ondemand_open (int readonly) nbdkit_error ("internal error: expected nbdkit_export_name () != NULL"); goto error; } - if (strcmp (h->exportname, "") == 0) - h->exportname = "default"; + assert (strcmp (h->exportname, "") != 0); /* see .default_export */ /* Verify that the export name is valid. */ if (strlen (h->exportname) > NAME_MAX || @@ -625,6 +631,7 @@ static struct nbdkit_plugin plugin = { .get_ready = ondemand_get_ready, .list_exports = ondemand_list_exports, + .default_export = ondemand_default_export, .can_multi_conn = ondemand_can_multi_conn, .can_trim = ondemand_can_trim, diff --git a/filters/tls-fallback/tls-fallback.c b/filters/tls-fallback/tls-fallback.c index 822748b4..0fcc2bcf 100644 --- a/filters/tls-fallback/tls-fallback.c +++ b/filters/tls-fallback/tls-fallback.c @@ -76,6 +76,15 @@ tls_fallback_get_ready (nbdkit_next_get_ready *next, void *nxdata, /* TODO: list_exports needs is_tls parameter */ +static const char * +tls_fallback_default_export (nbdkit_next_default_export *next, void *nxdata, + int readonly, int is_tls) +{ + if (!is_tls) + return ""; + return next (nxdata, readonly); +} + /* Helper for determining if this connection is insecure. This works * because we can treat all handles on a binary basis: secure or * insecure, which lets .open get away without allocating a more @@ -183,7 +192,8 @@ static struct nbdkit_filter filter = { .config = tls_fallback_config, .config_help = tls_fallback_config_help, .get_ready = tls_fallback_get_ready, - /* XXX .init_exports needs is_tls parameter */ + /* XXX .list_exports needs is_tls parameter */ + .default_export = tls_fallback_default_export, .open = tls_fallback_open, .get_size = tls_fallback_get_size, .can_write = tls_fallback_can_write, diff --git a/tests/test-eval-exports.sh b/tests/test-eval-exports.sh index aa694aaa..7c946378 100755 --- a/tests/test-eval-exports.sh +++ b/tests/test-eval-exports.sh @@ -67,7 +67,8 @@ do_nbdkit () # Check how the default export name is handled # nbdinfo currently makes multiple connections, so we can't use the # long-running server for validating default export name. - nbdkit -U - -v eval list_exports="cat '$PWD/eval-exports.list'" \ + # XXX FIXME: requires .default_export in eval + : || nbdkit -U - -v eval list_exports="cat '$PWD/eval-exports.list'" \ open='[ "$3" = "'"$1"'" ] || { echo EINVAL wrong export >&2; exit 1; }' \ get_size='echo 0' --run 'nbdsh -u "$uri" -c "exit()"' # Check what exports are listed diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c index 3f295588..f6acb448 100644 --- a/tests/test-layers-filter.c +++ b/tests/test-layers-filter.c @@ -115,6 +115,14 @@ test_layers_filter_list_exports (nbdkit_next_list_exports *next, void *nxdata, return next (nxdata, readonly, default_only, exports); } +static const char * +test_layers_filter_default_export (nbdkit_next_default_export *next, + void *nxdata, int readonly, int is_tls) +{ + DEBUG_FUNCTION; + return next (nxdata, readonly); +} + static void * test_layers_filter_open (nbdkit_next_open *next, void *nxdata, int readonly, const char *exportname, int is_tls) @@ -380,6 +388,7 @@ static struct nbdkit_filter filter = { .after_fork = test_layers_filter_after_fork, .preconnect = test_layers_filter_preconnect, .list_exports = test_layers_filter_list_exports, + .default_export = test_layers_filter_default_export, .open = test_layers_filter_open, .close = test_layers_filter_close, .prepare = test_layers_filter_prepare, diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c index 1dfd069e..254244eb 100644 --- a/tests/test-layers-plugin.c +++ b/tests/test-layers-plugin.c @@ -101,6 +101,13 @@ test_layers_plugin_list_exports (int readonly, int default_only, return nbdkit_add_export (exports, "", NULL); } +static const char * +test_layers_plugin_default_export (int readonly, int is_tls) +{ + DEBUG_FUNCTION; + return ""; +} + static void * test_layers_plugin_open (int readonly) { @@ -257,6 +264,7 @@ static struct nbdkit_plugin plugin = { .after_fork = test_layers_plugin_after_fork, .preconnect = test_layers_plugin_preconnect, .list_exports = test_layers_plugin_list_exports, + .default_export = test_layers_plugin_default_export, .open = test_layers_plugin_open, .close = test_layers_plugin_close, .get_size = test_layers_plugin_get_size, diff --git a/tests/test-layers.c b/tests/test-layers.c index dd826f36..db0a2da0 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -326,20 +326,10 @@ main (int argc, char *argv[]) "test_layers_plugin_preconnect", NULL); - /* list_exports methods called in outer-to-inner order, complete - * in inner-to-outer order. But since we didn't send NBD_OPT_LIST, - * the outer filter does not expose a list; rather, the rest of the - * chain is used to resolve the canonical name of the default export. + /* XXX We should test NBD_OPT_INFO here for coverage of + * .list_exports. However it would be easier to do by using libnbd + * than open-coding a naive client. */ - log_verify_seen_in_order - ("filter3: test_layers_filter_list_exports", - "testlayersfilter2: list_exports", - "filter2: test_layers_filter_list_exports", - "testlayersfilter1: list_exports", - "filter1: test_layers_filter_list_exports", - "testlayersplugin: list_exports", - "test_layers_plugin_list_exports", - NULL); /* open methods called in outer-to-inner order, but thanks to next * pointer, complete in inner-to-outer order. */ -- 2.28.0

On Wed, Aug 26, 2020 at 09:16:46PM -0500, Eric Blake wrote: I'm a bit confused by how nbdkit_add_default_export() should be used and the documentation additions in this patch definitely need some work, but I'm going to assume it's used like this: my_list_exports (exports) { nbdkit_add_export (exports, "", some_description); /* Mark the most recently added export as the default: */ nbdkit_add_default_export (exports); /* Some other exports: */ nbdkit_add_export (exports, "foo", NULL); nbdkit_add_export (exports, "bar", NULL); } You could then argue that .default_export should call nbdkit_add_export + nbdkit_add_default_export instead of having to deal with all the interning stuff ... What do you think about that change? my_default_export (exports) { /* nbdkit_add_export should only be called once. */ nbdkit_add_export (exports, "", some_description); nbdkit_add_default_export (exports); } But then again this is differently awkward to consume from the server side. And if we did this I can't really see why we need the .default_export callback at all. The server could just call .list_exports early on and keep track internally of the exports and default export name that the plugin advertises. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

Implementing .default_export with its 'const char *' return is tricky in the sh plugin: we must return dynamic memory, but must avoid a use-after-free. And we don't want to change the return type of .default_export to 'char *', because that would make our choice of malloc()/free() part of the API, preventing either nbdkit or a plugin from experimenting with an alternate allocator implementation. Elsewhere, we have done things like nbdkit_extents_new(), so that even though the client is directing allocation, the actual call to malloc() is done by nbdkit proper, so that nbdkit later calling free() does not tie the plugin's hands, and nbdkit could change its underlying allocation without breaking ABI. (Note that nbdkit_realpath() and nbdkit_absolute_path() require the caller to use free(), but as they are used during .config, it's less of a burden for plugins to take care of that in .unload.) We could document that the burden is on the plugin to avoid the memory leak, by making sh track all strings it returns, then finally clean them up during .unload. But this is still a memory leak in the case of .default_export, which can be called multiple times when there are multiple clients, and presumably with different values per client call; better would be freeing the memory at .close when each client goes away. Except that .default_export is called before .open, and therefore before we have access to the handle struct that will eventually make it to .close. So, the solution is to let nbdkit track an alternate string on our behalf, where nbdkit then cleans it up as the client goes away. There are several places in the existing code base that can also take advantage of this copying functionality, including in filters that want to pass altered strings to .config while still obeying lifetime rules. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/nbdkit-filter.pod | 9 +++++--- docs/nbdkit-plugin.pod | 33 +++++++++++++++++++++++++--- include/nbdkit-common.h | 4 ++++ server/internal.h | 7 +++++- server/connections.c | 2 +- server/main.c | 26 ++++++---------------- server/nbdkit.syms | 2 ++ server/plugins.c | 11 +++------- server/public.c | 48 +++++++++++++++++++++++++++++++++++++++++ filters/log/log.c | 10 +++------ 10 files changed, 110 insertions(+), 42 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index dd667c0c..7d72d138 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -183,7 +183,8 @@ You can modify parameters when you call the C<next> function. However be careful when modifying strings because for some methods (eg. C<.config>) the plugin may save the string pointer that you pass along. So you may have to ensure that the string is not freed for the -lifetime of the server. +lifetime of the server; you may find C<nbdkit_strdup_intern> helpful +for avoiding a memory leak while still obeying lifecycle constraints. Note that if your filter registers a callback but in that callback it doesn't call the C<next> function then the corresponding method in the @@ -450,8 +451,10 @@ requires write access to the underlying device in case a journal needs to be replayed for consistency as part of the mounting process. The C<exportname> string is only guaranteed to be available during the -call. If the filter needs to use it (other than immediately passing -it down to the next layer) it must take a copy. The C<exportname> and +call (different than the lifetime for the return of C<nbdkit_export_name> +used by plugins). If the filter needs to use it (other than immediately +passing it down to the next layer) it must take a copy, although +C<nbdkit_strdup_intern> is useful for this task. The C<exportname> and C<is_tls> parameters are provided so that filters do not need to use the plugin-only interfaces of C<nbdkit_export_name> and C<nbdkit_is_tls>. diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 50cf991d..056b1450 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -748,7 +748,10 @@ leak unintended information. If the plugin returns C<NULL> or an invalid string (such as longer than 4096 bytes), the client is not permitted to connect to the default export. However, this is not an error in the protocol, so it -is not necessary to call C<nbdkit_error>. +is not necessary to call C<nbdkit_error>. If the callback will +not be returning a compile-time constant string, you may find +C<nbdkit_strdup_intern> helpful for returning a value that avoids a +memory leak. =head2 C<.open> @@ -1504,8 +1507,9 @@ content. Return the optional NBD export name if one was negotiated with the current client (this uses thread-local magic so no parameter is -required). The returned string is only valid while the client is -connected, so if you need to store it in the plugin you must copy it. +required). The returned string is valid at least through the +C<.close> of the current connection, but if you need to store it +in the plugin for use by more than one client you must copy it. The export name is a free-form text string, it is not necessarily a path or filename and it does not need to begin with a C<'/'> @@ -1516,6 +1520,29 @@ client data, be cautious when parsing it.> On error, C<nbdkit_error> is called and the call returns C<NULL>. +=head1 STRING LIFETIME + +Some callbacks are specified to return C<const char *>, even when a +plugin may not have a suitable compile-time constant to return. +Returning dynamically-allocated memory for such a callback would +induce a memory leak or otherwise complicate the plugin to perform +additional bookkeeping. For these cases, nbdkit provides a +convenience function for creating a copy of a string for better +lifetime management. + + const char *nbdkit_strdup_intern (const char *str); + const char *nbdkit_strndup_intern (const char *str, size_t n); + +Returns a copy of C<str>, possibly limited to a maximum of C<n> bytes, +so that the caller may reclaim str and use the copy in its place. If +the copy is created outside the scope of a connection (such as during +C<.load> or C<.config>), the lifetime of the copy will last at least +through C<.unload>; if it was created within a connection (such as +during C<.preconnect>, C<.default_export>, or C<.open>), the lifetime +will last at least through C<.close>. + +On error, C<nbdkit_error> is called and the call returns C<NULL>. + =head1 AUTHENTICATION A server may use C<nbdkit_is_tls> to limit which export names work diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h index 5e8c0b6f..e9d31421 100644 --- a/include/nbdkit-common.h +++ b/include/nbdkit-common.h @@ -120,6 +120,10 @@ NBDKIT_EXTERN_DECL (int, nbdkit_nanosleep, (unsigned sec, unsigned nsec)); NBDKIT_EXTERN_DECL (int, nbdkit_peer_name, (struct sockaddr *addr, socklen_t *addrlen)); NBDKIT_EXTERN_DECL (void, nbdkit_shutdown, (void)); +NBDKIT_EXTERN_DECL (const char *, nbdkit_strdup_intern, + (const char *str)); +NBDKIT_EXTERN_DECL (const char *, nbdkit_strndup_intern, + (const char *str, size_t n)); struct nbdkit_extents; NBDKIT_EXTERN_DECL (int, nbdkit_add_extent, diff --git a/server/internal.h b/server/internal.h index e2a68513..f11f897e 100644 --- a/server/internal.h +++ b/server/internal.h @@ -237,6 +237,7 @@ reset_handle (struct handle *h) h->can_cache = -1; } +DEFINE_VECTOR_TYPE(string_vector, char *); struct connection { pthread_mutex_t request_lock; pthread_mutex_t read_lock; @@ -258,8 +259,9 @@ struct connection { bool structured_replies; bool meta_context_base_allocation; + string_vector interns; char *exportname_from_set_meta_context; - char *exportname; + const char *exportname; int sockin, sockout; connection_recv_function recv; @@ -298,6 +300,9 @@ extern int protocol_recv_request_send_reply (void); */ #define base_allocation_id 1 +/* public.c */ +extern void free_interns (void); + /* crypto.c */ #define root_tls_certificates_dir sysconfdir "/pki/" PACKAGE_NAME extern void crypto_init (bool tls_set_on_cli); diff --git a/server/connections.c b/server/connections.c index 67a68469..d9f685c9 100644 --- a/server/connections.c +++ b/server/connections.c @@ -360,7 +360,7 @@ free_connection (struct connection *conn) pthread_mutex_destroy (&conn->status_lock); free (conn->exportname_from_set_meta_context); - free (conn->exportname); + free_interns (); /* This is needed in order to free a field in struct handle. */ for_each_backend (b) diff --git a/server/main.c b/server/main.c index 17c4c324..ae552447 100644 --- a/server/main.c +++ b/server/main.c @@ -631,15 +631,9 @@ main (int argc, char *argv[]) * * Keys must live for the life of nbdkit. Since we want to avoid * modifying argv (so that /proc/PID/cmdline remains sane) but we - * need to create a key from argv[i] = "key=value" we must save the - * keys in an array which is freed at the end of main(). + * need to create a key from argv[i] = "key=value" we must intern + * the keys, which are then freed at the end of main(). */ - char **keys = calloc (argc, sizeof (char *)); - if (keys == NULL) { - perror ("calloc"); - exit (EXIT_FAILURE); - } - magic_config_key = top->magic_config_key (top); for (i = 0; optind < argc; ++i, ++optind) { size_t n; @@ -647,12 +641,10 @@ main (int argc, char *argv[]) p = strchr (argv[optind], '='); n = p - argv[optind]; if (p && is_config_key (argv[optind], n)) { /* Is it key=value? */ - keys[optind] = strndup (argv[optind], n); - if (keys[optind] == NULL) { - perror ("strndup"); + const char *key = nbdkit_strndup_intern (argv[optind], n); + if (key == NULL) exit (EXIT_FAILURE); - } - top->config (top, keys[optind], p+1); + top->config (top, key, p+1); } else if (magic_config_key == NULL) { if (i == 0) /* magic script parameter */ @@ -677,9 +669,7 @@ main (int argc, char *argv[]) if (dump_plugin) { top->dump_fields (top); top->free (top); - for (i = 1; i < argc; ++i) - free (keys[i]); - free (keys); + free_interns (); exit (EXIT_SUCCESS); } @@ -717,9 +707,7 @@ main (int argc, char *argv[]) crypto_free (); close_quit_pipe (); - for (i = 1; i < argc; ++i) - free (keys[i]); - free (keys); + free_interns (); /* Note: Don't exit here, otherwise this won't work when compiled * for libFuzzer. diff --git a/server/nbdkit.syms b/server/nbdkit.syms index 212e36aa..d17878b7 100644 --- a/server/nbdkit.syms +++ b/server/nbdkit.syms @@ -74,6 +74,8 @@ nbdkit_set_error; nbdkit_shutdown; nbdkit_stdio_safe; + nbdkit_strdup_intern; + nbdkit_strndup_intern; nbdkit_vdebug; nbdkit_verror; diff --git a/server/plugins.c b/server/plugins.c index da28b2f1..7425857e 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -328,17 +328,13 @@ plugin_open (struct backend *b, int readonly, const char *exportname, * will still need to save the export name in the handle because of * the lifetime issue. */ - conn->exportname = strdup (exportname); - if (conn->exportname == NULL) { - nbdkit_error ("strdup: %m"); + conn->exportname = nbdkit_strdup_intern (exportname); + if (conn->exportname == NULL) return NULL; - } r = p->plugin.open (readonly); - if (r == NULL) { - free (conn->exportname); + if (r == NULL) conn->exportname = NULL; - } return r; } @@ -367,7 +363,6 @@ plugin_close (struct backend *b, void *handle) if (handle && p->plugin.close) p->plugin.close (handle); - free (conn->exportname); conn->exportname = NULL; } diff --git a/server/public.c b/server/public.c index d10d466e..512e9caa 100644 --- a/server/public.c +++ b/server/public.c @@ -726,3 +726,51 @@ nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen) return 0; } + +/* Functions for manipulating intern'd strings. */ + +static string_vector global_interns; + +void +free_interns (void) +{ + struct connection *conn = threadlocal_get_conn (); + string_vector *list = conn ? &conn->interns : &global_interns; + + string_vector_iter (list, (void *) free); + string_vector_reset (list); +} + +const char * +nbdkit_strndup_intern (const char *str, size_t n) +{ + struct connection *conn = threadlocal_get_conn (); + string_vector *list = conn ? &conn->interns : &global_interns; + char *copy; + + if (str == NULL) { + nbdkit_error ("nbdkit_strndup_intern: no string given"); + errno = EINVAL; + return NULL; + } + + copy = strndup (str, n); + if (copy == NULL) { + nbdkit_error ("strndup: %m"); + return NULL; + } + + if (string_vector_append (list, copy) == -1) { + nbdkit_error ("malloc: %m"); + free (copy); + return NULL; + } + + return copy; +} + +const char * +nbdkit_strdup_intern (const char *str) +{ + return nbdkit_strndup_intern (str, SIZE_MAX); +} diff --git a/filters/log/log.c b/filters/log/log.c index 71c21211..4525f362 100644 --- a/filters/log/log.c +++ b/filters/log/log.c @@ -134,7 +134,7 @@ log_get_ready (nbdkit_next_get_ready *next, void *nxdata, int thread_model) struct handle { uint64_t connection; uint64_t id; - char *exportname; + const char *exportname; int tls; }; @@ -305,9 +305,8 @@ log_open (nbdkit_next_open *next, void *nxdata, * it in log_prepare. We must take a copy because this string has a * short lifetime. */ - h->exportname = strdup (exportname); + h->exportname = nbdkit_strdup_intern (exportname); if (h->exportname == NULL) { - nbdkit_error ("strdup: %m"); free (h); return NULL; } @@ -322,10 +321,7 @@ log_open (nbdkit_next_open *next, void *nxdata, static void log_close (void *handle) { - struct handle *h = handle; - - free (h->exportname); - free (h); + free (handle); } static int -- 2.28.0

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&gt; --- 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

Add a new filter to make it easier to add exports to a plugin that does advertise them, to avoid advertising where a plugin's list might be an information leak, and to alter which export name is used in place of "". I would love to be able to have a strict mode enforcing that .open is called only with an export name that the plugin was willing to advertise, but for that, we'll need a way to call the plugin's .list_exports from within the context of .open (since we can't guarantee the guest will call NBD_OPT_LIST). So for now, strict mode requires redundant exportname= command line parameters. It may also be worth adding a way to pass exportname-file=/path/to/file, which then gets parsed the same was as the sh plugin (right now with NAMES, INTERLEAVED, or NAMES+DESCRIPTIONS), to avoid having to do so many export names directly on the command line. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- .../exportname/nbdkit-exportname-filter.pod | 154 ++++++++ filters/ext2/nbdkit-ext2-filter.pod | 5 + plugins/file/nbdkit-file-plugin.pod | 9 +- configure.ac | 2 + filters/exportname/Makefile.am | 67 ++++ tests/Makefile.am | 4 + filters/exportname/exportname.c | 342 ++++++++++++++++++ tests/test-exportname.sh | 184 ++++++++++ TODO | 9 + 9 files changed, 773 insertions(+), 3 deletions(-) create mode 100644 filters/exportname/nbdkit-exportname-filter.pod create mode 100644 filters/exportname/Makefile.am create mode 100644 filters/exportname/exportname.c create mode 100755 tests/test-exportname.sh diff --git a/filters/exportname/nbdkit-exportname-filter.pod b/filters/exportname/nbdkit-exportname-filter.pod new file mode 100644 index 00000000..4de56dee --- /dev/null +++ b/filters/exportname/nbdkit-exportname-filter.pod @@ -0,0 +1,154 @@ +=head1 NAME + +nbdkit-exportname-filter - adjust export names between client and plugin + +=head1 SYNOPSIS + + nbdkit --filter=exportname plugin [default-export=NAME] + [exportname-list=MODE] [exportname-strict=true] [exportname=NAME]... + [exportdesc=DESC] + +=head1 DESCRIPTION + +Some plugins (such as C<nbdkit-file-plugin(1)> and filters (such as +C<nbdkit-ext2-filter(1)> are able to serve different content based on +the export name requested by the client. The NBD protocol allows a +server to advertise the set of export names it is serving. However, +the list advertised (or absent) from the plugin may not always match +what you want an actual client to see. This filter can be used to +alter the advertised list, as well as configuring which export should +be treated as the default when the client requests the empty string +(C<"">) as an export name. + +=head1 PARAMETERS + +=over 4 + +=item B<default-export=>NAME + +When the client requests the default export name (C<"">), request the +export C<NAME> from the underlying plugin instead of relying on the +plugin's choice of default export. Setting NAME to the empty string +has the same effect as omitting this parameter. + +=item B<exportname-list=keep> + +=item B<exportname-list=error> + +=item B<exportname-list=empty> + +=item B<exportname-list=defaultonly> + +=item B<exportname-list=explicit> + +This parameter determines which exports are advertised to a guest that +requests a listing via C<NBD_OPT_LIST>. The default mode is C<keep> +to advertise whatever the underlying plugin reports. Mode C<error> +causes clients to see an error rather than an export list. Mode +C<empty> returns an empty list. Mode C<defaultonly> returns a list +that contains only the canonical name of the default export. Mode +C<explicit> returns only the exports set by C<exportname=>. Note that +the list of advertised exports need not reflect reality: an advertised +name may be rejected, or a client may connect to an export name that +was not advertised, but learned through other means. + +=item B<exportname-strict=false> + +=item B<exportname-strict=true> + +Normally, a client can pass whatever export name it wants, regardless +of whether that name is advertised. But setting this parameter to +true will cause the connection to fail if a client requests an export +name that was not included via an B<exportname=> parameter. At this +time, it is not possible to restrict a client to exports advertised by +the plugin without repeating that list via B<exportname>; this +technical limitation may be lifted in the future. + +=item B<exportname=>NAME + +This parameter adds C<NAME> to the list of advertised exports; it may +be set multiple times. + +=item B<exportdesc=keep> + +=item B<exportdesc=none> + +=item B<exportdesc=fixed:>STRING + +=item B<exportdesc=script:>SCRIPT + +The C<exportdesc> parameter controls what optional descriptions are +sent alongside an export name. If set to C<keep> (the default), +descriptions are determined by the plugin. If set to C<none>, +descriptions from the plugin are ignored (useful if you are worried +about a potential information leak). If set to C<fixed:STRING>, the +same fixed string description is offered for every export. If set to +C<script:SCRIPT>, this filter executes script with C<$name> set to the +export to be described, and uses the output of that command as the +description. + +=back + +=head1 EXAMPLES + +Suppose that the directory /path/to/dir contains permanent files named +file1, file2, and file3. The following commands show various ways to +alter the use of export names while serving that directory: + +Allow a client requesting C<""> to get the contents of file2, rather +than an error: + + nbdkit --filter=exportname file dir=/path/to/dir default-export=file2 + +Do not advertise any exports; a client must know in advance what +export names to try: + + nbdkit --filter=exportname file dir=/path/to/dir exportname-list=empty + +Allow clients to connect to file1 and file3, but not file2: + + nbdkit --filter=exportname file dir=/path/to/dir \ + exportname-list=explicit exportname-strict=true \ + exportname=file1 exportname=file3 + +Offer C<ls(3)> long descriptions alongside each export name: + + nbdkit --filter=exportname file dir=/path/to/dir \ + exportdesc=script:'ls -l /path/to/dir/"$name"' + +=head1 FILES + +=over 4 + +=item F<$filterdir/nbdkit-exportname-filter.so> + +The filter. + +Use C<nbdkit --dump-config> to find the location of C<$filterdir>. + +=back + +=head1 VERSION + +C<nbdkit-exportname-filter> first appeared in nbdkit 1.22. + +=head1 SEE ALSO + +L<nbdkit(1)>, +L<nbdkit-filter(3)>, +L<nbdkit-ext2-filter(1)>, +L<nbdkit-extentlist-filter(1)>, +L<nbdkit-fua-filter(1)>, +L<nbdkit-nocache-filter(1)>, +L<nbdkit-noparallel-filter(1)>, +L<nbdkit-nozero-filter(1)>, +L<nbdkit-file-plugin(1)>, +L<nbdkit-info-plugin(1)>. + +=head1 AUTHORS + +Eric Blake + +=head1 COPYRIGHT + +Copyright (C) 2020 Red Hat Inc. diff --git a/filters/ext2/nbdkit-ext2-filter.pod b/filters/ext2/nbdkit-ext2-filter.pod index 78d27f8f..1aef9c2e 100644 --- a/filters/ext2/nbdkit-ext2-filter.pod +++ b/filters/ext2/nbdkit-ext2-filter.pod @@ -65,6 +65,10 @@ exportname passed by the client. Note that this mode allows the client to deduce which files exist within the disk image, which may be a security risk in some situations. +At present, when using this mode, the server does not advertise any +particular exports; however, you may use +L<nbdkit-exportname-filter(1)> to perform that task. + =back =head1 FILES @@ -89,6 +93,7 @@ and removed in nbdkit 1.22. L<nbdkit(1)>, L<nbdkit-plugin(3)>, +L<nbdkit-exportname-filter(1)>, L<nbdkit-partition-filter(1)>, L<nbdkit-guestfs-plugin(1)>, L<http://e2fsprogs.sourceforge.net/>;, diff --git a/plugins/file/nbdkit-file-plugin.pod b/plugins/file/nbdkit-file-plugin.pod index 3844d75f..f6f31a76 100644 --- a/plugins/file/nbdkit-file-plugin.pod +++ b/plugins/file/nbdkit-file-plugin.pod @@ -65,10 +65,12 @@ directory named C<DIRNAME>, including those found by following symbolic links. Other special files in the directory (such as subdirectories, fifos, or Unix sockets) are ignored. When this mode is used, the file to be served is chosen by the export name passed by -the client. For now, the client must already know what files are -in the directory, although there are plans to expose them via +the client. For now, the client must already know what files are in +the directory, although there are plans to expose them via NBD_OPT_LIST; likewise, a client that requests the default export -(C<"">) will be rejected. For security, when using directory mode, +(C<"">) will be rejected. However, you can use +L<nbdkit-exportname-filter(1)> to adjust what export names the client +sees or uses as a default. For security, when using directory mode, this plugin will not accept export names containing slash (C</>). =back @@ -183,6 +185,7 @@ L<nbdkit-plugin(3)>, L<nbdkit-split-plugin(1)>, L<nbdkit-partitioning-plugin(1)>, L<nbdkit-tmpdisk-plugin(1)>, +L<nbdkit-exportname-filter(1)>, L<nbdkit-fua-filter(1)>, L<nbdkit-noextents-filter(1)>. diff --git a/configure.ac b/configure.ac index c0142b62..a11a3c59 100644 --- a/configure.ac +++ b/configure.ac @@ -104,6 +104,7 @@ filters="\ delay \ error \ exitlast \ + exportname \ ext2 \ extentlist \ fua \ @@ -1161,6 +1162,7 @@ AC_CONFIG_FILES([Makefile filters/delay/Makefile filters/error/Makefile filters/exitlast/Makefile + filters/exportname/Makefile filters/ext2/Makefile filters/extentlist/Makefile filters/fua/Makefile diff --git a/filters/exportname/Makefile.am b/filters/exportname/Makefile.am new file mode 100644 index 00000000..1bf54518 --- /dev/null +++ b/filters/exportname/Makefile.am @@ -0,0 +1,67 @@ +# nbdkit +# Copyright (C) 2018-2020 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-exportname-filter.pod + +filter_LTLIBRARIES = nbdkit-exportname-filter.la + +nbdkit_exportname_filter_la_SOURCES = \ + exportname.c \ + $(top_srcdir)/include/nbdkit-filter.h \ + $(NULL) + +nbdkit_exportname_filter_la_CPPFLAGS = \ + -I$(top_srcdir)/include \ + -I$(top_srcdir)/common/include \ + -I$(top_srcdir)/common/utils \ + $(NULL) +nbdkit_exportname_filter_la_CFLAGS = $(WARNINGS_CFLAGS) +nbdkit_exportname_filter_la_LDFLAGS = \ + -module -avoid-version -shared $(SHARED_LDFLAGS) \ + -Wl,--version-script=$(top_srcdir)/filters/filters.syms \ + $(NULL) +nbdkit_exportname_filter_la_LIBADD = \ + $(top_builddir)/common/utils/libutils.la \ + $(NULL) + +if HAVE_POD + +man_MANS = nbdkit-exportname-filter.1 +CLEANFILES += $(man_MANS) + +nbdkit-exportname-filter.1: nbdkit-exportname-filter.pod + $(PODWRAPPER) --section=1 --man $@ \ + --html $(top_builddir)/html/$@.html \ + $< + +endif HAVE_POD diff --git a/tests/Makefile.am b/tests/Makefile.am index 29c4811b..a316decb 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1321,6 +1321,10 @@ EXTRA_DIST += \ TESTS += test-exitlast.sh EXTRA_DIST += test-exitlast.sh +# exportname filter test. +TESTS += test-exportname.sh +EXTRA_DIST += test-exportname.sh + # ext2 filter test. if HAVE_MKE2FS_WITH_D if HAVE_EXT2 diff --git a/filters/exportname/exportname.c b/filters/exportname/exportname.c new file mode 100644 index 00000000..910030ec --- /dev/null +++ b/filters/exportname/exportname.c @@ -0,0 +1,342 @@ +/* nbdkit + * Copyright (C) 2020 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 <stdbool.h> +#include <string.h> + +#include <nbdkit-filter.h> + +#include "cleanup.h" +#include "utils.h" + +static const char *default_export; +static enum { + LIST_KEEP, + LIST_ERROR, + LIST_EMPTY, + LIST_DEFAULT, + LIST_EXPLICIT, +} list; +static bool strict; +static enum { + DESC_KEEP, + DESC_NONE, + DESC_FIXED, + DESC_SCRIPT, +} desc_mode; +static const char *desc; +struct nbdkit_exports *exports; + +static void +exportname_load (void) +{ + exports = nbdkit_exports_new (); + if (!exports) { + nbdkit_error ("malloc: %m"); + exit (EXIT_FAILURE); + } +} + +static void +exportname_unload (void) +{ + nbdkit_exports_free (exports); +} + +/* Called for each key=value passed on the command line. */ +static int +exportname_config (nbdkit_next_config *next, void *nxdata, + const char *key, const char *value) +{ + int r; + + if (strcmp (key, "default-export") == 0 || + strcmp (key, "default_export") == 0) { + default_export = value; + return 0; + } + if (strcmp (key, "exportname-list") == 0 || + strcmp (key, "exportname_list") == 0) { + if (strcmp (value, "keep") == 0) + list = LIST_KEEP; + else if (strcmp (value, "error") == 0) + list = LIST_ERROR; + else if (strcmp (value, "empty") == 0) + list = LIST_EMPTY; + else if (strcmp (value, "defaultonly") == 0 || + strcmp (value, "default-only") == 0) + list = LIST_DEFAULT; + else if (strcmp (value, "explicit") == 0) + list = LIST_EXPLICIT; + else { + nbdkit_error ("unrecognized exportname-list mode: %s", value); + return -1; + } + return 0; + } + if (strcmp (key, "exportname-strict") == 0 || + strcmp (key, "exportname_strict") == 0) { + r = nbdkit_parse_bool (value); + if (r == -1) + return -1; + strict = r; + return 0; + } + if (strcmp (key, "exportname") == 0) + return nbdkit_add_export (exports, value, NULL); + if (strcmp (key, "exportdesc") == 0) { + if (strcmp (value, "keep") == 0) + desc_mode = DESC_KEEP; + else if (strcmp (value, "none") == 0) { + desc_mode = DESC_NONE; + desc = NULL; + } + else if (strncmp (value, "fixed:", 6) == 0) { + desc_mode = DESC_FIXED; + desc = value + 6; + } + else if (strncmp (value, "script:", 7) == 0) { + desc_mode = DESC_SCRIPT; + desc = value + 7; + } + else { + nbdkit_error ("unrecognized exportdesc mode: %s", value); + return -1; + } + return 0; + } + return next (nxdata, key, value); +} + +#define exportname_config_help \ + "default-export=<NAME> Canonical name for the \"\" default export.\n" \ + "exportname-list=<MODE> Which exports to advertise: keep (default), error,\n" \ + " empty, defaultonly, explicit.\n" \ + "exportname-strict=<BOOL> Limit clients to explicit exports (default false).\n" \ + "exportname=<NAME> Add an explicit export name, may be repeated.\n" \ + "exportdesc=<MODE> Set descriptions according to mode: keep (default),\n" \ + " none, fixed:STRING, script:SCRIPT.\n" \ + +static const char * +get_desc (const char *name, const char *def) +{ + FILE *fp; + CLEANUP_FREE char *cmd = NULL; + size_t cmdlen = 0; + char buf[4096]; /* Maximum NBD string; we truncate any longer response */ + size_t r; + + switch (desc_mode) { + case DESC_KEEP: + return def; + case DESC_NONE: + case DESC_FIXED: + return desc; + case DESC_SCRIPT: + break; + default: + abort (); + } + + /* Construct the command. */ + fp = open_memstream (&cmd, &cmdlen); + if (fp == NULL) { + nbdkit_debug ("open_memstream: %m"); + return NULL; + } + fprintf (fp, "export name; name="); + shell_quote (name, fp); + fprintf (fp, "\n%s\n", desc); + if (fclose (fp) == EOF) { + nbdkit_debug ("memstream failed: %m"); + return NULL; + } + nbdkit_debug ("%s", cmd); + fp = popen (cmd, "r"); + if (fp == NULL) { + nbdkit_debug ("popen: %m"); + return NULL; + } + + /* Now read the description */ + r = fread (buf, 1, sizeof buf, fp); + if (r == 0 && ferror (fp)) { + nbdkit_debug ("fread: %m"); + pclose (fp); + return NULL; + } + pclose (fp); + if (r && buf[r-1] == '\n') + r--; + return nbdkit_strndup_intern (buf, r); +} + +static int +exportname_list_exports (nbdkit_next_list_exports *next, void *nxdata, + int readonly, int is_tls, + struct nbdkit_exports *exps) +{ + size_t i; + struct nbdkit_exports *source; + CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps2 = NULL; + + switch (list) { + case LIST_KEEP: + source = exps2 = nbdkit_exports_new (); + if (exps2 == NULL) + return -1; + if (next (nxdata, readonly, exps2) == -1) + return -1; + break; + case LIST_ERROR: + nbdkit_error ("export list restricted by policy"); + return -1; + case LIST_EMPTY: + return 0; + case LIST_DEFAULT: + return nbdkit_add_default_export (exps); + case LIST_EXPLICIT: + source = exports; + break; + default: + abort (); + } + + for (i = 0; i < nbdkit_exports_count (source); i++) { + struct nbdkit_export e = nbdkit_get_export (source, i); + + if (nbdkit_add_export (exps, e.name, + get_desc (e.name, e.description)) == -1) + return -1; + } + return 0; +} + +static const char * +exportname_default_export (nbdkit_next_default_export *next, void *nxdata, + int readonly, int is_tls) +{ + size_t i; + + /* If we are strict, do not allow connection unless "" was advertised. */ + if (strict) { + for (i = 0; i < nbdkit_exports_count (exports); i++) { + if (nbdkit_get_export (exports, i).name[0] == '\0') + return default_export ?: ""; + } + return NULL; + } + + if (default_export) + return default_export; + return next (nxdata, readonly); +} + +struct handle { + const char *name; +}; + +static void * +exportname_open (nbdkit_next_open *next, void *nxdata, + int readonly, const char *exportname, int is_tls) +{ + size_t i; + struct handle *h; + + if (strict) { + for (i = 0; i < nbdkit_exports_count (exports); i++) { + if (strcmp (nbdkit_get_export (exports, i).name, exportname) == 0) + break; + } + if (i == nbdkit_exports_count (exports)) { + nbdkit_error ("export '%s not found", exportname); + errno = ENOENT; + return NULL; + } + } + + h = malloc (sizeof *h); + if (h == NULL) { + nbdkit_error ("malloc: %m"); + return NULL; + } + + h->name = nbdkit_strdup_intern (exportname); + if (h->name == NULL) { + free (h); + return NULL; + } + if (next (nxdata, readonly, exportname) == -1) { + free (h); + return NULL; + } + + return h; +} + +static void +exportname_close (void *handle) +{ + free (handle); +} + +static const char * +exportname_export_description (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + struct handle *h = handle; + const char *def = NULL; + + if (desc_mode == DESC_KEEP) + def = next_ops->export_description (nxdata); + + return get_desc (h->name, def); +} + +static struct nbdkit_filter filter = { + .name = "exportname", + .longname = "nbdkit exportname filter", + .load = exportname_load, + .unload = exportname_unload, + .config = exportname_config, + .config_help = exportname_config_help, + .list_exports = exportname_list_exports, + .default_export = exportname_default_export, + .open = exportname_open, + .close = exportname_close, + .export_description = exportname_export_description, +}; + +NBDKIT_REGISTER_FILTER(filter) diff --git a/tests/test-exportname.sh b/tests/test-exportname.sh new file mode 100755 index 00000000..2d30df4e --- /dev/null +++ b/tests/test-exportname.sh @@ -0,0 +1,184 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2020 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. + +source ./functions.sh +set -e +set -x + +requires_plugin sh +requires nbdinfo --version +requires nbdsh -c 'print (h.set_full_info)' +requires jq --version + +files="exportname.out exportname.sh" +rm -rf $files +cleanup_fn rm -rf $files + +query='[ [.exports[]] | sort_by(."export-name")[] | + [."export-name", .description, ."export-size"] ]' +fail=0 + +cat >exportname.sh <<\EOF +case $1 in + list_exports) + echo INTERLEAVED + echo a; echo x + echo b; echo y + echo c; echo z + ;; + default_export) echo a ;; + open) echo "$3" ;; + export_description | get_size) + case $2 in + a) echo 1 ;; + b) echo 2 ;; + c) echo 3 ;; + *) exit 1 ; + esac ;; + *) exit 2 ;; +esac +EOF +chmod +x exportname.sh + +# Establish a baseline +nbdkit -U - sh exportname.sh \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = \ + '[["a","x",1],["b","y",2],["c","z",3]]' + +# Set the default export +nbdkit -U - --filter=exportname sh exportname.sh default-export= \ + --run 'nbdinfo --no-content --json "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["","1",1]]' + +nbdkit -U - --filter=exportname sh exportname.sh default-export=b \ + --run 'nbdinfo --no-content --json "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["b","2",2]]' + +# Test export list policies +nbdkit -U - --filter=exportname sh exportname.sh exportname-list=keep \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = \ + '[["a","x",1],["b","y",2],["c","z",3]]' + +nbdkit -U - --filter=exportname sh exportname.sh exportname-list=error \ + --run 'nbdinfo --json --list "$uri"' > exportname.out && fail=1 || : +test ! -s exportname.out + +nbdkit -U - --filter=exportname sh exportname.sh exportname-list=empty \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[]' + +nbdkit -U - --filter=exportname sh exportname.sh default-export=b \ + exportname-list=defaultonly exportname=a exportname=b \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["b",null,2]]' + +nbdkit -U - --filter=exportname sh exportname.sh default-export=b \ + exportname-list=defaultonly exportname=a exportname=b \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["b",null,2]]' + +nbdkit -U - --filter=exportname sh exportname.sh \ + exportname-list=explicit exportname=b exportname=a \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["a",null,1],["b",null,2]]' + +# Test description modes with lists +nbdkit -U - --filter=exportname sh exportname.sh exportdesc=keep \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = \ + '[["a","x",1],["b","y",2],["c","z",3]]' + +nbdkit -U - --filter=exportname sh exportname.sh exportdesc=none \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = \ + '[["a",null,1],["b",null,2],["c",null,3]]' + +nbdkit -U - --filter=exportname sh exportname.sh exportdesc=fixed:hi \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = \ + '[["a","hi",1],["b","hi",2],["c","hi",3]]' + +nbdkit -U - --filter=exportname sh exportname.sh \ + exportdesc=script:'echo $name$name' \ + --run 'nbdinfo --json --list "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = \ + '[["a","aa",1],["b","bb",2],["c","cc",3]]' + +# Test description modes with connections +nbdkit -U - -e c --filter=exportname sh exportname.sh exportdesc=fixed:hi \ + --run 'nbdinfo --no-content --json "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["c","hi",3]]' + +nbdkit -U - -e c --filter=exportname sh exportname.sh \ + exportdesc=script:'echo $name$name' \ + --run 'nbdinfo --no-content --json "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["c","cc",3]]' + +# Test strict mode +nbdkit -U - --filter=exportname sh exportname.sh exportname-strict=true \ + --run 'nbdinfo --no-content --json "$uri"' > exportname.out && fail=1 +test ! -s exportname.out + +nbdkit -U - --filter=exportname sh exportname.sh exportname-strict=true \ + exportname=a exportname=b exportname=c \ + --run 'nbdinfo --no-content --json "$uri"' > exportname.out && fail=1 +test ! -s exportname.out + +nbdkit -U - --filter=exportname sh exportname.sh exportname-strict=true \ + exportname=a exportname=b exportname= default-export=a\ + --run 'nbdinfo --no-content --json "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["a","1",1]]' + +nbdkit -U - -e a --filter=exportname sh exportname.sh exportname-strict=true \ + exportname=a exportname=b exportname=c \ + --run 'nbdinfo --no-content --json "$uri"' > exportname.out +cat exportname.out +test "$(jq -c "$query" exportname.out)" = '[["a","1",1]]' + +exit $fail diff --git a/TODO b/TODO index ff902c7d..2032b27b 100644 --- a/TODO +++ b/TODO @@ -252,6 +252,15 @@ nbdkit-tls-fallback-filter: * Adjust handling of .list_exports to mask what the plugin advertises when tls is not yet active. +nbdkit-exportname-fitler: + +* find a way to call the plugin's .list_exports during .open, so that + we can enforce exportname-strict=true without command line redundancy + +* add a mode for passing in a file containing exportnames in the same + manner accepted by the sh/eval plugins, rather than one name (and no + description) per config parameter + Filters for security -------------------- -- 2.28.0