On 12/11/24 2:12 PM, Ales Musil wrote: > On Tue, Nov 12, 2024 at 9:40 AM Aleksandr Smirnov <[email protected]> > wrote: > >> From: Aleksandr Smirnov <[email protected]> >> >> 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 <[email protected]> >> Tested-by: Aleksandr Gnatyuk <[email protected]> >> --- >> > Review comments (from Dumitru) were adopted. >> >> --- >> northd/ovn-northd.c | 119 +++++++++++++++++++++++++++++--------------- >> tests/ovn-northd.at | 82 ++++++++++++++++++++++++++++++ >> 2 files changed, 162 insertions(+), 39 deletions(-) >> >> 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 >> + >> -- >> 2.47.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Reviewed-by: Ales Musil <[email protected]> >
Thanks, Aleksandr and Ales! Applied to main and backported to 24.09 and 24.03. I also added Aleksandr to the AUTHORS file. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
