[nbdkit PATCH 0/3] Content differentiation during --tls=on
Patch 3 still needs tests added, but it is at least working from my simple command line tests. Eric Blake (3): server: Implement nbdkit_is_tls for use during .open server: Expose final thread_model to filter's .get_ready tlsdummy: New filter docs/nbdkit-filter.pod | 21 +- docs/nbdkit-plugin.pod | 34 ++- docs/nbdkit-tls.pod | 8 +- filters/log/nbdkit-log-filter.pod | 2 +- filters/tlsdummy/nbdkit-tlsdummy-filter.pod | 72 ++++++ plugins/sh/nbdkit-sh-plugin.pod | 5 +- include/nbdkit-filter.h | 5 +- include/nbdkit-plugin.h | 1 + configure.ac | 2 + filters/tlsdummy/Makefile.am | 63 ++++++ server/internal.h | 3 +- server/backend.c | 6 +- server/filters.c | 10 +- server/nbdkit.syms | 1 + server/plugins.c | 14 +- server/public.c | 16 ++ plugins/sh/methods.c | 1 + filters/cow/cow.c | 2 +- filters/exitlast/exitlast.c | 2 +- filters/ext2/ext2.c | 2 +- filters/extentlist/extentlist.c | 3 +- filters/gzip/gzip.c | 2 +- filters/limit/limit.c | 2 +- filters/log/log.c | 18 +- filters/partition/partition.c | 2 +- filters/rate/rate.c | 4 +- filters/retry/retry.c | 2 +- filters/stats/stats.c | 2 +- filters/tar/tar.c | 2 +- filters/tlsdummy/tlsdummy.c | 235 ++++++++++++++++++++ filters/truncate/truncate.c | 2 +- filters/xz/xz.c | 2 +- tests/test-layers-filter.c | 4 +- TODO | 13 +- 34 files changed, 504 insertions(+), 59 deletions(-) create mode 100644 filters/tlsdummy/nbdkit-tlsdummy-filter.pod create mode 100644 filters/tlsdummy/Makefile.am create mode 100644 filters/tlsdummy/tlsdummy.c -- 2.28.0
Now that we can differentiate content based on export name, we also need to be able to differentiate content for a --tls=on server, since TLS is optional according to whether the client has authenticated. For internal code and filters, this means adding a new parameter; the sh plugin can do likewise. For plugins, we can't add a parameter until the V3 protocol, so in the meantime, we add nbdkit_is_tls(), even though it will be deprecated alongside nbdkit_export_name() when we do get to V3. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-filter.pod | 12 ++++++++---- docs/nbdkit-plugin.pod | 32 +++++++++++++++++++++++++++---- docs/nbdkit-tls.pod | 5 ++++- filters/log/nbdkit-log-filter.pod | 2 +- plugins/sh/nbdkit-sh-plugin.pod | 5 +++-- include/nbdkit-filter.h | 2 +- include/nbdkit-plugin.h | 1 + server/internal.h | 3 ++- server/backend.c | 6 +++--- server/filters.c | 6 ++++-- server/nbdkit.syms | 1 + server/plugins.c | 14 +++++++++----- server/public.c | 16 ++++++++++++++++ plugins/sh/methods.c | 1 + filters/cow/cow.c | 2 +- filters/exitlast/exitlast.c | 2 +- filters/ext2/ext2.c | 2 +- filters/gzip/gzip.c | 2 +- filters/limit/limit.c | 2 +- filters/log/log.c | 16 +++++++++------- filters/partition/partition.c | 2 +- filters/rate/rate.c | 2 +- filters/retry/retry.c | 2 +- filters/tar/tar.c | 2 +- filters/truncate/truncate.c | 2 +- filters/xz/xz.c | 2 +- tests/test-layers-filter.c | 2 +- TODO | 13 ++++++------- 28 files changed, 109 insertions(+), 50 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 12343dbf..b6ed5504 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -403,7 +403,7 @@ Returns a copy of the C<i>'th export. =head2 C<.open> void * (*open) (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname); + int readonly, const char *exportname, int is_tls); This is called when a new client connection is opened and can be used to allocate any per-connection data structures needed by the filter. @@ -420,8 +420,9 @@ error message and return C<NULL>. This callback is optional, but if provided, it must call C<next>, passing C<readonly> and C<exportname> possibly modified according to -how the filter plans to use the plugin. Typically, the filter passes -the same values as it received, or passes readonly=true to provide a +how the filter plans to use the plugin (C<is_tls> is not passed, +because a filter cannot modify it). Typically, the filter passes the +same values as it received, or passes readonly=true to provide a writable layer on top of a read-only backend. However, it is also acceptable to attempt write access to the plugin even if this filter is readonly, such as when a file system mounted read-only still @@ -430,7 +431,10 @@ 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. +it down to the next layer) it must take a copy. 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>. The filter should generally call C<next> as its first step, to allocate from the plugin outwards, so that C<.close> running from the diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 9341f282..6237b749 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -728,10 +728,18 @@ connection to be readonly (even if this flag is false) by returning false from the C<.can_write> callback. So if your plugin can only serve read-only, you can ignore this parameter. -This callback is called after the NBD handshake has completed, which -includes TLS authentication (if required). If the plugin defines a -C<.preconnect> callback, then it must be called and return with -success before C<.open> is called. +If the plugin wants to differentiate the content it served based on +client input, then this is the spot to use C<nbdkit_export_name()> to +determine which export the client requested. See also L</EXPORT NAME> +below. + +This callback is called after the NBD handshake has completed; if the +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. If there is an error, C<.open> should call C<nbdkit_error> with an error message and return C<NULL>. @@ -1466,6 +1474,22 @@ client data, be cautious when parsing it.> 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 +until after a client has completed TLS authentication. See +L<nbdkit-tls(1)>. + +=head2 C<nbdkit_is_tls> + + int nbdkit_is_tls (void); + +Return true if the client has completed TLS authentication, or false +if the connection is still plaintext. + +On error (such as calling this function outside of the context of +C<.open>), C<nbdkit_error> is called and the call returns C<-1>. + =head1 PEER NAME It is possible to get the address of the client when you are running diff --git a/docs/nbdkit-tls.pod b/docs/nbdkit-tls.pod index 628512a9..450e0850 100644 --- a/docs/nbdkit-tls.pod +++ b/docs/nbdkit-tls.pod @@ -22,7 +22,10 @@ connect, it will be rejected by the server (in other words, as if the server doesn't support TLS). I<--tls=on> means that the client may choose to connect either with or -without TLS. +without TLS. In this mode, a plugin may wish to serve different +content depending on whether the client has authenticated; the +function C<nbdkit_is_tls()> can be used during the C<.open> callback +for that purpose. Because I<--tls=on> is subject to downgrade attacks where a malicious proxy pretends not to support TLS in order to force either the client diff --git a/filters/log/nbdkit-log-filter.pod b/filters/log/nbdkit-log-filter.pod index 5f690457..721fc118 100644 --- a/filters/log/nbdkit-log-filter.pod +++ b/filters/log/nbdkit-log-filter.pod @@ -74,7 +74,7 @@ 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.080502 ...ListExports id=1 exports=[""] return=0 - 2020-08-06 02:07:23.080712 connection=1 Connect export='' 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.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 ... 2020-08-06 02:07:23.080927 connection=1 ...Read id=1 return=0 (Success) 2020-08-06 02:07:23.081255 connection=1 Disconnect transactions=1 diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod index 678116f2..07d90b57 100644 --- a/plugins/sh/nbdkit-sh-plugin.pod +++ b/plugins/sh/nbdkit-sh-plugin.pod @@ -320,11 +320,12 @@ as a name and no description. =item C<open> - /path/to/script open <readonly> <exportname> + /path/to/script open <readonly> <exportname> <tls> The C<readonly> parameter will be C<true> or C<false>. The C<exportname> parameter, if present, is the export name passed to the -server from the client. +server from the client. The C<tls> parameter, if present, will be +C<true> or C<false> depending on whether the client is using TLS. On success this should print the handle (any string) on stdout and exit with code C<0>. If the handle ends with a newline character then diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 08c6abc4..708a1b54 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -175,7 +175,7 @@ struct nbdkit_filter { struct nbdkit_exports *exports); void * (*open) (nbdkit_next_open *next, nbdkit_backend *nxdata, - int readonly, const char *exportname); + int readonly, const char *exportname, int is_tls); void (*close) (void *handle); int (*prepare) (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata, diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h index 86b8565d..e20391b8 100644 --- a/include/nbdkit-plugin.h +++ b/include/nbdkit-plugin.h @@ -146,6 +146,7 @@ struct nbdkit_plugin { extern void nbdkit_set_error (int err); extern const char *nbdkit_export_name (void); +extern int nbdkit_is_tls (void); #define NBDKIT_REGISTER_PLUGIN(plugin) \ NBDKIT_CXX_LANG_C \ diff --git a/server/internal.h b/server/internal.h index f84696ca..d043225a 100644 --- a/server/internal.h +++ b/server/internal.h @@ -367,7 +367,8 @@ struct backend { int (*preconnect) (struct backend *, int readonly); int (*list_exports) (struct backend *, int readonly, int default_only, struct nbdkit_exports *exports); - void *(*open) (struct backend *, int readonly, const char *exportname); + void *(*open) (struct backend *, int readonly, const char *exportname, + int is_tls); int (*prepare) (struct backend *, void *handle, int readonly); int (*finalize) (struct backend *, void *handle); void (*close) (struct backend *, void *handle); diff --git a/server/backend.c b/server/backend.c index 75ca53be..8f4fed9d 100644 --- a/server/backend.c +++ b/server/backend.c @@ -186,8 +186,8 @@ backend_open (struct backend *b, int readonly, const char *exportname) GET_CONN; struct handle *h = get_handle (conn, b->i); - controlpath_debug ("%s: open readonly=%d exportname=\"%s\"", - b->name, readonly, exportname); + controlpath_debug ("%s: open readonly=%d exportname=\"%s\" tls=%d", + b->name, readonly, exportname, conn->using_tls); assert (h->handle == NULL); assert ((h->state & HANDLE_OPEN) == 0); @@ -212,7 +212,7 @@ backend_open (struct backend *b, int readonly, const char *exportname) /* Most filters will call next_open first, resulting in * inner-to-outer ordering. */ - h->handle = b->open (b, readonly, exportname); + h->handle = b->open (b, readonly, exportname, conn->using_tls); controlpath_debug ("%s: open returned handle %p", b->name, h->handle); if (h->handle == NULL) { diff --git a/server/filters.c b/server/filters.c index 20518354..90a9a948 100644 --- a/server/filters.c +++ b/server/filters.c @@ -250,7 +250,8 @@ filter_list_exports (struct backend *b, int readonly, int default_only, } static void * -filter_open (struct backend *b, int readonly, const char *exportname) +filter_open (struct backend *b, int readonly, const char *exportname, + int is_tls) { struct backend_filter *f = container_of (b, struct backend_filter, backend); void *handle; @@ -259,7 +260,8 @@ filter_open (struct backend *b, int readonly, const char *exportname) * inner-to-outer ordering. */ if (f->filter.open) - handle = f->filter.open (backend_open, b->next, readonly, exportname); + handle = f->filter.open (backend_open, b->next, readonly, exportname, + is_tls); else if (backend_open (b->next, readonly, exportname) == -1) handle = NULL; else diff --git a/server/nbdkit.syms b/server/nbdkit.syms index 6cc6ed32..a67669b7 100644 --- a/server/nbdkit.syms +++ b/server/nbdkit.syms @@ -53,6 +53,7 @@ nbdkit_extents_free; nbdkit_extents_new; nbdkit_get_extent; + nbdkit_is_tls; nbdkit_nanosleep; nbdkit_parse_bool; nbdkit_parse_int8_t; diff --git a/server/plugins.c b/server/plugins.c index d4364cd2..bc57623a 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -292,7 +292,8 @@ plugin_list_exports (struct backend *b, int readonly, int default_only, } static void * -plugin_open (struct backend *b, int readonly, const char *exportname) +plugin_open (struct backend *b, int readonly, const char *exportname, + int is_tls) { GET_CONN; struct backend_plugin *p = container_of (b, struct backend_plugin, backend); @@ -302,11 +303,14 @@ plugin_open (struct backend *b, int readonly, const char *exportname) /* Save the exportname since the lifetime of the string passed in * here is likely to be brief. In addition this provides a place * for nbdkit_export_name to retrieve it if called from the plugin. + * While readonly and the export name can be altered in plugins, the + * tls mode cannot. * - * In API V3 we propose to pass the exportname as an extra parameter - * to the (new) plugin.open and deprecate nbdkit_export_name for V3 - * users. Even then we will still need to save it in the handle - * because of the lifetime issue. + * In API V3 we propose to pass the exportname and tls mode as extra + * parameters to the (new) plugin.open and deprecate + * nbdkit_export_name and nbdkit_is_tls for V3 users. Even then we + * will still need to save the export name in the handle because of + * the lifetime issue. */ if (conn->exportname == NULL) { conn->exportname = strdup (exportname); diff --git a/server/public.c b/server/public.c index c00a5cb6..f682d732 100644 --- a/server/public.c +++ b/server/public.c @@ -670,6 +670,22 @@ nbdkit_export_name (void) return conn->exportname; } +/* This function will be deprecated for API V3 users. The preferred + * approach will be to get the tls mode from .open(). + */ +int +nbdkit_is_tls (void) +{ + struct connection *conn = threadlocal_get_conn (); + + if (!conn) { + nbdkit_error ("no connection in this thread"); + return -1; + } + + return conn->using_tls; +} + int nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen) { diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c index 9f247524..c59198ca 100644 --- a/plugins/sh/methods.c +++ b/plugins/sh/methods.c @@ -341,6 +341,7 @@ sh_open (int readonly) { script, method, readonly ? "true" : "false", nbdkit_export_name () ? : "", + nbdkit_is_tls () ? "true" : "false", NULL }; struct sh_handle *h = malloc (sizeof *h); diff --git a/filters/cow/cow.c b/filters/cow/cow.c index 0faf2726..51ca64a4 100644 --- a/filters/cow/cow.c +++ b/filters/cow/cow.c @@ -94,7 +94,7 @@ cow_config (nbdkit_next_config *next, void *nxdata, static void * cow_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { /* Always pass readonly=1 to the underlying plugin. */ if (next (nxdata, 1, exportname) == -1) diff --git a/filters/exitlast/exitlast.c b/filters/exitlast/exitlast.c index 378cda75..4c879cc9 100644 --- a/filters/exitlast/exitlast.c +++ b/filters/exitlast/exitlast.c @@ -51,7 +51,7 @@ static _Atomic unsigned connections; static void * exitlast_open (nbdkit_next_open *next, nbdkit_backend *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { if (next (nxdata, readonly, exportname) == -1) return NULL; diff --git a/filters/ext2/ext2.c b/filters/ext2/ext2.c index 72b7ac9f..75ac2c4c 100644 --- a/filters/ext2/ext2.c +++ b/filters/ext2/ext2.c @@ -122,7 +122,7 @@ struct handle { /* Create the per-connection handle. */ static void * ext2_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct handle *h; diff --git a/filters/gzip/gzip.c b/filters/gzip/gzip.c index 8323b882..d92e00d9 100644 --- a/filters/gzip/gzip.c +++ b/filters/gzip/gzip.c @@ -75,7 +75,7 @@ gzip_thread_model (void) static void * gzip_open (nbdkit_next_open *next, nbdkit_backend *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { /* Always pass readonly=1 to the underlying plugin. */ if (next (nxdata, 1, exportname) == -1) diff --git a/filters/limit/limit.c b/filters/limit/limit.c index 7c4477eb..fb862df7 100644 --- a/filters/limit/limit.c +++ b/filters/limit/limit.c @@ -91,7 +91,7 @@ limit_preconnect (nbdkit_next_preconnect *next, nbdkit_backend *nxdata, static void * limit_open (nbdkit_next_open *next, nbdkit_backend *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { if (next (nxdata, readonly, exportname) == -1) return NULL; diff --git a/filters/log/log.c b/filters/log/log.c index c89f5001..f8da9ad8 100644 --- a/filters/log/log.c +++ b/filters/log/log.c @@ -135,6 +135,7 @@ struct handle { uint64_t connection; uint64_t id; char *exportname; + int tls; }; /* Compute the next id number on the current connection. */ @@ -283,7 +284,7 @@ log_list_exports (nbdkit_next_list_exports *next, void *nxdata, /* Open a connection. */ static void * log_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct handle *h; @@ -296,9 +297,9 @@ log_open (nbdkit_next_open *next, void *nxdata, return NULL; } - /* Save the exportname in the handle so we can display it in - * log_prepare. We must take a copy because this string has a short - * lifetime. + /* Save the exportname and tls state in the handle so we can display + * it in log_prepare. We must take a copy because this string has a + * short lifetime. */ h->exportname = strdup (exportname); if (h->exportname == NULL) { @@ -306,6 +307,7 @@ log_open (nbdkit_next_open *next, void *nxdata, free (h); return NULL; } + h->tls = is_tls; ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); h->connection = ++connections; @@ -343,9 +345,9 @@ log_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, e < 0 || c < 0 || Z < 0) return -1; - output (h, "Connect", 0, "export='%s' size=0x%" PRIx64 " write=%d flush=%d " - "rotational=%d trim=%d zero=%d fua=%d extents=%d cache=%d " - "fast_zero=%d", exportname, size, w, f, r, t, z, F, e, c, Z); + output (h, "Connect", 0, "export='%s' tls=%d size=0x%" PRIx64 " write=%d " + "flush=%d rotational=%d trim=%d zero=%d fua=%d extents=%d cache=%d " + "fast_zero=%d", exportname, h->tls, size, w, f, r, t, z, F, e, c, Z); return 0; } diff --git a/filters/partition/partition.c b/filters/partition/partition.c index c348664b..849f0cc7 100644 --- a/filters/partition/partition.c +++ b/filters/partition/partition.c @@ -88,7 +88,7 @@ struct handle { /* Open a connection. */ static void * partition_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct handle *h; diff --git a/filters/rate/rate.c b/filters/rate/rate.c index 6e99fcc7..32c47fdf 100644 --- a/filters/rate/rate.c +++ b/filters/rate/rate.c @@ -163,7 +163,7 @@ rate_get_ready (nbdkit_next_get_ready *next, void *nxdata) /* Create the per-connection handle. */ static void * rate_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct rate_handle *h; diff --git a/filters/retry/retry.c b/filters/retry/retry.c index be334c39..a2e57d77 100644 --- a/filters/retry/retry.c +++ b/filters/retry/retry.c @@ -113,7 +113,7 @@ struct retry_handle { static void * retry_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct retry_handle *h; diff --git a/filters/tar/tar.c b/filters/tar/tar.c index c04f09dd..ab041153 100644 --- a/filters/tar/tar.c +++ b/filters/tar/tar.c @@ -113,7 +113,7 @@ struct handle { static void * tar_open (nbdkit_next_open *next, nbdkit_backend *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct handle *h; diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c index ee384871..00b5d8ce 100644 --- a/filters/truncate/truncate.c +++ b/filters/truncate/truncate.c @@ -125,7 +125,7 @@ struct handle { /* Open a connection. */ static void * truncate_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct handle *h; diff --git a/filters/xz/xz.c b/filters/xz/xz.c index 9154dbeb..26cfa959 100644 --- a/filters/xz/xz.c +++ b/filters/xz/xz.c @@ -90,7 +90,7 @@ struct xz_handle { /* Create the per-connection handle. */ static void * xz_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct xz_handle *h; diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c index 397af575..5c5b3f0f 100644 --- a/tests/test-layers-filter.c +++ b/tests/test-layers-filter.c @@ -117,7 +117,7 @@ test_layers_filter_list_exports (nbdkit_next_list_exports *next, void *nxdata, static void * test_layers_filter_open (nbdkit_next_open *next, void *nxdata, - int readonly, const char *exportname) + int readonly, const char *exportname, int is_tls) { struct handle *h = malloc (sizeof *h); diff --git a/TODO b/TODO index 46f92443..262327fb 100644 --- a/TODO +++ b/TODO @@ -62,11 +62,11 @@ General ideas for improvements continue to keep their non-standard handshake while utilizing nbdkit to prototype new behaviors in serving the kernel. -* When using --tls=on (encryption is supported but not required), - plugins need to be able to differentiate which content they serve to - plaintext clients vs. secure clients. This may require an - alteration of the signature for .list_exports and .open, or a new - utility function nbdkit_tls_mode() similar to nbdkit_export_name(). +* 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. * Background thread for filters. Some filters (readahead, cache and proposed scan filter - see below) could be more effective if they @@ -308,7 +308,6 @@ using ‘#define NBDKIT_API_VERSION <version>’. and filters, and the type of the values, and both check and parse them for the plugin. -* Modify open() API so it takes an export name parameter, and maybe a - tls mode. +* Modify open() API so it takes an export name and tls parameter. * Change config_help from a variable to a function. -- 2.28.0
Largely a mechanical patch, and the changes at the beginning adding the new API and updating documentation were fine, so ACK. 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
The next patch wants to add a filter that will prevent DoS attacks from a plaintext client; to be successful, the filter must guarantee that nbdkit did not settle on SERIALIZE_CONNECTIONS. The easiest way to solve this is to expose the final thread model to .get_ready, which is after the point where .config_complete may have altered it, and before any connections are permitted. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-filter.pod | 9 ++++++++- include/nbdkit-filter.h | 3 ++- server/filters.c | 4 ++-- filters/extentlist/extentlist.c | 3 ++- filters/log/log.c | 2 +- filters/rate/rate.c | 2 +- filters/stats/stats.c | 2 +- tests/test-layers-filter.c | 2 +- 8 files changed, 18 insertions(+), 9 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index b6ed5504..32db0938 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -298,11 +298,18 @@ with an error message and return C<-1>. =head2 C<.get_ready> - int (*get_ready) (nbdkit_next_get_ready *next, void *nxdata); + int (*get_ready) (nbdkit_next_get_ready *next, void *nxdata, + int thread_model); This intercepts the plugin C<.get_ready> method and can be used by the filter to get ready to serve requests. +The C<thread_model> parameter informs the filter about the final +thread model chosen by nbdkit after considering the results of +C<.thread_model> of all filters in the chain after C<.config>. This +does not need to be passed on to C<next>, as the model can no longer +be altered at this point. + If there is an error, C<.get_ready> should call C<nbdkit_error> with an error message and return C<-1>. diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 708a1b54..6aba1aec 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -166,7 +166,8 @@ struct nbdkit_filter { nbdkit_backend *nxdata); const char *config_help; int (*thread_model) (void); - int (*get_ready) (nbdkit_next_get_ready *next, nbdkit_backend *nxdata); + int (*get_ready) (nbdkit_next_get_ready *next, nbdkit_backend *nxdata, + int thread_model); int (*after_fork) (nbdkit_next_after_fork *next, nbdkit_backend *nxdata); int (*preconnect) (nbdkit_next_preconnect *next, nbdkit_backend *nxdata, int readonly); diff --git a/server/filters.c b/server/filters.c index 90a9a948..0cfae344 100644 --- a/server/filters.c +++ b/server/filters.c @@ -183,10 +183,10 @@ filter_get_ready (struct backend *b) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - debug ("%s: get_ready", b->name); + debug ("%s: get_ready thread_model=%d", b->name, thread_model); if (f->filter.get_ready) { - if (f->filter.get_ready (next_get_ready, b->next) == -1) + if (f->filter.get_ready (next_get_ready, b->next, thread_model) == -1) exit (EXIT_FAILURE); } else diff --git a/filters/extentlist/extentlist.c b/filters/extentlist/extentlist.c index 3005b790..dfb5e808 100644 --- a/filters/extentlist/extentlist.c +++ b/filters/extentlist/extentlist.c @@ -260,7 +260,8 @@ parse_extentlist (void) } static int -extentlist_get_ready (nbdkit_next_get_ready *next, void *nxdata) +extentlist_get_ready (nbdkit_next_get_ready *next, void *nxdata, + int thread_model) { parse_extentlist (); diff --git a/filters/log/log.c b/filters/log/log.c index f8da9ad8..6a3a9b14 100644 --- a/filters/log/log.c +++ b/filters/log/log.c @@ -100,7 +100,7 @@ log_config_complete (nbdkit_next_config_complete *next, void *nxdata) /* Open the logfile. */ static int -log_get_ready (nbdkit_next_get_ready *next, void *nxdata) +log_get_ready (nbdkit_next_get_ready *next, void *nxdata, int thread_model) { int fd; diff --git a/filters/rate/rate.c b/filters/rate/rate.c index 32c47fdf..325f5657 100644 --- a/filters/rate/rate.c +++ b/filters/rate/rate.c @@ -145,7 +145,7 @@ rate_config (nbdkit_next_config *next, void *nxdata, } static int -rate_get_ready (nbdkit_next_get_ready *next, void *nxdata) +rate_get_ready (nbdkit_next_get_ready *next, void *nxdata, int thread_model) { /* Initialize the global buckets. */ bucket_init (&read_bucket, rate, BUCKET_CAPACITY); diff --git a/filters/stats/stats.c b/filters/stats/stats.c index 688078ec..687dd05b 100644 --- a/filters/stats/stats.c +++ b/filters/stats/stats.c @@ -210,7 +210,7 @@ stats_config_complete (nbdkit_next_config_complete *next, void *nxdata) } static int -stats_get_ready (nbdkit_next_get_ready *next, void *nxdata) +stats_get_ready (nbdkit_next_get_ready *next, void *nxdata, int thread_model) { int fd; diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c index 5c5b3f0f..3f295588 100644 --- a/tests/test-layers-filter.c +++ b/tests/test-layers-filter.c @@ -84,7 +84,7 @@ test_layers_filter_config_complete (nbdkit_next_config_complete *next, static int test_layers_filter_get_ready (nbdkit_next_get_ready *next, - void *nxdata) + void *nxdata, int thread_model) { DEBUG_FUNCTION; return next (nxdata); -- 2.28.0
On Fri, Aug 07, 2020 at 05:00:52PM -0500, Eric Blake wrote:
The next patch wants to add a filter that will prevent DoS attacks from a plaintext client; to be successful, the filter must guarantee that nbdkit did not settle on SERIALIZE_CONNECTIONS. The easiest way to solve this is to expose the final thread model to .get_ready, which is after the point where .config_complete may have altered it, and before any connections are permitted.
Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-filter.pod | 9 ++++++++- include/nbdkit-filter.h | 3 ++- server/filters.c | 4 ++-- filters/extentlist/extentlist.c | 3 ++- filters/log/log.c | 2 +- filters/rate/rate.c | 2 +- filters/stats/stats.c | 2 +- tests/test-layers-filter.c | 2 +- 8 files changed, 18 insertions(+), 9 deletions(-)
This is fine. Is this something that we would also with to add to plugin->get_ready in V3 API? If so it would be a good idea to add this to TODO. ACK Rich.
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index b6ed5504..32db0938 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -298,11 +298,18 @@ with an error message and return C<-1>.
=head2 C<.get_ready>
- int (*get_ready) (nbdkit_next_get_ready *next, void *nxdata); + int (*get_ready) (nbdkit_next_get_ready *next, void *nxdata, + int thread_model);
This intercepts the plugin C<.get_ready> method and can be used by the filter to get ready to serve requests.
+The C<thread_model> parameter informs the filter about the final +thread model chosen by nbdkit after considering the results of +C<.thread_model> of all filters in the chain after C<.config>. This +does not need to be passed on to C<next>, as the model can no longer +be altered at this point. + If there is an error, C<.get_ready> should call C<nbdkit_error> with an error message and return C<-1>.
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 708a1b54..6aba1aec 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -166,7 +166,8 @@ struct nbdkit_filter { nbdkit_backend *nxdata); const char *config_help; int (*thread_model) (void); - int (*get_ready) (nbdkit_next_get_ready *next, nbdkit_backend *nxdata); + int (*get_ready) (nbdkit_next_get_ready *next, nbdkit_backend *nxdata, + int thread_model); int (*after_fork) (nbdkit_next_after_fork *next, nbdkit_backend *nxdata); int (*preconnect) (nbdkit_next_preconnect *next, nbdkit_backend *nxdata, int readonly); diff --git a/server/filters.c b/server/filters.c index 90a9a948..0cfae344 100644 --- a/server/filters.c +++ b/server/filters.c @@ -183,10 +183,10 @@ filter_get_ready (struct backend *b) { struct backend_filter *f = container_of (b, struct backend_filter, backend);
- debug ("%s: get_ready", b->name); + debug ("%s: get_ready thread_model=%d", b->name, thread_model);
if (f->filter.get_ready) { - if (f->filter.get_ready (next_get_ready, b->next) == -1) + if (f->filter.get_ready (next_get_ready, b->next, thread_model) == -1) exit (EXIT_FAILURE); } else diff --git a/filters/extentlist/extentlist.c b/filters/extentlist/extentlist.c index 3005b790..dfb5e808 100644 --- a/filters/extentlist/extentlist.c +++ b/filters/extentlist/extentlist.c @@ -260,7 +260,8 @@ parse_extentlist (void) }
static int -extentlist_get_ready (nbdkit_next_get_ready *next, void *nxdata) +extentlist_get_ready (nbdkit_next_get_ready *next, void *nxdata, + int thread_model) { parse_extentlist ();
diff --git a/filters/log/log.c b/filters/log/log.c index f8da9ad8..6a3a9b14 100644 --- a/filters/log/log.c +++ b/filters/log/log.c @@ -100,7 +100,7 @@ log_config_complete (nbdkit_next_config_complete *next, void *nxdata)
/* Open the logfile. */ static int -log_get_ready (nbdkit_next_get_ready *next, void *nxdata) +log_get_ready (nbdkit_next_get_ready *next, void *nxdata, int thread_model) { int fd;
diff --git a/filters/rate/rate.c b/filters/rate/rate.c index 32c47fdf..325f5657 100644 --- a/filters/rate/rate.c +++ b/filters/rate/rate.c @@ -145,7 +145,7 @@ rate_config (nbdkit_next_config *next, void *nxdata, }
static int -rate_get_ready (nbdkit_next_get_ready *next, void *nxdata) +rate_get_ready (nbdkit_next_get_ready *next, void *nxdata, int thread_model) { /* Initialize the global buckets. */ bucket_init (&read_bucket, rate, BUCKET_CAPACITY); diff --git a/filters/stats/stats.c b/filters/stats/stats.c index 688078ec..687dd05b 100644 --- a/filters/stats/stats.c +++ b/filters/stats/stats.c @@ -210,7 +210,7 @@ stats_config_complete (nbdkit_next_config_complete *next, void *nxdata) }
static int -stats_get_ready (nbdkit_next_get_ready *next, void *nxdata) +stats_get_ready (nbdkit_next_get_ready *next, void *nxdata, int thread_model) { int fd;
diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c index 5c5b3f0f..3f295588 100644 --- a/tests/test-layers-filter.c +++ b/tests/test-layers-filter.c @@ -84,7 +84,7 @@ test_layers_filter_config_complete (nbdkit_next_config_complete *next,
static int test_layers_filter_get_ready (nbdkit_next_get_ready *next, - void *nxdata) + void *nxdata, int thread_model) { DEBUG_FUNCTION; return next (nxdata); -- 2.28.0
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On 8/10/20 8:01 AM, Richard W.M. Jones wrote:
On Fri, Aug 07, 2020 at 05:00:52PM -0500, Eric Blake wrote:
The next patch wants to add a filter that will prevent DoS attacks from a plaintext client; to be successful, the filter must guarantee that nbdkit did not settle on SERIALIZE_CONNECTIONS. The easiest way to solve this is to expose the final thread model to .get_ready, which is after the point where .config_complete may have altered it, and before any connections are permitted.
Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-filter.pod | 9 ++++++++- include/nbdkit-filter.h | 3 ++- server/filters.c | 4 ++-- filters/extentlist/extentlist.c | 3 ++- filters/log/log.c | 2 +- filters/rate/rate.c | 2 +- filters/stats/stats.c | 2 +- tests/test-layers-filter.c | 2 +- 8 files changed, 18 insertions(+), 9 deletions(-)
This is fine. Is this something that we would also with to add to plugin->get_ready in V3 API? If so it would be a good idea to add this to TODO.
We might as well, so I'll tweak TODO. But at the moment, I'm having a hard time coming up with a justification for why any existing v2 plugin would need to know this information, so without a client, I'm reluctant to introduce a plugin-only nbdkit_final_thread_model() just to be deprecated when v3 comes out. Oh, one other oddity: I've been thinking about adding a fifth threading model (SERIALIZE_RETIREMENT), where commands can be serviced in parallel on a single connection, but replies to the client are reordered to match the client's submission order; for filters, exposing a new threading model through .get_ready is not a problem (all filters are in tree), but for plugins, such an action would mean that the plugin may get a thread model selected at runtime that was unknown to the plugin at compile time. If the plugin is trying to make decisions on how much internal locking it needs to create, those decisions become more awkward when having to deal with an unknown thread model (then again, robust code should always be written to assume as much parallelism as possible, and only optimize for less locking on known models that are more serialized). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Aug 10, 2020 at 08:13:31AM -0500, Eric Blake wrote:
On 8/10/20 8:01 AM, Richard W.M. Jones wrote:
On Fri, Aug 07, 2020 at 05:00:52PM -0500, Eric Blake wrote:
The next patch wants to add a filter that will prevent DoS attacks from a plaintext client; to be successful, the filter must guarantee that nbdkit did not settle on SERIALIZE_CONNECTIONS. The easiest way to solve this is to expose the final thread model to .get_ready, which is after the point where .config_complete may have altered it, and before any connections are permitted.
Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-filter.pod | 9 ++++++++- include/nbdkit-filter.h | 3 ++- server/filters.c | 4 ++-- filters/extentlist/extentlist.c | 3 ++- filters/log/log.c | 2 +- filters/rate/rate.c | 2 +- filters/stats/stats.c | 2 +- tests/test-layers-filter.c | 2 +- 8 files changed, 18 insertions(+), 9 deletions(-)
This is fine. Is this something that we would also with to add to plugin->get_ready in V3 API? If so it would be a good idea to add this to TODO.
We might as well, so I'll tweak TODO. But at the moment, I'm having a hard time coming up with a justification for why any existing v2 plugin would need to know this information, so without a client, I'm reluctant to introduce a plugin-only nbdkit_final_thread_model() just to be deprecated when v3 comes out.
It's certainly hard to imagine why a plugin would worry about things being more serialized than they requested, but OTOH it does make the filter and plugin APIs consistent (w.r.t. the informational parameters they both receive) which is nice.
Oh, one other oddity: I've been thinking about adding a fifth threading model (SERIALIZE_RETIREMENT), where commands can be serviced in parallel on a single connection, but replies to the client are reordered to match the client's submission order; for filters, exposing a new threading model through .get_ready is not a problem (all filters are in tree), but for plugins, such an action would mean that the plugin may get a thread model selected at runtime that was unknown to the plugin at compile time. If the plugin is trying to make decisions on how much internal locking it needs to create, those decisions become more awkward when having to deal with an unknown thread model (then again, robust code should always be written to assume as much parallelism as possible, and only optimize for less locking on known models that are more serialized).
Sure; are there really clients which have this restriction? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 8/10/20 8:45 AM, Richard W.M. Jones wrote:
Oh, one other oddity: I've been thinking about adding a fifth threading model (SERIALIZE_RETIREMENT), where commands can be serviced in parallel on a single connection, but replies to the client are reordered to match the client's submission order; for filters, exposing a new threading model through .get_ready is not a problem (all filters are in tree), but for plugins, such an action would mean that the plugin may get a thread model selected at runtime that was unknown to the plugin at compile time. If the plugin is trying to make decisions on how much internal locking it needs to create, those decisions become more awkward when having to deal with an unknown thread model (then again, robust code should always be written to assume as much parallelism as possible, and only optimize for less locking on known models that are more serialized).
Sure; are there really clients which have this restriction?
I'm not sure if there are actual clients that depend on this, but it's easy enough with libnbd to write a client that can batch up multiple AIO requests, and still be confused if the replies aren't in the same order as what got sent. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Take advantage of the fact that we can now detect the type of client during --tls=on in order to provide safe dummy content for plaintext clients without having to rewrite plugins to do so. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-plugin.pod | 4 +- docs/nbdkit-tls.pod | 5 +- filters/tlsdummy/nbdkit-tlsdummy-filter.pod | 72 ++++++ configure.ac | 2 + filters/tlsdummy/Makefile.am | 63 ++++++ filters/tlsdummy/tlsdummy.c | 235 ++++++++++++++++++++ 6 files changed, 379 insertions(+), 2 deletions(-) create mode 100644 filters/tlsdummy/nbdkit-tlsdummy-filter.pod create mode 100644 filters/tlsdummy/Makefile.am create mode 100644 filters/tlsdummy/tlsdummy.c diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 6237b749..43b0f6f9 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -1478,7 +1478,9 @@ On error, C<nbdkit_error> is called and the call returns C<NULL>. A server may use C<nbdkit_is_tls> to limit which export names work until after a client has completed TLS authentication. See -L<nbdkit-tls(1)>. +L<nbdkit-tls(1)>. It is also possible to use +L<nbdkit-tlsdummy-filter(1)> to automatically ensure that the plugin +is only used with authentication. =head2 C<nbdkit_is_tls> diff --git a/docs/nbdkit-tls.pod b/docs/nbdkit-tls.pod index 450e0850..fd78c21b 100644 --- a/docs/nbdkit-tls.pod +++ b/docs/nbdkit-tls.pod @@ -25,7 +25,9 @@ I<--tls=on> means that the client may choose to connect either with or without TLS. In this mode, a plugin may wish to serve different content depending on whether the client has authenticated; the function C<nbdkit_is_tls()> can be used during the C<.open> callback -for that purpose. +for that purpose. Or, you can opt to use L<nbdkit-tlsdummy-filter(1)> +to provide safe dummy content to plaintext connections, saving the +plugin content only for secure connections. Because I<--tls=on> is subject to downgrade attacks where a malicious proxy pretends not to support TLS in order to force either the client @@ -275,6 +277,7 @@ More information can be found in L<gnutls_priority_init(3)>. =head1 SEE ALSO L<nbdkit(1)>, +L<nbdkit-tlsdummy-filter(1)>, L<gnutls_priority_init(3)>, L<psktool(1)>, L<https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md>, diff --git a/filters/tlsdummy/nbdkit-tlsdummy-filter.pod b/filters/tlsdummy/nbdkit-tlsdummy-filter.pod new file mode 100644 index 00000000..f8eef69f --- /dev/null +++ b/filters/tlsdummy/nbdkit-tlsdummy-filter.pod @@ -0,0 +1,72 @@ +=head1 NAME + +nbdkit-tlsdummy-filter - nbdkit TLS protection filter + +=head1 SYNOPSIS + + nbdkit --tls=on --filter=tlsdummy plugin [plugin-args...] [tlsreadme=MESSAGE] + +=head1 DESCRIPTION + +C<nbdkit-tlsdummy-filter> is designed to be used when offering a +connection that allows but does not require TLS from clients, in order +to offer safe alternative content to plaintext clients, only exposing +the underlying plugin to authenticated users. This may provide a +nicer failure mode for plaintext clients than the harsher C<nbdkit +--tls=require>. + +When this filter detects a plaintext connection, it ignores the +client's export name, and provides a single read-only export with 512 +bytes of data and content that defaults to the message "This NBD +server requires TLS authentication before it will serve useful data." + +When using this filter, it is recommended to place this filter first +in the command line, to reduce the chance that any work done by +C<.open> in later filters or the plugin can be exploited by plaintext +connections as a denial of service attack to starve further +authenticated connections. Note that this plugin will fail to load if +the plugin requests the C<SERIALIZE_CONNECTIONS> thread model, as a +plaintext client holding its connection open indefinitely would be +such a starvation. + +=head1 PARAMETERS + +=over 4 + +=item B<tlsreadme=>MESSAGE + +This optional parameter can be used to use C<MESSAGE> as the contents +of the dummy export exposed to plaintext clients, using trailing NUL +bytes to round the size up to 512 bytes. + +=back + +=head1 FILES + +=over 4 + +=item F<$filterdir/nbdkit-tlsdummy-filter.so> + +The filter. + +Use C<nbdkit --dump-config> to find the location of C<$filterdir>. + +=back + +=head1 VERSION + +C<nbdkit-tlsdummy-filter> first appeared in nbdkit 1.22. + +=head1 SEE ALSO + +L<nbdkit(1)>, +L<nbdkit-tls(1)>, +L<nbdkit-filter(3)>. + +=head1 AUTHORS + +Eric Blake + +=head1 COPYRIGHT + +Copyright (C) 2020 Red Hat Inc. diff --git a/configure.ac b/configure.ac index 28f4a3cd..34730000 100644 --- a/configure.ac +++ b/configure.ac @@ -124,6 +124,7 @@ filters="\ stats \ swab \ tar \ + tlsdummy \ truncate \ xz \ " @@ -1165,6 +1166,7 @@ AC_CONFIG_FILES([Makefile filters/stats/Makefile filters/swab/Makefile filters/tar/Makefile + filters/tlsdummy/Makefile filters/truncate/Makefile filters/xz/Makefile fuzzing/Makefile diff --git a/filters/tlsdummy/Makefile.am b/filters/tlsdummy/Makefile.am new file mode 100644 index 00000000..a1577511 --- /dev/null +++ b/filters/tlsdummy/Makefile.am @@ -0,0 +1,63 @@ +# 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 $(top_srcdir)/common-rules.mk + +EXTRA_DIST = nbdkit-tlsdummy-filter.pod + +filter_LTLIBRARIES = nbdkit-tlsdummy-filter.la + +nbdkit_tlsdummy_filter_la_SOURCES = \ + tlsdummy.c \ + $(top_srcdir)/include/nbdkit-filter.h \ + $(NULL) + +nbdkit_tlsdummy_filter_la_CPPFLAGS = \ + -I$(top_srcdir)/include \ + -I$(top_srcdir)/common/include \ + $(NULL) +nbdkit_tlsdummy_filter_la_CFLAGS = $(WARNINGS_CFLAGS) +nbdkit_tlsdummy_filter_la_LDFLAGS = \ + -module -avoid-version -shared $(SHARED_LDFLAGS) \ + -Wl,--version-script=$(top_srcdir)/filters/filters.syms \ + $(NULL) + +if HAVE_POD + +man_MANS = nbdkit-tlsdummy-filter.1 +CLEANFILES += $(man_MANS) + +nbdkit-tlsdummy-filter.1: nbdkit-tlsdummy-filter.pod + $(PODWRAPPER) --section=1 --man $@ \ + --html $(top_builddir)/html/$@.html \ + $< + +endif HAVE_POD diff --git a/filters/tlsdummy/tlsdummy.c b/filters/tlsdummy/tlsdummy.c new file mode 100644 index 00000000..6e1a7414 --- /dev/null +++ b/filters/tlsdummy/tlsdummy.c @@ -0,0 +1,235 @@ +/* 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 <string.h> +#include <assert.h> + +#include <nbdkit-filter.h> + +/* Needed to shut up newer gcc about use of strncpy on our message buffer */ +#if __GNUC__ >= 8 +#define NONSTRING __attribute__ ((nonstring)) +#else +#define NONSTRING +#endif + +static char message[512] NONSTRING = "This NBD server requires TLS " + "authentication before it will serve useful data.\n"; + +/* Called for each key=value passed on the command line. */ +static int +tlsdummy_config (nbdkit_next_config *next, void *nxdata, + const char *key, const char *value) +{ + if (strcmp (key, "tlsreadme") == 0) { + strncpy (message, value, sizeof message); /* Yes, we really mean strncpy */ + return 0; + } + return next (nxdata, key, value); +} + +#define tlsdummy_config_help \ + "tlsreadme=<MESSAGE> Alternative contents for the plaintext dummy export.\n" + +int +tlsdummy_get_ready (nbdkit_next_get_ready *next, void *nxdata, + int thread_model) +{ + if (thread_model == NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) { + nbdkit_error ("the tlsdummy filter requires parallel connection support"); + return -1; + } + return next (nxdata); +} + +/* TODO: init_exports needs is_tls parameter */ + +/* 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 + * complex handle. + */ +#define NOT_TLS (handle == &message) + +static void * +tlsdummy_open (nbdkit_next_open *next, void *nxdata, int readonly, + const char *exportname, int is_tls) +{ + /* Bummer - we really do NOT want to call next() when insecure, + * because we don't know how long it will take. But that requires a + * fix to nbdkit; right now, it asserts if we skip this :( + */ + if (next (nxdata, readonly, exportname) == -1) + return NULL; + if (!is_tls) + return &message; /* See NOT_TLS for this choice of handle */ + return NBDKIT_HANDLE_NOT_NEEDED; +} + +/* When insecure, override ALL plugin .can_FOO to avoid an information leak */ + +static int64_t +tlsdummy_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return sizeof message; + return next_ops->get_size (nxdata); +} + +static int +tlsdummy_can_write (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return 0; + return next_ops->can_write (nxdata); +} + +static int +tlsdummy_can_flush (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return 0; + return next_ops->can_flush (nxdata); +} + +static int +tlsdummy_is_rotational (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return 0; + return next_ops->is_rotational (nxdata); +} + +static int +tlsdummy_can_trim (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return 0; + return next_ops->can_trim (nxdata); +} + +static int +tlsdummy_can_zero (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return NBDKIT_ZERO_NONE; + return next_ops->can_zero (nxdata); +} + +static int +tlsdummy_can_fast_zero (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return 0; + return next_ops->can_fast_zero (nxdata); +} + +static int +tlsdummy_can_extents (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return 0; + return next_ops->can_extents (nxdata); +} + +static int +tlsdummy_can_fua (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return NBDKIT_FUA_NONE; + return next_ops->can_fua (nxdata); +} + +static int +tlsdummy_can_multi_conn (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return 0; + return next_ops->can_multi_conn (nxdata); +} + +static int +tlsdummy_can_cache (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (NOT_TLS) + return NBDKIT_CACHE_NONE; + return next_ops->can_cache (nxdata); +} + +static int +tlsdummy_pread (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, void *b, uint32_t count, uint64_t offs, + uint32_t flags, int *err) +{ + if (NOT_TLS) { + memcpy (b, message + offs, count); + return 0; + } + return next_ops->pread (nxdata, b, count, offs, flags, err); +} + +static struct nbdkit_filter filter = { + .name = "tlsdummy", + .longname = "nbdkit tlsdummy filter", + .config = tlsdummy_config, + .config_help = tlsdummy_config_help, + .get_ready = tlsdummy_get_ready, + /* XXX .init_exports needs is_tls parameter */ + .open = tlsdummy_open, + .get_size = tlsdummy_get_size, + .can_write = tlsdummy_can_write, + .can_flush = tlsdummy_can_flush, + .is_rotational = tlsdummy_is_rotational, + .can_trim = tlsdummy_can_trim, + .can_zero = tlsdummy_can_zero, + .can_fast_zero = tlsdummy_can_fast_zero, + .can_extents = tlsdummy_can_extents, + .can_fua = tlsdummy_can_fua, + .can_multi_conn = tlsdummy_can_multi_conn, + .can_cache = tlsdummy_can_cache, + .pread = tlsdummy_pread, +}; + +NBDKIT_REGISTER_FILTER(filter) -- 2.28.0
On 8/7/20 5:00 PM, Eric Blake wrote:
Take advantage of the fact that we can now detect the type of client during --tls=on in order to provide safe dummy content for plaintext clients without having to rewrite plugins to do so.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
I got a test working (although it still shows that we are bit awkward until nbdkit makes it easier for filters to skip calling next_open when it wants to) diff --git c/tests/Makefile.am i/tests/Makefile.am index b5ef96a7..dd756723 100644 --- c/tests/Makefile.am +++ i/tests/Makefile.am @@ -1501,6 +1501,10 @@ EXTRA_DIST += \ test-truncate-extents.sh \ $(NULL) +# tlsdummy filter test. +TESTS += test-tlsdummy.sh +EXTRA_DIST += test-tlsdummy.sh + # xz filter test. LIBGUESTFS_TESTS += test-xz diff --git c/tests/test-tlsdummy.sh i/tests/test-tlsdummy.sh new file mode 100755 index 00000000..40c29602 --- /dev/null +++ i/tests/test-tlsdummy.sh @@ -0,0 +1,98 @@ +#!/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 nbdsh -c 'exit (not h.supports_tls ())' + +# Does the nbdkit binary support TLS? +if ! nbdkit --dump-config | grep -sq tls=yes; then + echo "$0: nbdkit built without TLS support" + exit 77 +fi + +# Did we create the PSK keys file? +# Probably 'certtool' is missing. +if [ ! -s keys.psk ]; then + echo "$0: PSK keys file was not created by the test harness" + exit 77 +fi + +export sock=`mktemp -u` +pid="tlsdummy.pid" + +files="$sock $pid" +rm -f $files +cleanup_fn rm -f $files + +# Run dual-mode server +start_nbdkit -P $pid -U $sock --tls=on --tls-psk=keys.psk \ + --filter=tlsdummy sh - tlsreadme=$'dummy\n' <<\EOF +if test "$2" = insecure; then + echo 'EINVAL insecure handle' 2>&1; exit 1 +fi +case $1 in + list_exports) echo NAMES; echo hello; echo world ;; + open) if test "$4" != true; then + # nbdkit needs a fix to let tlsdummy skip .open when insecure... + # echo 'EINVAL unexpected tls mode' 2>&1; exit 1 + echo insecure; exit 0 + fi + echo $3 ;; + get_size) echo 6 ;; + pread) echo "$2" | dd skip=$4 count=$3 iflag=skip_bytes,count_bytes ;; + *) exit 2; +esac +EOF + +# Plaintext client sees only dummy volume +nbdsh -c ' +import os +h.set_export_name ("hello") +h.connect_unix (os.environ["sock"]) +assert h.get_size () == 512 +assert h.pread (5, 0) == b"dummy" +' + +# Encrypted client sees desired volumes +nbdsh -c ' +import os +h.set_export_name ("hello") +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.get_size () == 6 +assert h.pread (5, 0) == b"hello" +' -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Sure, ACK Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On 8/7/20 5:00 PM, Eric Blake wrote:
Take advantage of the fact that we can now detect the type of client during --tls=on in order to provide safe dummy content for plaintext clients without having to rewrite plugins to do so.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
After playing with this a bit more, I'm thinking: nbdkit --tls=on --filter=tls-fallback PLUGIN ... reads better than tlsdummy. Renaming the filter is not too difficult of a search-and-replace, so I'll go ahead and make that change before pushing this.
docs/nbdkit-plugin.pod | 4 +- docs/nbdkit-tls.pod | 5 +- filters/tlsdummy/nbdkit-tlsdummy-filter.pod | 72 ++++++ configure.ac | 2 + filters/tlsdummy/Makefile.am | 63 ++++++ filters/tlsdummy/tlsdummy.c | 235 ++++++++++++++++++++ 6 files changed, 379 insertions(+), 2 deletions(-) create mode 100644 filters/tlsdummy/nbdkit-tlsdummy-filter.pod create mode 100644 filters/tlsdummy/Makefile.am create mode 100644 filters/tlsdummy/tlsdummy.c
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones