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,
>
> 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
