Make GID its own type to prevent defaulting to 0 (#1449)

A follow up to #1407 which
had the unintended side effect of missing gid value defaulting to 0

---------

Signed-off-by: Josh Dolitsky <[email protected]>

pkg/build/accounts.go

@@ -44,10 +44,15 @@ func userToUserEntry(user types.User) passwd.UserEntry {
 	if user.HomeDir == "" {
 		user.HomeDir = "/home/" + user.UserName
 	}
+	// Default the GID to the UID if not provided
+	gid := user.UID
+	if user.GID != nil {
+		gid = *user.GID
+	}
 	return passwd.UserEntry{
 		UserName: user.UserName,
 		UID:      user.UID,
-		GID:      user.GID,
+		GID:      gid,
 		HomeDir:  user.HomeDir,
 		Password: "x",
 		Info:     "Account created by apko",

pkg/build/accounts_test.go

@@ -0,0 +1,80 @@
+// Copyright 2024 Chainguard, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package build
+
+import (
+	"testing"
+
+	"chainguard.dev/apko/pkg/build/types"
+)
+
+var (
+	id0     = uint32(0)
+	id0T    = types.GID(&id0)
+	id1234  = uint32(1234)
+	id1235  = uint32(1235)
+	id1235T = types.GID(&id1235)
+)
+
+func Test_userToUserEntry_UID_GID_mapping(t *testing.T) {
+	for _, test := range []struct {
+		desc        string
+		user        types.User
+		expectedUID uint32
+		expectedGID uint32
+	}{
+		{
+			desc: "Unique GID gets propogated",
+			user: types.User{
+				UID: id1234,
+				GID: id1235T,
+			},
+			expectedUID: id1234,
+			expectedGID: id1235,
+		},
+		{
+			desc: "Nil GID defaults to UID",
+			user: types.User{
+				UID: id1234,
+			},
+			expectedUID: id1234,
+			expectedGID: id1234,
+		},
+		{
+			desc: "Able to set GID to 0",
+			user: types.User{
+				UID: id1234,
+				GID: id0T,
+			},
+			expectedUID: id1234,
+			expectedGID: id0,
+		},
+		{
+			// TODO: This may be unintentional but matches historical behavior
+			desc:        "Missing UID and GID means both are 0",
+			user:        types.User{},
+			expectedUID: id0,
+			expectedGID: id0,
+		},
+	} {
+		userEntry := userToUserEntry(test.user)
+		if userEntry.UID != test.expectedUID {
+			t.Errorf("%s: expected UID %d got UID %d", test.desc, test.expectedUID, userEntry.UID)
+		}
+		if userEntry.GID != test.expectedGID {
+			t.Errorf("%s: expected GID %d got GID %d", test.desc, test.expectedGID, userEntry.GID)
+		}
+	}
+}

pkg/build/types/image_configuration_test.go

@@ -11,6 +11,13 @@ import (
 	"chainguard.dev/apko/pkg/build/types"
 )
 
+var (
+	gid1000  = uint32(1000)
+	gid1001  = uint32(1001)
+	gid1000T = types.GID(&gid1000)
+	gid1001T = types.GID(&gid1001)
+)
+
 func TestOverlayWithEmptyContents(t *testing.T) {
 	ctx := context.Background()
 
@@ -152,7 +159,7 @@ func TestMergeInto(t *testing.T) {
 				Users: []types.User{{
 					UserName: "foo",
 					UID:      1000,
-					GID:      1000,
+					GID:      gid1000T,
 					HomeDir:  "/home/foo",
 				}},
 			},
@@ -174,7 +181,7 @@ func TestMergeInto(t *testing.T) {
 				Users: []types.User{{
 					UserName: "bar",
 					UID:      1001,
-					GID:      1001,
+					GID:      gid1001T,
 					HomeDir:  "/home/bar",
 				}},
 			},
@@ -200,12 +207,12 @@ func TestMergeInto(t *testing.T) {
 				Users: []types.User{{
 					UserName: "foo",
 					UID:      1000,
-					GID:      1000,
+					GID:      gid1000T,
 					HomeDir:  "/home/foo",
 				}, {
 					UserName: "bar",
 					UID:      1001,
-					GID:      1001,
+					GID:      gid1001T,
 					HomeDir:  "/home/bar",
 				}},
 			},

pkg/build/types/types.go

@@ -29,13 +29,15 @@ type User struct {
 	// Required: The user ID
 	UID uint32 `json:"uid,omitempty"`
 	// Required: The user's group ID
-	GID uint32 `json:"gid,omitempty"`
+	GID GID `json:"gid,omitempty" yaml:"gid,omitempty"`
 	// Optional: The user's shell
 	Shell string `json:"shell,omitempty"`
 	// Optional: The user's home directory
 	HomeDir string `json:"homedir,omitempty"`
 }
 
+type GID *uint32
+
 type Group struct {
 	// Required: The name of the group
 	GroupName string `json:"groupname,omitempty"`

pkg/build/types/types_test.go

@@ -15,9 +15,12 @@
 package types
 
 import (
+	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v3"
 )
 
 func TestParseArchitectures(t *testing.T) {
@@ -91,3 +94,162 @@ func TestOCIPlatform(t *testing.T) {
 		})
 	}
 }
+
+var (
+	id0     = uint32(0)
+	id0T    = GID(&id0)
+	id1234  = uint32(1234)
+	id1235  = uint32(1235)
+	id1235T = GID(&id1235)
+)
+
+// Ensure unmarshalling YAML into an ImageConfiguration
+// does not result in unexpected GID=0
+func Test_YAML_Unmarshalling_UID_GID_mapping(t *testing.T) {
+	for _, test := range []struct {
+		desc        string
+		expectedUID uint32
+		expectedGID GID
+		rawYAML     string
+	}{
+		{
+			desc:        "Unique GID gets propogated",
+			expectedUID: id1234,
+			expectedGID: id1235T,
+			rawYAML: `
+accounts:
+  users:
+    - username: testing
+      uid: 1234
+      gid: 1235
+`,
+		},
+		{
+			desc:        "Nil GID is treated as nil (not 0)",
+			expectedUID: id1234,
+			expectedGID: nil,
+			rawYAML: `
+accounts:
+  users:
+    - username: testing
+      uid: 1234
+`,
+		},
+		{
+			desc:        "Able to set GID to 0",
+			expectedUID: id1234,
+			expectedGID: id0T,
+			rawYAML: `
+accounts:
+  users:
+    - username: testing
+      uid: 1234
+      gid: 0
+`,
+		},
+		{
+			// TODO: This may be unintentional but matches historical behavior
+			desc:        "Missing UID and GID means UID is 0 and GID is nil",
+			expectedUID: 0,
+			expectedGID: nil,
+			rawYAML: `
+accounts:
+  users:
+    - username: testing
+`,
+		},
+	} {
+		var ic ImageConfiguration
+		if err := yaml.Unmarshal([]byte(test.rawYAML), &ic); err != nil {
+			t.Errorf("%s: unable to unmarshall: %v", test.desc, err)
+			continue
+		}
+		if numUsers := len(ic.Accounts.Users); numUsers != 1 {
+			t.Errorf("%s: expected 1 user, got %d", test.desc, numUsers)
+			continue
+		}
+		user := ic.Accounts.Users[0]
+		if test.expectedUID != user.UID {
+			t.Errorf("%s: expected UID %d got UID %d", test.desc, test.expectedUID, user.UID)
+		}
+		if diff := cmp.Diff(test.expectedGID, user.GID); diff != "" {
+			t.Errorf("%s: diff in GID: (-want, +got) = %s", test.desc, diff)
+		}
+	}
+}
+
+// Ensure marshalling YAML from a User
+// does not result in unexpected GID=0
+func Test_YAML_Marshalling_UID_GID_mapping(t *testing.T) {
+	for _, test := range []struct {
+		desc         string
+		user         User
+		expectedYAML string
+	}{
+		{
+			desc: "Unique UID and GID",
+			user: User{
+				UserName: "testing",
+				UID:      id1234,
+				GID:      id1235T,
+			},
+			expectedYAML: `
+username: testing
+uid: 1234
+gid: 1235
+shell: ""
+homedir: ""
+`,
+		},
+		{
+			desc: "Nil GID gets omitted",
+			user: User{
+				UserName: "testing",
+				UID:      id1234,
+			},
+			expectedYAML: `
+username: testing
+uid: 1234
+shell: ""
+homedir: ""
+`,
+		},
+		{
+			desc: "Able to set GID to 0",
+			user: User{
+				UserName: "testing",
+				UID:      id1234,
+				GID:      id0T,
+			},
+			expectedYAML: `
+username: testing
+uid: 1234
+gid: 0
+shell: ""
+homedir: ""
+`,
+		},
+		{
+			// TODO: This may be unintentional but matches historical behavior
+			desc: "Missing UID and GID means UID is 0 and GID gets omitted",
+			user: User{
+				UserName: "testing",
+			},
+			expectedYAML: `
+username: testing
+uid: 0
+shell: ""
+homedir: ""
+`,
+		},
+	} {
+		b, err := yaml.Marshal(test.user)
+		if err != nil {
+			t.Errorf("%s: unable to marshall: %v", test.desc, err)
+			continue
+		}
+		if diff := cmp.Diff(strings.TrimPrefix(test.expectedYAML, "\n"), string(b)); diff != "" {
+			t.Errorf("%s: diff in marshalled user YAML: (-want, +got) = %s", test.desc, diff)
+		}
+	}
+}