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