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]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
