diff mbox series

[ovs-dev,v2] northd: Fix issues in RBAC tables recovery.

Message ID 20241112083939.126224-1-alekssmirnov@k2.cloud
State New
Headers show
Series [ovs-dev,v2] northd: Fix issues in RBAC tables recovery. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Aleksandr Smirnov (K2 Cloud) Nov. 12, 2024, 8:39 a.m. UTC
From: Aleksandr Smirnov <AleksSmirnov@k2.cloud>

Northd creates hardcoded RBAC role 'ovn-controller' with number of
predefined permissions. Then it watches for alternations of the role
and permissions and recover them if they were changed.
An original code have issues that prevents an user to create any other roles.
Also, builtin permissions are not fully protected against modifications.

The fix reworks this code to recover builtin permissions properly after
all possible modification scenarios. Also, creation of custom roles and
permissions becomes available.

Signed-off-by: Aleksandr Smirnov <AleksSmirnov@k2.cloud>
Tested-by: Aleksandr Gnatyuk <agnatyuk@k2.cloud>
---
Review comments (from Dumitru) were adopted.

---
 northd/ovn-northd.c | 119 +++++++++++++++++++++++++++++---------------
 tests/ovn-northd.at |  82 ++++++++++++++++++++++++++++++
 2 files changed, 162 insertions(+), 39 deletions(-)
diff mbox series

Patch

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index fb29c3c21..6d8718c8b 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -277,20 +277,16 @@  static struct gen_opts_map supported_dhcpv6_opts[] = {
     DHCPV6_OPT_FQDN,
 };
 
+/*
+ * Compare predefined permission against RBAC_Permission record.
+ * Returns true if match, false otherwise.
+ */
 static bool
-ovn_rbac_validate_perm(const struct sbrec_rbac_permission *perm)
+ovn_rbac_match_perm(const struct sbrec_rbac_permission *perm,
+                    const struct rbac_perm_cfg *pcfg)
 {
-    struct rbac_perm_cfg *pcfg;
     int i, j, n_found;
 
-    for (pcfg = rbac_perm_cfg; pcfg->table; pcfg++) {
-        if (!strcmp(perm->table, pcfg->table)) {
-            break;
-        }
-    }
-    if (!pcfg->table) {
-        return false;
-    }
     if (perm->n_authorization != pcfg->n_auth ||
         perm->n_update != pcfg->n_update) {
         return false;
@@ -326,54 +322,71 @@  ovn_rbac_validate_perm(const struct sbrec_rbac_permission *perm)
         return false;
     }
 
-    /* Success, db state matches expected state */
-    pcfg->row = perm;
     return true;
 }
 
+/*
+ * Search predefined permission pcfg in the RBAC_Permission.
+ * If there is no record that match, recover the permission.
+ */
 static void
-ovn_rbac_create_perm(struct rbac_perm_cfg *pcfg,
-                     struct ovsdb_idl_txn *ovnsb_txn,
-                     const struct sbrec_rbac_role *rbac_role)
+ovn_rbac_validate_perm(struct rbac_perm_cfg *pcfg,
+                       struct ovsdb_idl_txn *ovnsb_txn,
+                       struct ovsdb_idl *ovnsb_idl)
 {
-    struct sbrec_rbac_permission *rbac_perm;
+    const struct sbrec_rbac_permission *perm_row;
 
-    rbac_perm = sbrec_rbac_permission_insert(ovnsb_txn);
-    sbrec_rbac_permission_set_table(rbac_perm, pcfg->table);
-    sbrec_rbac_permission_set_authorization(rbac_perm,
+    SBREC_RBAC_PERMISSION_FOR_EACH (perm_row, ovnsb_idl) {
+        if (!strcmp(perm_row->table, pcfg->table)
+            && ovn_rbac_match_perm(perm_row, pcfg)) {
+            pcfg->row = perm_row;
+
+            return;
+        }
+    }
+
+    pcfg->row = sbrec_rbac_permission_insert(ovnsb_txn);
+    sbrec_rbac_permission_set_table(pcfg->row, pcfg->table);
+    sbrec_rbac_permission_set_authorization(pcfg->row,
                                             pcfg->auth,
                                             pcfg->n_auth);
-    sbrec_rbac_permission_set_insert_delete(rbac_perm, pcfg->insdel);
-    sbrec_rbac_permission_set_update(rbac_perm,
+    sbrec_rbac_permission_set_insert_delete(pcfg->row, pcfg->insdel);
+    sbrec_rbac_permission_set_update(pcfg->row,
                                      pcfg->update,
                                      pcfg->n_update);
-    sbrec_rbac_role_update_permissions_setkey(rbac_role, pcfg->table,
-                                              rbac_perm);
 }
 
+/*
+ * Make sure that DB Role 'ovn-controller' exists, has no duplicates
+ * permission list exactly match to predefined permissions.  Recreate if
+ * matching fails.
+ */
 static void
 check_and_update_rbac(struct ovsdb_idl_txn *ovnsb_txn,
                       struct ovsdb_idl *ovnsb_idl)
 {
     const struct sbrec_rbac_role *rbac_role = NULL;
-    const struct sbrec_rbac_permission *perm_row;
     const struct sbrec_rbac_role *role_row;
-    struct rbac_perm_cfg *pcfg;
 
-    for (pcfg = rbac_perm_cfg; pcfg->table; pcfg++) {
-        pcfg->row = NULL;
+    /*
+     * Make sure predefined permissions are presented in the RBAC_Permissions
+     * table. Otherwise create consistent permissions.
+     */
+    for (struct rbac_perm_cfg *pcfg = rbac_perm_cfg; pcfg->table; pcfg++) {
+        ovn_rbac_validate_perm(pcfg, ovnsb_txn, ovnsb_idl);
     }
 
-    SBREC_RBAC_PERMISSION_FOR_EACH_SAFE (perm_row, ovnsb_idl) {
-        if (!ovn_rbac_validate_perm(perm_row)) {
-            sbrec_rbac_permission_delete(perm_row);
-        }
-    }
+    /*
+     * Make sure the role 'ovn-controller' is presented in the RBAC_Role table.
+     * Otherwise create the role. Remove duplicates if any.
+     */
     SBREC_RBAC_ROLE_FOR_EACH_SAFE (role_row, ovnsb_idl) {
-        if (strcmp(role_row->name, "ovn-controller")) {
-            sbrec_rbac_role_delete(role_row);
-        } else {
-            rbac_role = role_row;
+        if (!strcmp(role_row->name, "ovn-controller")) {
+            if (rbac_role) {
+                sbrec_rbac_role_delete(role_row);
+            } else {
+                rbac_role = role_row;
+            }
         }
     }
 
@@ -382,11 +395,39 @@  check_and_update_rbac(struct ovsdb_idl_txn *ovnsb_txn,
         sbrec_rbac_role_set_name(rbac_role, "ovn-controller");
     }
 
-    for (pcfg = rbac_perm_cfg; pcfg->table; pcfg++) {
-        if (!pcfg->row) {
-            ovn_rbac_create_perm(pcfg, ovnsb_txn, rbac_role);
+    /*
+     * Make sure the permission list attached to the role 'ovn-controller'
+     * exactly matches to predefined permissions.
+     * Reassign permission list to the role if any difference has found.
+     */
+    if (ARRAY_SIZE(rbac_perm_cfg) - 1 != rbac_role->n_permissions) {
+        goto rebuild_role_perms;
+    }
+
+    for (struct rbac_perm_cfg *pcfg = rbac_perm_cfg; pcfg->table; pcfg++) {
+        size_t i;
+        for (i = 0; i < rbac_role->n_permissions; i++) {
+            if (!strcmp(pcfg->table, rbac_role->key_permissions[i])
+                && pcfg->row == rbac_role->value_permissions[i]) {
+                break;
+            }
+        }
+
+        if (i == rbac_role->n_permissions) {
+            goto rebuild_role_perms;
         }
     }
+
+    return;
+
+rebuild_role_perms:
+    sbrec_rbac_role_set_permissions(rbac_role, NULL, NULL, 0);
+
+    for (struct rbac_perm_cfg *pcfg = rbac_perm_cfg; pcfg->table; pcfg++) {
+        sbrec_rbac_role_update_permissions_setkey(rbac_role,
+                                                  pcfg->table,
+                                                  pcfg->row);
+    }
 }
 
 static void
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 4e5b3b172..4ee8572b7 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -14233,3 +14233,85 @@  AT_CHECK([grep "lr_in_dnat" lr1flows | ovn_strip_lflows | grep "30.0.0.1"], [0],
 
 AT_CLEANUP
 ])
+
+AT_SETUP([RBAC -- Recover builtin role and permissions])
+ovn_start
+
+RBR_BUILTIN_PNAMES=$(fetch_column RBAC_Role permissions name=ovn-controller | uuidfilt | sed -e 's/<[[^<>]]>*//g' | tr -d '=,{}')
+
+declare -A RBR_BUILTIN_PERM
+
+for V in $RBR_BUILTIN_PNAMES
+do
+  _UUID=`ovn-sbctl get RBAC_Role ovn-controller permissions:$V`
+  RBR_BUILTIN_PERM[[$V]]=`ovn-sbctl get RBAC_Permission $_UUID authorization insert_delete table update | paste -s -d " "`
+done
+
+rbr_match_builtin() {
+  local RBR_REAL_PNAMES=$(fetch_column RBAC_Role permissions name=ovn-controller | uuidfilt | sed -e 's/<[[^<>]]>*//g' | tr -d '=,{}')
+
+  AT_CHECK([test "$RBR_BUILTIN_PNAMES" == "$RBR_REAL_PNAMES"])
+
+  for V in $RBR_BUILTIN_PNAMES
+  do
+    local _UUID=`ovn-sbctl get RBAC_Role ovn-controller permissions:$V`
+    local RBR_REAL_PERM=`ovn-sbctl get RBAC_Permission $_UUID authorization insert_delete table update | paste -s -d " "`
+    AT_CHECK([test "${RBR_BUILTIN_PERM[[$V]]}" == "$RBR_REAL_PERM"])
+  done
+}
+
+rbr_match_custom() {
+  local RBR_BUILTIN_PNAMES='Load_Balancer'
+  local RBR_BUILTIN_ROLE='Load_Balancer=<0>'
+  local RBR_REAL_ROLE=$(fetch_column RBAC_Role permissions name=custom-role | uuidfilt)
+
+  declare -A RBR_BUILTIN_PERM=(
+   [[Load_Balancer]]='[[]] false Load_Balancer [[]]'
+  )
+
+  AT_CHECK([test "$RBR_BUILTIN_ROLE" == "$RBR_REAL_ROLE"])
+
+  for V in $RBR_BUILTIN_PNAMES
+  do
+    local _UUID=`ovn-sbctl get RBAC_Role custom-role permissions:$V`
+    local _PREAL=`ovn-sbctl get RBAC_Permission $_UUID authorization insert_delete table update | paste -s -d " "`
+    AT_CHECK([test "${RBR_BUILTIN_PERM[[$V]]}" == "$_PREAL"])
+  done
+}
+
+AS_BOX([Recover after role delete])
+check ovn-sbctl destroy RBAC_Role ovn-controller
+OVS_WAIT_UNTIL([rbr_match_builtin])
+
+AS_BOX([Recover after permissions alternation])
+check ovn-sbctl remove RBAC_Permission Chassis update vtep_logical_switches
+OVS_WAIT_UNTIL([rbr_match_builtin])
+
+AS_BOX([Recover after permission delete])
+PERM_UID=`ovn-sbctl get RBAC_Role ovn-controller permissions:Chassis`
+check check ovn-sbctl destroy RBAC_Permission $PERM_UID
+OVS_WAIT_UNTIL([rbr_match_builtin])
+
+AS_BOX([Recover after permission link removal from role])
+check ovn-sbctl remove RBAC_Role ovn-controller permissions Chassis
+OVS_WAIT_UNTIL([rbr_match_builtin])
+
+AS_BOX([Clean unwanted permission added to role])
+AT_CHECK([ovn-sbctl --id=@nr create RBAC_Permission table=Load_Balancer \
+-- set RBAC_Role ovn-controller permissions:Load_Balancer=@nr | uuidfilt], [0], [<0>
+])
+OVS_WAIT_UNTIL([rbr_match_builtin])
+
+AS_BOX([Clean duplicated role])
+AT_CHECK([ovn-sbctl create RBAC_Role name=ovn-controller | uuidfilt], [0], [<0>
+])
+OVS_WAIT_UNTIL([rbr_match_builtin])
+
+AS_BOX([Ensure custom role and permission are not automatically deleted])
+PERM_UID=$(fetch_column RBAC_Permission _uuid table=Load_Balancer)
+ovn-sbctl create RBAC_Role name=custom-role permissions:Load_Balancer=$PERM_UID
+OVS_WAIT_UNTIL([rbr_match_builtin])
+check rbr_match_custom
+
+AT_CLEANUP
+