CR

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

@@ -7869,17 +7869,19 @@ message GitCommandAction {
   string New = 4 [(gogoproto.jsontag) = "new,omitempty"];
 }
 
-// AccessListInvalidMetadata contains metadata for access list skipped events.
+// 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 user who attempted to consume this access list.
+  // 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 logs in with an invalid access list.
+// 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 [

lib/auth/userloginstate/generator.go

@@ -20,6 +20,7 @@ package userloginstate
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/gravitational/trace"
 	"github.com/jonboulle/clockwork"
@@ -197,7 +198,8 @@ func (g *Generator) Generate(ctx context.Context, user types.User, ulsService se
 	return uls, nil
 }
 
-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)
@@ -232,67 +234,81 @@ func (g *Generator) addAccessListsToState(ctx context.Context, user types.User,
 }
 
 // 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)
-	if err != nil {
-		g.log.WithError(err).Warn("checking access list membership")
-	}
-	if err == nil && membershipKind != accesslists.MembershipOrOwnershipTypeNone {
-		// Validate that all the roles in the access list exist.
-		missingRoles, notFoundErrs, err := g.validateRoles(ctx, accessList.Spec.Grants.Roles)
+	// 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 {
-			return nil, nil, trace.Wrap(err)
+			g.log.WithError(err).Warn("checking access list membership")
 		}
+		return inheritedRoles, inheritedTraits, nil
+	}
 
-		// If there are any missing roles, emit an audit event and return early.
-		if missingRoles != nil {
-			g.emitSkippedAccessListEvent(ctx, accessList.Spec.Title, missingRoles, user.GetName(), trace.NewAggregate(notFoundErrs...))
-			return nil, nil, 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)
+	}
 
-		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...)
-			}
+	// If there are any missing roles, then we cannot apply the access list.
+	// Emit an audit event and return early.
+	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 owner grants and traits from the access list to the user if they are an owner of the access list.
+// 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)
-	if err != nil {
-		g.log.WithError(err).Warn("checking access list ownership")
-	}
-	if err == nil && ownershipType != accesslists.MembershipOrOwnershipTypeNone {
-		// Validate that all the roles in the access list exist.
-		missingRoles, notFoundErrs, err := g.validateRoles(ctx, accessList.Spec.OwnerGrants.Roles)
+	// 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 {
-			return nil, nil, trace.Wrap(err)
+			g.log.WithError(err).Warn("checking access list ownership")
 		}
+		return inheritedRoles, inheritedTraits, nil
+	}
 
-		// If there are any missing roles, emit an audit event and return early.
-		if missingRoles != nil {
-			g.emitSkippedAccessListEvent(ctx, accessList.Spec.Title, missingRoles, user.GetName(), trace.NewAggregate(notFoundErrs...))
-			return nil, nil, 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)
+	}
 
-		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...)
-			}
+	// If there are any missing roles, then we cannot apply the access list.
+	// Emit an audit event and return early.
+	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...)
 		}
 	}
 
@@ -432,32 +448,31 @@ func (g *Generator) LoginHook(ulsService services.UserLoginStates) func(context.
 	}
 }
 
-// validateRoles is a helper function which validates that a given list of roles exist.
-func (g *Generator) validateRoles(ctx context.Context, roles []string) ([]string, []error, error) {
+// 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
-	var notFoundErrs []error
 
 	for _, role := range roles {
 		_, err := g.access.GetRole(ctx, role)
 		if err != nil {
 			if trace.IsNotFound(err) {
 				missingRoles = append(missingRoles, role)
-				notFoundErrs = append(notFoundErrs, err)
 				continue
 			}
-			return nil, nil, trace.Wrap(err)
+			return nil, trace.Wrap(err)
 		}
 	}
 
 	if len(missingRoles) > 0 {
-		return missingRoles, notFoundErrs, nil
+		return missingRoles, nil
 	}
 
-	return nil, nil, nil
+	return nil, nil
 }
 
-// emitSkippedAccessListEvent emits an audit log event to warn that an access list was skipped due to it referencing a non-existent role.
-func (g *Generator) emitSkippedAccessListEvent(ctx context.Context, accessListName string, missingRoles []string, username string, returnedErr error) {
+// 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,
@@ -470,8 +485,8 @@ func (g *Generator) emitSkippedAccessListEvent(ctx context.Context, accessListNa
 		},
 		Status: apievents.Status{
 			Success:     false,
-			Error:       trace.Unwrap(returnedErr).Error(),
-			UserMessage: "access list skipped because it references non-existent role",
+			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.")