[v17] Fix missing roles in access lists causing users to be locked out of their account

api/proto/teleport/legacy/types/events/events.proto

@@ -4694,6 +4694,7 @@ message OneOf {
     events.WorkloadIdentityCreate WorkloadIdentityCreate = 194;
     events.WorkloadIdentityUpdate WorkloadIdentityUpdate = 195;
     events.WorkloadIdentityDelete WorkloadIdentityDelete = 196;
+    events.UserLoginAccessListInvalid UserLoginAccessListInvalid = 198;
   }
 }
 
@@ -7689,3 +7690,39 @@ message WorkloadIdentityDelete {
     (gogoproto.jsontag) = ""
   ];
 }
+
+// AccessListInvalidMetadata contains metadata about invalid access lists.
+message AccessListInvalidMetadata {
+  // AccessListName is the name of the invalid access list.
+  string AccessListName = 1 [(gogoproto.jsontag) = "access_list_name, omitempty"];
+  // User is the username of the access list member who attempted to log in.
+  string User = 2 [(gogoproto.jsontag) = "user,omitempty"];
+  // MissingRoles are the names of the non-existent roles being referenced by the access list, causing it to be invalid.
+  repeated string MissingRoles = 3 [(gogoproto.jsontag) = "missing_roles,omitempty"];
+}
+
+// UserLoginAccessListInvalid is emitted when a user who is a member of an invalid
+// access list logs in. It is used to indicate that the access list could not be
+// applied to the user's session.
+message UserLoginAccessListInvalid {
+  // Metadata is common event metadata
+  Metadata Metadata = 1 [
+    (gogoproto.nullable) = false,
+    (gogoproto.embed) = true,
+    (gogoproto.jsontag) = ""
+  ];
+
+  // AccessListInvalidMetadata is the metadata for this access list invalid event.
+  AccessListInvalidMetadata AccessListInvalidMetadata = 2 [
+    (gogoproto.nullable) = false,
+    (gogoproto.embed) = true,
+    (gogoproto.jsontag) = ""
+  ];
+
+  // Status contains fields to indicate whether attempt was successful or not.
+  Status Status = 3 [
+    (gogoproto.nullable) = false,
+    (gogoproto.embed) = true,
+    (gogoproto.jsontag) = ""
+  ];
+}

api/types/events/events.go

@@ -1962,6 +1962,25 @@ func (m *AccessListMemberDeleteAllForAccessList) TrimToMaxSize(maxSize int) Audi
 	return out
 }
 
+func (m *UserLoginAccessListInvalid) TrimToMaxSize(maxSize int) AuditEvent {
+	size := m.Size()
+	if size <= maxSize {
+		return m
+	}
+
+	out := utils.CloneProtoMsg(m)
+	out.Status = Status{}
+
+	maxSize = adjustedMaxSize(out, maxSize)
+
+	customFieldsCount := m.Status.nonEmptyStrs()
+	maxFieldsSize := maxSizePerField(maxSize, customFieldsCount)
+
+	out.Status = m.Status.trimToMaxSize(maxFieldsSize)
+
+	return out
+}
+
 func (m *AuditQueryRun) TrimToMaxSize(maxSize int) AuditEvent {
 	size := m.Size()
 	if size <= maxSize {

api/types/events/oneof.go

@@ -604,6 +604,10 @@ func ToOneOf(in AuditEvent) (*OneOf, error) {
 		out.Event = &OneOf_AccessListMemberDeleteAllForAccessList{
 			AccessListMemberDeleteAllForAccessList: e,
 		}
+	case *UserLoginAccessListInvalid:
+		out.Event = &OneOf_UserLoginAccessListInvalid{
+			UserLoginAccessListInvalid: e,
+		}
 	case *AuditQueryRun:
 		out.Event = &OneOf_AuditQueryRun{
 			AuditQueryRun: e,

lib/auth/auth.go

@@ -644,6 +644,7 @@ func NewServer(cfg *InitConfig, opts ...ServerOption) (*Server, error) {
 		Access:      &as,
 		UsageEvents: &as,
 		Clock:       cfg.Clock,
+		Emitter:     as.emitter,
 	})
 	if err != nil {
 		return nil, trace.Wrap(err)

lib/auth/userloginstate/generator.go

@@ -20,6 +20,7 @@ package userloginstate
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/gravitational/trace"
 	"github.com/jonboulle/clockwork"
@@ -29,10 +30,12 @@ import (
 	usageeventsv1 "github.com/gravitational/teleport/api/gen/proto/go/usageevents/v1"
 	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/api/types/accesslist"
+	apievents "github.com/gravitational/teleport/api/types/events"
 	"github.com/gravitational/teleport/api/types/header"
 	"github.com/gravitational/teleport/api/types/userloginstate"
 	"github.com/gravitational/teleport/api/utils"
 	"github.com/gravitational/teleport/lib/accesslists"
+	"github.com/gravitational/teleport/lib/events"
 	"github.com/gravitational/teleport/lib/modules"
 	"github.com/gravitational/teleport/lib/services"
 )
@@ -59,6 +62,9 @@ type GeneratorConfig struct {
 
 	// Clock is the clock to use for the generator.
 	Clock clockwork.Clock
+
+	// Emitter is the emitter for audit events.
+	Emitter apievents.Emitter
 }
 
 // UsageEventsClient is an interface that allows for submitting usage events to Posthog.
@@ -80,6 +86,10 @@ func (g *GeneratorConfig) CheckAndSetDefaults() error {
 		return trace.BadParameter("missing access")
 	}
 
+	if g.Emitter == nil {
+		return trace.BadParameter("missing audit event emitter")
+	}
+
 	if modules.GetModules().Features().Cloud {
 		if g.UsageEvents == nil {
 			return trace.BadParameter("missing usage events")
@@ -102,6 +112,7 @@ type Generator struct {
 	access      services.Access
 	usageEvents UsageEventsClient
 	clock       clockwork.Clock
+	emitter     apievents.Emitter
 }
 
 // NewGenerator creates a new user login state generator.
@@ -116,6 +127,7 @@ func NewGenerator(config GeneratorConfig) (*Generator, error) {
 		access:      config.Access,
 		usageEvents: config.UsageEvents,
 		clock:       config.Clock,
+		emitter:     config.Emitter,
 	}, nil
 }
 
@@ -169,9 +181,8 @@ func (g *Generator) Generate(ctx context.Context, user types.User) (*userloginst
 	return uls, nil
 }
 
-// addAccessListsToState will add the user's applicable access lists to the user login state,
-// returning any inherited roles and traits.
-func (g *Generator) addAccessListsToState(ctx context.Context, user types.User, state *userloginstate.UserLoginState) ([]string, map[string][]string, error) {
+// addAccessListsToState will add the user's applicable access lists to the user login state after validating them, returning any inherited roles and traits.
+func (g *Generator) addAccessListsToState(ctx context.Context, user types.User, state *userloginstate.UserLoginState) (inheritedRoles []string, inheritedTraits map[string][]string, err error) {
 	accessLists, err := g.accessLists.GetAccessLists(ctx)
 	if err != nil {
 		return nil, nil, trace.Wrap(err)
@@ -182,38 +193,119 @@ func (g *Generator) addAccessListsToState(ctx context.Context, user types.User,
 
 	for _, accessList := range accessLists {
 		// Grants are inherited if the user is a member of the access list, explicitly or via inheritance.
-		membershipKind, err := accesslists.IsAccessListMember(ctx, user, accessList, g.accessLists, g.accessLists, g.clock)
+		inheritedRoles, inheritedTraits, err := g.handleAccessListMembership(ctx, user, accessList, state)
 		if err != nil {
-			g.log.WithError(err).Warn("checking access list membership")
+			return nil, nil, trace.Wrap(err)
 		}
-		if err == nil && membershipKind != accesslists.MembershipOrOwnershipTypeNone {
-			g.grantRolesAndTraits(accessList.Spec.Grants, state)
-			if membershipKind == accesslists.MembershipOrOwnershipTypeInherited {
-				allInheritedRoles = append(allInheritedRoles, accessList.Spec.Grants.Roles...)
-				for k, values := range accessList.Spec.Grants.Traits {
-					allInheritedTraits[k] = append(allInheritedTraits[k], values...)
-				}
-			}
+		allInheritedRoles = append(allInheritedRoles, inheritedRoles...)
+		for k, values := range inheritedTraits {
+			allInheritedTraits[k] = append(allInheritedTraits[k], values...)
 		}
+
 		// OwnerGrants are inherited if the user is an owner of the access list, explicitly or via inheritance.
-		ownershipType, err := accesslists.IsAccessListOwner(ctx, user, accessList, g.accessLists, g.accessLists, g.clock)
+		inheritedRoles, inheritedTraits, err = g.handleAccessListOwnership(ctx, user, accessList, state)
 		if err != nil {
-			g.log.WithError(err).Warn("checking access list ownership")
+			return nil, nil, trace.Wrap(err)
 		}
-		if err == nil && ownershipType != accesslists.MembershipOrOwnershipTypeNone {
-			g.grantRolesAndTraits(accessList.Spec.OwnerGrants, state)
-			if ownershipType == accesslists.MembershipOrOwnershipTypeInherited {
-				allInheritedRoles = append(allInheritedRoles, accessList.Spec.OwnerGrants.Roles...)
-				for k, values := range accessList.Spec.OwnerGrants.Traits {
-					allInheritedTraits[k] = append(allInheritedTraits[k], values...)
-				}
-			}
+		allInheritedRoles = append(allInheritedRoles, inheritedRoles...)
+		for k, values := range inheritedTraits {
+			allInheritedTraits[k] = append(allInheritedTraits[k], values...)
 		}
 	}
 
 	return allInheritedRoles, allInheritedTraits, nil
 }
 
+// handleAccessListMembership validates the access list and applies the grants and traits from the access list to the user if they are a member of the access list.
+// If the access list is invalid (because it references a non-existent role, for example,
+// then it will not be applied.
+func (g *Generator) handleAccessListMembership(ctx context.Context, user types.User, accessList *accesslist.AccessList, state *userloginstate.UserLoginState) ([]string, map[string][]string, error) {
+	var inheritedRoles []string
+	inheritedTraits := make(map[string][]string)
+
+	membershipKind, err := accesslists.IsAccessListMember(ctx, user, accessList, g.accessLists, g.accessLists, g.clock)
+	// Return early if there was an error or the user isn't a member of the access list.
+	if err != nil || membershipKind == accesslists.MembershipOrOwnershipTypeNone {
+		// Log any error.
+		if err != nil {
+			g.log.WithError(err).Warn("checking access list membership")
+		}
+		return inheritedRoles, inheritedTraits, nil
+	}
+
+	// Validate that all the roles in the access list exist.
+	missingRoles, err := g.identifyMissingRoles(ctx, accessList.Spec.Grants.Roles)
+	if err != nil {
+		return nil, nil, trace.Wrap(err)
+	}
+
+	// If there are any missing roles, then we cannot apply the access list.
+	// Emit an audit event and return early.
+	// This flow is designed to skip the entire access list rather than processing individual roles within it.
+	// This approach ensures that access lists are treated as cohesive units of access control. Partial
+	// application of an access list could result in unintended permission configurations, potentially leading
+	// to security vulnerabilities or unpredictable behavior.
+	if missingRoles != nil {
+		g.emitSkippedAccessListEvent(ctx, accessList.Spec.Title, missingRoles, user.GetName())
+		return nil, nil, nil
+	}
+
+	g.grantRolesAndTraits(accessList.Spec.Grants, state)
+	if membershipKind == accesslists.MembershipOrOwnershipTypeInherited {
+		inheritedRoles = append(inheritedRoles, accessList.Spec.Grants.Roles...)
+		for k, values := range accessList.Spec.Grants.Traits {
+			inheritedTraits[k] = append(inheritedTraits[k], values...)
+		}
+	}
+
+	return inheritedRoles, inheritedTraits, nil
+}
+
+// handleAccessListOwnership validates the access list and applies the grants and traits from the access list to the user if they are an owner of the access list.
+// If the access list is invalid (because it references a non-existent role, for example,
+// then it will not be applied.
+func (g *Generator) handleAccessListOwnership(ctx context.Context, user types.User, accessList *accesslist.AccessList, state *userloginstate.UserLoginState) ([]string, map[string][]string, error) {
+	var inheritedRoles []string
+	inheritedTraits := make(map[string][]string)
+
+	ownershipType, err := accesslists.IsAccessListOwner(ctx, user, accessList, g.accessLists, g.accessLists, g.clock)
+	// Return early if there was an error or the user isn't an owner of the access list.
+	if err != nil || ownershipType == accesslists.MembershipOrOwnershipTypeNone {
+		// Log any error.
+		if err != nil {
+			g.log.WithError(err).Warn("checking access list ownership")
+		}
+		return inheritedRoles, inheritedTraits, nil
+	}
+
+	// Validate that all the roles in the access list exist.
+	missingRoles, err := g.identifyMissingRoles(ctx, accessList.Spec.OwnerGrants.Roles)
+	if err != nil {
+		return nil, nil, trace.Wrap(err)
+	}
+
+	// If there are any missing roles, then we cannot apply the access list.
+	// Emit an audit event and return early.
+	// This flow is designed to skip the entire access list rather than processing individual roles within it.
+	// This approach ensures that access lists are treated as cohesive units of access control. Partial
+	// application of an access list could result in unintended permission configurations, potentially leading
+	// to security vulnerabilities or unpredictable behavior.
+	if missingRoles != nil {
+		g.emitSkippedAccessListEvent(ctx, accessList.Spec.Title, missingRoles, user.GetName())
+		return nil, nil, nil
+	}
+
+	g.grantRolesAndTraits(accessList.Spec.OwnerGrants, state)
+	if ownershipType == accesslists.MembershipOrOwnershipTypeInherited {
+		inheritedRoles = append(inheritedRoles, accessList.Spec.OwnerGrants.Roles...)
+		for k, values := range accessList.Spec.OwnerGrants.Traits {
+			inheritedTraits[k] = append(inheritedTraits[k], values...)
+		}
+	}
+
+	return inheritedRoles, inheritedTraits, nil
+}
+
 // grantRolesAndTraits will append the roles and traits from the provided Grants to the UserLoginState,
 // returning inherited roles and traits if membershipOrOwnershipType is inherited.
 func (g *Generator) grantRolesAndTraits(grants accesslist.Grants, state *userloginstate.UserLoginState) {
@@ -242,7 +334,6 @@ func (g *Generator) postProcess(ctx context.Context, state *userloginstate.UserL
 	}
 
 	// Make sure all the roles exist. If they don't, error out.
-	// Since InheritedRoles are always a subset of Roles, we don't need to check them.
 	var existingRoles []string
 	for _, role := range state.Spec.Roles {
 		_, err := g.access.GetRole(ctx, role)
@@ -327,3 +418,48 @@ func (g *Generator) LoginHook(ulsService services.UserLoginStates) func(context.
 		return trace.Wrap(err)
 	}
 }
+
+// identifyMissingRoles is a helper function which identifies any roles from the provided list that don't exist, and returns nil if they all exist.
+func (g *Generator) identifyMissingRoles(ctx context.Context, roles []string) ([]string, error) {
+	var missingRoles []string
+
+	for _, role := range roles {
+		_, err := g.access.GetRole(ctx, role)
+		if err != nil {
+			if trace.IsNotFound(err) {
+				missingRoles = append(missingRoles, role)
+				continue
+			}
+			return nil, trace.Wrap(err)
+		}
+	}
+
+	if len(missingRoles) > 0 {
+		return missingRoles, nil
+	}
+
+	return nil, nil
+}
+
+// emitSkippedAccessListEvent emits an audit log event to indicate that an invalid
+// access list could not be applied during user login.
+func (g *Generator) emitSkippedAccessListEvent(ctx context.Context, accessListName string, missingRoles []string, username string) {
+	if err := g.emitter.EmitAuditEvent(ctx, &apievents.UserLoginAccessListInvalid{
+		Metadata: apievents.Metadata{
+			Type: events.UserLoginAccessListInvalidEvent,
+			Code: events.UserLoginAccessListInvalidCode,
+		},
+		AccessListInvalidMetadata: apievents.AccessListInvalidMetadata{
+			AccessListName: accessListName,
+			User:           username,
+			MissingRoles:   missingRoles,
+		},
+		Status: apievents.Status{
+			Success:     false,
+			Error:       fmt.Sprintf("roles %v were not found", missingRoles),
+			UserMessage: "access list skipped because it references non-existent role(s)",
+		},
+	}); err != nil {
+		g.log.WithError(err).Warn("Failed to emit access list skipped warning audit event.")
+	}
+}