On 11/8/24 2:29 PM, Dumitru Ceara wrote: > On 11/8/24 12:26, Aleksandr Smirnov (K2 Cloud) wrote: >> On 11/8/24 12:43 PM, Dumitru Ceara wrote: >>> On 11/8/24 10:14, Aleksandr Smirnov (K2 Cloud) wrote: >>>>> On 10/4/24 13:41, Aleksandr Smirnov 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]> >>>>>> --- >>>>> Hi Aleksandr, >>>>> >>>>> Sorry for the delay in reviewing this patch. Please see some comments >>>>> below. >>>>> >>>>> Also cc-ing Han, Numan and Mark as RBAC is very important and I want to >>>>> make >>>>> sure we don't miss anything. >>>> Hi Dumitru, >>>> >>> Hi Aleksandr, >> Hi Dumitru, >> >> Should I take a break and wait until other guys looked into review or I >> need to adopt your comments right now and relaunch review? >> > From my perspective, the patch would be OK to merge once my comments are > addressed. I only explicitly asked for other maintainers to have a look > because I think it's a critical area and I wanted to make sure I'm not > missing anything important. > > I think you should post v2 already if that's OK with you too. > > Thanks!
Hi Dumitru, I have your comments adopted with minor differences: ARRAY_SIZE() --> ARRAY_SIZE() - 1 (because of fake element at end of array) ovn-sbctl get --> fetch_column only where it is possible New patch https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Thanks, Alexander >>>> Thank you for your review! >>>> >>>> Generally I always agree with minor comments while they are really working. >>>> >>>> For big comments see my answers below. >>>> >>>> >>>>>> northd/ovn-northd.c | 120 >>>>>> ++++++++++++++++++++++++++++++-------------- >>>>>> tests/ovn-northd.at | 80 +++++++++++++++++++++++++++++ >>>>>> 2 files changed, 163 insertions(+), 37 deletions(-) >>>>>> >>>>>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >>>>>> index d71114f35..8978e674e 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,74 @@ 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_SAFE (perm_row, ovnsb_idl) { >>>>> No need for _SAFE here. >>>>> >>>>>> + 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; >>>>>> + bool rebuild_role_perm = false; >>>>>> + int pcfg_len, i; >>>>>> >>>>>> + /* >>>>>> + * Make sure predefined permissions are presented in the >>>>>> RBAC_Permissions >>>>>> + * table. Otherwise create consistent permissions. >>>>>> + */ >>>>>> for (pcfg = rbac_perm_cfg; pcfg->table; pcfg++) { >>>>>> - pcfg->row = NULL; >>>>>> + 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,9 +398,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 a permission list attached to the role 'ovn-controller' >>>>> Nit: s/Make sure a permission/Make sure the permission/ >>>>> >>>>>> + * exactly matches to predefined permissions. >>>>>> + * Reassign permission list to the role if any difference has found. >>>>>> + */ >>>>>> + for (pcfg = rbac_perm_cfg, pcfg_len = 0; pcfg->table; pcfg++, >>>>>> pcfg_len++) { >>>>>> + for (i = 0; i < rbac_role->n_permissions; i++) {> + >>>>>> if (strcmp(pcfg->table, rbac_role->key_permissions[i]) == 0 >>>>>> + && pcfg->row == rbac_role->value_permissions[i]) { >>>>>> + break; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + if (i == rbac_role->n_permissions) { >>>>>> + rebuild_role_perm = true; >>>>>> + break; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + if (pcfg_len != rbac_role->n_permissions) { >>>>>> + rebuild_role_perm = true; >>>>>> + } >>>>>> + >>>>>> + /* >>>>>> + * Rebuild role's permission list. >>>>>> + */ >>>>>> + if (rebuild_role_perm) { >>>>>> + sbrec_rbac_role_set_permissions(rbac_role, NULL, NULL, 0); >>>>>> + >>>>>> + for (pcfg = rbac_perm_cfg; pcfg->table; pcfg++) { >>>>>> + sbrec_rbac_role_update_permissions_setkey(rbac_role, >>>>>> + pcfg->table, >>>>>> + pcfg->row); >>>>>> } >>>>>> } >>>>> I think I'd rewrite this part as: >>>>> >>>>> [...] >>>>> if (ARRAY_SIZE(rbac_perm_cfg) != 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); >>>>> } >>>>> } >>>>> >>>>> What do you think? >>>> I think your proposal is good if we feel ourselves comfortable with >>>> 'goto' logic. Also I had imagination that local variables needs to be >>>> gathered at top of function. If it is wrong I totally agree with your >>>> change. >>>> >>> We try to avoid gotos in general but for situations like this one when >>> they simplify the code and the control path is clear they're fine. We >>> use them in similar ways in a few places in the code base. >>> >>> Regarding variables, we should try to define them such that we minimize >>> as much as possiblethe scope they can be used in. This is partially >>> covered by our coding style guidelines: >>> >>> https://github.com/ovn-org/ovn/blob/main/Documentation/internals/contributing/coding-style.rst#c-dialect >>> >>>>>> } >>>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>>>>> index d459c23c0..3c6b719c1 100644 >>>>>> --- a/tests/ovn-northd.at >>>>>> +++ b/tests/ovn-northd.at >>>>>> @@ -13867,3 +13867,83 @@ check_no_redirect >>>>>> >>>>>> AT_CLEANUP >>>>>> ]) >>>>>> + >>>>>> +AT_SETUP([RBAC -- Recover builtin role and permissions]) >>>>>> +ovn_start >>>>>> + >>>>>> +RBR_BUILTIN_PNAMES=`ovn-sbctl get RBAC_Role ovn-controller permissions >>>>>> | uuidfilt | sed -e 's/<[[^<>]]>*//g' | tr -d '=,{}'` >>>>> This can be simplified a bit by using fetch_column: >>>>> RBR_BUILTIN_PNAMES=$(fetch_column RBAC_Role permissions >>>>> name=ovn-controller | uuidfilt | sed -e 's/<[[^<>]]>*//g' | tr -d '=,{}') >>>>> >>>>> The same comment applies to multiple instances below. >>>>> >>>>>> +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=`ovn-sbctl get RBAC_Role ovn-controller >>>>>> permissions | 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=`ovn-sbctl get RBAC_Role custom-role permissions >>>>>> | 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=`ovn-sbctl get RBAC_Permission Load_Balancer _uuid` >>>>>> +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 >>>>>> + >>>>> Regards, >>>>> Dumitru >>>>> >>> Thanks, >>> Dumitru >>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
