On Mon, May 01, 2017 at 10:13:21AM -0400, Lance Richardson wrote:
> Add suport for ovsdb RBAC (role-based access control). This includes:
>
> - Support for "RBAC_Role" table. A db schema containing a table
> by this name will enable role-based access controls using
> this table for RBAC role configuration.
>
> The "RBAC_Role" 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 the following 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 from the remote connection table
> and client id, to determine whether operations modifying database
> contents should be permitted.
>
> Signed-off-by: Lance Richardson <[email protected]>
This is really nice. Thank you for investing time in this! I've had my
doubts, repeatedly, about the value of access control in OVSDB, but the
OVN use case has finally convinced, and I really appreciate the work
you've done to make it happen.
As a high-level comment, it looks to me like documentation is missing
for the ways that this affects the schema and the wire protocol. We try
to document those kinds of changes, relative to the RFC 7074
specification, in ovsdb/ovsdb-server.1.in.
I'm appending an incremental patch that I suggest folding in. It is
mostly little style points I noticed. I also fixed a few minor bugs I
noticed, and I added a new --rbac-role option to ovsdb-tool to allow
users to test without firing up an OVSDB server.
I fussed with the style on ovsdb_util_read_map_string_uuid_column()
enough that the bug fix might not be obvious: it assumed that
column->type.value.u.uuid.refTable is nonnull, even though that might
not be the case.
Thanks!
--8<--------------------------cut here-------------------------->8--
diff --git a/lib/ovsdb-error.c b/lib/ovsdb-error.c
index b1aaf29a43fd..d8161e6d7b9a 100644
--- a/lib/ovsdb-error.c
+++ b/lib/ovsdb-error.c
@@ -167,7 +167,8 @@ ovsdb_internal_error(struct ovsdb_error *inner_error,
return error;
}
-struct ovsdb_error *ovsdb_perm_error(const char *details, ...)
+struct ovsdb_error *
+ovsdb_perm_error(const char *details, ...)
{
struct ovsdb_error *error;
va_list args;
diff --git a/ovsdb/execution.c b/ovsdb/execution.c
index 9f27457fef61..0b638b915722 100644
--- a/ovsdb/execution.c
+++ b/ovsdb/execution.c
@@ -27,12 +27,12 @@
#include "ovsdb-parser.h"
#include "ovsdb.h"
#include "query.h"
+#include "rbac.h"
#include "row.h"
#include "server.h"
#include "table.h"
#include "timeval.h"
#include "transaction.h"
-#include "rbac.h"
struct ovsdb_execution {
struct ovsdb *db;
diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 6edfe241b535..1770c2616b57 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -271,7 +271,7 @@ ovsdb_jsonrpc_server_add_remote(struct ovsdb_jsonrpc_server
*svr,
ovs_list_init(&remote->sessions);
remote->dscp = options->dscp;
remote->read_only = options->read_only;
- remote->role = options->role ? xstrdup(options->role) : NULL;
+ remote->role = nullable_xstrdup(options->role);
shash_add(&svr->remotes, name, remote);
if (!listener) {
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 32ee96c6aed3..030d86ba467f 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -702,7 +702,9 @@ add_manager_options(struct shash *remotes, const struct
ovsdb_row *row)
if (ovsdb_util_read_bool_column(row, "read_only", &read_only)) {
options->read_only = read_only;
}
+
free(options->role);
+ options->role = NULL;
if (ovsdb_util_read_string_column(row, "role", &role) && role) {
options->role = xstrdup(role);
}
@@ -1354,7 +1356,6 @@ parse_options(int *argcp, char **argvp[],
OPT_SYNC_FROM,
OPT_SYNC_EXCLUDE,
OPT_ACTIVE,
- OPT_RBAC,
VLOG_OPTION_ENUMS,
DAEMON_OPTION_ENUMS,
SSL_OPTION_ENUMS,
diff --git a/ovsdb/ovsdb-tool.1.in b/ovsdb/ovsdb-tool.1.in
index d01531e567c9..8c799f4cc990 100644
--- a/ovsdb/ovsdb-tool.1.in
+++ b/ovsdb/ovsdb-tool.1.in
@@ -131,7 +131,7 @@ will print a blank line.
.
.SS "Other Commands"
.
-.IP "\fBquery\fI db transaction\fR"
+.IP "[\fB\-\-rbac\-role=\fIrole\fR] \fBquery\fI db transaction\fR"
Opens \fIdb\fR, executes \fItransaction\fR on it, and prints the
results. The \fItransaction\fR must be a JSON array in the format of
the \fBparams\fR array for the JSON-RPC \fBtransact\fR method, as
@@ -142,8 +142,11 @@ safely run concurrently with other database activity,
including
\fBovsdb\-server\fR and other database writers. The \fItransaction\fR
may specify database modifications, but these will have no effect on
\fIdb\fR.
+.IP
+By default, the transaction is executed using the ``superuser'' RBAC
+role. Use \fB\-\-rbac\-role\fR to specify a different role.
.
-.IP "\fBtransact\fI db transaction\fR"
+.IP "[\fR\-\-rbac\-role=\fIrole\fR] \fBtransact\fI db transaction\fR"
Opens \fIdb\fR, executes \fItransaction\fR on it, prints the results,
and commits any changes to \fIdb\fR. The \fItransaction\fR must be a
JSON array in the format of the \fBparams\fR array for the JSON-RPC
@@ -154,6 +157,9 @@ command will fail if the database is opened for writing by
any other
process, including \fBovsdb\-server\fR(1). Use \fBovsdb\-client\fR(1),
instead, to write to a database that is served by
\fBovsdb\-server\fR(1).
+.IP
+By default, the transaction is executed using the ``superuser'' RBAC
+role. Use \fB\-\-rbac\-role\fR to specify a different role.
.
.IP "\fBshow\-log\fI db\fR"
Prints a summary of the records in \fIdb\fR's log, including the time
diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index fb8eb1eff74e..8908bae3628a 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -44,6 +44,9 @@
/* -m, --more: Verbosity level for "show-log" command output. */
static int show_log_verbosity;
+/* --role: RBAC role to use for "transact" and "query" commands. */
+static const char *rbac_role;
+
static const struct ovs_cmdl_command *get_all_commands(void);
OVS_NO_RETURN static void usage(void);
@@ -68,8 +71,12 @@ main(int argc, char *argv[])
static void
parse_options(int argc, char *argv[])
{
+ enum {
+ OPT_RBAC_ROLE = UCHAR_MAX + 1
+ };
static const struct option long_options[] = {
{"more", no_argument, NULL, 'm'},
+ {"rbac-role", required_argument, NULL, OPT_RBAC_ROLE},
{"verbose", optional_argument, NULL, 'v'},
{"help", no_argument, NULL, 'h'},
{"option", no_argument, NULL, 'o'},
@@ -91,6 +98,10 @@ parse_options(int argc, char *argv[])
show_log_verbosity++;
break;
+ case OPT_RBAC_ROLE:
+ rbac_role = optarg;
+ break;
+
case 'h':
usage();
@@ -135,10 +146,12 @@ usage(void)
"The default SCHEMA is %s.\n",
program_name, program_name, default_db(), default_schema());
vlog_usage();
- printf("\nOther options:\n"
- " -m, --more increase show-log verbosity\n"
- " -h, --help display this help message\n"
- " -V, --version display version information\n");
+ printf("\
+\nOther options:\n\
+ -m, --more increase show-log verbosity\n\
+ --rbac-role=ROLE RBAC role for transact and query commands\n\
+ -h, --help display this help message\n\
+ -V, --version display version information\n");
exit(EXIT_SUCCESS);
}
@@ -366,7 +379,7 @@ transact(bool read_only, int argc, char *argv[])
check_ovsdb_error(ovsdb_file_open(db_file_name, read_only, &db, NULL));
request = parse_json(transaction);
- result = ovsdb_execute(db, NULL, request, false, NULL, NULL, 0, NULL);
+ result = ovsdb_execute(db, NULL, request, false, rbac_role, NULL, 0, NULL);
json_destroy(request);
print_and_free_json(result);
diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c
index 3f7e1afb8c17..5ee5e4ddaf8d 100644
--- a/ovsdb/ovsdb-util.c
+++ b/ovsdb/ovsdb-util.c
@@ -95,34 +95,28 @@ ovsdb_util_read_map_string_uuid_column(const struct
ovsdb_row *row,
const char *column_name,
const char *key)
{
- union ovsdb_atom *atom_key = NULL, *atom_value = NULL;
- const struct ovsdb_table *ref_table;
- const struct ovsdb_column *column;
- const struct ovsdb_datum *datum;
- size_t i;
-
- column = ovsdb_table_schema_get_column(row->table->schema, column_name);
+ const struct ovsdb_column *column
+ = ovsdb_table_schema_get_column(row->table->schema, column_name);
if (!column ||
column->type.key.type != OVSDB_TYPE_STRING ||
column->type.value.type != OVSDB_TYPE_UUID) {
return NULL;
}
- datum = &row->fields[column->index];
+ const struct ovsdb_table *ref_table = column->type.value.u.uuid.refTable;
+ if (!ref_table) {
+ return NULL;
+ }
- for (i = 0; i < datum->n; i++) {
- atom_key = &datum->keys[i];
+ const struct ovsdb_datum *datum = &row->fields[column->index];
+ for (size_t i = 0; i < datum->n; i++) {
+ union ovsdb_atom *atom_key = &datum->keys[i];
if (!strcmp(atom_key->string, key)) {
- atom_value = &datum->values[i];
- break;
+ const union ovsdb_atom *atom_value = &datum->values[i];
+ return ovsdb_table_get_row(ref_table, &atom_value->uuid);
}
}
-
- if (!atom_value) {
- return NULL;
- }
- ref_table = column->type.value.u.uuid.refTable;
- return ovsdb_table_get_row(ref_table, &atom_value->uuid);
+ return NULL;
}
const union ovsdb_atom *
diff --git a/ovsdb/rbac.c b/ovsdb/rbac.c
index 6d8aa67bcf76..296c532bf12d 100644
--- a/ovsdb/rbac.c
+++ b/ovsdb/rbac.c
@@ -14,15 +14,21 @@
* limitations under the License.
*/
#include <config.h>
+
+#include "rbac.h"
+
#include <limits.h>
#include "column.h"
#include "condition.h"
+#include "condition.h"
#include "file.h"
#include "mutation.h"
+#include "openvswitch/vlog.h"
#include "ovsdb-data.h"
#include "ovsdb-error.h"
#include "ovsdb-parser.h"
+#include "ovsdb-util.h"
#include "ovsdb.h"
#include "query.h"
#include "row.h"
@@ -30,10 +36,6 @@
#include "table.h"
#include "timeval.h"
#include "transaction.h"
-#include "ovsdb-util.h"
-#include "condition.h"
-#include "rbac.h"
-#include "openvswitch/vlog.h"
VLOG_DEFINE_THIS_MODULE(ovsdb_rbac);
@@ -43,17 +45,15 @@ ovsdb_find_row_by_string_key(const struct ovsdb_table
*table,
const char *key)
{
const struct ovsdb_column *column;
- const struct ovsdb_row *row;
-
column = ovsdb_table_schema_get_column(table->schema, column_name);
if (column) {
+ /* XXX This is O(n) in the size of the table. If the table has an
+ * index on the column, then we could implement it in O(1). */
+ const struct ovsdb_row *row;
HMAP_FOR_EACH (row, hmap_node, &table->rows) {
- const struct ovsdb_datum *datum;
- size_t i;
-
- datum = &row->fields[column->index];
- for (i = 0; i < datum->n; i++) {
+ const struct ovsdb_datum *datum = &row->fields[column->index];
+ for (size_t i = 0; i < datum->n; i++) {
if (datum->keys[i].string[0] &&
!strcmp(key, datum->keys[i].string)) {
return row;
@@ -61,6 +61,7 @@ ovsdb_find_row_by_string_key(const struct ovsdb_table *table,
}
}
}
+
return NULL;
}
diff --git a/ovsdb/rbac.h b/ovsdb/rbac.h
index 1c2664b5760c..45e0fcf6b4c7 100644
--- a/ovsdb/rbac.h
+++ b/ovsdb/rbac.h
@@ -16,23 +16,31 @@
#ifndef OVSDB_RBAC_H
#define OVSDB_RBAC_H 1
+
+#include <stdbool.h>
+
+struct ovsdb;
+struct ovsdb_column_set;
+struct ovsdb_condition;
struct ovsdb_mutation_set;
+struct ovsdb_table;
bool ovsdb_rbac_insert(const struct ovsdb *,
const struct ovsdb_table *,
const char *, const char *);
bool ovsdb_rbac_delete(const struct ovsdb *,
- struct ovsdb_table *table,
- struct ovsdb_condition *condition,
+ struct ovsdb_table *,
+ struct ovsdb_condition *,
const char *role, const char *id);
bool ovsdb_rbac_update(const struct ovsdb *,
- struct ovsdb_table *table,
- struct ovsdb_column_set *columns,
+ struct ovsdb_table *,
+ struct ovsdb_column_set *,
struct ovsdb_condition *condition,
const char *role, const char *id);
bool ovsdb_rbac_mutate(const struct ovsdb *,
- struct ovsdb_table *table,
- struct ovsdb_mutation_set *mutations,
- struct ovsdb_condition *condition,
+ struct ovsdb_table *,
+ struct ovsdb_mutation_set *,
+ struct ovsdb_condition *,
const char *role, const char *id);
+
#endif /* ovsdb/rbac.h */
diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
index 94b95ea00e06..1b960f5723ec 100644
--- a/ovsdb/trigger.c
+++ b/ovsdb/trigger.c
@@ -43,8 +43,8 @@ ovsdb_trigger_init(struct ovsdb_session *session, struct
ovsdb *db,
trigger->created = now;
trigger->timeout_msec = LLONG_MAX;
trigger->read_only = read_only;
- trigger->role = role ? xstrdup(role): NULL;
- trigger->id = id ? xstrdup(id): NULL;
+ trigger->role = nullable_xstrdup(role);
+ trigger->id = nullable_xstrdup(id);
ovsdb_trigger_try(trigger, now);
}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
