[ovs-dev] [PATCH] [RFC] ovn-controller: Experiment with restricting access to columns.

[Mark Michelson]

On 06/13/2018 11:29 PM, Han Zhou wrote:
> On Wed, Jun 13, 2018 at 3:37 PM, Ben Pfaff <blp at ovn.org> wrote:
>>
>> To make ovn-controller recompute incrementally, we need accurate
>> dependencies for each function that reads or writes a table.  It's
>> currently difficult to be sure about these dependencies, and certainly
>> difficult to maintain them over time, because there's no way to actually
>> enforce them.
>>
>> This commit experiments with an approach that allows for fairly
>> fine-grained access control within ovn-controller to tables and columns.
>> It's based on generating a new version of the IDL data structures for each
>> case where we want different access control.  All of these data structures
>> have the same format, but the columns that a given piece of code is not
>> supposed to touch are renamed to discourage programmers from using them,
>> e.g. they're given names suffixed with "__accessdenied".  (This means
>> that there is no runtime overhead to the access control since it only
>> requires a cast to convert between the regular and restricted versions.)
>> In addition, when a columns is supposed to be read-only, functions for
>> modifying the column are not supplied.
>>
>> This commit only tries out this experiment for a single file within
>> ovn-controller, the BFD implementation (mostly because that's
>> alphabetically first, no other real reason).  It would require a little
>> more work to apply it everywhere, but it's probably not a huge deal.
>>
>> Comments?
>>
>> CC: Han Zhou <zhouhan at gmail.com>
>> Signed-off-by: Ben Pfaff <blp at ovn.org>
>> ---
>>   ovn/controller/automake.mk         |   5 +
>>   ovn/controller/bfd-vswitch-idl.def |  21 ++++
>>   ovn/controller/bfd.c               |  20 ++--
>>   ovn/controller/bfd.h               |   8 +-
>>   ovn/controller/ovn-controller.c    |  13 ++-
>>   ovsdb/ovsdb-idlc.in                | 223 ++++++++++++++++++++++++++++++
> ++++++-
>>   6 files changed, 268 insertions(+), 22 deletions(-)
>>   create mode 100644 ovn/controller/bfd-vswitch-idl.def
>>
> 
> I wanted to have a quick test but it didn't pass the compile:
> In file included from ovn/controller/bfd.c:17:0:
> ovn/controller/bfd.h:19:44: fatal error: ovn/controller/bfd-vswitch-idl.h:
> No such file or directory

Here's a different datapoint in the same category.

I got a slightly different error when I tried to compile. 
ovn/controller/bfd-vswitch-idl.h was auto-generated and everything 
worked up until the very end:

"The following files are in git but not the distribution:
ovn/controller/bfd-vswitch-idl.def"

The make command I ran was `make sandbox SANDBOXFLAGS="--ovn"`

I tried running `make distclean` then reconfiguring, but this didn't help.

For comparison, Han, these are my software versions, in case that might 
be why auto-generation worked for me but not you:
gcc version is 7.3.1
make version is 4.2.1
autoconf version is 2.69


> 
> I didn't debug, but by looking at the code I got the idea. It ensures the
> purpose is declared through the generated data type and violations are
> captured at compile time.
> I think this is a brilliant way to achieve the check with such a small
> change, however, I am not sure how is it supposed to help for generating
> the dependency. I think instead of caring about whether it is read-only, we
> should care about whether it is write-only.
> 
> For example, a engine node A reads on Table T1's column C1, reads & writes
> on Table T2's column C2, and writes on T3's C3. In this case A depends on
> T1 and T2, but not depends on T3.
> So I think what matters to the dependency generation is whether it is
> Write-Only. Read-Only is no different from ReadWrite.
> 
> If my understanding is correct (that we don't care about the difference
> between RO and RW), for WO columns, we can simply just don't monitor its
> change. Then we don't even need this feature from dependency generation
> point of view.
> Did I miss something?
> 
> Another thing, however, I think is really important for the dependency
> generation is the columns that reference other tables, e.g. bfd_run()
> doesn't has table "ovsrec_port" as input, but in fact it depends on this
> table by referencing from ovsrec_bridge->ports. We need the table being
> referenced appear in the input parameters.
> The approach in the patch may not solve that problem directly, but
> something similar may work: we could declare the usage for a referenced
> column as "direct" or "indirect". "indirect" means the column can be
> accessed only with "get functions" instead of pointers. The "get functions"
> requires an additional parameter that is the instance of the table being
> referenced. To call the "get function" to access the column, the caller
> function needs to have the table being referenced passed in as parameter,
> so we will be able to catch the dependency. I am not sure if this approach
> is too heavy, or is tricky to implement.
> 
> Thanks,
> Han
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>