DAOS-15288 client: Verify user/group for daos_cont_set_owner (#14290)

- Ensure the requested user/group exists before setting it. - Add a second API, daos_cont_set_owner_no_check(), for the case where the new owner/group can't be verified locally. - Modify daos_test to verify both check and no_check cases. - Add --no-check flag to daos cont set-owner. Signed-off-by: Kris Jacque <[email protected]>
daos-stack · May 20, 2024 · 2a9ea3a · 2a9ea3a
1 parent b090745
commit 2a9ea3a
Show file tree

Hide file tree

Showing 14 changed files with 382 additions and 73 deletions.
diff --git a/src/client/api/container.c b/src/client/api/container.c
@@ -1,5 +1,5 @@
 /**
- * (C) Copyright 2015-2023 Intel Corporation.
+ * (C) Copyright 2015-2024 Intel Corporation.
  *
  * SPDX-License-Identifier: BSD-2-Clause-Patent
  */
@@ -444,9 +444,9 @@ daos_cont_delete_acl(daos_handle_t coh, enum daos_acl_principal_type type,
 	return dc_task_schedule(task, true);
 }
 
-int
-daos_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group,
-		    daos_event_t *ev)
+static int
+cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group, bool check_exists,
+	       daos_event_t *ev)
 {
 	daos_prop_t	*prop;
 	uint32_t	nr = 0;
@@ -459,6 +459,16 @@ daos_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group,
 			return -DER_INVAL;
 		}
 
+		if (check_exists) {
+			uid_t uid;
+
+			rc = daos_acl_principal_to_uid(user, &uid);
+			if (rc != 0) {
+				DL_ERROR(rc, "unable to determine local ID for user %s", user);
+				return rc;
+			}
+		}
+
 		nr++;
 	}
 
@@ -468,6 +478,16 @@ daos_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group,
 			return -DER_INVAL;
 		}
 
+		if (check_exists) {
+			gid_t gid;
+
+			rc = daos_acl_principal_to_gid(group, &gid);
+			if (rc != 0) {
+				DL_ERROR(rc, "unable to determine local ID for group %s", group);
+				return rc;
+			}
+		}
+
 		nr++;
 	}
 
@@ -500,6 +520,18 @@ daos_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group,
 	return rc;
 }
 
+int
+daos_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group, daos_event_t *ev)
+{
+	return cont_set_owner(coh, user, group, true, ev);
+}
+
+int
+daos_cont_set_owner_no_check(daos_handle_t coh, d_string_t user, d_string_t group, daos_event_t *ev)
+{
+	return cont_set_owner(coh, user, group, false, ev);
+}
+
 int
 daos_cont_aggregate(daos_handle_t coh, daos_epoch_t epoch, daos_event_t *ev)
 {

diff --git a/src/client/dfs/cont.c b/src/client/dfs/cont.c
@@ -1256,11 +1256,21 @@ dfs_get_size_by_oid(dfs_t *dfs, daos_obj_id_t oid, daos_size_t chunk_size, daos_
 	return daos_der2errno(rc);
 }
 
+inline static bool
+is_uid_invalid(uid_t uid)
+{
+	return uid == (uid_t)-1;
+}
+
+inline static bool
+is_gid_invalid(gid_t gid)
+{
+	return gid == (gid_t)-1;
+}
+
 int
-dfs_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group)
+dfs_cont_set_owner(daos_handle_t coh, d_string_t user, uid_t uid, d_string_t group, gid_t gid)
 {
-	uid_t                      uid;
-	gid_t                      gid;
 	daos_key_t                 dkey;
 	d_sg_list_t                sgl;
 	d_iov_t                    sg_iovs[4];
@@ -1325,10 +1335,13 @@ dfs_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group)
 	i++;
 
 	if (user != NULL) {
-		rc = daos_acl_principal_to_uid(user, &uid);
-		if (rc) {
-			D_ERROR("daos_acl_principal_to_uid() failed: " DF_RC "\n", DP_RC(rc));
-			D_GOTO(out_prop, rc = daos_der2errno(rc));
+		if (is_uid_invalid(uid)) {
+			rc = daos_acl_principal_to_uid(user, &uid);
+			if (rc) {
+				D_ERROR("daos_acl_principal_to_uid() failed: " DF_RC "\n",
+					DP_RC(rc));
+				D_GOTO(out_prop, rc = EINVAL);
+			}
 		}
 		d_iov_set(&sg_iovs[i], &uid, sizeof(uid_t));
 		recxs[i].rx_idx = UID_IDX;
@@ -1337,19 +1350,22 @@ dfs_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group)
 	}
 
 	if (group != NULL) {
-		rc = daos_acl_principal_to_gid(group, &gid);
-		if (rc) {
-			D_ERROR("daos_acl_principal_to_gid() failed: " DF_RC "\n", DP_RC(rc));
-			D_GOTO(out_prop, rc = daos_der2errno(rc));
+		if (is_gid_invalid(gid)) {
+			rc = daos_acl_principal_to_gid(group, &gid);
+			if (rc) {
+				D_ERROR("daos_acl_principal_to_gid() failed: " DF_RC "\n",
+					DP_RC(rc));
+				D_GOTO(out_prop, rc = EINVAL);
+			}
 		}
 		d_iov_set(&sg_iovs[i], &gid, sizeof(gid_t));
 		recxs[i].rx_idx = GID_IDX;
 		recxs[i].rx_nr  = sizeof(gid_t);
 		i++;
 	}
 
-	/** set the owner ACL */
-	rc = daos_cont_set_owner(coh, user, group, NULL);
+	/* set the owner ACL - already checked user/group are real above, if needed */
+	rc = daos_cont_set_owner_no_check(coh, user, group, NULL);
 	if (rc) {
 		D_ERROR("daos_cont_set_owner() failed, " DF_RC "\n", DP_RC(rc));
 		D_GOTO(out_prop, rc = daos_der2errno(rc));

diff --git a/src/control/cmd/daos/acl.go b/src/control/cmd/daos/acl.go
@@ -18,6 +18,18 @@ free_ace_list(char **str, size_t str_count)
 		D_FREE(str[i]);
 	D_FREE(str);
 }
+
+uid_t
+invalid_uid(void)
+{
+	return (uid_t)-1;
+}
+
+gid_t
+invalid_gid(void)
+{
+	return (gid_t)-1;
+}
 */
 import "C"
 import (
@@ -360,8 +372,11 @@ func (cmd *containerGetACLCmd) Execute(args []string) error {
 type containerSetOwnerCmd struct {
 	existingContainerCmd
 
-	User  string `long:"user" short:"u" description:"user who will own the container"`
-	Group string `long:"group" short:"g" description:"group who will own the container"`
+	User    string `long:"user" short:"u" description:"user who will own the container"`
+	Uid     *int   `long:"uid" description:"with --no-check, specify a uid for POSIX container ownership"`
+	Group   string `long:"group" short:"g" description:"group who will own the container"`
+	Gid     *int   `long:"gid" description:"with --no-check, specify a gid for POSIX container ownership"`
+	NoCheck bool   `long:"no-check" description:"don't check whether the user/group exists on the local system"`
 }
 
 func (cmd *containerSetOwnerCmd) Execute(args []string) error {
@@ -413,11 +428,41 @@ func (cmd *containerSetOwnerCmd) Execute(args []string) error {
 
 	lType := C.get_dpe_val(&entries[0])
 	if lType == C.DAOS_PROP_CO_LAYOUT_POSIX {
-		if err := dfsError(C.dfs_cont_set_owner(ap.cont, user, group)); err != nil {
-			return errors.Wrapf(err, "%s failed", fsOpString((ap.fs_op)))
+		uid := C.invalid_uid()
+		gid := C.invalid_gid()
+		if cmd.NoCheck {
+			if cmd.User != "" {
+				if cmd.Uid == nil {
+					return errors.New("POSIX container requires --uid to use --no-check")
+				}
+				uid = C.uid_t(*cmd.Uid)
+			}
+
+			if cmd.Group != "" {
+				if cmd.Gid == nil {
+					return errors.New("POSIX container requires --gid to use --no-check")
+				}
+				gid = C.gid_t(*cmd.Gid)
+			}
+		} else if cmd.Uid != nil || cmd.Gid != nil {
+			return errors.New("--no-check is required to use the --uid and --gid options")
+		}
+
+		if err := dfsError(C.dfs_cont_set_owner(ap.cont, user, uid, group, gid)); err != nil {
+			return errors.Wrapf(err, "failed to set owner for POSIX container %s",
+				cmd.ContainerID())
 		}
 	} else {
-		rc := C.daos_cont_set_owner(ap.cont, user, group, nil)
+		if cmd.Uid != nil || cmd.Gid != nil {
+			return errors.New("--uid and --gid options apply for POSIX containers only")
+		}
+
+		var rc C.int
+		if cmd.NoCheck {
+			rc = C.daos_cont_set_owner_no_check(ap.cont, user, group, nil)
+		} else {
+			rc = C.daos_cont_set_owner(ap.cont, user, group, nil)
+		}
 		if err := daosError(rc); err != nil {
 			return errors.Wrapf(err, "failed to set owner for container %s",
 				cmd.ContainerID())

diff --git a/src/include/daos_cont.h b/src/include/daos_cont.h
@@ -1,5 +1,5 @@
 /*
- * (C) Copyright 2020-2023 Intel Corporation.
+ * (C) Copyright 2020-2024 Intel Corporation.
  *
  * SPDX-License-Identifier: BSD-2-Clause-Patent
  */
@@ -441,6 +441,7 @@ daos_cont_delete_acl(daos_handle_t coh, enum daos_acl_principal_type type,
  *			0		Success
  *			-DER_INVAL	Invalid parameter
  *			-DER_NO_PERM	Permission denied
+ *			-DER_NONEXIST	User or group does not exist
  *			-DER_UNREACH	Network is unreachable
  *			-DER_NO_HDL	Invalid container handle
  *			-DER_NOMEM	Out of memory
@@ -449,6 +450,29 @@ int
 daos_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group,
 		    daos_event_t *ev);
 
+/**
+ * Update a container's owner user and/or owner group, without verifying the user/group exists
+ * locally on the machine.
+ *
+ * \param[in]	coh	Container handle
+ * \param[in]	user	New owner user (NULL if not updating)
+ * \param[in]	group	New owner group (NULL if not updating)
+ * \param[in]	ev	Completion event, it is optional and can be NULL.
+ *			The function will run in blocking mode if \a ev is NULL.
+ *
+ * \return		These values will be returned by \a ev::ev_error in
+ *			non-blocking mode:
+ *			0		Success
+ *			-DER_INVAL	Invalid parameter
+ *			-DER_NO_PERM	Permission denied
+ *			-DER_UNREACH	Network is unreachable
+ *			-DER_NO_HDL	Invalid container handle
+ *			-DER_NOMEM	Out of memory
+ */
+int
+daos_cont_set_owner_no_check(daos_handle_t coh, d_string_t user, d_string_t group,
+			     daos_event_t *ev);
+
 /**
  * List the names of all user-defined container attributes.
  *

diff --git a/src/include/daos_fs.h b/src/include/daos_fs.h
@@ -1187,12 +1187,14 @@ dfs_cont_check(daos_handle_t poh, const char *cont, uint64_t flags, const char *
  *
  * \param[in]	coh	Open container handle
  * \param[in]	user	New owner user (NULL if not updating)
+ * \param[in]	uid	New owner uid, if different from user's on local system (-1 otherwise)
  * \param[in]	group	New owner group (NULL if not updating)
+ * \param[in]	gid	New owner gid, if different from group's on local system (-1 otherwise)
  *
  * \return		0 on success, errno code on failure.
  */
 int
-dfs_cont_set_owner(daos_handle_t coh, d_string_t user, d_string_t group);
+dfs_cont_set_owner(daos_handle_t coh, d_string_t user, uid_t uid, d_string_t group, gid_t gid);
 
 /*
  * The Pipeline DFS API (everything under this comment) is under heavy development and should not be

diff --git a/src/tests/ftest/launch.py b/src/tests/ftest/launch.py
@@ -347,8 +347,8 @@ def _run(self, args):
             group.update_test_yaml(
                 logger, args.scm_size, args.scm_mount, args.extra_yaml,
                 args.timeout_multiplier, args.override, args.verbose, args.include_localhost)
-        except (RunException, YamlException):
-            message = "Error modifying the test yaml files"
+        except (RunException, YamlException) as e:
+            message = "Error modifying the test yaml files: {}".format(e)
             status |= self.get_exit_status(1, message, "Setup", sys.exc_info())
         except StorageException:
             message = "Error detecting storage information for test yaml files"

diff --git a/src/tests/ftest/security/cont_acl.py b/src/tests/ftest/security/cont_acl.py
@@ -1,22 +1,27 @@
 """
-  (C) Copyright 2020-2023 Intel Corporation.
+  (C) Copyright 2020-2024 Intel Corporation.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 """
 import os
 
 import security_test_base as secTestBase
+from agent_utils import include_local_host
 from cont_security_test_base import ContSecurityTestBase
 from pool_security_test_base import PoolSecurityTestBase
 
 
-class DaosContainterSecurityTest(ContSecurityTestBase, PoolSecurityTestBase):
+class DaosContainerSecurityTest(ContSecurityTestBase, PoolSecurityTestBase):
     # pylint: disable=too-few-public-methods,too-many-ancestors
     """Test daos_container user acls.
 
     :avocado: recursive
     """
 
+    def _get_acl_file_name(self):
+        return os.path.join(self.tmp, self.params.get("acl_file_name", "/run/container_acl/*",
+                                                      "cont_test_acl.txt"))
+
     def test_container_user_acl(self):
         """
         Description:
@@ -56,7 +61,7 @@ def test_container_user_acl(self):
         :avocado: tags=all,full_regression
         :avocado: tags=vm
         :avocado: tags=security,container,container_acl,cont_user_sec,cont_group_sec,cont_sec
-        :avocado: tags=DaosContainterSecurityTest,test_container_user_acl
+        :avocado: tags=DaosContainerSecurityTest,test_container_user_acl
         """
 
         # (1)Setup
@@ -70,12 +75,10 @@ def test_container_user_acl(self):
         property_name, property_value = self.params.get(
             "property", "/run/container_acl/*")
         secTestBase.add_del_user(
-            self.hostlist_clients, "useradd", new_test_user)
+            include_local_host(self.hostlist_clients), "useradd", new_test_user)
         secTestBase.add_del_user(
-            self.hostlist_clients, "groupadd", new_test_group)
-        acl_file_name = os.path.join(
-            self.tmp, self.params.get(
-                "acl_file_name", "/run/container_acl/*", "cont_test_acl.txt"))
+            include_local_host(self.hostlist_clients), "groupadd", new_test_group)
+        acl_file_name = self._get_acl_file_name()
         test_user = self.params.get(
             "testuser", "/run/container_acl/daos_user/*")
         test_user_type = secTestBase.get_user_type(test_user)

diff --git a/src/tests/ftest/security/cont_acl.yaml b/src/tests/ftest/security/cont_acl.yaml
@@ -1,7 +1,7 @@
 # change host names to your reserved nodes, the
 # required quantity is indicated by the placeholders
 hosts:
-  test_servers: 2
+  test_servers: 1
   test_clients: 1
 timeout: 1200
 server_config: