Re: [ovs-dev] [RFC 3/5] ovsdb: add support for role-based access controls

[Ben Pfaff]

On Mon, Mar 27, 2017 at 02:56:11PM -0400, Lance Richardson wrote:
> Add suport for ovsdb RBAC (role-based access control). This includes:
> 
>- Support for new "--rbac " command-line option to
>  ovsdb-server, to specify the RBAC roles table to be used.
> 
>  This table has one row per role, with each row having a
>  "name" column (role name) and a "permissions" column (map of
>  table name to UUID of row in separate permission table.) The
>  permission table has one row per access control configuration,
>  with columns:
>   "name" - name of table to which this row applies
>   "authorization" - set of column names and column:key pairs
> to be compared against client ID to
> determine authorization status
>   "insert_delete" - boolean, true if insertions and
> authorized deletions are allowed.
>   "update"- Set of columns and column:key pairs for
> which authorized updates are allowed.
>- Support for a new "role" column in the remote configuration
>  table.
>- Logic for applying the RBAC role and permission tables, in
>  combination with session role and client id, to determine
>  whether operations modifying database contents should be
>  permitted.
> 
> Signed-off-by: Lance Richardson 

Thank you for working on this.  I have some preliminary comments; I did
not scrutinize all the code.

"git am" says:

.git/rebase-apply/patch:892: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

There's a too-deep indentation in the new code in
ovsdb_execute_insert().

I've tried to make the OVSDB errors a little more conversational in the
past, so that they'd be formatted something more like "RBAC for client X
with role Y prohibits update to column Z in table T."  Not that it's
important.

I'm not sure that a syntax error is the best choice of errors to use.
Access denied errors are pretty different from syntax errors, and in
theory a client might want to react differently in each case.  OVSDB
does not have an existing tag for errors that is completely appropriate,
so it might make sense to create a "permission denied" tag and use
that.

I don't think it makes sense to invent a tag named "mutate execution
denied by RBAC"; it is too specific.

Maybe the distinction between tags and details for errors is not clear.
The tag is supposed to be a kind of category, perhaps roughly equivalent
to an errno value.  It should be a short and generally pretty generic
string, which a machine can recognize.  The details, on the other hand,
are freeform text that is supposed to help a human find out what went
wrong in detail.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [RFC 3/5] ovsdb: add support for role-based access controls

[Lance Richardson]

Add suport for ovsdb RBAC (role-based access control). This includes:

   - Support for new "--rbac " command-line option to
 ovsdb-server, to specify the RBAC roles table to be used.

 This table has one row per role, with each row having a
 "name" column (role name) and a "permissions" column (map of
 table name to UUID of row in separate permission table.) The
 permission table has one row per access control configuration,
 with columns:
  "name" - name of table to which this row applies
  "authorization" - set of column names and column:key pairs
to be compared against client ID to
determine authorization status
  "insert_delete" - boolean, true if insertions and
authorized deletions are allowed.
  "update"- Set of columns and column:key pairs for
which authorized updates are allowed.
   - Support for a new "role" column in the remote configuration
 table.
   - Logic for applying the RBAC role and permission tables, in
 combination with session role and client id, to determine
 whether operations modifying database contents should be
 permitted.

Signed-off-by: Lance Richardson 
---
 lib/jsonrpc.c  |  10 ++
 lib/jsonrpc.h  |   2 +
 ovsdb/automake.mk  |   2 +
 ovsdb/execution.c  |  38 ++-
 ovsdb/jsonrpc-server.c |   6 +-
 ovsdb/jsonrpc-server.h |   1 +
 ovsdb/mutation.c   |  11 +-
 ovsdb/mutation.h   |   6 +-
 ovsdb/ovsdb-server.c   |  70 -
 ovsdb/ovsdb-tool.c |   2 +-
 ovsdb/ovsdb-util.c |  40 +++
 ovsdb/ovsdb-util.h |   3 +
 ovsdb/ovsdb.h  |   1 +
 ovsdb/rbac.c   | 279 +
 ovsdb/rbac.h   |  26 +
 ovsdb/trigger.c|   8 +-
 ovsdb/trigger.h|   5 +-
 tests/test-ovsdb.c |   7 +-
 18 files changed, 503 insertions(+), 14 deletions(-)
 create mode 100644 ovsdb/rbac.c
 create mode 100644 ovsdb/rbac.h

diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
index a0ade9c..2fae057 100644
--- a/lib/jsonrpc.c
+++ b/lib/jsonrpc.c
@@ -1005,6 +1005,16 @@ jsonrpc_session_get_name(const struct jsonrpc_session *s)
 return reconnect_get_name(s->reconnect);
 }
 
+const char *
+jsonrpc_session_get_id(const struct jsonrpc_session *s)
+{
+if (s->rpc && s->rpc->stream) {
+return stream_get_peer_id(s->rpc->stream);
+} else {
+return NULL;
+}
+}
+
 /* Always takes ownership of 'msg', regardless of success. */
 int
 jsonrpc_session_send(struct jsonrpc_session *s, struct jsonrpc_msg *msg)
diff --git a/lib/jsonrpc.h b/lib/jsonrpc.h
index 982017a..6a82954 100644
--- a/lib/jsonrpc.h
+++ b/lib/jsonrpc.h
@@ -130,5 +130,7 @@ void jsonrpc_session_set_probe_interval(struct 
jsonrpc_session *,
 int probe_interval);
 void jsonrpc_session_set_dscp(struct jsonrpc_session *,
   uint8_t dscp);
+const char *jsonrpc_session_get_id(const struct jsonrpc_session *);
+
 
 #endif /* jsonrpc.h */
diff --git a/ovsdb/automake.mk b/ovsdb/automake.mk
index c218bf5..ac0f741 100644
--- a/ovsdb/automake.mk
+++ b/ovsdb/automake.mk
@@ -24,6 +24,8 @@ ovsdb_libovsdb_la_SOURCES = \
ovsdb/monitor.h \
ovsdb/query.c \
ovsdb/query.h \
+   ovsdb/rbac.c \
+   ovsdb/rbac.h \
ovsdb/replication.c \
ovsdb/replication.h \
ovsdb/row.c \
diff --git a/ovsdb/execution.c b/ovsdb/execution.c
index e2d320e..2c9ca3d 100644
--- a/ovsdb/execution.c
+++ b/ovsdb/execution.c
@@ -32,6 +32,7 @@
 #include "table.h"
 #include "timeval.h"
 #include "transaction.h"
+#include "rbac.h"
 
 struct ovsdb_execution {
 struct ovsdb *db;
@@ -39,6 +40,8 @@ struct ovsdb_execution {
 struct ovsdb_txn *txn;
 struct ovsdb_symbol_table *symtab;
 bool durable;
+const char *role;
+const char *id;
 
 /* Triggers. */
 long long int elapsed_msec;
@@ -97,6 +100,7 @@ lookup_executor(const char *name, bool *read_only)
 struct json *
 ovsdb_execute(struct ovsdb *db, const struct ovsdb_session *session,
   const struct json *params, bool read_only,
+  const char *role, const char *id,
   long long int elapsed_msec, long long int *timeout_msec)
 {
 struct ovsdb_execution x;
@@ -126,6 +130,8 @@ ovsdb_execute(struct ovsdb *db, const struct ovsdb_session 
*session,
 x.txn = ovsdb_txn_create(db);
 x.symtab = ovsdb_symbol_table_create();
 x.durable = false;
+x.role = role;
+x.id = id;
 x.elapsed_msec = elapsed_msec;
 x.timeout_msec = LLONG_MAX;
 results = NULL;
@@ -305,6 +311,13 @@ ovsdb_execute_insert(struct ovsdb_execution *x, struct 
ovsdb_parser *parser,
 return error;
 }
 
+if (!ovsdb_rbac_insert(table, x->role, x->id)) {
+return ovsdb_syntax_error(parser->json,
+