guest/spec, rego, rego_test: Fix GetUserInfo by micromaomao · Pull Request #2465 · microsoft/hcsshim

micromaomao · 2025-06-30T22:20:47Z

This PR fixes an issue with the latest GetUserInfo implementation and adds some unit tests for this.

Issue

A workload using a custom user for one of its container now fails to start with policy denial: invalid user.

In the "broken" version, we see this in the input into Rego:

...
    "user": {
      "id": "100",
      "name": ""
    },
...

in the working version the input has

...
    "user": {
      "id": "100",
      "name": "curl_user"
    },
...

The issue seems to be that the new GetUserInfo, when given a valid userstr, does not return any username/groupnames in the IDName structs, only IDs. Therefore, policies written to enforce the name will fail.

Testing

Tested on confidential. Not tested non-conf yet.

micromaomao · 2025-07-01T15:15:00Z

Added one more test case, made some test changes

helsaawy · 2025-07-02T16:30:00Z

+			err = errors.Wrapf(err, "failed to find user by username: %s", parts[0])
+			return


can we stick to explicit returns?
the flow is easier to read for than the err = <>; return statements.

Suggested change

err = errors.Wrapf(err, "failed to find user by username: %s", parts[0])

return

return res, errors.Wrapf(err, "failed to find user by username: %s", parts[0])

@helsaawy fixed - can you review?

btw, the other place where I've also made use of named returns is in regoEnforcer.GetUserInfo. In that case there are more return variables and return IDName{}, nil, "", err felt a bit awkward to me, but if you want I'm happy to change that to use explicit returns as well.

I think the general recommendation is to avoid naked returns if the function isn't a couple lines, since its difficult to track down visually what is being returned and where its being set
though no need to belabor the point here

micromaomao · 2025-07-10T14:51:38Z

@helsaawy I rebased it to main to resolve the merge conflict, but didn't realize there is such a huge change on main (otherwise should probably have just done a merge). There is no code changes:

> git range-diff origin/main a0aedfa 3c4c79a 
1:  ad23bcfc8 = 1:  38242b74e guest/spec, rego: Fix user/group/additionalGIDs enforcement
2:  6617071ab ! 2:  fdc032ffa securitypolicyenforcer_rego: Re-use getUser/getGroup from specGuest in GetUserInfo
    @@ vendor/modules.txt: github.com/opencontainers/image-spec/specs-go/v1
      ## explicit; go 1.22
      github.com/opencontainers/runc/libcontainer/devices
     -github.com/opencontainers/runc/libcontainer/user
    - # github.com/opencontainers/runtime-spec v1.2.0
    + # github.com/opencontainers/runtime-spec v1.2.1
      ## explicit
      github.com/opencontainers/runtime-spec/specs-go
3:  737138a00 = 3:  1285415f7 rego: GetUserInfo: explicitly pass in rootPath
4:  e2b49dd68 = 4:  de5bd679b regoEnforcer: do not allow failure from ParseUserStr
5:  a0aedfa39 = 5:  3c4c79a5c regopolicy_test: Add tests for GetUserInfo

Can you approve again and merge this?

ambarve · 2025-07-15T14:45:21Z

-		userInfo, err := getUser(passwdPath, func(user user.User) bool {
-			return uint32(user.Uid) == uid
-		})
+	if !parsedUserStr {


Can this fallback code also be refactored? I think the ParseUserStr method already has same/similar code. Can we do something like :

if process.User.Username != "" { specGuest.ParseUserStr(rootPath, process.User.Username) } else { specGuest.ParseUserStr(rotPath, fmt.Sprintf("%d:%d",process.User.UID, process.User.GID) }

ambarve · 2025-07-15T14:47:48Z

 	return nil
 }

+type ParseUserStrResult struct {


nit: I think ParsedUserStr/ParsedUserStrResult is slightly better.

micromaomao · 2025-07-21T09:40:01Z

Hi @ambarve, this set of changes is already in the C-LCOW release branch (and this PR is in fact more of a backport into main). Can we merge this, then I can implement your suggestions in another PR which would also get ported to that branch separately?

anmaxvl · 2025-09-03T17:14:43Z

seems like this needs a rebase. we removed 2025 runners.

micromaomao · 2025-09-03T17:21:38Z

seems like this needs a rebase. we removed 2025 runners.

done, pls merge if it passes

jterry75 · 2025-09-11T17:37:58Z

-
-func (policy *regoEnforcer) GetUserInfo(containerID string, process *oci.Process) (IDName, []IDName, string, error) {
-	rootPath := filepath.Join(guestpath.LCOWRootPrefixInUVM, containerID, guestpath.RootfsPath)
+func (policy *regoEnforcer) GetUserInfo(process *oci.Process, rootPath string) (


We should assert that rootPath is not empty IMO. This doesnt make sense and you will accidently end up with a valid path to the root /etc/passwd right?

The code outside is this:

user, groups, umask, err := h.securityPolicyEnforcer.GetUserInfo(settings.OCISpecification.Process, settings.OCISpecification.Root.Path)

and

user, groups, umask, err = h.securityPolicyEnforcer.GetUserInfo(params.OCIProcess, c.spec.Root.Path)

so it really should not be empty. While we can add an assert, if it's empty then it would fail much earlier than this.
Given that this PR is already merged in the C-ACI release branch, any changes would require a separate PR, so I'm probably not adding it here.

btw I will make sure to pick up this suggestion, and the one above, in another set of changes :)

A workload using a custom user for one of its container now fails to start with policy denial: invalid user. In the "broken" version, we see this in the input into Rego: ... "user": { "id": "100", "name": "" }, ... in the working version the input has ... "user": { "id": "100", "name": "curl_user" }, ... The issue seems to be that the new GetUserInfo, when given a valid userstr, does not return any username/groupnames in the IDName structs, only IDs. Therefore, policies written to enforce the name will fail. To fix this, we extend ParseUserStr to return the user and group names, and update GetUserInfo to store the names in the IDName structs. Since the function now contains extra logic to e.g. fetch the group name even when we have known UID/GID, some slight refactoring was done to avoid code repetition. Looking at azcri container_create_windows.go (this file handles LCOW as well) generateContainerSpec and container_create.go:generateUserString, my understanding is that it would always set Username even if in the container JSON it's given as run_as_user/run_as_group with a UID (but the username will be set to a string of the integers). The userstr that GCS sees would only be empty if neither the container JSON nor the container image itself specify any users, in which case we would default to uid 0 and gid 0. Fixes: 7084bd2 ("rego policy enforcer should use the same user parsing logic as GCS"...) Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>

…n GetUserInfo Get rid of some duplicate code in the Rego policy enforcer, by exporting GetUser and GetGroup from guest/spec. Since this code path in the enforcer is now only supposed to happen for rare cases where the user string in the OCI Spec is invalid but we somehow still gets to start the container, removing this duplicate code prevents staleness. Also make both specGuest and the enforcer use moby/sys/user instead of the now deprecated libcontainer/user package, which in the latest version simply redirects to moby/sys/user [1]. [1]: https://github.com/opencontainers/runc/blob/v1.3.0/libcontainer/user/user_deprecated.go Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>

The primary purpose of this is to make testing this function easier. This also reduces assumptions. We remove the containerID parameter as it is thus not needed anymore. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>

Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>

Test various user strings, with or without /etc/{passwd,group}. Make sure that empty user strings defaults to 0:0. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>

micromaomao requested a review from a team as a code owner June 30, 2025 22:20

micromaomao force-pushed the rego-get-user-info branch from 568912a to 47d5b76 Compare July 1, 2025 15:13

micromaomao force-pushed the rego-get-user-info branch from 47d5b76 to 4cfa792 Compare July 1, 2025 15:35

helsaawy requested changes Jul 2, 2025

View reviewed changes

anmaxvl self-assigned this Jul 2, 2025

msscotb assigned helsaawy, ambarve and kiashok and unassigned anmaxvl Jul 2, 2025

micromaomao force-pushed the rego-get-user-info branch from 4cfa792 to a0aedfa Compare July 2, 2025 17:38

helsaawy approved these changes Jul 3, 2025

View reviewed changes

micromaomao force-pushed the rego-get-user-info branch from a0aedfa to 3c4c79a Compare July 10, 2025 14:45

ambarve reviewed Jul 15, 2025

View reviewed changes

ambarve approved these changes Jul 21, 2025

View reviewed changes

micromaomao force-pushed the rego-get-user-info branch from 3c4c79a to a3f6d85 Compare September 3, 2025 17:20

jterry75 reviewed Sep 11, 2025

View reviewed changes

micromaomao added 5 commits September 15, 2025 18:14

rego: GetUserInfo: explicitly pass in rootPath

bb69ddd

The primary purpose of this is to make testing this function easier. This also reduces assumptions. We remove the containerID parameter as it is thus not needed anymore. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>

regoEnforcer: do not allow failure from ParseUserStr

13d29ac

Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>

regopolicy_test: Add tests for GetUserInfo

08db90c

Test various user strings, with or without /etc/{passwd,group}. Make sure that empty user strings defaults to 0:0. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>

micromaomao force-pushed the rego-get-user-info branch from a3f6d85 to 08db90c Compare September 15, 2025 17:17

anmaxvl merged commit 9e98487 into microsoft:main Sep 16, 2025
28 of 29 checks passed

		err = errors.Wrapf(err, "failed to find user by username: %s", parts[0])
		return

	err = errors.Wrapf(err, "failed to find user by username: %s", parts[0])
	return
	return res, errors.Wrapf(err, "failed to find user by username: %s", parts[0])

Uh oh!

Conversation

micromaomao commented Jun 30, 2025

Issue

Testing

Uh oh!

micromaomao commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

helsaawy Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

micromaomao Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micromaomao Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

helsaawy Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

micromaomao commented Jul 10, 2025

Uh oh!

ambarve Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ambarve Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

micromaomao commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmaxvl commented Sep 3, 2025

Uh oh!

micromaomao commented Sep 3, 2025

Uh oh!

jterry75 Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

micromaomao Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

micromaomao Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

micromaomao commented Jul 1, 2025 •

edited

Loading

micromaomao Jul 2, 2025 •

edited

Loading

ambarve Jul 15, 2025 •

edited

Loading

micromaomao commented Jul 21, 2025 •

edited

Loading