[Libguestfs] [PATCH libnbd 9/9] FOR DISCUSSION ONLY: api: Add ‘allow’ parameter to nbd_connect_uri to control permitted URIs.

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190810/9d061c9d/attachment.sig>