From b2f7eacd347da4fee35cd34e96b850e3d7bfe1bc Mon Sep 17 00:00:00 2001 From: "Theodor S. Midtlien" Date: Tue, 8 Sep 2026 11:38:33 +0200 Subject: [PATCH] Add known mark to identity, fix credentials comparison bugs --- client/cmd/service_json_gateway_test.go | 4 +- client/internal/ipcauth/creds_windows.go | 1 + client/internal/ipcauth/forward.go | 3 +- client/internal/ipcauth/forward_test.go | 14 +- client/internal/ipcauth/identity.go | 47 +++++- .../internal/ipcauth/identity_known_test.go | 157 ++++++++++++++++++ .../ipcauth/identity_sameuser_test.go | 28 ++-- client/internal/ipcauth/peercred_bsd.go | 2 +- client/internal/ipcauth/peercred_linux.go | 2 +- client/internal/ipcauth/pipeserver_windows.go | 4 +- client/internal/ipcauth/policy.go | 12 +- client/internal/ipcauth/privileged.go | 19 ++- client/internal/ipcauth/privileged_test.go | 109 ++++++------ client/internal/ipcauth/self_unix.go | 5 +- client/internal/ipcauth/self_windows.go | 1 + client/internal/ipcauth/testing.go | 9 + .../internal/profilemanager/profilemanager.go | 2 +- client/internal/profilemanager/service.go | 32 ++-- client/server/jwt_cache_test.go | 4 +- client/server/server.go | 31 ++-- client/server/ssh_gate_test.go | 8 +- 21 files changed, 350 insertions(+), 144 deletions(-) create mode 100644 client/internal/ipcauth/identity_known_test.go create mode 100644 client/internal/ipcauth/testing.go diff --git a/client/cmd/service_json_gateway_test.go b/client/cmd/service_json_gateway_test.go index dfeef1c46..b05e0fd90 100644 --- a/client/cmd/service_json_gateway_test.go +++ b/client/cmd/service_json_gateway_test.go @@ -86,7 +86,7 @@ func TestJSONGateway_ForgedIdentityHeaderIsDropped(t *testing.T) { req.Header.Set("Grpc-Metadata-X-Netbird-Fwd", "1") req.Header.Set("Grpc-Metadata-X-Netbird-Fwd-Sid", "S-1-5-18") - caller := ipcauth.Identity{UID: 31000, GID: 31000} + caller := ipcauth.KnownForTest(ipcauth.Identity{UID: 31000, GID: 31000}) md := gatewayMetadata(t, req, clientCtx(caller, true)) id, ok := ipcauth.CallerIdentity(daemonSideCtx(t, md)) @@ -114,7 +114,7 @@ func TestJSONGateway_HeaderlessRequestIsStillMarkedForwarded(t *testing.T) { req.Header = http.Header{} req.Host = "" - caller := ipcauth.Identity{UID: 31000, GID: 31000} + caller := ipcauth.KnownForTest(ipcauth.Identity{UID: 31000, GID: 31000}) ctx := clientCtx(caller, true) // Pin the skip path itself: if grpc-gateway ever produced a pair here, this diff --git a/client/internal/ipcauth/creds_windows.go b/client/internal/ipcauth/creds_windows.go index 37f902c52..a7a13172c 100644 --- a/client/internal/ipcauth/creds_windows.go +++ b/client/internal/ipcauth/creds_windows.go @@ -158,6 +158,7 @@ func identityFromToken(token windows.Token) (Identity, error) { SID: user.User.Sid.String(), Groups: groups, Elevated: token.IsElevated(), + known: true, }, nil } diff --git a/client/internal/ipcauth/forward.go b/client/internal/ipcauth/forward.go index 57749528c..9f79697e8 100644 --- a/client/internal/ipcauth/forward.go +++ b/client/internal/ipcauth/forward.go @@ -216,6 +216,7 @@ func forwardedIdentity(ctx context.Context) (Identity, bool) { // the forwarding proof has been verified. Groups: md.Get(mdFwdGroup), Elevated: mdSingle(md, mdFwdElevated) == "1", + known: true, }, true } @@ -224,7 +225,7 @@ func forwardedIdentity(ctx context.Context) (Identity, bool) { return Identity{}, false } - id := Identity{UID: uint32(uid)} + id := Identity{UID: uint32(uid), known: true} if gid, err := strconv.ParseUint(mdSingle(md, mdFwdGID), 10, 32); err == nil { id.GID = uint32(gid) } diff --git a/client/internal/ipcauth/forward_test.go b/client/internal/ipcauth/forward_test.go index d9adf05da..6981510f7 100644 --- a/client/internal/ipcauth/forward_test.go +++ b/client/internal/ipcauth/forward_test.go @@ -26,8 +26,8 @@ func transportCtx(id Identity, md metadata.MD) context.Context { } var ( - root = Identity{UID: 0} - unprivUser = Identity{UID: 1000, GID: 1000} + root = Identity{known: true, UID: 0} + unprivUser = Identity{known: true, UID: 1000, GID: 1000} ) // asDaemon pins which identity counts as this process for the duration of a test. @@ -35,9 +35,9 @@ var ( // "the gateway" means. func asDaemon(t *testing.T, id Identity) { t.Helper() - prevID, prevKnown, prevDelegate := selfIdentity, selfKnown, selfMayDelegate - t.Cleanup(func() { selfIdentity, selfKnown, selfMayDelegate = prevID, prevKnown, prevDelegate }) - selfIdentity, selfKnown = id, true + prevID, prevDelegate := selfIdentity, selfMayDelegate + t.Cleanup(func() { selfIdentity, selfMayDelegate = prevID, prevDelegate }) + selfIdentity = id selfMayDelegate = !id.IsPrivileged() } @@ -163,7 +163,7 @@ func TestCallerIdentity_GatewayForwarding(t *testing.T) { injected.Append(mdFwdGroup, sidAdministrators) ctx := WithForwardedIdentity(metadata.NewOutgoingContext(context.Background(), injected), - Identity{SID: "S-1-5-21-1-2-3-1001"}, true) + Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, true) out, ok := metadata.FromOutgoingContext(ctx) if !ok { t.Fatal("no outgoing metadata") @@ -202,7 +202,7 @@ func TestForwardIdentityMetadata_AlwaysMarksForwarded(t *testing.T) { }{ {"known unix identity", unprivUser, true}, {"unknown identity", Identity{}, false}, - {"windows identity", Identity{SID: "S-1-5-21-1-2-3-1001", Elevated: true}, true}, + {"windows identity", Identity{known: true, SID: "S-1-5-21-1-2-3-1001", Elevated: true}, true}, } { t.Run(tc.name, func(t *testing.T) { md := ForwardIdentityMetadata(tc.id, tc.known) diff --git a/client/internal/ipcauth/identity.go b/client/internal/ipcauth/identity.go index 9d0699c26..cbd2c2fa7 100644 --- a/client/internal/ipcauth/identity.go +++ b/client/internal/ipcauth/identity.go @@ -56,8 +56,15 @@ type Identity struct { // process dialling itself, which is what the JSON gateway does, and is never // used to grant anything. PID int32 + + // known marks if an identity was provided by the kernel. Without it, an + // empty Identity struct would resolve as root. + known bool } +// Known reports whether this identity came from a kernel credential read. +func (i Identity) Known() bool { return i.known } + // IsWindows reports whether this identity is a Windows principal (SID-based) // rather than a Unix uid/gid principal. func (i Identity) IsWindows() bool { @@ -77,6 +84,9 @@ func (i Identity) IsWindows() bool { // (Domain Admins and friends) are deliberately not consulted: they say // nothing about what this token may do on this machine. func (i Identity) IsPrivileged() bool { + if !i.known { + return false + } if !i.IsWindows() { return i.UID == 0 } @@ -100,6 +110,9 @@ func (i Identity) IsPrivileged() bool { // happen to leave at zero. The zero Identity carries uid 0, so callers must // establish that both identities are real before the answer means anything. func (i Identity) SameUser(other Identity) bool { + if !i.known || !other.known { + return false + } if i.SID != "" || other.SID != "" { return i.SID == other.SID } @@ -108,6 +121,11 @@ func (i Identity) SameUser(other Identity) bool { // String renders the identity for audit logs and denial messages. func (i Identity) String() string { + // An unknown identity has a zero UID, which would print as "uid=0" and read + // as root in an audit trail. + if !i.known { + return "unidentified" + } if i.IsWindows() { return fmt.Sprintf("sid=%s elevated=%t", i.SID, i.Elevated) } @@ -187,17 +205,32 @@ func OwnerPrincipalForIdentity(id Identity) string { return UIDPrincipal(id.UID) } -func IdentityFromPrincipal(p Principal) (Identity, error) { +// Matches reports whether a kernel-attested caller satisfies this stored owner +// principal. +// +// A principal is a config value, not a caller, so it is never converted into an +// Identity. +func (p Principal) Matches(id Identity) bool { + if !id.Known() { + return false + } switch p.Kind { case KindUID: - uid, err := strconv.ParseUint(p.Value, 10, 32) - if err != nil { - return Identity{}, err + if id.IsWindows() { + return false } - return Identity{UID: uint32(uid)}, nil + uid, err := strconv.ParseUint(p.Value, 10, 32) + return err == nil && uint32(uid) == id.UID case KindSID: - return Identity{SID: p.Value}, nil + if !id.IsWindows() { + return false + } + // Only the user SID. Group ownership is not supported yet. + return id.SID == p.Value default: - return Identity{}, nil + return false } } + +// String renders the principal as the kind:value form it is stored in. +func (p Principal) String() string { return string(p.Kind) + ":" + p.Value } diff --git a/client/internal/ipcauth/identity_known_test.go b/client/internal/ipcauth/identity_known_test.go new file mode 100644 index 000000000..57e2f7e90 --- /dev/null +++ b/client/internal/ipcauth/identity_known_test.go @@ -0,0 +1,157 @@ +package ipcauth + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// The zero Identity carries uid 0, so every predicate that reads UID has to +// refuse it explicitly without the known marker. +func TestZeroIdentityIsInert(t *testing.T) { + var zero Identity + + assert.False(t, zero.Known(), "the zero identity must not be known") + assert.False(t, zero.IsPrivileged(), "the zero identity must not read as root") + assert.False(t, zero.SameUser(Identity{}), "two unknown identities must not match") + assert.False(t, zero.SameUser(Identity{known: true, UID: 0}), "an unknown identity must not match root") + assert.False(t, Identity{known: true, UID: 0}.SameUser(zero), "SameUser must be symmetric here too") + assert.Equal(t, "unidentified", zero.String(), "an unknown identity must not print as uid=0") +} + +// IsDaemonSelf compares UIDs, so on the usual install, where the daemon is root, +// an unknown identity would match it. +func TestIsDaemonSelfRejectsUnknownIdentity(t *testing.T) { + prevID, prevDelegate := selfIdentity, selfMayDelegate + t.Cleanup(func() { selfIdentity, selfMayDelegate = prevID, prevDelegate }) + + selfIdentity = Identity{known: true, UID: 0} + selfMayDelegate = false + + assert.False(t, IsDaemonSelf(Identity{}), "an unknown identity is not the daemon") + assert.True(t, IsDaemonSelf(Identity{known: true, UID: 0}), "precondition: a known root caller is the daemon here") +} + +// A daemon whose own identity could not be read delegates to nobody, and the +// zero selfIdentity is what records that. +func TestUnknownSelfDelegatesToNobody(t *testing.T) { + prevID, prevDelegate := selfIdentity, selfMayDelegate + t.Cleanup(func() { selfIdentity, selfMayDelegate = prevID, prevDelegate }) + + selfIdentity = Identity{} + selfMayDelegate = true + + assert.False(t, IsDaemonSelf(Identity{known: true, UID: 1000})) + _, delegates := SelfDelegatesTo() + assert.False(t, delegates, "an unknown self identity must not be delegated to") +} + +func TestKnownForTestMarksIdentity(t *testing.T) { + id := KnownForTest(Identity{UID: 1000, GID: 1000}) + require.True(t, id.Known()) + assert.Equal(t, uint32(1000), id.UID, "KnownForTest must not alter the identity") +} + +type stubState struct { + holder Principal + held bool +} + +func (s stubState) SessionHolder() (Principal, bool) { return s.holder, s.held } + +func uidHolder(uid uint32) Principal { + p, ok := ParsePrincipal(UIDPrincipal(uid)) + if !ok { + panic("bad uid principal") + } + return p +} + +// The holder comes from a profile JSON, the caller from a peercred read. When +// both were an Identity, the known marker made every such comparison false and +// the session holder could never be recognised. +func TestRequireSessionHolderMatchesConfigOwner(t *testing.T) { + asDaemon(t, KnownForTest(Identity{UID: 0})) + + caller := KnownForTest(Identity{UID: 1000, GID: 1000}) + assert.NoError(t, RequireSessionHolder(caller, stubState{holder: uidHolder(1000), held: true})) +} + +func TestRequireSessionHolderRejectsAnotherUser(t *testing.T) { + asDaemon(t, KnownForTest(Identity{UID: 0})) + + caller := KnownForTest(Identity{UID: 1000, GID: 1000}) + err := RequireSessionHolder(caller, stubState{holder: uidHolder(1001), held: true}) + assert.Equal(t, codes.PermissionDenied, status.Code(err)) +} + +// With nobody connected there is no session to protect. +func TestRequireSessionHolderAllowsWhenUnheld(t *testing.T) { + asDaemon(t, KnownForTest(Identity{UID: 0})) + + caller := KnownForTest(Identity{UID: 1000, GID: 1000}) + assert.NoError(t, RequireSessionHolder(caller, stubState{held: false})) +} + +// An owner that could not be parsed leaves the zero Principal, which matches +// nobody, so the session locks rather than opening. +func TestRequireSessionHolderLocksOnUnparseableOwner(t *testing.T) { + asDaemon(t, KnownForTest(Identity{UID: 0})) + + caller := KnownForTest(Identity{UID: 1000, GID: 1000}) + err := RequireSessionHolder(caller, stubState{holder: Principal{}, held: true}) + assert.Equal(t, codes.PermissionDenied, status.Code(err)) +} + +// Root takes over whoever holds the session. +func TestRequireSessionHolderAllowsPrivilegedCaller(t *testing.T) { + asDaemon(t, KnownForTest(Identity{UID: 1000})) + + root := KnownForTest(Identity{UID: 0}) + assert.NoError(t, RequireSessionHolder(root, stubState{holder: uidHolder(1001), held: true})) +} + +// A uid:0 owner is a config value, so it grants nothing on its own. +func TestConfigOwnerCannotGrantPrivilege(t *testing.T) { + root, ok := ParsePrincipal("uid:0") + require.True(t, ok) + assert.False(t, root.Matches(KnownForTest(Identity{UID: 1000})), + "a root owner must not match an unrelated caller") +} + +// Group ownership is not supported yet, so a group SID in an owner field must +// not match a caller who merely belongs to that group. +func TestPrincipalDoesNotMatchGroupSID(t *testing.T) { + group, ok := ParsePrincipal("sid:S-1-5-21-1-2-3-513") + require.True(t, ok) + + member := KnownForTest(Identity{ + SID: "S-1-5-21-1-2-3-1001", + Groups: []string{"S-1-5-21-1-2-3-513"}, + }) + assert.False(t, group.Matches(member), "a group SID owner must not match a group member") +} + +func TestPrincipalMatchingIsPlatformScoped(t *testing.T) { + unix, ok := ParsePrincipal("uid:1000") + require.True(t, ok) + windows, ok := ParsePrincipal("sid:S-1-5-21-1-2-3-1001") + require.True(t, ok) + + unixCaller := KnownForTest(Identity{UID: 1000}) + windowsCaller := KnownForTest(Identity{SID: "S-1-5-21-1-2-3-1001"}) + + assert.True(t, unix.Matches(unixCaller)) + assert.True(t, windows.Matches(windowsCaller)) + assert.False(t, unix.Matches(windowsCaller), "uid must never match a windows identity") + assert.False(t, windows.Matches(unixCaller), "sid must never match a unix identity") +} + +func TestPrincipalDoesNotMatchUnknownIdentity(t *testing.T) { + p, ok := ParsePrincipal("uid:0") + require.True(t, ok) + assert.False(t, p.Matches(Identity{}), "an unknown caller matches no principal") +} diff --git a/client/internal/ipcauth/identity_sameuser_test.go b/client/internal/ipcauth/identity_sameuser_test.go index c98f583db..534df2b3a 100644 --- a/client/internal/ipcauth/identity_sameuser_test.go +++ b/client/internal/ipcauth/identity_sameuser_test.go @@ -15,44 +15,44 @@ func TestIdentitySameUser(t *testing.T) { }{ { name: "same uid", - a: Identity{UID: 1000, GID: 1000}, - b: Identity{UID: 1000, GID: 1000}, + a: Identity{known: true, UID: 1000, GID: 1000}, + b: Identity{known: true, UID: 1000, GID: 1000}, want: true, }, { name: "same uid, different gid and pid still the same user", - a: Identity{UID: 1000, GID: 1000, PID: 11}, - b: Identity{UID: 1000, GID: 27, PID: 22}, + a: Identity{known: true, UID: 1000, GID: 1000, PID: 11}, + b: Identity{known: true, UID: 1000, GID: 27, PID: 22}, want: true, }, { name: "different uid", - a: Identity{UID: 1000}, - b: Identity{UID: 1001}, + a: Identity{known: true, UID: 1000}, + b: Identity{known: true, UID: 1001}, want: false, }, { name: "same sid", - a: Identity{SID: "S-1-5-21-1-2-3-1001"}, - b: Identity{SID: "S-1-5-21-1-2-3-1001"}, + a: Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, + b: Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, want: true, }, { name: "same sid, elevation and groups differ", - a: Identity{SID: "S-1-5-21-1-2-3-1001", Elevated: true, Groups: []string{sidAdministrators}}, - b: Identity{SID: "S-1-5-21-1-2-3-1001"}, + a: Identity{known: true, SID: "S-1-5-21-1-2-3-1001", Elevated: true, Groups: []string{sidAdministrators}}, + b: Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, want: true, }, { name: "different sid", - a: Identity{SID: "S-1-5-21-1-2-3-1001"}, - b: Identity{SID: "S-1-5-21-1-2-3-1002"}, + a: Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, + b: Identity{known: true, SID: "S-1-5-21-1-2-3-1002"}, want: false, }, { name: "a windows principal is never a unix one", - a: Identity{SID: "S-1-5-18"}, - b: Identity{UID: 0}, + a: Identity{known: true, SID: "S-1-5-18"}, + b: Identity{known: true, UID: 0}, want: false, }, } diff --git a/client/internal/ipcauth/peercred_bsd.go b/client/internal/ipcauth/peercred_bsd.go index 6d9c5247f..88dffe865 100644 --- a/client/internal/ipcauth/peercred_bsd.go +++ b/client/internal/ipcauth/peercred_bsd.go @@ -35,7 +35,7 @@ func PeerIdentity(conn net.Conn) (Identity, error) { return Identity{}, fmt.Errorf("read LOCAL_PEERCRED: %w", credErr) } - id := Identity{UID: cred.Uid} + id := Identity{UID: cred.Uid, known: true} if cred.Ngroups > 0 { id.GID = cred.Groups[0] } diff --git a/client/internal/ipcauth/peercred_linux.go b/client/internal/ipcauth/peercred_linux.go index 417cc1e00..5c07b70b7 100644 --- a/client/internal/ipcauth/peercred_linux.go +++ b/client/internal/ipcauth/peercred_linux.go @@ -35,5 +35,5 @@ func PeerIdentity(conn net.Conn) (Identity, error) { return Identity{}, fmt.Errorf("read SO_PEERCRED: %w", credErr) } - return Identity{UID: cred.Uid, GID: cred.Gid, PID: cred.Pid}, nil + return Identity{UID: cred.Uid, GID: cred.Gid, PID: cred.Pid, known: true}, nil } diff --git a/client/internal/ipcauth/pipeserver_windows.go b/client/internal/ipcauth/pipeserver_windows.go index 7ba59d574..c7dea2cdd 100644 --- a/client/internal/ipcauth/pipeserver_windows.go +++ b/client/internal/ipcauth/pipeserver_windows.go @@ -56,7 +56,7 @@ func PipeOwnedBySelf(conn net.Conn) bool { log.Debugf("read daemon pipe owner: %v", err) return false } - return selfKnown && selfIdentity.SID != "" && owner == selfIdentity.SID + return selfIdentity.Known() && selfIdentity.SID != "" && owner == selfIdentity.SID } // pipeOwnerSID reads the owner of the pipe object a client is connected to. The @@ -83,5 +83,5 @@ func trustedPipeOwner(owner string) bool { case sidLocalSystem, sidLocalService, sidNetworkService, sidAdministrators: return true } - return selfKnown && selfIdentity.SID != "" && owner == selfIdentity.SID + return selfIdentity.Known() && selfIdentity.SID != "" && owner == selfIdentity.SID } diff --git a/client/internal/ipcauth/policy.go b/client/internal/ipcauth/policy.go index c1deee937..39616c731 100644 --- a/client/internal/ipcauth/policy.go +++ b/client/internal/ipcauth/policy.go @@ -11,7 +11,11 @@ import ( ) type DaemonState interface { - SessionHolder() (Identity, bool) + // SessionHolder returns the principal entitled to the live connection and + // whether one is held. A held session whose owner cannot be parsed returns + // the zero Principal with true, which matches nobody, so a corrupt owner + // field locks the session instead of opening it. + SessionHolder() (Principal, bool) } type Rule func(id Identity, st DaemonState) error @@ -44,12 +48,14 @@ func (g *RuleGate) state() DaemonState { return g.st } +// RequireSessionHolder allows the caller to act on the live connection. With no +// session held there is nothing to protect, and root can always take over. func RequireSessionHolder(id Identity, st DaemonState) error { holder, running := st.SessionHolder() - log.Debugf("id : %v, session holder: %v", id, holder) - if !running || holder.SameUser(id) || holder.IsPrivileged() { + if !running || IsPrivilegedCaller(id) || holder.Matches(id) { return nil } + log.Debugf("caller %v is not the session holder %v", id, holder) return status.Errorf(codes.PermissionDenied, "session is held by another user (%v)", holder) } diff --git a/client/internal/ipcauth/privileged.go b/client/internal/ipcauth/privileged.go index 3c2e68432..f5e4b1d59 100644 --- a/client/internal/ipcauth/privileged.go +++ b/client/internal/ipcauth/privileged.go @@ -22,12 +22,12 @@ const ( ) // The identity of the process evaluating callers, captured once because it cannot -// change. selfKnown is false when it could not be read, in which case nothing is -// ever treated as this process. selfMayDelegate additionally requires this -// process to be unprivileged: see IsPrivilegedCaller. +// change. It stays the zero Identity when it could not be read, and the zero +// Identity is not Known, so nothing is ever treated as this process. +// selfMayDelegate additionally requires this process to be unprivileged: see +// IsPrivilegedCaller. var ( selfIdentity Identity - selfKnown bool selfMayDelegate bool // selfPID is this process's PID, used to recognise the daemon dialling itself. selfPID = os.Getpid() @@ -38,7 +38,7 @@ func init() { if err != nil { return } - selfIdentity, selfKnown = id, true + selfIdentity = id // Only an unprivileged daemon delegates its authority to its own identity. // When it is root or LocalSystem, sharing its identity does not mean sharing // its power: on Windows a filtered and a full token carry the same SID, so @@ -52,7 +52,12 @@ func init() { // runs inside the daemon and re-dials it locally, so this is what distinguishes // the gateway from any other caller, whatever user the daemon runs as. func IsDaemonSelf(id Identity) bool { - if !selfKnown || id.IsWindows() != selfIdentity.IsWindows() { + // An identity the kernel did not vouch for is nobody, least of all us: the + // zero Identity carries uid 0, which would otherwise match a root daemon. + if !id.Known() || !selfIdentity.Known() { + return false + } + if id.IsWindows() != selfIdentity.IsWindows() { return false } if id.IsWindows() { @@ -85,7 +90,7 @@ func IsPrivilegedCaller(id Identity) bool { // operation, because on such a host root is neither required nor necessarily // available. func SelfDelegatesTo() (Identity, bool) { - if !selfKnown || !selfMayDelegate { + if !selfIdentity.Known() || !selfMayDelegate { return Identity{}, false } return selfIdentity, true diff --git a/client/internal/ipcauth/privileged_test.go b/client/internal/ipcauth/privileged_test.go index c1c7c1543..055c57468 100644 --- a/client/internal/ipcauth/privileged_test.go +++ b/client/internal/ipcauth/privileged_test.go @@ -9,96 +9,85 @@ func TestIsPrivilegedCaller_SelfRule(t *testing.T) { tests := []struct { name string // self stands in for the process the daemon runs as. - self Identity - selfKnown bool - caller Identity - want bool + self Identity + caller Identity + want bool }{ { - name: "root is privileged whatever the daemon runs as", - self: Identity{UID: 1000}, - selfKnown: true, - caller: Identity{UID: 0}, - want: true, + name: "root is privileged whatever the daemon runs as", + self: Identity{known: true, UID: 1000}, + caller: Identity{known: true, UID: 0}, + want: true, }, { - name: "an unprivileged daemon delegates to its own user (rootless container)", - self: Identity{UID: 1000}, - selfKnown: true, - caller: Identity{UID: 1000}, - want: true, + name: "an unprivileged daemon delegates to its own user (rootless container)", + self: Identity{known: true, UID: 1000}, + caller: Identity{known: true, UID: 1000}, + want: true, }, { - name: "an unprivileged daemon delegates to nobody else", - self: Identity{UID: 1000}, - selfKnown: true, - caller: Identity{UID: 1001}, - want: false, + name: "an unprivileged daemon delegates to nobody else", + self: Identity{known: true, UID: 1000}, + caller: Identity{known: true, UID: 1001}, + want: false, }, { // The daemon is root on a normal install, so sharing its identity is // already covered by being root; nothing else may match. - name: "a root daemon delegates to nobody", - self: Identity{UID: 0}, - selfKnown: true, - caller: Identity{UID: 1000}, - want: false, + name: "a root daemon delegates to nobody", + self: Identity{known: true, UID: 0}, + caller: Identity{known: true, UID: 1000}, + want: false, }, { // Windows netstack mode: the daemon needs no administrator rights. - name: "an unprivileged windows daemon delegates to its own SID", - self: Identity{SID: "S-1-5-21-1-2-3-1001"}, - selfKnown: true, - caller: Identity{SID: "S-1-5-21-1-2-3-1001"}, - want: true, + name: "an unprivileged windows daemon delegates to its own SID", + self: Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, + caller: Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, + want: true, }, { - name: "an unprivileged windows daemon delegates to no other SID", - self: Identity{SID: "S-1-5-21-1-2-3-1001"}, - selfKnown: true, - caller: Identity{SID: "S-1-5-21-1-2-3-1002"}, - want: false, + name: "an unprivileged windows daemon delegates to no other SID", + self: Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, + caller: Identity{known: true, SID: "S-1-5-21-1-2-3-1002"}, + want: false, }, { // The UAC boundary: a filtered and a full token of the same account // carry the same SID but not the same power, so an elevated daemon must // never delegate to its own SID. - name: "an elevated windows daemon does not delegate to its own SID", - self: Identity{SID: "S-1-5-21-1-2-3-500", Elevated: true}, - selfKnown: true, - caller: Identity{SID: "S-1-5-21-1-2-3-500"}, - want: false, + name: "an elevated windows daemon does not delegate to its own SID", + self: Identity{known: true, SID: "S-1-5-21-1-2-3-500", Elevated: true}, + caller: Identity{known: true, SID: "S-1-5-21-1-2-3-500"}, + want: false, }, { - name: "LocalSystem is privileged on its own merits, not by delegation", - self: Identity{SID: sidLocalSystem}, - selfKnown: true, - caller: Identity{SID: sidLocalSystem}, - want: true, // LocalSystem is privileged on its own merits + name: "LocalSystem is privileged on its own merits, not by delegation", + self: Identity{known: true, SID: sidLocalSystem}, + caller: Identity{known: true, SID: sidLocalSystem}, + want: true, // LocalSystem is privileged on its own merits }, { - name: "identities of different kinds never match", - self: Identity{UID: 1000}, - selfKnown: true, - caller: Identity{SID: "S-1-5-21-1-2-3-1001"}, - want: false, + name: "identities of different kinds never match", + self: Identity{known: true, UID: 1000}, + caller: Identity{known: true, SID: "S-1-5-21-1-2-3-1001"}, + want: false, }, { - name: "an unknown self identity delegates to nobody", - self: Identity{}, - selfKnown: false, - caller: Identity{UID: 1000}, - want: false, + name: "an unknown self identity delegates to nobody", + self: Identity{}, + caller: Identity{known: true, UID: 1000}, + want: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - prevID, prevKnown, prevDelegate := selfIdentity, selfKnown, selfMayDelegate - t.Cleanup(func() { selfIdentity, selfKnown, selfMayDelegate = prevID, prevKnown, prevDelegate }) + prevID, prevDelegate := selfIdentity, selfMayDelegate + t.Cleanup(func() { selfIdentity, selfMayDelegate = prevID, prevDelegate }) - selfIdentity, selfKnown = tt.self, tt.selfKnown - selfMayDelegate = tt.selfKnown && !tt.self.IsPrivileged() + selfIdentity = tt.self + selfMayDelegate = tt.self.Known() && !tt.self.IsPrivileged() if got := IsPrivilegedCaller(tt.caller); got != tt.want { t.Fatalf("IsPrivilegedCaller(%v) with daemon %v = %t, want %t", @@ -124,9 +113,9 @@ func TestIsPrivilegedCaller_ThisProcess(t *testing.T) { // A caller that is neither root nor this process must be refused, whatever // this process happens to be. - other := Identity{UID: id.UID + 1} + other := Identity{known: true, UID: id.UID + 1} if id.IsWindows() { - other = Identity{SID: id.SID + "9"} + other = Identity{known: true, SID: id.SID + "9"} } if IsPrivilegedCaller(other) { t.Errorf("an unrelated identity %v was treated as privileged", other) diff --git a/client/internal/ipcauth/self_unix.go b/client/internal/ipcauth/self_unix.go index 1b86c4fc0..603930962 100644 --- a/client/internal/ipcauth/self_unix.go +++ b/client/internal/ipcauth/self_unix.go @@ -11,7 +11,8 @@ import "os" // Identity.IsPrivileged the daemon applies. func CurrentProcessIdentity() (Identity, error) { return Identity{ - UID: uint32(os.Geteuid()), - GID: uint32(os.Getegid()), + UID: uint32(os.Geteuid()), + GID: uint32(os.Getegid()), + known: true, }, nil } diff --git a/client/internal/ipcauth/self_windows.go b/client/internal/ipcauth/self_windows.go index 5474cc101..38a9d0782 100644 --- a/client/internal/ipcauth/self_windows.go +++ b/client/internal/ipcauth/self_windows.go @@ -31,5 +31,6 @@ func CurrentProcessIdentity() (Identity, error) { SID: user.User.Sid.String(), Groups: groups, Elevated: token.IsElevated(), + known: true, }, nil } diff --git a/client/internal/ipcauth/testing.go b/client/internal/ipcauth/testing.go new file mode 100644 index 000000000..5c578c02d --- /dev/null +++ b/client/internal/ipcauth/testing.go @@ -0,0 +1,9 @@ +package ipcauth + +// KnownForTest returns a copy of id marked as kernel-attested, so packages +// outside ipcauth can build identity fixtures. A real Identity is only ever +// produced by a peer-credential read, nothing outside a test should call this. +func KnownForTest(id Identity) Identity { + id.known = true + return id +} diff --git a/client/internal/profilemanager/profilemanager.go b/client/internal/profilemanager/profilemanager.go index b6eb735fa..44f578ae0 100644 --- a/client/internal/profilemanager/profilemanager.go +++ b/client/internal/profilemanager/profilemanager.go @@ -31,7 +31,7 @@ type Profile struct { // loader so callers do not have to reconstruct it from ID + dir. Path string IsActive bool - Owners []ipcauth.Identity + Owners []ipcauth.Principal } func (p *Profile) FilePath() (string, error) { diff --git a/client/internal/profilemanager/service.go b/client/internal/profilemanager/service.go index 5c93e9176..68d1c23a1 100644 --- a/client/internal/profilemanager/service.go +++ b/client/internal/profilemanager/service.go @@ -506,7 +506,7 @@ func (s *ServiceManager) loadAllProfiles(username string) ([]Profile, error) { Path: DefaultConfigPath, IsActive: activeIsDefault, // TODO: determine how to seed default owners - Owners: []ipcauth.Identity{}, + Owners: []ipcauth.Principal{}, }} configDir, err := s.getConfigDir(username) @@ -585,28 +585,32 @@ func readProfileName(path string) string { return meta.Name } -func readProfileOwners(path string) ([]ipcauth.Identity, error) { +// readProfileOwners parses the owner principals from a profile JSON. Owners stay +// principals so they are never mistaken for a kernel-attested caller. +// +// Only the first entry is read. The field is a list on disk so multiple owners +// can be added later without a format change, but multiple owners are not +// supported yet. +func readProfileOwners(path string) ([]ipcauth.Principal, error) { data, err := os.ReadFile(path) if err != nil { - return []ipcauth.Identity{}, err + return nil, err } var meta ownerMeta if err := json.Unmarshal(data, &meta); err != nil { - return []ipcauth.Identity{}, err + return nil, err } - if len(meta.Owners) < 1 { - return []ipcauth.Identity{}, nil + if len(meta.Owners) == 0 { + return nil, nil } - owner := meta.Owners[0] - principal, ok := ipcauth.ParsePrincipal(owner) + + principal, ok := ipcauth.ParsePrincipal(meta.Owners[0]) if !ok { - return []ipcauth.Identity{}, fmt.Errorf("unexpected owner principal: %s", owner) + // A malformed entry is ignored rather than trusted. + log.Warnf("ignoring unparseable owner %q in %s", meta.Owners[0], path) + return nil, nil } - id, err := ipcauth.IdentityFromPrincipal(principal) - if err != nil { - return []ipcauth.Identity{id}, fmt.Errorf("parsing identity from principal failed: %w", err) - } - return []ipcauth.Identity{id}, nil + return []ipcauth.Principal{principal}, nil } // nolint: unused,unusedfunc diff --git a/client/server/jwt_cache_test.go b/client/server/jwt_cache_test.go index 11d208ade..f2179a8da 100644 --- a/client/server/jwt_cache_test.go +++ b/client/server/jwt_cache_test.go @@ -13,11 +13,11 @@ import ( const testTTL = time.Minute func unixCaller(uid uint32) ipcauth.Identity { - return ipcauth.Identity{UID: uid, GID: uid} + return ipcauth.KnownForTest(ipcauth.Identity{UID: uid, GID: uid}) } func windowsCaller(sid string) ipcauth.Identity { - return ipcauth.Identity{SID: sid} + return ipcauth.KnownForTest(ipcauth.Identity{SID: sid}) } func TestJWTCache_ServesTheOwner(t *testing.T) { diff --git a/client/server/server.go b/client/server/server.go index ca8a65bf9..86a0eabf5 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -2699,29 +2699,28 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. return ctx, activeProf, nil } -// SessionHolder returns the Identity that owns the active and connected -// profile. The boolean indicates if the session is connected and an owner -// is defined in the config. -func (s *Server) SessionHolder() (ipcauth.Identity, bool) { +// SessionHolder returns the principal that owns the active profile while it is +// connected. The owner is a config value, so it stays a principal and is never +// turned into an identity. +// +// Only the first owner is read. The field is a list on disk so multiple owners +// can be added later without a format change, but multiple owners are not +// supported yet. +func (s *Server) SessionHolder() (ipcauth.Principal, bool) { s.mutex.Lock() defer s.mutex.Unlock() - activeOwners := s.config.Owners - if !s.clientRunning || len(activeOwners) < 1 { - return ipcauth.Identity{}, false + if !s.clientRunning || len(s.config.Owners) == 0 { + return ipcauth.Principal{}, false } - principal, ok := ipcauth.ParsePrincipal(activeOwners[0]) + // The zero Principal matches nobody, so an unparseable owner locks the + // session rather than opening it. + principal, ok := ipcauth.ParsePrincipal(s.config.Owners[0]) if !ok { - return ipcauth.Identity{}, false + log.Warnf("active profile has an unparseable owner %q", s.config.Owners[0]) } - - id, err := ipcauth.IdentityFromPrincipal(principal) - if err != nil { - return ipcauth.Identity{}, false - } - - return id, true + return principal, true } func persistLoginOverrides(activeProf *profilemanager.ActiveProfileState, managementURL string, preSharedKey *string) error { diff --git a/client/server/ssh_gate_test.go b/client/server/ssh_gate_test.go index b4712c64a..b730ffaa6 100644 --- a/client/server/ssh_gate_test.go +++ b/client/server/ssh_gate_test.go @@ -44,18 +44,18 @@ func userCtx() context.Context { return ctxWithIdentity(unprivilegedIdentity()) func privilegedIdentity() ipcauth.Identity { if runtime.GOOS == "windows" { // LocalSystem, which is what the Windows service account is. - return ipcauth.Identity{SID: "S-1-5-18"} + return ipcauth.KnownForTest(ipcauth.Identity{SID: "S-1-5-18"}) } - return ipcauth.Identity{UID: 0} + return ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) } func unprivilegedIdentity() ipcauth.Identity { if runtime.GOOS == "windows" { // A plain user SID: no groups, so no BUILTIN\Administrators, and not // elevated. - return ipcauth.Identity{SID: "S-1-5-21-1-2-3-1001"} + return ipcauth.KnownForTest(ipcauth.Identity{SID: "S-1-5-21-1-2-3-1001"}) } - return ipcauth.Identity{UID: unprivUID, GID: unprivUID} + return ipcauth.KnownForTest(ipcauth.Identity{UID: unprivUID, GID: unprivUID}) } func noIdentityCtx() context.Context { return context.Background() }