CA-403744: Implement other_config operations for task (#6239)

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.

Author: contificate

ocaml/idl/datamodel.ml

@@ -551,6 +551,80 @@ module Task = struct
   let task_allowed_operations =
     Enum ("task_allowed_operations", List.map operation_enum [cancel; destroy])
 
+  module Special = struct
+    (* These keys are usually ascribed to the field directly but,
+       since we are providing custom implementations, we ascribe them
+       to the messages themselves.
+
+       Note that only the "add_to" and "remove_from" messages are
+       protected by these keys. This is because the current RBAC logic
+       is special cased to those messages. The "set_other_config"
+       message has a relaxed RBAC restriction by comparison, and its
+       checking logic is defined in terms of the permissions created
+       for the "add_to" and "remove_from" operations.
+
+       The difference is subtle: if a session attempts to perform
+       "add_to"/"remove_from" upon "other_config", those operations
+       are purely destructive and RBAC checking can be done by the
+       current logic in Rbac.check (which guards the action). However,
+       in the case of "set_other_config", we relax the restriction and
+       must do the RBAC checking ourselves. The relaxed restriction is
+       that the call may maintain entries that it cannot change
+       itself. This means a read-only user can technically supply a
+       map containing privileged entries, so long as those entries are
+       already present. This allows read-only users to update a subset
+       of the entries within "other_config".
+    *)
+    let protected_keys =
+      [
+        ("applies_to", _R_VM_OP)
+      ; ("XenCenterUUID", _R_VM_OP)
+      ; ("XenCenterMeddlingActionTitle", _R_VM_OP)
+      ]
+
+    let call = call ~lifecycle:[] ~errs:[] ~allowed_roles:_R_READ_ONLY
+
+    let add_to_other_config =
+      call ~name:"add_to_other_config"
+        ~doc:
+          "Add the given key-value pair to the other_config field of the given \
+           task."
+        ~params:
+          [
+            (Ref _task, "self", "Task object to modify")
+          ; (String, "key", "Key to add")
+          ; (String, "value", "Value to add")
+          ]
+        ~map_keys_roles:protected_keys ()
+
+    let remove_from_other_config =
+      call ~name:"remove_from_other_config"
+        ~doc:
+          "Remove the given key and its corresponding value from the \
+           other_config field of the given task. If the key is not in that \
+           Map, then do nothing."
+        ~params:
+          [
+            (Ref _task, "self", "Task object to modify")
+          ; (String, "key", "Key of entry to remove")
+          ]
+          (* Privileged key permissions are generated for each of these protected keys. *)
+        ~map_keys_roles:protected_keys ()
+
+    (* We cannot cite the protected keys here as the current RBAC
+       logic only works for "add_to" and "remove_from" and, even if it
+       did, it is too strict. *)
+    let set_other_config =
+      call ~name:"set_other_config"
+        ~doc:"Set the other_config field of the given task."
+        ~params:
+          [
+            (Ref _task, "self", "Task object to modify")
+          ; (Map (String, String), "value", "New value to set")
+          ]
+        ()
+  end
+
   let t =
     create_obj ~in_db:true
       ~lifecycle:[(Published, rel_rio, "A long-running asynchronous task")]
@@ -567,6 +641,9 @@ module Task = struct
         ; set_progress
         ; set_result
         ; set_error_info
+        ; Special.add_to_other_config
+        ; Special.remove_from_other_config
+        ; Special.set_other_config
         ]
       ~contents:
         ([
@@ -716,7 +793,7 @@ module Task = struct
               "error_info"
               "if the task has failed, this field contains the set of \
                associated error strings. Undefined otherwise."
-          ; field
+          ; field ~qualifier:DynamicRO
               ~lifecycle:[(Published, rel_miami, "additional configuration")]
               ~default_value:(Some (VMap []))
               ~ty:(Map (String, String))

ocaml/idl/datamodel_lifecycle.ml

@@ -247,5 +247,11 @@ let prototyped_of_message = function
       Some "22.27.0"
   | "pool", "set_custom_uefi_certificates" ->
       Some "24.0.0"
+  | "task", "set_other_config" ->
+      Some "25.2.0-next"
+  | "task", "remove_from_other_config" ->
+      Some "25.2.0-next"
+  | "task", "add_to_other_config" ->
+      Some "25.2.0-next"
   | _ ->
       None

ocaml/idl/schematest.ml

@@ -3,7 +3,7 @@ let hash x = Digest.string x |> Digest.to_hex
 (* BEWARE: if this changes, check that schema has been bumped accordingly in
    ocaml/idl/datamodel_common.ml, usually schema_minor_vsn *)
 
-let last_known_schema_hash = "05ac9223f9c17b07b12e328d5dc3db52"
+let last_known_schema_hash = "e6b99e0d07ccf68df8f45c851e5d8dbf"
 
 let current_schema_hash : string =
   let open Datamodel_types in

ocaml/xapi/rbac.ml

@@ -161,80 +161,95 @@ let is_permission_in_session ~session_id ~permission ~session =
 
 open Db_actions
 
-(* look up the list generated in xapi_session.get_permissions *)
-let is_access_allowed ~__context ~session_id ~permission =
-  (* always allow local system access *)
-  if Session_check.is_local_session __context session_id then
-    true (* normal user session *)
+(* Given a list of permissions, determine if the given session is
+   permitted to perform the related actions. If not, stop and return the
+   first disallowed permission. This stops us doing redundant checks
+   but also is consistent with the current RBAC error reporting, where
+   a single action is usually reported. *)
+let find_first_disallowed_permission ~__context ~session_id ~permissions =
+  let is_local_session () =
+    Session_check.is_local_session __context session_id
+  in
+  let doesn't_have_permission session permission =
+    is_permission_in_session ~session_id ~permission ~session = false
+  in
+  (* Test session properties before querying permission sets. *)
+  if is_local_session () then
+    None
   else
     let session = DB_Action.Session.get_record ~__context ~self:session_id in
-    (* the root user can always execute anything *)
     if session.API.session_is_local_superuser then
-      true
-    (* not root user, so let's decide if permission is allowed or denied *)
+      None
     else
-      is_permission_in_session ~session_id ~permission ~session
+      List.find_opt (doesn't_have_permission session) permissions
+
+(* Determine if session has a given permission. *)
+let is_access_allowed ~__context ~session_id ~permission =
+  find_first_disallowed_permission ~__context ~session_id
+    ~permissions:[permission]
+  |> Option.is_none
+
+let get_session_of_context ~__context ~permission =
+  try Context.get_session_id __context
+  with Failure _ ->
+    let msg = "no session in context" in
+    raise Api_errors.(Server_error (rbac_permission_denied, [permission; msg]))
+
+let disallowed_permission_exn ?(extra_dmsg = "") ?(extra_msg = "") ~__context
+    ~permission ~action =
+  let session_id = get_session_of_context ~__context ~permission in
+  let allowed_roles =
+    try
+      Xapi_role.get_by_permission_name_label ~__context ~label:permission
+      |> List.map (fun self -> Xapi_role.get_name_label ~__context ~self)
+      |> String.concat ", "
+    with e ->
+      debug "Could not obtain allowed roles for %s (%s)" permission
+        (ExnHelper.string_of_exn e) ;
+      "<Could not obtain the list.>"
+  in
+  let msg =
+    Printf.sprintf
+      "No permission in user session. (Roles with this permission: %s)%s"
+      allowed_roles extra_msg
+  in
+  debug "%s[%s]: %s %s %s" action permission msg (trackid session_id) extra_dmsg ;
+  raise Api_errors.(Server_error (rbac_permission_denied, [permission; msg]))
 
-(* Execute fn if rbac access is allowed for action, otherwise fails. *)
 let nofn () = ()
 
 let check ?(extra_dmsg = "") ?(extra_msg = "") ?args ?(keys = []) ~__context ~fn
     session_id action =
   let permission = permission_of_action action ?args ~keys in
-  if is_access_allowed ~__context ~session_id ~permission then (
-    (* allow access to action *)
+  let allow_access () =
+    (* Allow access to action. *)
     let sexpr_of_args = Rbac_audit.allowed_pre_fn ~__context ~action ?args () in
     try
-      let result = fn () (* call rbac-protected function *) in
+      (* Call the RBAC-protected function. *)
+      let result = fn () in
       Rbac_audit.allowed_post_fn_ok ~__context ~session_id ~action ~permission
         ?sexpr_of_args ?args ~result () ;
       result
     with error ->
+      (* Catch all exceptions and log to RBAC audit log. *)
       Backtrace.is_important error ;
-      (* catch all exceptions *)
       Rbac_audit.allowed_post_fn_error ~__context ~session_id ~action
         ~permission ?sexpr_of_args ?args ~error () ;
+      (* Re-raise. *)
       raise error
-  ) else (* deny access to action *)
-    let allowed_roles_string =
-      try
-        let allowed_roles =
-          Xapi_role.get_by_permission_name_label ~__context ~label:permission
-        in
-        List.fold_left
-          (fun acc allowed_role ->
-            acc
-            ^ (if acc = "" then "" else ", ")
-            ^ Xapi_role.get_name_label ~__context ~self:allowed_role
-          )
-          "" allowed_roles
-      with e ->
-        debug "Could not obtain allowed roles for %s (%s)" permission
-          (ExnHelper.string_of_exn e) ;
-        "<Could not obtain the list.>"
-    in
-    let msg =
-      Printf.sprintf
-        "No permission in user session. (Roles with this permission: %s)%s"
-        allowed_roles_string extra_msg
-    in
-    debug "%s[%s]: %s %s %s" action permission msg (trackid session_id)
-      extra_dmsg ;
+  in
+  let deny_access () =
+    (*  Deny access to action, raising an exception. *)
     Rbac_audit.denied ~__context ~session_id ~action ~permission ?args () ;
     raise
-      (Api_errors.Server_error
-         (Api_errors.rbac_permission_denied, [permission; msg])
-      )
-
-let get_session_of_context ~__context ~permission =
-  try Context.get_session_id __context
-  with Failure _ ->
-    raise
-      (Api_errors.Server_error
-         ( Api_errors.rbac_permission_denied
-         , [permission; "no session in context"]
-         )
+      (disallowed_permission_exn ~extra_dmsg ~extra_msg ~__context ~permission
+         ~action
       )
+  in
+  if is_access_allowed ~__context ~session_id ~permission then
+    allow_access ()
+  else
+    deny_access ()
 
 let assert_permission_name ~__context ~permission =
   let session_id = get_session_of_context ~__context ~permission in

ocaml/xapi/rbac.mli

@@ -22,6 +22,26 @@ val is_access_allowed :
     (on the coordinator only) to benefit successive queries for the
     same session. *)
 
+val find_first_disallowed_permission :
+     __context:Context.t
+  -> session_id:[`session] Ref.t
+  -> permissions:string list
+  -> string option
+(** Given a list of permissions, determine if the given session is
+    permitted to perform the related actions. If not, stop and return
+    the first disallowed permssion (without considering the remaining ones). *)
+
+val disallowed_permission_exn :
+     ?extra_dmsg:string
+  -> ?extra_msg:string
+  -> __context:Context.t
+  -> permission:string
+  -> action:string
+  -> exn
+(** Create an RBAC_PERMISSION_DENIED exception for the given
+    permission. Attempts to report the role(s) which do have the given
+    permission (if any). *)
+
 val check :
      ?extra_dmsg:string
   -> ?extra_msg:string