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