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

kjacque · 2024-05-01T01:13:25Z

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.

Required-githooks: true

Features: container security

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

- 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. Required-githooks: true Features: container security Signed-off-by: Kris Jacque <[email protected]>

github-actions · 2024-05-01T01:13:43Z

Ticket title is 'DAOS contianer set-owner behaves differently between posix and non-posix containers'
Status is 'In Review'
Labels: 'scrubbed,triaged'
https://daosio.atlassian.net/browse/DAOS-15288

daosbuild1 · 2024-05-01T08:18:23Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14290/1/execution/node/1198/log

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

daosbuild1 · 2024-05-02T06:47:55Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14290/2/execution/node/1173/log

Feature: container security

daosbuild1 · 2024-05-03T05:00:58Z

Test stage Functional on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14290/3/testReport/

Also fix existing test to ensure user names are created on all client nodes. Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Features: container security

daosbuild1 · 2024-05-09T04:54:11Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14290/4/execution/node/1454/log

kjacque · 2024-05-09T14:29:07Z

Test failure is an existing issue on master: https://daosio.atlassian.net/issues/DAOS-15124

src/control/cmd/daos/acl.go

daltonbohning

ftest LGTM

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Features: container security

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

daosbuild1 · 2024-05-14T00:36:22Z

Test stage NLT on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-14290/6/display/redirect

daosbuild1 · 2024-05-14T06:26:32Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14290/7/execution/node/1184/log

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Features: container security Allow-unstable-test: true

Features: container security

daosbuild1 · 2024-05-16T20:32:47Z

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14290/14/execution/node/145/log

mchaarawi · 2024-05-17T20:35:48Z

src/include/daos_fs.h

 *
 * \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);


just a side comment for other reviewers, since this was added in master (not in 2.4.2) this is fine, no API breakage.

mchaarawi · 2024-05-17T20:39:09Z

src/client/dfs/cont.c

+static bool
+is_uid_invalid(uid_t uid)
+{
+	return uid == (uid_t)-1;
+}
+
+static bool
+is_gid_invalid(gid_t gid)
+{
+	return gid == (gid_t)-1;
+}
+


just curious why have a function call and not do the == in the if condition below directly?

I thought it communicated more clearly what -1 indicated. Figured the compiler will end up inlining this anyway during optimization.

I just made a change to explicitly inline it.

src/tests/ftest/security/cont_owner.py

src/tests/ftest/security/cont_owner.yaml

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

mjmac

Go changes LGTM.

daltonbohning

ftest LGTM. Just nits. thanks for the updates!

daltonbohning · 2024-05-20T14:53:38Z

src/tests/ftest/security/cont_owner.py

+        :avocado: tags=DaosContainerOwnerTest
+        :avocado: tags=test_container_set_owner_no_check_non_posix


nit - it's preferred to put these on one line

Suggested change

:avocado: tags=DaosContainerOwnerTest

:avocado: tags=test_container_set_owner_no_check_non_posix

:avocado: tags=DaosContainerOwnerTest,test_container_set_owner_no_check_non_posix

daltonbohning · 2024-05-20T14:54:36Z

src/tests/ftest/security/cont_owner.py

+        :avocado: tags=DaosContainerOwnerTest
+        :avocado: tags=test_container_set_owner_no_check_posix


nit

Suggested change

:avocado: tags=DaosContainerOwnerTest

:avocado: tags=test_container_set_owner_no_check_posix

:avocado: tags=DaosContainerOwnerTest,test_container_set_owner_no_check_posix

kjacque self-assigned this May 1, 2024

Update functional tests

3c568e2

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Merge branch 'master' into kjacque/validate-owner-uid

357b4d1

Feature: container security

kjacque added 2 commits May 8, 2024 09:27

Add ftest for --no-check flag

a5b494f

Also fix existing test to ensure user names are created on all client nodes. Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Merge branch 'master' into kjacque/validate-owner-uid

bc5457b

Features: container security

kjacque marked this pull request as ready for review May 9, 2024 14:26

kjacque requested review from a team as code owners May 9, 2024 14:26

kjacque requested review from mjmac, tanabarr and mchaarawi May 9, 2024 14:29

mchaarawi reviewed May 9, 2024

View reviewed changes

src/control/cmd/daos/acl.go Outdated Show resolved Hide resolved

daltonbohning reviewed May 9, 2024

View reviewed changes

kjacque added 5 commits May 10, 2024 14:21

Merge branch 'master' into kjacque/validate-owner-uid

ebc902a

Add --uid and --gid options for POSIX no-check

3a51549

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Fix python linter

44b4917

Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Merge branch 'master' into kjacque/validate-owner-uid

7c71bc7

Features: container security

Fix ftest tags

5ef37d7

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

kjacque added 3 commits May 14, 2024 13:02

Fix paths in test

e88a244

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

Merge branch 'master' into kjacque/validate-owner-uid

5358524

Features: container security Allow-unstable-test: true

Merge branch 'master' into kjacque/validate-owner-uid

7059aec

Features: container security

kjacque requested review from mchaarawi and daltonbohning May 17, 2024 20:10

mchaarawi previously approved these changes May 17, 2024

View reviewed changes

daltonbohning reviewed May 17, 2024

View reviewed changes

kjacque added 2 commits May 17, 2024 17:24

Merge branch 'master' into kjacque/validate-owner-uid

9eb4933

Address review comments

9b9eb22

Features: container security Required-githooks: true Signed-off-by: Kris Jacque <[email protected]>

kjacque dismissed mchaarawi’s stale review via 9b9eb22 May 18, 2024 00:37

kjacque requested review from daltonbohning and mchaarawi May 18, 2024 00:38

mjmac approved these changes May 20, 2024

View reviewed changes

daltonbohning reviewed May 20, 2024

View reviewed changes

tanabarr approved these changes May 20, 2024

View reviewed changes

kjacque requested a review from a team May 20, 2024 15:49

mchaarawi approved these changes May 20, 2024

View reviewed changes

mchaarawi merged commit 2a9ea3a into master May 20, 2024
55 checks passed

mchaarawi deleted the kjacque/validate-owner-uid branch May 20, 2024 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kjacque commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024 •

edited

Loading

daosbuild1 commented May 1, 2024

daosbuild1 commented May 2, 2024

daosbuild1 commented May 3, 2024

daosbuild1 commented May 9, 2024

kjacque commented May 9, 2024

daltonbohning left a comment

daosbuild1 commented May 14, 2024

daosbuild1 commented May 14, 2024

daosbuild1 commented May 16, 2024

mchaarawi May 17, 2024

mchaarawi May 17, 2024

kjacque May 17, 2024

kjacque May 18, 2024

mjmac left a comment •

edited

Loading

daltonbohning left a comment

daltonbohning May 20, 2024

daltonbohning May 20, 2024

		:avocado: tags=DaosContainerOwnerTest
		:avocado: tags=test_container_set_owner_no_check_non_posix

	:avocado: tags=DaosContainerOwnerTest
	:avocado: tags=test_container_set_owner_no_check_non_posix
	:avocado: tags=DaosContainerOwnerTest,test_container_set_owner_no_check_non_posix

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

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

Conversation

kjacque commented May 1, 2024 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented May 1, 2024 • edited Loading

daosbuild1 commented May 1, 2024

daosbuild1 commented May 2, 2024

daosbuild1 commented May 3, 2024

daosbuild1 commented May 9, 2024

kjacque commented May 9, 2024

daltonbohning left a comment

Choose a reason for hiding this comment

daosbuild1 commented May 14, 2024

daosbuild1 commented May 14, 2024

daosbuild1 commented May 16, 2024

mchaarawi May 17, 2024

Choose a reason for hiding this comment

mchaarawi May 17, 2024

Choose a reason for hiding this comment

kjacque May 17, 2024

Choose a reason for hiding this comment

kjacque May 18, 2024

Choose a reason for hiding this comment

mjmac left a comment • edited Loading

Choose a reason for hiding this comment

daltonbohning left a comment

Choose a reason for hiding this comment

daltonbohning May 20, 2024

Choose a reason for hiding this comment

daltonbohning May 20, 2024

Choose a reason for hiding this comment

kjacque commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024 •

edited

Loading

mjmac left a comment •

edited

Loading