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

[Richard W.M. Jones]

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