WithAppendAdditionalGroups: better /etc/group handling

rumpl · rumpl · commit 55e570844473 · 2023-12-14T11:09:17.000+01:00
Scratch images don't necessarily have the /etc/group file, so we shouldn't
fail if opening/parsing it is not needed: if all the group to add are numeric.

Signed-off-by: Djordje Lukic &lt;djordje.lukic@docker.com&gt;
diff --git a/oci/spec_opts.go b/oci/spec_opts.go
@@ -893,9 +893,9 @@ func WithAppendAdditionalGroups(groups ...string) SpecOpts {
 			if err != nil {
 				return err
 			}
-			ugroups, err := user.ParseGroupFile(gpath)
-			if err != nil {
-				return err
+			ugroups, groupErr := user.ParseGroupFile(gpath)
+			if groupErr != nil && !os.IsNotExist(groupErr) {
+				return groupErr
 			}
 			groupMap := make(map[string]user.Group)
 			for _, group := range ugroups {
@@ -909,6 +909,9 @@ func WithAppendAdditionalGroups(groups ...string) SpecOpts {
 				} else {
 					g, ok := groupMap[group]
 					if !ok {
+						if groupErr != nil {
+							return fmt.Errorf("unable to find group %s: %w", group, groupErr)
+						}
 						return fmt.Errorf("unable to find group %s", group)
 					}
 					gids = append(gids, uint32(g.Gid))
diff --git a/oci/spec_opts_linux_test.go b/oci/spec_opts_linux_test.go
@@ -517,6 +517,65 @@ daemon:x:2:root,bin,daemon
 		})
 	}
 }
+
+func TestWithAppendAdditionalGroupsNoEtcGroup(t *testing.T) {
+	t.Parallel()
+	td := t.TempDir()
+	apply := fstest.Apply()
+	if err := apply.Apply(td); err != nil {
+		t.Fatalf("failed to apply: %v", err)
+	}
+	c := containers.Container{ID: t.Name()}
+
+	testCases := []struct {
+		name           string
+		additionalGIDs []uint32
+		groups         []string
+		expected       []uint32
+		err            string
+	}{
+		{
+			name:     "no additional gids",
+			groups:   []string{},
+			expected: []uint32{0},
+		},
+		{
+			name:     "no additional gids, append root group",
+			groups:   []string{"root"},
+			err:      fmt.Sprintf("unable to find group root: open %s: no such file or directory", filepath.Join(td, "etc", "group")),
+			expected: []uint32{0},
+		},
+		{
+			name:     "append group id",
+			groups:   []string{"999"},
+			expected: []uint32{0, 999},
+		},
+	}
+
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Parallel()
+			s := Spec{
+				Version: specs.Version,
+				Root: &specs.Root{
+					Path: td,
+				},
+				Process: &specs.Process{
+					User: specs.User{
+						AdditionalGids: testCase.additionalGIDs,
+					},
+				},
+			}
+			err := WithAppendAdditionalGroups(testCase.groups...)(context.Background(), nil, &c, &s)
+			if err != nil {
+				assert.EqualError(t, err, testCase.err)
+			}
+			assert.Equal(t, testCase.expected, s.Process.User.AdditionalGids)
+		})
+	}
+}
+
 func TestWithLinuxDeviceFollowSymlinks(t *testing.T) {
 
 	// Create symlink to /dev/zero for the symlink test case
