[Libguestfs] [nbdkit PATCH 1/3] server: Implement nbdkit_is_tls for use during .open
Eric Blake
eblake at redhat.com
Fri Aug 7 22:00:51 UTC 2020
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 at 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
More information about the Libguestfs
mailing list