Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-403744: Implement other_config operations for task #6239

Merged

Conversation

contificate
Copy link
Contributor

Currently, users with read-only permissions can freely manipulate their own tasks (create, cancel, destroy, etc.). However, they cannot easily manipulate the other_config field. Some keys are specified in the datamodel as being writeable by subjects with the VM operator role.

However, RBAC checking for keys was only ever implemented for the individual "add" and "remove" operations, not the set_other_config operation. This makes sense because the typical scenario is that the required writer role for an "other_config" field is more privileged than each of the individually-specified key-specific roles.

In the case of tasks, the desired role for the set_other_config operation is the read-only role. However, we cannot simply demote the required "writer role" for the other_config field, because:

  1. We must maintain key-based RBAC checks for the operation. For
    example, if a read-only user attempts to modify a task's other_config
    using set_other_config, the operation must fail if they've attempted
    to modify the value mapped to by some key that they are not
    privileged to use.

  2. Key-based RBAC checking is special cased to add_to and
    remove_from. You can attempt to ascribe key-related role
    restrictions to arbitrary messages but these will all fail at
    runtime, when invoked - because Rbac.check is special cased to expect
    to be able to extract out the key name from a key argument it
    receives, which is emitted for add_to and remove_from.

  3. There is an additional restriction that read-only users should not be
    able to modify arbitrary tasks, only their own.

Both of these points require a custom implementation. To this end, we mark other_config as read-only (DynamicRO) and implement custom handlers for the add_to, remove_from, and set operations. In doing so, we implement a subset of the RBAC protection logic for keys.

This custom implementation amounts to a relaxation of the usual RBAC rules: where add_to and remove_from (both purely destructive operations) cite a protected key (that they are not permitted to write), RBAC fails. In the custom implementation of set_other_config, an under-privileged session can cite any key so long as their change is not destructive (it must preserve what is already there).


The motivation for this change is in fusing serial add_to_other_config and remove_from_other_config operations into a single set_other_config call. I've included (possibly excessive) commentary within the code as the RBAC checking stuff is not touched a lot. This change may not be desirable overall, as it has low impact.

@contificate contificate marked this pull request as ready for review January 20, 2025 13:27
@contificate contificate force-pushed the private/cbarr/CA-403744 branch from 965ab3d to e49438c Compare January 21, 2025 11:53
@lindig
Copy link
Contributor

lindig commented Jan 23, 2025

What is the use case to let r/o users to manipulate the other-config field? I understand that symmetry with other operations is desirable; on the other hand, other-config is mostly an escape hatch for clients that in this case would not be available.

@contificate
Copy link
Contributor Author

What is the use case to let r/o users to manipulate the other-config field? I understand that symmetry with other operations is desirable; on the other hand, other-config is mostly an escape hatch for clients that in this case would not be available.

The exact use case is to fuse a bunch of (add_to|remove_from)_other_config calls into a single set_other_config call. Currently, the call requires higher privileges than the add and remove operations because the usual situation is that set is highly privileged and key-related RBAC permissions exist to grant access to specific keys for specific roles (like VM operator). In this case, the scenario is inverted: a read-only user can do many things with tasks, but not this call. The PR commentary mentions read-only users, but that's just the most general role (the exact use case is a VM operator who can't mutate keys it can mutate with add/remove via the set call).

This PR resolves the issue but it could go as a "WONTFIX" as it is a lot of special casing for a rather niche use-case. If anything has come out of it, I may be able to document RBAC and its usage in the auto-generated server.ml a bit better.

@lindig
Copy link
Contributor

lindig commented Jan 23, 2025

I generally like this. I understand this such that keys become more structured (using a dot notation) and that keys can be protected using special cases based on their prefix. This is more flexible than using flat names but is also a little fancy given that we haven't run into this problem.

@edwintorok
Copy link
Contributor

There is a merge conflict here

@contificate contificate force-pushed the private/cbarr/CA-403744 branch from e49438c to 5e63d6b Compare February 17, 2025 10:59
@contificate
Copy link
Contributor Author

Rebased atop master and bumped the schema hash.

Currently, users with read-only permissions can freely manipulate their
own tasks (create, cancel, destroy, etc.). However, they cannot easily
manipulate the "other_config" field. Some keys are specified in the
datamodel as being writeable by subjects with the VM operator role.

However, RBAC checking for keys was only ever implemented for the
individual "add" and "remove" operations, not the "set_other_config"
operation. This makes sense because the typical scenario is that the
required writer role for an "other_config" field is more privileged than
each of the individually-specified key-specific roles.

In the case of tasks, the desired role for the set_other_config
operation is the read-only role. However, we cannot simply
demote the required "writer role" for the other_config field, because:

1) We must maintain key-based RBAC checks for the operation. For
   example, if a read-only user attempts to modify a task's other_config
   using set_other_config, the operation must fail if they've attempted
   to modify the value mapped to by some key that they are not
   privileged to use.

2) Key-based RBAC checking is special cased to "add_to" and
   "remove_from". You can attempt to ascribe key-related role
   restrictions to arbitrary messages but these will all fail at
   runtime, when invoked - because Rbac.check is special cased to expect
   to be able to extract out the key name from a "key" argument it
   receives, which is emitted for "add_to" and "remove_from".

3) There is an additional restriction that read-only users should not be
   able to modify arbitrary tasks, only their own.

Both of these points require a custom implementation. To this end, we
mark "other_config" as read-only (DynamicRO) and implement custom
handlers for the "add_to", "remove_from", and "set" operations. In doing
so, we implement a subset of the RBAC protection logic for keys.

This custom implementation amounts to a relaxation of the usual RBAC
rules: where "add_to" and "remove_from" (both purely destructive
operations) cite a protected key (that they are not permitted to write),
RBAC fails. In the custom implementation of "set_other_config", an
under-privileged session can cite any key so long as their change is not
destructive (it must preserve what is already there).

Signed-off-by: Colin James <[email protected]>
@contificate contificate force-pushed the private/cbarr/CA-403744 branch from 5e63d6b to a3f2c6e Compare February 19, 2025 10:26
@contificate contificate added this pull request to the merge queue Feb 20, 2025
Merged via the queue into xapi-project:master with commit 1131ecb Feb 20, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants