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

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

On Sat, Aug 10, 2019 at 04:50:18PM -0500, Eric Blake
wrote:
> On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
> > +++ 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?
Yes I believe it's wrong.
> > +++ 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.
Indeed.

My other idea for this patch was to have a list of features which are
denied.  Of course this fails open in the case where we add new
features, but I think it would fix this case.

[...]
> > +  /* 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?
Yes this was just an initial set of ideas for flags.

I also thought that instead of these "easy" checks we might instead
(or as well) check for more purpose-oriented features. For example
"allow connections to remote servers" (if not set, then TCP
connections to localhost would be OK).  "Allow connections to other
users on the system" could take into account the authority section.
Actually implementing these can be tricky.

Yet another way to think about this problem is that we leave it up to
the caller to sort it out.  We might offer some help such as some
functions to parse fields out of NBD URIs, but leave the details of
validating to them.
> >    /* 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/.
I made the checks match what the tests need currently.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 8/11/19 3:39 AM, Richard W.M. Jones wrote:
> On Sat, Aug 10, 2019 at 04:50:18PM -0500, Eric Blake wrote:
>> On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
>>> +++ 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?
> 
> Yes I believe it's wrong.
> 
>>> +++ 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.
> 
> Indeed.
> 
> My other idea for this patch was to have a list of features which are
> denied.  Of course this fails open in the case where we add new
> features, but I think it would fix this case.
In libvirt, we added various ListAll APIs with a notion of filtering
groups; for example, virConnectListAllDomains has the following list of
flags:

typedef enum {
    VIR_CONNECT_LIST_DOMAINS_ACTIVE         = 1 << 0,
    VIR_CONNECT_LIST_DOMAINS_INACTIVE       = 1 << 1,

    VIR_CONNECT_LIST_DOMAINS_PERSISTENT     = 1 << 2,
    VIR_CONNECT_LIST_DOMAINS_TRANSIENT      = 1 << 3,

    VIR_CONNECT_LIST_DOMAINS_RUNNING        = 1 << 4,
    VIR_CONNECT_LIST_DOMAINS_PAUSED         = 1 << 5,
    VIR_CONNECT_LIST_DOMAINS_SHUTOFF        = 1 << 6,
    VIR_CONNECT_LIST_DOMAINS_OTHER          = 1 << 7,

    VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE    = 1 << 8,
    VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE = 1 << 9,

    VIR_CONNECT_LIST_DOMAINS_AUTOSTART      = 1 << 10,
    VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART   = 1 << 11,

    VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT   = 1 << 12,
    VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT    = 1 << 13,

    VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT = 1 << 14,
    VIR_CONNECT_LIST_DOMAINS_NO_CHECKPOINT  = 1 << 15,
} virConnectListAllDomainsFlags;

The way it is documented is that every group of flags (active/inactive,
persistent/transient, running/paused/shutoff/other, etc) is an
orthogonal coverage of the complete list of domains.  If a user does not
request any of the filters within a group, then the API does not do
filtering based on that group; likewise, if a user requests all of the
filters within a group, there is usually no observable difference
because it selects all of the domains.  But where it gets interesting is
that if we add a bit to an existing group (such as adding a fifth bit
alongside running/paused...), passing none of the bits in the group
selects the domains meeting the new category, but passing all of the
previously-known bits in the group now performs a filtering that
excludes domains matching the new fifth bit.  On the other hand, if we
add a new feature that is an entirely orthogonal group, then only the
domains compiled against the newer libvirt are able to filter on that
new feature (an example: libvirt 5.6 added the
has_checkpoint/no_checkpoint group).  Older clients that don't know
about that filter will never set any bits in that group, and therefore
be unaffected by the new filtering capability.

So taking that same approach to libnbd, we would declare that the bits
exposed to nbd_connect_uri are designed to be arranged in orthogonal
groups each with 100% coverage, and that any new features would be
supported by adding a new group of bits.  Our initial groups could thus be:
  allow_tcp/allow_unix, allow_uncrypted/allow_tls
but still leaving the door open to future groups such as:
  allow_ipv4/allow_ipv6

When adding a new feature, we then decide if the new feature belongs to
an existing group; a user passing 0 (for that group) gets to use the new
feature, a user passing all_bits avoids the new feature.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org