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

[Eric Blake]

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

-------------- 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/20190812/2c449715/attachment.sig>