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