Use ipcauth.Principal for the socket restriction instead of a second principal type

client/cmd/service_allow_group.go

@@ -7,18 +7,10 @@ import (
 	"slices"
 	"strings"
 
+	"github.com/netbirdio/netbird/client/internal/ipcauth"
 	"github.com/netbirdio/netbird/client/mdm"
 )
 
-// Principal kinds an --allow-group value resolves to. The kind:value form is
-// the same one profile owners use, so a value in service.json or in a service
-// unit says which namespace it belongs to instead of being a bare number that
-// means one thing on Unix and another on Windows.
-const (
-	allowGroupKindGID = "gid"
-	allowGroupKindSID = "sid"
-)
-
 // resolveAllowGroups turns configured group values into the typed principals
 // the daemon enforces on its sockets, dropping empty entries and duplicates.
 // Names are resolved through the platform's directory service, so an entry that
@@ -43,8 +35,8 @@ func resolveAllowGroups(values []string) ([]string, error) {
 			if err != nil {
 				return nil, fmt.Errorf("resolve allowed group %q: %w", entry, err)
 			}
-			if !slices.Contains(resolved, principal) {
-				resolved = append(resolved, principal)
+			if value := principal.String(); !slices.Contains(resolved, value) {
+				resolved = append(resolved, value)
 			}
 		}
 	}
@@ -89,24 +81,32 @@ func daemonSocketPrincipals(policy *mdm.Policy) ([]string, string, error) {
 	return resolved, source, nil
 }
 
-// cutKind splits a value on the kind separator. ok is false when the value
-// carries no kind, which is the case for a plain group or account name: neither
-// a Unix group name nor a Windows account name may contain a colon, so the
-// separator is unambiguous.
-func cutKind(value string) (kind, rest string, ok bool) {
-	kind, rest, ok = strings.Cut(value, ":")
-	if !ok || rest == "" {
-		return "", value, false
+// typedPrincipal parses a value that carries an explicit kind, as
+// ipcauth.Principal renders it. ok is false when the value carries no kind at
+// all, which is the case for a plain group or account name: neither a Unix
+// group name nor a Windows account name may contain a colon, so the separator
+// is unambiguous. A value that has a kind the shared type does not know is an
+// error rather than a name to look up.
+func typedPrincipal(value string, want ipcauth.PrincipalKind) (ipcauth.Principal, bool, error) {
+	if !strings.Contains(value, ":") {
+		return ipcauth.Principal{}, false, nil
 	}
-	return kind, rest, true
+
+	principal, ok := ipcauth.ParsePrincipal(value)
+	if !ok {
+		return ipcauth.Principal{}, false, fmt.Errorf("not a principal, use a name or %s:<value>", want)
+	}
+	if principal.Kind != want {
+		return ipcauth.Principal{}, false, fmt.Errorf("unsupported principal kind %q on this platform, use a name or %s:<value>", principal.Kind, want)
+	}
+	return principal, true, nil
 }
 
-// principalValue returns the value part of a kind:value principal of the
-// expected kind.
-func principalValue(principal, kind string) (string, bool) {
-	got, value, ok := strings.Cut(principal, ":")
-	if !ok || got != kind || value == "" {
-		return "", false
+// principalOfKind parses a stored principal that must be of the given kind.
+func principalOfKind(value string, want ipcauth.PrincipalKind) (ipcauth.Principal, error) {
+	principal, ok := ipcauth.ParsePrincipal(value)
+	if !ok || principal.Kind != want {
+		return ipcauth.Principal{}, fmt.Errorf("not a %s principal: %q", want, value)
 	}
-	return value, true
+	return principal, nil
 }

client/cmd/service_allow_group_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/netbirdio/netbird/client/internal/ipcauth"
 	"github.com/netbirdio/netbird/client/mdm"
 )
 
@@ -151,44 +152,47 @@ func TestTCPListenerRefusesARestriction(t *testing.T) {
 	})
 }
 
-func TestCutKind(t *testing.T) {
-	tests := []struct {
-		value string
-		kind  string
-		rest  string
-		ok    bool
-	}{
-		{value: "gid:1000", kind: "gid", rest: "1000", ok: true},
-		{value: "sid:S-1-5-32-544", kind: "sid", rest: "S-1-5-32-544", ok: true},
-		{value: "netbird-users", rest: "netbird-users"},
-		{value: "S-1-5-32-544", rest: "S-1-5-32-544"},
-		{value: `NETBIRD\Users`, rest: `NETBIRD\Users`},
-		// A kind with no value is not a kind: it must not be mistaken for one
-		// and accepted as an empty principal.
-		{value: "gid:", rest: "gid:"},
-	}
+func TestTypedPrincipal(t *testing.T) {
+	t.Run("a value with no kind is a name to look up", func(t *testing.T) {
+		for _, value := range []string{"netbird-users", `NETBIRD\Users`, "1000"} {
+			_, typed, err := typedPrincipal(value, ipcauth.KindGID)
+			require.NoError(t, err, value)
+			assert.False(t, typed, "%q carries no kind", value)
+		}
+	})
 
-	for _, tc := range tests {
-		t.Run(tc.value, func(t *testing.T) {
-			kind, rest, ok := cutKind(tc.value)
-			assert.Equal(t, tc.ok, ok)
-			assert.Equal(t, tc.kind, kind)
-			assert.Equal(t, tc.rest, rest)
-		})
-	}
+	t.Run("a value of the wanted kind is parsed", func(t *testing.T) {
+		principal, typed, err := typedPrincipal("gid:1000", ipcauth.KindGID)
+		require.NoError(t, err)
+		assert.True(t, typed)
+		assert.Equal(t, ipcauth.KindGID, principal.Kind)
+		assert.Equal(t, "1000", principal.Value)
+	})
+
+	t.Run("a kind for another platform is an error, not a name", func(t *testing.T) {
+		_, _, err := typedPrincipal("sid:S-1-5-32-544", ipcauth.KindGID)
+		require.Error(t, err)
+	})
+
+	t.Run("an unknown kind is an error, not a name", func(t *testing.T) {
+		for _, value := range []string{"user:alice", "gid:"} {
+			_, _, err := typedPrincipal(value, ipcauth.KindGID)
+			require.Error(t, err, value)
+		}
+	})
 }
 
-func TestPrincipalValue(t *testing.T) {
-	value, ok := principalValue("gid:1000", allowGroupKindGID)
-	assert.True(t, ok)
-	assert.Equal(t, "1000", value)
+func TestPrincipalOfKind(t *testing.T) {
+	principal, err := principalOfKind("gid:1000", ipcauth.KindGID)
+	require.NoError(t, err)
+	assert.Equal(t, "1000", principal.Value)
 
-	_, ok = principalValue("sid:S-1-5-32-544", allowGroupKindGID)
-	assert.False(t, ok, "a principal of another kind must not be read as this one")
+	_, err = principalOfKind("sid:S-1-5-32-544", ipcauth.KindGID)
+	assert.Error(t, err, "a principal of another kind must not be read as this one")
 
-	_, ok = principalValue("1000", allowGroupKindGID)
-	assert.False(t, ok, "an untyped value is not a principal")
+	_, err = principalOfKind("1000", ipcauth.KindGID)
+	assert.Error(t, err, "an untyped value is not a principal")
 
-	_, ok = principalValue("gid:", allowGroupKindGID)
-	assert.False(t, ok, "an empty value is not a principal")
+	_, err = principalOfKind("gid:", ipcauth.KindGID)
+	assert.Error(t, err, "an empty value is not a principal")
 }

client/cmd/service_allow_group_unix.go

@@ -11,6 +11,7 @@ import (
 	"syscall"
 
 	"github.com/netbirdio/netbird/client/internal/getent"
+	"github.com/netbirdio/netbird/client/internal/ipcauth"
 )
 
 // Socket modes. openSocketMode is the historical one: any local account may
@@ -47,12 +48,13 @@ func listenUnixPrivate(address string) (net.Listener, error) {
 // principal. A numeric value, with or without the prefix, is the GID itself;
 // anything else is a group name resolved through NSS, so groups that only
 // LDAP, SSSD or winbind know about work as well as ones in /etc/group.
-func resolveAllowGroup(value string) (string, error) {
-	if kind, rest, ok := cutKind(value); ok {
-		if kind != allowGroupKindGID {
-			return "", fmt.Errorf("unsupported principal kind %q, use a group name or %s:<id>", kind, allowGroupKindGID)
-		}
-		return gidPrincipal(rest)
+func resolveAllowGroup(value string) (ipcauth.Principal, error) {
+	principal, typed, err := typedPrincipal(value, ipcauth.KindGID)
+	if err != nil {
+		return ipcauth.Principal{}, err
+	}
+	if typed {
+		return gidPrincipal(principal.Value)
 	}
 
 	if _, err := parseGID(value); err == nil {
@@ -61,7 +63,7 @@ func resolveAllowGroup(value string) (string, error) {
 
 	group, err := getent.LookupGroupName(value)
 	if err != nil {
-		return "", fmt.Errorf("look up group: %w", err)
+		return ipcauth.Principal{}, fmt.Errorf("look up group: %w", err)
 	}
 	return gidPrincipal(group.Gid)
 }
@@ -99,11 +101,11 @@ func applySocketAccess(path string, principals []string) error {
 		return nil
 	}
 
-	value, ok := principalValue(principals[0], allowGroupKindGID)
-	if !ok {
-		return fmt.Errorf("not a %s principal: %q", allowGroupKindGID, principals[0])
+	principal, err := principalOfKind(principals[0], ipcauth.KindGID)
+	if err != nil {
+		return err
 	}
-	gid, err := parseGID(value)
+	gid, err := parseGID(principal.Value)
 	if err != nil {
 		return err
 	}
@@ -169,12 +171,16 @@ func requireTrustedSocketDir(dir string) error {
 // gidPrincipal renders a GID as a principal in its canonical decimal form, so
 // that spellings of the same group ("gid:01" and "gid:1") produce one principal
 // rather than two that later look like a request to use two groups.
-func gidPrincipal(gid string) (string, error) {
+func gidPrincipal(gid string) (ipcauth.Principal, error) {
 	parsed, err := parseGID(gid)
 	if err != nil {
-		return "", err
+		return ipcauth.Principal{}, err
 	}
-	return allowGroupKindGID + ":" + strconv.Itoa(parsed), nil
+	principal, ok := ipcauth.ParsePrincipal(ipcauth.GIDPrincipal(uint32(parsed)))
+	if !ok {
+		return ipcauth.Principal{}, fmt.Errorf("build gid principal for %d", parsed)
+	}
+	return principal, nil
 }
 
 // unchangedGID is the value chown reads as "leave the group alone". A

client/cmd/service_allow_group_unix_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/netbirdio/netbird/client/internal/getent"
+	"github.com/netbirdio/netbird/client/internal/ipcauth"
 )
 
 // testAllowGroupPrincipal is a principal that resolves on any Unix host.
@@ -24,7 +25,8 @@ func TestResolveAllowGroup_NumericGID(t *testing.T) {
 		t.Run(value, func(t *testing.T) {
 			principal, err := resolveAllowGroup(value)
 			require.NoError(t, err)
-			assert.Equal(t, "gid:0", principal)
+			assert.Equal(t, ipcauth.KindGID, principal.Kind)
+			assert.Equal(t, "gid:0", principal.String())
 		})
 	}
 }
@@ -40,7 +42,7 @@ func TestResolveAllowGroup_ByName(t *testing.T) {
 
 	principal, err := resolveAllowGroup(group.Name)
 	require.NoError(t, err)
-	assert.Equal(t, "gid:"+gid, principal)
+	assert.Equal(t, "gid:"+gid, principal.String())
 }
 
 // Spellings of the same GID must collapse to one principal, otherwise

client/cmd/service_allow_group_windows.go

@@ -20,12 +20,13 @@ import (
 // Both a group and a user account are accepted. The DACL grants a SID without
 // caring which it is, and an administrator restricting the daemon to a single
 // service account should not have to create a group for it.
-func resolveAllowGroup(value string) (string, error) {
-	if kind, rest, ok := cutKind(value); ok {
-		if kind != allowGroupKindSID {
-			return "", fmt.Errorf("unsupported principal kind %q, use an account name or %s:<SID>", kind, allowGroupKindSID)
-		}
-		return sidPrincipal(rest)
+func resolveAllowGroup(value string) (ipcauth.Principal, error) {
+	principal, typed, err := typedPrincipal(value, ipcauth.KindSID)
+	if err != nil {
+		return ipcauth.Principal{}, err
+	}
+	if typed {
+		return sidPrincipal(principal.Value)
 	}
 
 	if _, err := windows.StringToSid(value); err == nil {
@@ -34,9 +35,9 @@ func resolveAllowGroup(value string) (string, error) {
 
 	sid, _, _, err := windows.LookupSID("", value)
 	if err != nil {
-		return "", fmt.Errorf("look up account: %w", err)
+		return ipcauth.Principal{}, fmt.Errorf("look up account: %w", err)
 	}
-	return allowGroupKindSID + ":" + sid.String(), nil
+	return sidPrincipal(sid.String())
 }
 
 // checkAllowGroupSet accepts any number of principals: a pipe descriptor holds
@@ -70,20 +71,26 @@ func listenUnixPrivate(address string) (net.Listener, error) {
 // connect.
 func allowedPipeSDDL(principals []string) (string, error) {
 	sids := make([]string, 0, len(principals))
-	for _, principal := range principals {
-		sid, ok := principalValue(principal, allowGroupKindSID)
-		if !ok {
-			return "", fmt.Errorf("not a %s principal: %q", allowGroupKindSID, principal)
+	for _, value := range principals {
+		principal, err := principalOfKind(value, ipcauth.KindSID)
+		if err != nil {
+			return "", err
 		}
-		sids = append(sids, sid)
+		sids = append(sids, principal.Value)
 	}
 	return ipcauth.RestrictedPipeSDDL(sids), nil
 }
 
-func sidPrincipal(value string) (string, error) {
+// sidPrincipal validates a SID and renders it in its canonical form, so that
+// two spellings of the same SID produce one principal.
+func sidPrincipal(value string) (ipcauth.Principal, error) {
 	sid, err := windows.StringToSid(value)
 	if err != nil {
-		return "", fmt.Errorf("parse SID %q: %w", value, err)
+		return ipcauth.Principal{}, fmt.Errorf("parse SID %q: %w", value, err)
 	}
-	return allowGroupKindSID + ":" + sid.String(), nil
+	principal, ok := ipcauth.ParsePrincipal(ipcauth.SIDPrincipal(sid.String()))
+	if !ok {
+		return ipcauth.Principal{}, fmt.Errorf("build sid principal for %q", sid.String())
+	}
+	return principal, nil
 }

client/internal/ipcauth/identity.go

@@ -164,6 +164,7 @@ type PrincipalKind string
 
 const (
 	KindUID PrincipalKind = "uid" // Unix user ID
+	KindGID PrincipalKind = "gid" // Unix group ID
 	KindSID PrincipalKind = "sid" // Windows user or group SID
 )
 
@@ -181,7 +182,7 @@ func ParsePrincipal(s string) (Principal, bool) {
 		return Principal{}, false
 	}
 	switch PrincipalKind(kind) {
-	case KindUID, KindSID:
+	case KindUID, KindGID, KindSID:
 		return Principal{Kind: PrincipalKind(kind), Value: value}, true
 	default:
 		return Principal{}, false
@@ -193,6 +194,11 @@ func UIDPrincipal(uid uint32) string {
 	return string(KindUID) + ":" + strconv.FormatUint(uint64(uid), 10)
 }
 
+// GIDPrincipal builds the principal string for a Unix group ID.
+func GIDPrincipal(gid uint32) string {
+	return string(KindGID) + ":" + strconv.FormatUint(uint64(gid), 10)
+}
+
 // SIDPrincipal builds the owner string for a Windows SID.
 func SIDPrincipal(sid string) string { return string(KindSID) + ":" + sid }
 
@@ -227,6 +233,15 @@ func (p Principal) Matches(id Identity) bool {
 		}
 		// Only the user SID. Group ownership is not supported yet.
 		return id.SID == p.Value
+	case KindGID:
+		// A group principal never confers ownership. It exists for the daemon
+		// socket restriction, which the kernel enforces at connect() from the
+		// caller's full group set; the identity here carries only the primary
+		// GID, so matching on it would grant ownership to members of a group
+		// and deny it to others in the same group, depending on which one
+		// happens to be primary. Deciding this properly is the group-ownership
+		// work that is still ahead.
+		return false
 	default:
 		return false
 	}

client/internal/ipcauth/identity_known_test.go

@@ -74,6 +74,21 @@ func TestPrincipalDoesNotMatchGroupSID(t *testing.T) {
 	assert.False(t, group.Matches(member), "a group SID owner must not match a group member")
 }
 
+// A GID principal is parseable because the daemon socket restriction stores one,
+// but it confers no ownership: an owner field holding one must match nobody
+// rather than admit everyone whose primary group happens to be it.
+func TestGIDPrincipalNeverMatches(t *testing.T) {
+	group, ok := ParsePrincipal(GIDPrincipal(1000))
+	require.True(t, ok, "a gid principal must parse, the socket restriction stores it")
+	assert.Equal(t, KindGID, group.Kind)
+	assert.Equal(t, "gid:1000", group.String())
+
+	assert.False(t, group.Matches(KnownForTest(Identity{UID: 1000, GID: 1000})),
+		"a gid owner must not match a caller whose primary group it is")
+	assert.False(t, group.Matches(KnownForTest(Identity{UID: 0, GID: 0})))
+	assert.False(t, group.Matches(Identity{}))
+}
+
 func TestPrincipalMatchingIsPlatformScoped(t *testing.T) {
 	unix, ok := ParsePrincipal("uid:1000")
 	require.True(t, ok)