diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 1f26b77d3..f3eb13c35 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -1732,10 +1732,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) @@ -1745,40 +1742,16 @@ 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) @@ -1789,12 +1762,11 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val "error": err, }).Debug("ValidateSession: user not found") //nolint:nilerr - return &proto.ValidateSessionResponse{ - Valid: false, - DeniedReason: deniedReasonUserNotFound, - }, nil + return deniedSessionResponse(deniedReasonUserNotFound), nil } + // 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 user.AccountID != service.AccountID { log.WithFields(log.Fields{ "domain": domain, @@ -1802,20 +1774,12 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val "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 reason := userStatusDeniedReason(user); reason != "" { - log.WithFields(log.Fields{ - "domain": domain, - "user_id": userID, - "reason": reason, - }).Debug("ValidateSession: user status denies access") - groupIDs, groupNames := pairGroupIDsAndNames(userGroups) + groupIDs, groupNames := pairGroupIDsAndNames(userGroups) + + if reason := s.accountUserDeniedReason(domain, service, user); reason != "" { return &proto.ValidateSessionResponse{ Valid: false, UserId: user.Id, @@ -1826,31 +1790,12 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val }, 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 - return &proto.ValidateSessionResponse{ - Valid: false, - UserId: user.Id, - UserEmail: user.Email, - DeniedReason: "not_in_group", - PeerGroupIds: groupIDs, - PeerGroupNames: groupNames, - }, nil - } - log.WithFields(log.Fields{ "domain": domain, "user_id": userID, "email": user.Email, }).Debug("ValidateSession: access granted") - groupIDs, groupNames := pairGroupIDsAndNames(userGroups) return &proto.ValidateSessionResponse{ Valid: true, UserId: user.Id, @@ -1860,6 +1805,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 { diff --git a/management/internals/shared/grpc/proxy_group_access_test.go b/management/internals/shared/grpc/proxy_group_access_test.go index 4adbc1589..7d2bcb13b 100644 --- a/management/internals/shared/grpc/proxy_group_access_test.go +++ b/management/internals/shared/grpc/proxy_group_access_test.go @@ -559,7 +559,7 @@ func TestValidateTunnelPeerUserEmailEnrichment(t *testing.T) { expectEmail: "idp@example.com", expectUserID: userID, expectIdPHit: true, - expectDeniedReason: "user_not_found", + expectDeniedReason: deniedReasonUserNotFound, }, { name: "unlinked peer uses peer name and never consults idp", @@ -625,6 +625,15 @@ 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. +func TestDeniedReasonValues(t *testing.T) { + assert.Equal(t, "pending_approval", deniedReasonPendingApproval) + assert.Equal(t, "user_blocked", deniedReasonUserBlocked) + assert.Equal(t, "user_not_found", deniedReasonUserNotFound) +} + // 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 @@ -653,13 +662,13 @@ func TestValidateTunnelPeerOwnerStatus(t *testing.T) { name: "owner pending approval denied", peerUserID: userID, owner: &types.User{Id: userID, AccountID: accountID, Email: "user@example.com", Blocked: true, PendingApproval: true}, - expectDeniedReason: "pending_approval", + 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: "user_blocked", + expectDeniedReason: deniedReasonUserBlocked, }, { name: "unlinked machine peer stays allowed", diff --git a/management/internals/shared/grpc/validate_session_test.go b/management/internals/shared/grpc/validate_session_test.go index c7065f99f..f35cde3af 100644 --- a/management/internals/shared/grpc/validate_session_test.go +++ b/management/internals/shared/grpc/validate_session_test.go @@ -245,7 +245,7 @@ func TestValidateSession_PendingApprovalUserDenied(t *testing.T) { require.NoError(t, err) assert.False(t, resp.Valid, "User pending approval should be denied") - assert.Equal(t, "pending_approval", resp.DeniedReason) + assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason) assert.Equal(t, pendingUserID, resp.UserId) 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") @@ -270,7 +270,7 @@ func TestValidateSession_PendingApprovalUserInAllUsersGroupDenied(t *testing.T) require.NoError(t, err) assert.False(t, resp.Valid, "User pending approval should be denied even in the All Users group") - assert.Equal(t, "pending_approval", resp.DeniedReason) + assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason) assert.Equal(t, pendingAllUsersID, resp.UserId) assert.Equal(t, []string{allUsersGroupID}, resp.GetPeerGroupIds(), "PeerGroupIds must mirror the resolved user's group memberships on denial") } @@ -293,7 +293,7 @@ func TestValidateSession_BlockedUserDenied(t *testing.T) { require.NoError(t, err) assert.False(t, resp.Valid, "Blocked user should be denied") - assert.Equal(t, "user_blocked", resp.DeniedReason) + assert.Equal(t, deniedReasonUserBlocked, resp.DeniedReason) assert.Equal(t, blockedUserID, resp.UserId) } @@ -318,7 +318,7 @@ func TestValidateSession_UserAllowedAfterApproval(t *testing.T) { 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, "pending_approval", resp.DeniedReason) + assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason) user, err := setup.store.GetUserByUserID(ctx, store.LockingStrengthNone, pendingUserID) require.NoError(t, err) diff --git a/management/server/http/handlers/proxy/auth.go b/management/server/http/handlers/proxy/auth.go index 9fbcb2fa0..62725e8d4 100644 --- a/management/server/http/handlers/proxy/auth.go +++ b/management/server/http/handlers/proxy/auth.go @@ -130,14 +130,13 @@ func (h *AuthCallbackHandler) handleCallback(w http.ResponseWriter, r *http.Requ // 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 { - switch { - case errors.Is(err, nbgrpc.ErrUserPendingApproval): + if errors.Is(err, nbgrpc.ErrUserPendingApproval) { return "Your account is pending approval by an administrator" - case errors.Is(err, nbgrpc.ErrUserBlocked): - return "Your account is blocked" - default: - return "Service configuration error" } + 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 {