From 2064b2a0bdd5452db46fcc30648e99cf25ec6264 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 9 Sep 2026 10:16:54 +0200 Subject: [PATCH] Use ipcauth.Principal for the socket restriction instead of a second principal type --- client/cmd/service_allow_group.go | 54 +++++++------- client/cmd/service_allow_group_test.go | 72 ++++++++++--------- client/cmd/service_allow_group_unix.go | 34 +++++---- client/cmd/service_allow_group_unix_test.go | 6 +- client/cmd/service_allow_group_windows.go | 39 +++++----- client/internal/ipcauth/identity.go | 17 ++++- .../internal/ipcauth/identity_known_test.go | 15 ++++ 7 files changed, 143 insertions(+), 94 deletions(-) diff --git a/client/cmd/service_allow_group.go b/client/cmd/service_allow_group.go index 3e217b57b..482aeec49 100644 --- a/client/cmd/service_allow_group.go +++ b/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:", want) + } + if principal.Kind != want { + return ipcauth.Principal{}, false, fmt.Errorf("unsupported principal kind %q on this platform, use a name or %s:", 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 } diff --git a/client/cmd/service_allow_group_test.go b/client/cmd/service_allow_group_test.go index ca901e2cf..46c90f2de 100644 --- a/client/cmd/service_allow_group_test.go +++ b/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") } diff --git a/client/cmd/service_allow_group_unix.go b/client/cmd/service_allow_group_unix.go index c06e18dd8..946742aa3 100644 --- a/client/cmd/service_allow_group_unix.go +++ b/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:", 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 diff --git a/client/cmd/service_allow_group_unix_test.go b/client/cmd/service_allow_group_unix_test.go index 6e9310289..0928a895c 100644 --- a/client/cmd/service_allow_group_unix_test.go +++ b/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 diff --git a/client/cmd/service_allow_group_windows.go b/client/cmd/service_allow_group_windows.go index 608ae8a1e..b7ba00172 100644 --- a/client/cmd/service_allow_group_windows.go +++ b/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:", 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 } diff --git a/client/internal/ipcauth/identity.go b/client/internal/ipcauth/identity.go index cbd2c2fa7..485d8726f 100644 --- a/client/internal/ipcauth/identity.go +++ b/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 } diff --git a/client/internal/ipcauth/identity_known_test.go b/client/internal/ipcauth/identity_known_test.go index 0ce269f3a..84e476529 100644 --- a/client/internal/ipcauth/identity_known_test.go +++ b/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)