On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
Add an extra parameter to nbd_connect_uri to control what URIs are permitted, in case the caller wants to pass in user-controlled URIs but have some control over who/what/how the connection happens. For example:
nbd_connect_uri (nbd, "nbd://localhost", LIBNBD_CONNECT_URI_REQUIRE_TLS) => error: URI must specify an encrypted connection: Permission denied
This obviously breaks the existing C API.
Alternative: we could leave nbd_connect_uri() alone, and make it forward to a new API nbd_connect_uri_flags(, default_flags).
+++ b/docs/libnbd.pod @@ -364,7 +364,7 @@ when it is available.
To connect to a URI via the high level API, use:
- nbd_connect_uri (nbd, "nbd://example.com/"); + nbd_connect_uri (nbd, "nbd://example.com/", LIBNBD_CONNECT_URI_ALL);
As written later in this patch, this change in the docs and example code implies the use of the REQUIRE_TLS flag. Is that intentional that passing all flags forbids the use of non-encrypted connections?
+++ b/generator/generator @@ -939,7 +939,17 @@ let cmd_flags = { "REQ_ONE", 1 lsl 3; ] } -let all_flags = [ cmd_flags ] +let connect_uri_allow_flags = { + flag_prefix = "CONNECT_URI"; + all_flags_bitmask = true; + flags = [ + "ALLOW_TCP", 1 lsl 0; + "ALLOW_UNIX", 1 lsl 1; + "ALLOW_TLS", 1 lsl 2; + "REQUIRE_TLS", 1 lsl 3; + ]
The REQUIRE_TLS flag is worth having, but putting it in the bitmask for all flags makes for some odd semantics.
+} +let all_flags = [ cmd_flags; connect_uri_allow_flags ]
(* Calls. * @@ -1225,7 +1235,8 @@ C<\"qemu:dirty-bitmap:...\"> for qemu-nbd
"connect_uri", { default_call with - args = [ String "uri" ]; ret = RErr; + args = [ String "uri"; Flags ("allow", connect_uri_allow_flags) ]; + ret = RErr; permitted_states = [ Created ]; shortdesc = "connect to NBD URI"; longdesc = "\ @@ -1238,7 +1249,37 @@ the connection has been made. This call will fail if libnbd was not compiled with libxml2; you can test whether this is the case with C<nbd_supports_uri>. Support for URIs that require TLS will fail if libnbd was not compiled with -gnutls; you can test whether this is the case with C<nbd_supports_tls>."; +gnutls; you can test whether this is the case with C<nbd_supports_tls>. + +The C<allow> parameter lets you choose which NBD URI features +are enabled, in case for example you don't want to allow +remote connections. Currently defined flags are: + +=over 4 + +=item C<LIBNBD_CONNECT_URI_ALLOW_TCP> + +Allow TCP sockets. + +=item C<LIBNBD_CONNECT_URI_ALLOW_UNIX> + +Allow Unix domain sockets. + +=item C<LIBNBD_CONNECT_URI_ALLOW_TLS> + +Allow TLS encryption. + +=item C<LIBNBD_CONNECT_URI_REQUIRE_TLS> + +Require TLS encryption. + +=item C<LIBNBD_CONNECT_URI_ALL> + +Enables all features which are defined at the time that +the program is compiled. Later features added to libnbd +will not be allowed unless you recompile your program.
This probably needs to call more attention to the fact that all flags means encryption will be required.
@@ -276,6 +278,31 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) goto cleanup; }
+ /* If the user specified the REQUIRE_TLS flag, we assume they must + * also mean to ALLOW_TLS. + */ + if ((allow & LIBNBD_CONNECT_URI_REQUIRE_TLS) != 0) + allow |= LIBNBD_CONNECT_URI_ALLOW_TLS; + + /* Check allow flags. */ + if (tcp && !(allow & LIBNBD_CONNECT_URI_ALLOW_TCP)) { + set_error (EPERM, "TCP URIs are not allowed"); + goto cleanup; + } + if (!tcp && !(allow & LIBNBD_CONNECT_URI_ALLOW_UNIX)) { + set_error (EPERM, "Unix domain socket URIs are not allowed"); + goto cleanup; + } + if (tls && !(allow & LIBNBD_CONNECT_URI_ALLOW_TLS)) { + set_error (EPERM, "TLS encrypted URIs are not allowed"); + goto cleanup; + } + if (!tls && (allow & LIBNBD_CONNECT_URI_REQUIRE_TLS)) { + set_error (EPERM, "URI must specify an encrypted connection " + "(use nbds: or nbds+unix:)"); + goto cleanup; + } +
Are there any other flags we might want to support, such as permitting or forbidding an authority section that specifies a username?
/* Insist on the scheme://[authority][/absname][?queries] form. */ if (strncmp (raw_uri + strlen (uri->scheme), "://", 3)) { set_error (EINVAL, "URI must begin with '%s://'", uri->scheme); diff --git a/tests/aio-parallel-load.c b/tests/aio-parallel-load.c index 614c22b..16c2aa2 100644 --- a/tests/aio-parallel-load.c +++ b/tests/aio-parallel-load.c @@ -220,7 +220,8 @@ start_thread (void *arg)
/* Connect to nbdkit. */ if (strstr (connection, "://")) { - if (nbd_connect_uri (nbd, connection) == -1) { + if (nbd_connect_uri (nbd, connection, + LIBNBD_CONNECT_URI_ALLOW_UNIX) == -1) {
Is this too strict? 'make check' may not use TCP, but I don't see that as something we can't support for other uses of this binary. Similar question to other changes under tests/. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org