Behavior change when duplicate additionalGids are specified

[saku3]

Description

In runc v1.3.0, the behavior has changed when specifying duplicate AdditionalGids.

Previously, duplicate group IDs in AdditionalGids were deduplicated.
In the current version, duplicates are no longer removed.

This change in behavior is due to this PR: #3999

Previously, deduplication occurred because the GetAdditionalGroupsPath (which calls GetAdditionalGroups) function stored the GIDs in a map.

The OCI runtime-spec does not define behavior regarding duplicated group IDs.
(Should this be standardized?)

Steps to reproduce the issue

Specify the following fields in the process section of your spec:

"process": {
  "terminal": false,
  "user": {
    "uid": 0,
    "gid": 0,
    "additionalGids": [1000, 2000, 3000, 3000]
  },
  "args": ["id"]
}

runc v1.3.0
Duplicate GIDs appear in the output:

$ runc --version
runc version 1.3.0
commit: v1.3.0-0-g4ca628d1
spec: 1.2.1
go: go1.23.0
libseccomp: 2.5.5

$ runc create testcon
$ runc start testcon
uid=0(root) gid=0(root) groups=1000,2000,3000,3000

runc v1.2.6
Duplicate GIDs are removed:

$ runc --version
runc version 1.2.6
commit: v1.2.6-0-ge89a2992
spec: 1.2.0
go: go1.22.4
libseccomp: 2.5.5

$ runc create testcon
$ runc start testcon
uid=0(root) gid=0(root) groups=1000,2000,3000

Describe the results you received and expected

The results are as shown in the reproduction steps.

There are a few options to consider:

• Modify the implementation to preserve the previous behavior (i.e., deduplicate duplicate group IDs).
• Define the expected behavior in the runtime-spec specification.

Additionally, crun does not deduplicate duplicates.

$ crun --version
crun version 1.14.1
commit: de537a7965bfbe9992e2cfae0baeb56a08128171
rundir: /run/crun
spec: 1.0.0
+SYSTEMD +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +WASM:wasmedge +YAJL

$ crun create testcon
$ crun start testcon
uid=0(root) gid=0(root) groups=1000,2000,3000,3000

youki references the older runc behavior and deduplicates additionalGids.

What version of runc are you using?

runc --version
runc version 1.3.0
commit: v1.3.0-0-g4ca628d1
spec: 1.2.1
go: go1.23.0
libseccomp: 2.5.5

Host OS information

cat /etc/os-release

PRETTY_NAME="Ubuntu 24.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="24.04"
VERSION="24.04.2 LTS (Noble Numbat)"
VERSION_CODENAME=noble
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=noble
LOGO=ubuntu-logo

Host kernel information

No response

[lifubang]

Sorry, I can't reproduce this issue in v1.3.0.

root@acmcoder:/opt/debian# cat /etc/os-release 
PRETTY_NAME="Ubuntu 24.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="24.04"
VERSION="24.04.2 LTS (Noble Numbat)"
VERSION_CODENAME=noble
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=noble
LOGO=ubuntu-logo
root@acmcoder:/opt/debian# cat config.json | jq .process.user
{
  "uid": 0,
  "gid": 0,
  "additionalGids": [
    1000,
    2000,
    3000,
    3000
  ]
}
root@acmcoder:/opt/debian# ./runc -v
runc version 1.3.0
commit: v1.3.0-0-g4ca628d1
spec: 1.2.1
go: go1.24.1
libseccomp: 2.5.5
root@acmcoder:/opt/debian# ./runc run test
uid=0(root) gid=0(root) groups=0(root),1000(ubuntu),2000,3000
root@acmcoder:/opt/debian#

Did I miss something here? @saku3

[saku3]

Sorry, I didn't explain it clearly.
I was usingbusybox image for the rootfs.

$ mkdir rootfs
$ docker export $(docker create busybox) | tar -C rootfs -xvf -

$runc run test
uid=0(root) gid=0(root) groups=1000,2000,3000,3000

When I use the ubuntu image instead, I get the same output as yours.

$ mkdir rootfs
$ docker export $(docker create ubuntu) | tar -C rootfs -xvf -

$ runc run test
uid=0(root) gid=0(root) groups=0(root),1000(ubuntu),2000,3000

[lifubang]

I guess the difference between busybox and ubuntu is the implementation of the binary file id, because the output of cat /proc/1/status | grep Groups are always: Groups: 1000 2000 3000 3000.
So the kernel will never deduplicate the array of additionalGids when calling SYS_SETGROUPS.

The reason is that: before the PR #3999, the deduplicate operation was done in moby/sys/user, but after this PR, we pass the original array additionalGids directly to the kernel without any deduplicate operations.

From my opinion, I think we should provide a backward compatibility in runc's newer release, though this is not a bug for me. @opencontainers/runc-maintainers If you have some other opinions, please let us know.

Would you mind to open a PR to provide this compatibility in runc?

[cyphar]

Given that we generally try to just pass what users specify directly to the kernel, I think that the new behaviour is actually more preferable. Do we have an example of a downstream user program breaking as a result of this behaviour change? I guess hypothetically a user could see different output from id -a and that could break a test.

[lifubang]

we generally try to just pass what users specify directly to the kernel

Compare to provide a backward compatibility, this seems more reasonable.
So I agree to keep it as is if there is no breaking for downstream users.

[saku3]

Do we have an example of a downstream user program breaking as a result of this behaviour change?

As far as I know, there is no such program.
Therefore, I agree that no changes are necessary.

However, I believe the OCI runtime-spec should define the expected behavior in case of duplicates.
What do you think about that?

[tianon]

From the spec perspective, the most I'd personally be comfortable with is something like "bundle authors SHOULD ensure there are no duplicates" and "runtimes MAY deduplicate" but IMO it's not really worth it unless we can find some software that actually cares (or some place the kernel treats it differently besides preserving the duplicates).

[saku3]

Thanks for the discussion. Closing this as current behavior is reasonable and causes no issues.