From b4bed4392e800cb06cf7f8fb3dba6e82d904bc24 Mon Sep 17 00:00:00 2001 From: Aleksandr Smirnov Date: Tue, 12 Nov 2024 11:39:36 +0300 Subject: [PATCH] northd: Fix issues in RBAC tables recovery. 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 Tested-by: Aleksandr Gnatyuk Signed-off-by: Dumitru Ceara (cherry picked from commit 90dc1c235dee0e19face4a5d042549e2db96aa31) --- northd/ovn-northd.c | 119 +++++++++++++++++++++++++++++--------------- tests/ovn-northd.at | 81 ++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 39 deletions(-) diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 096698fffc..094a754773 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 2d132f27cc..a78223ca74 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -12414,3 +12414,84 @@ check_engine_stats northd recompute nocompute check_engine_stats lflow recompute nocompute 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