DAOS-15849 control: Add client uid map to agent config #14381

mjmac · 2024-05-15T16:43:48Z

Allow daos_agent to optionally handle unresolvable client
uids via custom mapping. In deployments where the agent
may not have access to the same user namespace as client
applications (e.g. in containerized deployments), the
client_user_map can provide a fallback mechanism for
resolving the client uids to known usernames for the
purpose of applying ACL permissions tests.

Example agent config:

credential_config:
  client_user_map:
    default:
      user: nobody
      group: nobody
    1000:
      user: joe
      group: blow

Features: control
Required-githooks: true
Change-Id: I72905ccc5ddee27fc2101aa4358a14e352c86253
Signed-off-by: Michael MacDonald mjmac@google.com

github-actions · 2024-05-15T16:44:08Z

Ticket title is 'Provide a mechanism for mapping unknown client uids to user/group names'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-15849

daosbuild1 · 2024-05-15T17:28:33Z

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

daosbuild1 · 2024-05-16T13:02:45Z

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

daosbuild1 · 2024-05-16T15:44:11Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14381/4/testReport/

Allow daos_agent to optionally handle unresolvable client uids via custom mapping. In deployments where the agent may not have access to the same user namespace as client applications (e.g. in containerized deployments), the client_user_map can provide a fallback mechanism for resolving the client uids to known usernames for the purpose of applying ACL permissions tests. Example agent config: credential_config: client_user_map: default: user: nobody group: nobody 1000: user: joe group: blow Features: control Required-githooks: true Change-Id: I72905ccc5ddee27fc2101aa4358a14e352c86253 Signed-off-by: Michael MacDonald <mjmac@google.com>

kjacque

I wouldn't block on any of my suggestions. This is pretty cool. Nice work.

I find myself wondering if there might be some benefit to making use of interface types for this scenario. Having two different data sources for user/group/etc., it seems like a good fit for the concept.

kjacque · 2024-05-21T22:52:16Z

src/control/security/config_test.go

+			for uid, exp := range tc.expMap {
+				gotUser := result.Lookup(uid)
+				if diff := cmp.Diff(exp.User, gotUser.User); diff != "" {
+					t.Fatalf("unexpected User (-want, +got)\n %s", diff)
+				}
+			}
+
+			if expDefUser, found := tc.expMap[defaultMapKey]; found {
+				gotDefUser := result.Lookup(1234567)
+				if diff := cmp.Diff(expDefUser, gotDefUser); diff != "" {
+					t.Fatalf("unexpected DefaultUser (-want, +got)\n %s", diff)
+				}
+			}


Minor - I'd be inclined to break these out into a separate test for the Lookup method.

kjacque · 2024-05-21T23:01:11Z

src/control/lib/control/pool_test.go

-			if tc.req.userExt == nil {
-				tc.req.userExt = mockExt
-			}


Huh, so the test wasn't actually using the mocks for anything?

looks like it was until this change https://github.com/daos-stack/daos/pull/14381/files#diff-b86b7f9f25785e10ce878665ccc27f1a331e16ab733a0ff7729b7e404d0d659fR53

kjacque · 2024-05-21T23:20:43Z

src/control/security/auth/auth_sys.go

+	req.getGroupIdsFn = func() ([]string, error) {
+		u, err := req.user()
+		if err != nil {
+			return nil, err
+		}
+		return u.GroupIds()
+	}
+	req.getGroupNamesFn = func() ([]string, error) {
+		groupIds, err := req.getGroupIdsFn()
+		if err != nil {
+			return nil, err
+		}
+
+		groupNames := make([]string, len(groupIds))
+		for i, gID := range groupIds {
+			g, err := req.getGroupFn(gID)
+			if err != nil {
+				return nil, err
+			}
+			groupNames[i] = g.Name
+		}
+
+		return groupNames, nil
+	}


suggestion - To test the internals of these functions, I'd suggest breaking them out as normal named methods, and keeping the mockable interface as thin as possible.

tanabarr · 2024-05-22T13:36:53Z

src/control/cmd/daos_agent/security_rpc.go

 	if err != nil {
-		m.log.Errorf("%s: failed to get AuthSys struct: %s", info, err)
-		return m.credRespWithStatus(daos.MiscError)
+		if err := func() error {


What's the reason for making this an anonymous function rather than just inline?

tanabarr · 2024-05-22T13:55:27Z

src/control/cmd/daos_agent/security_rpc_test.go

+	}
+	successBytes, err := proto.Marshal(
+		&auth.GetCredResp{
+			Status: 0,


nit: don't need to specify zero value

tanabarr · 2024-05-22T13:55:58Z

src/control/cmd/daos_agent/security_rpc_test.go

+			}(),
+			responses: []response{
+				{
+					cred: nil,


nit: don't need to specify zero value, and again below

tanabarr · 2024-05-22T14:02:22Z

src/control/cmd/daos_agent/security_rpc_test.go

+			defer cleanup()
+
+			mod := NewSecurityModule(log, tc.secCfg)
+			mod.signCredential = func() func(req *auth.CredentialRequest) (*auth.Credential, error) {


Why func() sig {...} () Rather than just sig {...} here and specify var idx int at top-level inside t.Run()?

tanabarr · 2024-05-22T14:06:13Z

src/control/lib/control/pool_test.go

-			if tc.req.userExt == nil {
-				tc.req.userExt = mockExt
-			}


looks like it was until this change https://github.com/daos-stack/daos/pull/14381/files#diff-b86b7f9f25785e10ce878665ccc27f1a331e16ab733a0ff7729b7e404d0d659fR53

tanabarr

Are we sure this change won't enable any undesired security consequences outside container environments? The prospect of nefarious mappings seems like something we really should think about, but perhaps it has? Is the assumption that the mappings can only be applied by an administrator who has write access to the configuration files and there aren't any other vectors that we need to be concerned about?

Also the Commit message seems to me to imply that the changes are only required for testing purposes: "...client_user_map can provide a fallback mechanism for resolving the client uids to known usernames for the purpose of applying ACL permissions tests.". I assume that this is a typo (or me misunderstanding) and the changes are indeed needed for production deployments?

tanabarr · 2024-05-22T14:19:11Z

src/control/security/auth/auth_sys.go

+		DomainInfo      *security.DomainInfo
+		SigningKey      crypto.PrivateKey
+		getHostnameFn   func() (string, error)
+		getUserFn       func(string) (*user.User, error)


nit: would be nice to have aliases for these function signatures

tanabarr · 2024-05-22T14:19:36Z

src/control/security/auth/auth_sys.go

+	req.getGroupIdsFn = func() ([]string, error) {
+		u, err := req.user()
+		if err != nil {
+			return nil, err
+		}
+		return u.GroupIds()
+	}
+	req.getGroupNamesFn = func() ([]string, error) {
+		groupIds, err := req.getGroupIdsFn()
+		if err != nil {
+			return nil, err
+		}
+
+		groupNames := make([]string, len(groupIds))
+		for i, gID := range groupIds {
+			g, err := req.getGroupFn(gID)
+			if err != nil {
+				return nil, err
+			}
+			groupNames[i] = g.Name
+		}
+
+		return groupNames, nil
+	}


kjacque · 2024-05-22T16:02:43Z

Are we sure this change won't enable any undesired security consequences outside container environments? The prospect of nefarious mappings seems like something we really should think about, but perhaps it has? Is the assumption that the mappings can only be applied by an administrator who has write access to the configuration files and there aren't any other vectors that we need to be concerned about?

I thought about this for a bit, too. As long as the admin secures their DAOS config files and certificates from unauthorized users, I think we should be OK. If the admin has secured the system with certificates with appropriate permissions (private key readable by daos_agent only), attackers will not be able to start their own private daos_agent to interface with the system with a custom config either. I see these as the two main attack vectors. @dpquigl can you think of anything else?

tanabarr

Concerns assuaged by @dpquigl 's comments elsewhere:

So if I understand this correctly you have a multi-container environment where the socket for daos-agent is exported from one container and used in another. So when you check the sender’s credentials on the socket you’re getting he euid and egid from the process in the other container. However since the daos-agent container probably only has the users needed for the applications running in the container you can’t do a lookup on the users and groups. This seems like a reasonable solution. NFSv4 has a similar mechanism where it will translate a local uid to a name and back into a local uid on the remote side because of just this issue. The agent is a trusted entity in the security architecture so I agree that as long as you keep the agent integrity its ok for it to take on the additional responsibility of performing the translation. Is the reason this is in the same config file just for convenience? It might be a good idea to have it in a separate file for manageability.

I think moving the new section to a separate file is probably something we don't have bandwidth for currently so I'm happy to merge this now.

mjmac · 2024-06-04T18:25:25Z

Are we sure this change won't enable any undesired security consequences outside container environments? The prospect of nefarious mappings seems like something we really should think about, but perhaps it has? Is the assumption that the mappings can only be applied by an administrator who has write access to the configuration files and there aren't any other vectors that we need to be concerned about?

As Kris & David have pointed out, the agent is considered a trusted component, and only the admin should be able to modify its configuration. As long as the agent trust is maintained, then there's no concern that malicious mappings could be created. If the agent trust is broken (due to file permissions or whatever), then pretty much anything could happen.

Also the Commit message seems to me to imply that the changes are only required for testing purposes: "...client_user_map can provide a fallback mechanism for resolving the client uids to known usernames for the purpose of applying ACL permissions tests.". I assume that this is a typo (or me misunderstanding) and the changes are indeed needed for production deployments?

Perhaps there's a better term, but I was using "test" in the sense that an access request is tested by applying the ACL and either accepting or rejecting the request. Maybe "check" would have been clearer.

mjmac force-pushed the mjmac/DAOS-15849 branch from d283800 to 310423d Compare May 15, 2024 17:44

mjmac force-pushed the mjmac/DAOS-15849 branch from 310423d to 38e87ba Compare May 20, 2024 16:40

mjmac marked this pull request as ready for review May 21, 2024 13:47

mjmac requested review from a team as code owners May 21, 2024 13:47

mjmac force-pushed the mjmac/DAOS-15849 branch from c4d7940 to a5651bf Compare May 21, 2024 19:42

kjacque approved these changes May 21, 2024

View reviewed changes

mjmac requested a review from tanabarr May 22, 2024 13:24

tanabarr reviewed May 22, 2024

View reviewed changes

tanabarr self-requested a review May 29, 2024 21:34

tanabarr approved these changes May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-15849 control: Add client uid map to agent config #14381

DAOS-15849 control: Add client uid map to agent config #14381

mjmac commented May 15, 2024 •

edited

github-actions bot commented May 15, 2024

daosbuild1 commented May 15, 2024

daosbuild1 commented May 16, 2024

daosbuild1 commented May 16, 2024

kjacque left a comment

kjacque May 21, 2024

kjacque May 21, 2024

tanabarr May 22, 2024

kjacque May 21, 2024

tanabarr May 22, 2024

tanabarr May 22, 2024

tanabarr May 22, 2024

tanabarr May 22, 2024

tanabarr May 22, 2024

tanabarr May 22, 2024

tanabarr left a comment •

edited

tanabarr May 22, 2024

tanabarr May 22, 2024

kjacque commented May 22, 2024

tanabarr left a comment

mjmac commented Jun 4, 2024

DAOS-15849 control: Add client uid map to agent config #14381

Are you sure you want to change the base?

DAOS-15849 control: Add client uid map to agent config #14381

Conversation

mjmac commented May 15, 2024 • edited

github-actions bot commented May 15, 2024

daosbuild1 commented May 15, 2024

daosbuild1 commented May 16, 2024

daosbuild1 commented May 16, 2024

kjacque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanabarr left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjacque commented May 22, 2024

tanabarr left a comment

Choose a reason for hiding this comment

mjmac commented Jun 4, 2024

mjmac commented May 15, 2024 •

edited

tanabarr left a comment •

edited