diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 8f24de116..4cae872e2 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -29,6 +29,7 @@ import ( "github.com/netbirdio/netbird/shared/management/domain" + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork" "github.com/netbirdio/netbird/management/internals/modules/peers" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/accesslogs" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy" @@ -36,7 +37,6 @@ import ( "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/sessionkey" "github.com/netbirdio/netbird/management/server/idp" "github.com/netbirdio/netbird/management/server/peer" - "github.com/netbirdio/netbird/management/internals/modules/agentnetwork" "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/management/server/users" proxyauth "github.com/netbirdio/netbird/proxy/auth" @@ -1579,9 +1579,62 @@ func (s *ProxyServiceServer) ValidateState(state string) (verifier, redirectURL return verifier, redirectURL, nil } +// Denied reasons reported to the proxy when access is refused because of the +// account status of the user behind the request. +const ( + deniedReasonPendingApproval = "pending_approval" + deniedReasonUserBlocked = "user_blocked" + deniedReasonUserNotFound = "user_not_found" +) + +var ( + // ErrUserPendingApproval reports a user whose account still awaits approval + // by an administrator and may therefore not hold a proxy session. + ErrUserPendingApproval = errors.New("user pending approval") + + // ErrUserBlocked reports a blocked user, who may not hold a proxy session. + ErrUserBlocked = errors.New("user blocked") + + errUserUnresolved = errors.New("user could not be resolved") +) + +// checkUserStatus reports whether the user's account status permits reverse +// proxy access, returning the denied reason for the proxy access log together +// with the sentinel error callers match on. A user awaiting approval is stored +// as both pending and blocked, so the pending state is reported first: it is +// the one an administrator can act on. +func checkUserStatus(user *types.User) (string, error) { + switch { + case user == nil: + return deniedReasonUserNotFound, errUserUnresolved + case user.PendingApproval: + return deniedReasonPendingApproval, ErrUserPendingApproval + case user.IsBlocked(): + return deniedReasonUserBlocked, ErrUserBlocked + default: + return "", nil + } +} + +// userStatusDeniedReason returns the denied reason for callers that report a +// decision rather than an error, and an empty string when the user may proceed. +func userStatusDeniedReason(user *types.User) string { + reason, _ := checkUserStatus(user) + return reason +} + +// sameAccount reports whether a user belongs to a service's account. An empty +// identifier on either side never matches: two unset accounts must not compare +// equal into a grant. +func sameAccount(userAccountID, serviceAccountID string) bool { + return userAccountID != "" && serviceAccountID != "" && userAccountID == serviceAccountID +} + // GenerateSessionToken creates a signed session JWT for the given domain and // user. The user's group memberships are embedded in the token so policy-aware // middlewares on the proxy can authorise without an extra management round-trip. +// A user the store cannot resolve, or whose account is pending approval or +// blocked, gets no token at all, so the browser never receives a session cookie. func (s *ProxyServiceServer) GenerateSessionToken(ctx context.Context, domain, userID string, method proxyauth.Method) (string, error) { service, err := s.getServiceByDomain(ctx, domain) if err != nil { @@ -1592,25 +1645,37 @@ func (s *ProxyServiceServer) GenerateSessionToken(ctx context.Context, domain, u return "", fmt.Errorf("no session key configured for domain: %s", domain) } - var ( - email string - groupIDs []string - groupNames []string - ) - if s.usersManager != nil { - user, userGroups, uerr := s.usersManager.GetUserWithGroups(ctx, userID) - if uerr != nil { - log.WithContext(ctx).Debugf("session token mint: lookup user %s: %v", userID, uerr) - } else if user != nil { - email = user.Email - groupIDs, groupNames = pairGroupIDsAndNames(userGroups) - } + if s.usersManager == nil { + return "", errors.New("users manager not configured") } + user, userGroups, err := s.usersManager.GetUserWithGroups(ctx, userID) + if err != nil { + return "", fmt.Errorf("get user %s: %w", userID, err) + } + + if user == nil { + return "", fmt.Errorf("get user %s: %w", userID, errUserUnresolved) + } + + // Bind the OIDC identity to the service's account before signing anything + // with that service's session key. The proxy validates an installed cookie + // locally against the service public key, so a token minted for a user of + // another account would be honoured without a management round-trip. + if !sameAccount(user.AccountID, service.AccountID) { + return "", fmt.Errorf("user %s does not belong to the service account", userID) + } + + if _, err := checkUserStatus(user); err != nil { + return "", fmt.Errorf("session token for user %s: %w", userID, err) + } + + groupIDs, groupNames := pairGroupIDsAndNames(userGroups) + return sessionkey.SignToken( service.SessionPrivateKey, userID, - email, + user.Email, domain, method, groupIDs, @@ -1628,6 +1693,10 @@ func (s *ProxyServiceServer) ValidateUserGroupAccess(ctx context.Context, domain return fmt.Errorf("user not found: %s", userID) } + if _, err := checkUserStatus(user); err != nil { + return fmt.Errorf("user %s denied access to domain %s: %w", userID, domain, err) + } + service, err := s.getAccountServiceByDomain(ctx, user.AccountID, domain) if err != nil { return err @@ -1682,10 +1751,7 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val sessionToken := req.GetSessionToken() if domain == "" || sessionToken == "" { - return &proto.ValidateSessionResponse{ - Valid: false, - DeniedReason: "missing domain or session_token", - }, nil + return deniedSessionResponse("missing domain or session_token"), nil } service, err := s.getServiceByDomain(ctx, domain) @@ -1695,83 +1761,49 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val "error": err.Error(), }).Debug("ValidateSession: service not found") //nolint:nilerr - return &proto.ValidateSessionResponse{ - Valid: false, - DeniedReason: "service_not_found", - }, nil + return deniedSessionResponse("service_not_found"), nil } if err := enforceAccountScope(ctx, service.AccountID); err != nil { return nil, err } - pubKeyBytes, err := base64.StdEncoding.DecodeString(service.SessionPublicKey) - if err != nil { - log.WithFields(log.Fields{ - "domain": domain, - "error": err.Error(), - }).Error("ValidateSession: decode public key") - //nolint:nilerr - return &proto.ValidateSessionResponse{ - Valid: false, - DeniedReason: "invalid_service_config", - }, nil - } - - userID, _, _, _, _, err := proxyauth.ValidateSessionJWT(sessionToken, domain, pubKeyBytes) - if err != nil { - log.WithFields(log.Fields{ - "domain": domain, - "error": err.Error(), - }).Debug("ValidateSession: invalid session token") - //nolint:nilerr - return &proto.ValidateSessionResponse{ - Valid: false, - DeniedReason: "invalid_token", - }, nil + userID, reason := sessionTokenSubject(domain, service, sessionToken) + if reason != "" { + return deniedSessionResponse(reason), nil } user, userGroups, err := s.usersManager.GetUserWithGroups(ctx, userID) - if err != nil { + if err != nil || user == nil { log.WithFields(log.Fields{ "domain": domain, "user_id": userID, - "error": err.Error(), + "error": err, }).Debug("ValidateSession: user not found") //nolint:nilerr - return &proto.ValidateSessionResponse{ - Valid: false, - DeniedReason: "user_not_found", - }, nil + return deniedSessionResponse(deniedReasonUserNotFound), nil } - if user.AccountID != service.AccountID { + // A user from another account gets a bare response: none of their identity + // belongs in an answer to a proxy serving a different account. + if !sameAccount(user.AccountID, service.AccountID) { log.WithFields(log.Fields{ "domain": domain, "user_id": userID, "user_account": user.AccountID, "service_account": service.AccountID, }).Debug("ValidateSession: user account mismatch") - //nolint:nilerr - return &proto.ValidateSessionResponse{ - Valid: false, - DeniedReason: "account_mismatch", - }, nil + return deniedSessionResponse("account_mismatch"), nil } - if err := s.checkGroupAccess(service, user); err != nil { - log.WithFields(log.Fields{ - "domain": domain, - "user_id": userID, - "error": err.Error(), - }).Debug("ValidateSession: access denied") - groupIDs, groupNames := pairGroupIDsAndNames(userGroups) - //nolint:nilerr + groupIDs, groupNames := pairGroupIDsAndNames(userGroups) + + if reason := s.accountUserDeniedReason(domain, service, user); reason != "" { return &proto.ValidateSessionResponse{ Valid: false, UserId: user.Id, UserEmail: user.Email, - DeniedReason: "not_in_group", + DeniedReason: reason, PeerGroupIds: groupIDs, PeerGroupNames: groupNames, }, nil @@ -1783,7 +1815,6 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val "email": user.Email, }).Debug("ValidateSession: access granted") - groupIDs, groupNames := pairGroupIDsAndNames(userGroups) return &proto.ValidateSessionResponse{ Valid: true, UserId: user.Id, @@ -1793,6 +1824,66 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val }, nil } +// deniedSessionResponse builds a denial that carries no identity, for the +// checks that run before a user of this service's account is resolved. +func deniedSessionResponse(reason string) *proto.ValidateSessionResponse { + return &proto.ValidateSessionResponse{ + Valid: false, + DeniedReason: reason, + } +} + +// sessionTokenSubject verifies the session token against the service's session +// key and returns the user it was minted for, or the reason it cannot be +// trusted. +func sessionTokenSubject(domain string, service *rpservice.Service, sessionToken string) (userID, deniedReason string) { + pubKeyBytes, err := base64.StdEncoding.DecodeString(service.SessionPublicKey) + if err != nil { + log.WithFields(log.Fields{ + "domain": domain, + "error": err.Error(), + }).Error("ValidateSession: decode public key") + return "", "invalid_service_config" + } + + userID, _, _, _, _, err = proxyauth.ValidateSessionJWT(sessionToken, domain, pubKeyBytes) + if err != nil { + log.WithFields(log.Fields{ + "domain": domain, + "error": err.Error(), + }).Debug("ValidateSession: invalid session token") + return "", "invalid_token" + } + + return userID, "" +} + +// accountUserDeniedReason gates a user of the service's own account, returning +// an empty string when access is granted. Account status comes before group +// membership: a user awaiting approval or blocked has no access regardless of +// the groups they were auto-assigned. +func (s *ProxyServiceServer) accountUserDeniedReason(domain string, service *rpservice.Service, user *types.User) string { + if reason := userStatusDeniedReason(user); reason != "" { + log.WithFields(log.Fields{ + "domain": domain, + "user_id": user.Id, + "reason": reason, + }).Debug("ValidateSession: user status denies access") + return reason + } + + if err := s.checkGroupAccess(service, user); err != nil { + log.WithFields(log.Fields{ + "domain": domain, + "user_id": user.Id, + "error": err.Error(), + }).Debug("ValidateSession: access denied") + return "not_in_group" + } + + return "" +} + func (s *ProxyServiceServer) getServiceByDomain(ctx context.Context, domain string) (*rpservice.Service, error) { service, err := s.serviceManager.GetServiceByDomain(ctx, domain) if err == nil { @@ -1907,7 +1998,20 @@ func (s *ProxyServiceServer) ValidateTunnelPeer(ctx context.Context, req *proto. } groupIDs, groupNames := pairGroupIDsAndNames(peerGroups) - principalID, displayIdentity := s.getTunnelPeerInfo(ctx, domain, service, peer) + owner := s.resolvePeerOwner(ctx, peer, service.AccountID) + principalID, displayIdentity := s.getTunnelPeerInfo(ctx, domain, service, peer, owner) + + if reason := peerOwnerDeniedReason(peer, owner); reason != "" { + log.WithFields(log.Fields{"domain": domain, "peer_id": peer.ID, "user_id": peer.UserID, "reason": reason}).Debug("ValidateTunnelPeer: owner status denies access") + return &proto.ValidateTunnelPeerResponse{ + Valid: false, + UserId: principalID, + UserEmail: displayIdentity, + DeniedReason: reason, + PeerGroupIds: groupIDs, + PeerGroupNames: groupNames, + }, nil + } if err := checkPeerGroupAccess(service, groupIDs); err != nil { log.WithFields(log.Fields{"domain": domain, "peer_id": peer.ID, "error": err.Error()}).Debug("ValidateTunnelPeer: access denied") @@ -1944,9 +2048,55 @@ func (s *ProxyServiceServer) ValidateTunnelPeer(ctx context.Context, req *proto. }, nil } +// resolvePeerOwner returns the user a peer is linked to, once per request so +// the status gate and the identity resolution below share a single lookup. +// Unlinked peers (machine agents) have no owner. A lookup that fails returns +// nil rather than an error: both callers treat an unresolved owner the same +// way, and neither may trust one it could not read. +func (s *ProxyServiceServer) resolvePeerOwner(ctx context.Context, peer *peer.Peer, accountID string) *types.User { + if peer.UserID == "" { + return nil + } + + user, err := s.usersManager.GetUser(ctx, peer.UserID) + if err != nil { + log.WithContext(ctx).Debugf("ValidateTunnelPeer: look up owner %s of peer %s: %v", peer.UserID, peer.ID, err) + return nil + } + + // The lookup is by user ID alone, so a peer row pointing outside the + // service's account would otherwise resolve a foreign user. Leave the owner + // unresolved instead: the gate denies it, and neither the response nor the + // minted token carries an identity from another account. + if !sameAccount(user.AccountID, accountID) { + log.WithContext(ctx).Debugf("ValidateTunnelPeer: owner %s of peer %s belongs to another account", peer.UserID, peer.ID) + return nil + } + + return user +} + +// peerOwnerDeniedReason gates the mesh fast-path on the account status of the +// peer's owning user, so a user blocked after registering a peer loses +// mesh-origin access too. Unlinked peers (machine agents) have no owner to gate +// on and stay first-class callers. An owner the store cannot resolve denies: +// an unavailable lookup must not grant access. +func peerOwnerDeniedReason(peer *peer.Peer, owner *types.User) string { + if peer.UserID == "" { + return "" + } + + if owner == nil { + return deniedReasonUserNotFound + } + + return userStatusDeniedReason(owner) +} + // getTunnelPeerInfo returns the principal ID and display name for a peer, e.g. a -// user or peer ID, and peer name or user email. -func (s *ProxyServiceServer) getTunnelPeerInfo(ctx context.Context, domain string, service *rpservice.Service, peer *peer.Peer) (string, string) { +// user or peer ID, and peer name or user email. owner is the already-resolved +// user the peer is linked to, or nil. +func (s *ProxyServiceServer) getTunnelPeerInfo(ctx context.Context, domain string, service *rpservice.Service, peer *peer.Peer, owner *types.User) (string, string) { // Resolve the principal: when the peer is linked to a user, the human is the // principal so multiple peers owned by the same user share a single // identity. Unlinked peers (machine agents) are their own principal keyed on @@ -1963,10 +2113,10 @@ func (s *ProxyServiceServer) getTunnelPeerInfo(ctx context.Context, domain strin principalID := peer.UserID displayIdentity := peer.Name // Stored column first (cheap, but often empty for OIDC-provisioned users). - if user, uerr := s.usersManager.GetUser(ctx, peer.UserID); uerr == nil && user != nil { - principalID = user.Id - if user.Email != "" { - displayIdentity = user.Email + if owner != nil { + principalID = owner.Id + if owner.Email != "" { + displayIdentity = owner.Email } } // IdP enrichment wins when available — the stored email column is a diff --git a/management/internals/shared/grpc/proxy_group_access_test.go b/management/internals/shared/grpc/proxy_group_access_test.go index 532cb7cc3..cb30a07b9 100644 --- a/management/internals/shared/grpc/proxy_group_access_test.go +++ b/management/internals/shared/grpc/proxy_group_access_test.go @@ -119,11 +119,13 @@ func (m *mockReverseProxyManager) GetClusters(_ context.Context, _, _ string) ([ } type mockUsersManager struct { - users map[string]*types.User - err error + users map[string]*types.User + err error + getUserCalls int } func (m *mockUsersManager) GetUser(ctx context.Context, userID string) (*types.User, error) { + m.getUserCalls++ if m.err != nil { return nil, m.err } @@ -350,6 +352,64 @@ func TestValidateUserGroupAccess(t *testing.T) { }, expectErr: false, }, + { + name: "user pending approval denied despite group membership", + domain: "app.example.com", + userID: "user1", + proxiesByAccount: map[string][]*service.Service{ + "account1": {{ + Domain: "app.example.com", + AccountID: "account1", + Auth: service.AuthConfig{ + BearerAuth: &service.BearerAuthConfig{ + Enabled: true, + DistributionGroups: []string{"group1"}, + }, + }, + }}, + }, + users: map[string]*types.User{ + // The approval flow stores a pending user as blocked as well. + "user1": {Id: "user1", AccountID: "account1", AutoGroups: []string{"group1"}, Blocked: true, PendingApproval: true}, + }, + expectErr: true, + expectErrMsg: "user pending approval", + }, + { + name: "blocked user denied despite group membership", + domain: "app.example.com", + userID: "user1", + proxiesByAccount: map[string][]*service.Service{ + "account1": {{ + Domain: "app.example.com", + AccountID: "account1", + Auth: service.AuthConfig{ + BearerAuth: &service.BearerAuthConfig{ + Enabled: true, + DistributionGroups: []string{"group1"}, + }, + }, + }}, + }, + users: map[string]*types.User{ + "user1": {Id: "user1", AccountID: "account1", AutoGroups: []string{"group1"}, Blocked: true}, + }, + expectErr: true, + expectErrMsg: "user blocked", + }, + { + name: "blocked user denied on a service with no auth configured", + domain: "app.example.com", + userID: "user1", + proxiesByAccount: map[string][]*service.Service{ + "account1": {{Domain: "app.example.com", AccountID: "account1", Auth: service.AuthConfig{}}}, + }, + users: map[string]*types.User{ + "user1": {Id: "user1", AccountID: "account1", Blocked: true}, + }, + expectErr: true, + expectErrMsg: "user blocked", + }, { name: "proxy manager error", domain: "app.example.com", @@ -421,17 +481,18 @@ func TestValidateTunnelPeerUserEmailEnrichment(t *testing.T) { storedUserNoEmail := map[string]*types.User{userID: {Id: userID, AccountID: accountID, Email: ""}} tests := []struct { - name string - peerUserID string - storedUsers map[string]*types.User - storedErr error - noIdP bool - idpEmail string - idpHasData bool - idpErr error - expectEmail string - expectUserID string - expectIdPHit bool + name string + peerUserID string + storedUsers map[string]*types.User + storedErr error + noIdP bool + idpEmail string + idpHasData bool + idpErr error + expectEmail string + expectUserID string + expectIdPHit bool + expectDeniedReason string }{ { name: "idp email wins over stored email", @@ -490,14 +551,17 @@ func TestValidateTunnelPeerUserEmailEnrichment(t *testing.T) { expectIdPHit: true, }, { - name: "idp email when stored user missing keeps peer.UserID as principal", - peerUserID: userID, - storedUsers: map[string]*types.User{}, - idpEmail: "idp@example.com", - idpHasData: true, - expectEmail: "idp@example.com", - expectUserID: userID, - expectIdPHit: true, + // The identity still resolves from the IdP, but an owner the store + // cannot resolve denies the fast-path rather than granting it. + name: "idp email when stored user missing keeps peer.UserID as principal", + peerUserID: userID, + storedUsers: map[string]*types.User{}, + idpEmail: "idp@example.com", + idpHasData: true, + expectEmail: "idp@example.com", + expectUserID: userID, + expectIdPHit: true, + expectDeniedReason: deniedReasonUserNotFound, }, { name: "unlinked peer uses peer name and never consults idp", @@ -545,9 +609,13 @@ func TestValidateTunnelPeerUserEmailEnrichment(t *testing.T) { require.NoError(t, err) require.NotNil(t, resp) - assert.True(t, resp.GetValid(), "expected access granted") + assert.Equal(t, tt.expectDeniedReason == "", resp.GetValid(), "unexpected access decision") + assert.Equal(t, tt.expectDeniedReason, resp.GetDeniedReason(), "unexpected denied reason") assert.Equal(t, tt.expectEmail, resp.GetUserEmail()) assert.Equal(t, tt.expectUserID, resp.GetUserId()) + if tt.expectDeniedReason != "" { + assert.Empty(t, resp.GetSessionToken(), "a denied peer must not receive a session token") + } if idpMock != nil { if tt.expectIdPHit { @@ -562,6 +630,121 @@ func TestValidateTunnelPeerUserEmailEnrichment(t *testing.T) { } } +// TestDeniedReasonValues pins the wire values of the account status denied +// reasons. The proxy logs them and operators filter access logs on them, so a +// rename is a breaking change rather than an internal detail. +// TestSameAccount pins the fail-closed behaviour of the account binding: an +// unset account on either side must never compare equal into a grant. +func TestSameAccount(t *testing.T) { + assert.True(t, sameAccount("account1", "account1"), "matching accounts should bind") + assert.False(t, sameAccount("account1", "account2"), "different accounts must not bind") + assert.False(t, sameAccount("", ""), "two unset accounts must not bind") + assert.False(t, sameAccount("account1", ""), "an unset service account must not bind") + assert.False(t, sameAccount("", "account1"), "an unset user account must not bind") +} + +func TestDeniedReasonValues(t *testing.T) { + assert.Equal(t, "pending_approval", deniedReasonPendingApproval, "pending approval denied reason wire value") + assert.Equal(t, "user_blocked", deniedReasonUserBlocked, "blocked user denied reason wire value") + assert.Equal(t, "user_not_found", deniedReasonUserNotFound, "unresolved user denied reason wire value") +} + +// TestValidateTunnelPeerOwnerStatus verifies that the mesh fast-path gates on +// the account status of the peer's owning user. A peer whose owner was blocked +// after the peer registered must lose access, while an unlinked machine peer +// keeps it. +func TestValidateTunnelPeerOwnerStatus(t *testing.T) { + const ( + domain = "app.example.com" + accountID = "account1" + peerID = "peer1" + peerName = "peer-display-name" + userID = "user1" + ) + + tests := []struct { + name string + peerUserID string + owner *types.User + expectDeniedReason string + expectEmail string + }{ + { + name: "active owner allowed", + peerUserID: userID, + owner: &types.User{Id: userID, AccountID: accountID, Email: "user@example.com"}, + }, + { + name: "owner pending approval denied", + peerUserID: userID, + owner: &types.User{Id: userID, AccountID: accountID, Email: "user@example.com", Blocked: true, PendingApproval: true}, + expectDeniedReason: deniedReasonPendingApproval, + }, + { + name: "owner blocked after registering the peer denied", + peerUserID: userID, + owner: &types.User{Id: userID, AccountID: accountID, Email: "user@example.com", Blocked: true}, + expectDeniedReason: deniedReasonUserBlocked, + }, + { + name: "unlinked machine peer stays allowed", + peerUserID: "", + owner: &types.User{Id: userID, AccountID: accountID, Blocked: true}, + }, + { + // The user lookup is not account-scoped, so a peer row pointing at + // another account's user must not resolve into an owner: the peer is + // denied and the foreign email never reaches the response. + name: "owner in another account denied and not disclosed", + peerUserID: userID, + owner: &types.User{Id: userID, AccountID: "otherAccount", Email: "foreign@example.com"}, + expectDeniedReason: deniedReasonUserNotFound, + expectEmail: peerName, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := &service.Service{Domain: domain, AccountID: accountID} + usersManager := &mockUsersManager{users: map[string]*types.User{userID: tt.owner}} + server := &ProxyServiceServer{ + serviceManager: &mockReverseProxyManager{ + proxiesByAccount: map[string][]*service.Service{accountID: {svc}}, + }, + peersManager: &mockTunnelPeersManager{ + peer: &peer.Peer{ID: peerID, Name: peerName, UserID: tt.peerUserID}, + }, + usersManager: usersManager, + } + + resp, err := server.ValidateTunnelPeer(context.Background(), &proto.ValidateTunnelPeerRequest{ + Domain: domain, + TunnelIp: "100.64.0.1", + }) + + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, tt.expectDeniedReason, resp.GetDeniedReason(), "unexpected denied reason") + assert.Equal(t, tt.expectDeniedReason == "", resp.GetValid(), "unexpected access decision") + if tt.expectDeniedReason != "" { + assert.Empty(t, resp.GetSessionToken(), "a denied peer must not receive a session token") + } + + if tt.expectEmail != "" { + assert.Equal(t, tt.expectEmail, resp.GetUserEmail(), "unexpected identity on the response") + } + + // The status gate and the identity resolution share one lookup; + // an unlinked peer has no owner to look up at all. + wantLookups := 1 + if tt.peerUserID == "" { + wantLookups = 0 + } + assert.Equal(t, wantLookups, usersManager.getUserCalls, "owner must be resolved exactly once per request") + }) + } +} + func TestGetAccountProxyByDomain(t *testing.T) { tests := []struct { name string diff --git a/management/internals/shared/grpc/validate_session_test.go b/management/internals/shared/grpc/validate_session_test.go index d649102a1..03f200414 100644 --- a/management/internals/shared/grpc/validate_session_test.go +++ b/management/internals/shared/grpc/validate_session_test.go @@ -46,6 +46,7 @@ func setupValidateSessionTest(t *testing.T) *validateSessionTestSetup { proxyService.SetServiceManager(serviceManager) createTestProxies(t, ctx, testStore) + createStatusTestUsers(t, ctx, testStore) return &validateSessionTestSetup{ proxyService: proxyService, @@ -91,6 +92,82 @@ func createTestProxies(t *testing.T, ctx context.Context, testStore store.Store) }, } require.NoError(t, testStore.CreateService(ctx, restrictedProxy)) + + // Distributed to the account's "All" group, the configuration that hands a + // service to every user in the account. + allUsersProxy := &service.Service{ + ID: "allUsersProxyId", + AccountID: "testAccountId", + Name: "All Users Proxy", + Domain: "all-users-proxy.example.com", + Enabled: true, + SessionPrivateKey: privKey, + SessionPublicKey: pubKey, + Auth: service.AuthConfig{ + BearerAuth: &service.BearerAuthConfig{ + Enabled: true, + DistributionGroups: []string{allUsersGroupID}, + }, + }, + } + require.NoError(t, testStore.CreateService(ctx, allUsersProxy)) +} + +const ( + allUsersGroupID = "allUsersGroupId" + pendingUserID = "pendingUserId" + blockedUserID = "blockedUserId" + pendingAllUsersID = "pendingAllUsersUserId" +) + +// createStatusTestUsers adds the users whose account status must keep them out +// of a proxy session. A user awaiting approval is persisted as both blocked and +// pending approval, the way the approval flow stores one. +func createStatusTestUsers(t *testing.T, ctx context.Context, testStore store.Store) { + t.Helper() + + require.NoError(t, testStore.CreateGroup(ctx, &types.Group{ + ID: allUsersGroupID, + AccountID: "testAccountId", + Name: "All", + Issued: types.GroupIssuedAPI, + })) + + users := []*types.User{ + { + Id: pendingUserID, + AccountID: "testAccountId", + Role: types.UserRoleUser, + AutoGroups: []string{"allowedGroupId"}, + Blocked: true, + PendingApproval: true, + Issued: "api", + CreatedAt: time.Now(), + }, + { + Id: pendingAllUsersID, + AccountID: "testAccountId", + Role: types.UserRoleUser, + AutoGroups: []string{allUsersGroupID}, + Blocked: true, + PendingApproval: true, + Issued: "api", + CreatedAt: time.Now(), + }, + { + Id: blockedUserID, + AccountID: "testAccountId", + Role: types.UserRoleUser, + AutoGroups: []string{"allowedGroupId"}, + Blocked: true, + PendingApproval: false, + Issued: "api", + CreatedAt: time.Now(), + }, + } + for _, user := range users { + require.NoError(t, testStore.SaveUser(ctx, user)) + } } func generateSessionKeyPair(t *testing.T) (string, string) { @@ -149,6 +226,114 @@ func TestValidateSession_UserNotInAllowedGroup(t *testing.T) { assert.Empty(t, resp.GetPeerGroupIds(), "PeerGroupIds must mirror the resolved user's actual (empty) memberships on denial") } +// TestValidateSession_PendingApprovalUserDenied covers a user who is a member of +// the service's distribution group but is still waiting for an administrator to +// approve the account. Group membership alone must not open the service. +func TestValidateSession_PendingApprovalUserDenied(t *testing.T) { + setup := setupValidateSessionTest(t) + defer setup.cleanup() + + proxy, err := setup.store.GetServiceByID(context.Background(), store.LockingStrengthNone, "testAccountId", "restrictedProxyId") + require.NoError(t, err) + + token := createSessionToken(t, proxy.SessionPrivateKey, pendingUserID, "restricted-proxy.example.com") + + resp, err := setup.proxyService.ValidateSession(context.Background(), &proto.ValidateSessionRequest{ + Domain: "restricted-proxy.example.com", + SessionToken: token, + }) + + require.NoError(t, err) + assert.False(t, resp.Valid, "User pending approval should be denied") + assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason, "Denied reason should name the pending approval state") + assert.Equal(t, pendingUserID, resp.UserId, "Denial should identify the user it applies to") + assert.Equal(t, []string{"allowedGroupId"}, resp.GetPeerGroupIds(), "PeerGroupIds must mirror the resolved user's group memberships on denial") + assert.Equal(t, []string{"Allowed Group"}, resp.GetPeerGroupNames(), "PeerGroupNames must pair with PeerGroupIds on denial") +} + +// TestValidateSession_PendingApprovalUserInAllUsersGroupDenied covers the same +// user against a service distributed to the account's "All" group, where every +// user of the account is a member by default. +func TestValidateSession_PendingApprovalUserInAllUsersGroupDenied(t *testing.T) { + setup := setupValidateSessionTest(t) + defer setup.cleanup() + + proxy, err := setup.store.GetServiceByID(context.Background(), store.LockingStrengthNone, "testAccountId", "allUsersProxyId") + require.NoError(t, err) + + token := createSessionToken(t, proxy.SessionPrivateKey, pendingAllUsersID, "all-users-proxy.example.com") + + resp, err := setup.proxyService.ValidateSession(context.Background(), &proto.ValidateSessionRequest{ + Domain: "all-users-proxy.example.com", + SessionToken: token, + }) + + require.NoError(t, err) + assert.False(t, resp.Valid, "User pending approval should be denied even in the All Users group") + assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason, "Denied reason should name the pending approval state") + assert.Equal(t, pendingAllUsersID, resp.UserId, "Denial should identify the user it applies to") + assert.Equal(t, []string{allUsersGroupID}, resp.GetPeerGroupIds(), "PeerGroupIds must mirror the resolved user's group memberships on denial") +} + +// TestValidateSession_BlockedUserDenied covers a user blocked after having been +// approved, so PendingApproval is false and only the blocked flag is set. +func TestValidateSession_BlockedUserDenied(t *testing.T) { + setup := setupValidateSessionTest(t) + defer setup.cleanup() + + proxy, err := setup.store.GetServiceByID(context.Background(), store.LockingStrengthNone, "testAccountId", "restrictedProxyId") + require.NoError(t, err) + + token := createSessionToken(t, proxy.SessionPrivateKey, blockedUserID, "restricted-proxy.example.com") + + resp, err := setup.proxyService.ValidateSession(context.Background(), &proto.ValidateSessionRequest{ + Domain: "restricted-proxy.example.com", + SessionToken: token, + }) + + require.NoError(t, err) + assert.False(t, resp.Valid, "Blocked user should be denied") + assert.Equal(t, deniedReasonUserBlocked, resp.DeniedReason, "Denied reason should name the blocked state") + assert.Equal(t, blockedUserID, resp.UserId, "Denial should identify the user it applies to") +} + +// TestValidateSession_UserAllowedAfterApproval walks the same session token +// through the approval transition: denied while pending, allowed once an +// administrator clears both flags. +func TestValidateSession_UserAllowedAfterApproval(t *testing.T) { + setup := setupValidateSessionTest(t) + defer setup.cleanup() + + ctx := context.Background() + + proxy, err := setup.store.GetServiceByID(ctx, store.LockingStrengthNone, "testAccountId", "restrictedProxyId") + require.NoError(t, err) + + token := createSessionToken(t, proxy.SessionPrivateKey, pendingUserID, "restricted-proxy.example.com") + req := &proto.ValidateSessionRequest{ + Domain: "restricted-proxy.example.com", + SessionToken: token, + } + + resp, err := setup.proxyService.ValidateSession(ctx, req) + require.NoError(t, err) + require.False(t, resp.Valid, "User pending approval should be denied before approval") + assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason, "Denied reason should name the pending approval state") + + user, err := setup.store.GetUserByUserID(ctx, store.LockingStrengthNone, pendingUserID) + require.NoError(t, err) + user.PendingApproval = false + user.Blocked = false + require.NoError(t, setup.store.SaveUser(ctx, user)) + + resp, err = setup.proxyService.ValidateSession(ctx, req) + require.NoError(t, err) + assert.True(t, resp.Valid, "Approved user should be allowed access") + assert.Empty(t, resp.DeniedReason) + assert.Equal(t, pendingUserID, resp.UserId, "Approved user should be identified in the response") + assert.Equal(t, []string{"allowedGroupId"}, resp.GetPeerGroupIds(), "PeerGroupIds must mirror the approved user's group memberships") +} + func TestValidateSession_UserInDifferentAccount(t *testing.T) { setup := setupValidateSessionTest(t) defer setup.cleanup() diff --git a/management/server/http/handlers/proxy/auth.go b/management/server/http/handlers/proxy/auth.go index 0120fad0e..62725e8d4 100644 --- a/management/server/http/handlers/proxy/auth.go +++ b/management/server/http/handlers/proxy/auth.go @@ -2,6 +2,7 @@ package proxy import ( "context" + "errors" "net" "net/http" "net/netip" @@ -108,7 +109,7 @@ func (h *AuthCallbackHandler) handleCallback(w http.ResponseWriter, r *http.Requ redirectURL.Scheme = "https" query := redirectURL.Query() query.Set("error", "access_denied") - query.Set("error_description", "Service configuration error") + query.Set("error_description", sessionTokenErrorDescription(err)) redirectURL.RawQuery = query.Encode() http.Redirect(w, r, redirectURL.String(), http.StatusFound) return @@ -124,6 +125,20 @@ func (h *AuthCallbackHandler) handleCallback(w http.ResponseWriter, r *http.Requ http.Redirect(w, r, redirectURL.String(), http.StatusFound) } +// sessionTokenErrorDescription maps a session token failure to the text the +// proxy renders on its access denied page. Account status denials get a message +// the user can act on, while everything else stays generic so a lookup or +// signing failure does not describe management internals to the browser. +func sessionTokenErrorDescription(err error) string { + if errors.Is(err, nbgrpc.ErrUserPendingApproval) { + return "Your account is pending approval by an administrator" + } + if errors.Is(err, nbgrpc.ErrUserBlocked) { + return "Your account is blocked" + } + return "Service configuration error" +} + func extractUserIDFromToken(ctx context.Context, provider *oidc.Provider, config nbgrpc.ProxyOIDCConfig, token *oauth2.Token) string { rawIDToken, ok := token.Extra("id_token").(string) if !ok { diff --git a/management/server/http/handlers/proxy/auth_callback_integration_test.go b/management/server/http/handlers/proxy/auth_callback_integration_test.go index a24857066..51e54ebce 100644 --- a/management/server/http/handlers/proxy/auth_callback_integration_test.go +++ b/management/server/http/handlers/proxy/auth_callback_integration_test.go @@ -360,6 +360,51 @@ func createTestAccountsAndUsers(t *testing.T, ctx context.Context, testStore sto Issued: "api", } require.NoError(t, testStore.SaveUser(ctx, allowedUser)) + + // A second tenant, whose users must never be issued a token signed with + // the first tenant's service session key. + otherAccount := &types.Account{ + Id: "otherAccountId", + Domain: "other.com", + DomainCategory: "private", + IsDomainPrimaryAccount: true, + CreatedAt: time.Now(), + } + require.NoError(t, testStore.SaveAccount(ctx, otherAccount)) + + otherAccountUser := &types.User{ + Id: "otherAccountUserId", + AccountID: "otherAccountId", + Role: types.UserRoleUser, + CreatedAt: time.Now(), + Issued: "api", + } + require.NoError(t, testStore.SaveUser(ctx, otherAccountUser)) + + // A user awaiting approval is stored as blocked and pending approval, and + // carries the same group membership as the approved one. + pendingUser := &types.User{ + Id: "pendingUserId", + AccountID: "testAccountId", + Role: types.UserRoleUser, + AutoGroups: []string{"allowedGroupId"}, + Blocked: true, + PendingApproval: true, + CreatedAt: time.Now(), + Issued: "api", + } + require.NoError(t, testStore.SaveUser(ctx, pendingUser)) + + blockedUser := &types.User{ + Id: "blockedUserId", + AccountID: "testAccountId", + Role: types.UserRoleUser, + AutoGroups: []string{"allowedGroupId"}, + Blocked: true, + CreatedAt: time.Now(), + Issued: "api", + } + require.NoError(t, testStore.SaveUser(ctx, blockedUser)) } // testServiceManager is a minimal implementation for testing. @@ -490,6 +535,64 @@ func TestAuthCallback_UserAllowedToLogin(t *testing.T) { require.Empty(t, parsedLocation.Query().Get("error"), "Should not have error parameter") } +// TestAuthCallback_UserDeniedByAccountStatus asserts that a user whose account +// is pending approval or blocked never receives a session token from the OIDC +// callback, and that the redirect carries a description the proxy can render. +func TestAuthCallback_UserDeniedByAccountStatus(t *testing.T) { + tests := []struct { + name string + subject string + expectErrorDesc string + }{ + { + name: "pending approval", + subject: "pendingUserId", + expectErrorDesc: "Your account is pending approval by an administrator", + }, + { + name: "blocked", + subject: "blockedUserId", + expectErrorDesc: "Your account is blocked", + }, + { + name: "unknown to management", + subject: "userMissingFromStoreId", + expectErrorDesc: "Service configuration error", + }, + { + // The account topology stays out of the browser-visible message. + name: "belongs to another account", + subject: "otherAccountUserId", + expectErrorDesc: "Service configuration error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setup := setupAuthCallbackTest(t) + defer setup.cleanup() + + setup.oidcServer.tokenSubject = tt.subject + + state := createTestState(t, setup.proxyService, "https://test-proxy.example.com/dashboard") + + req := httptest.NewRequest(http.MethodGet, "/reverse-proxy/callback?code=test-auth-code&state="+url.QueryEscape(state), nil) + rec := httptest.NewRecorder() + + setup.router.ServeHTTP(rec, req) + + require.Equal(t, http.StatusFound, rec.Code) + + parsedLocation, err := url.Parse(rec.Header().Get("Location")) + require.NoError(t, err) + + require.Empty(t, parsedLocation.Query().Get("session_token"), "Denied user must not receive a session token") + require.Equal(t, "access_denied", parsedLocation.Query().Get("error")) + require.Equal(t, tt.expectErrorDesc, parsedLocation.Query().Get("error_description")) + }) + } +} + func TestAuthCallback_ProxyNotFound(t *testing.T) { setup := setupAuthCallbackTest(t) defer setup.cleanup()