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