Make LockTarget comparable (#68874)

* Benchmark authz.Context.LockTargets

* Make LockTarget comparable

* Add a test against the addition of non-simple fields to LockTarget

* Clarify comment about gogoproto options

* Fix TestSSHAccessLockTargets tests

* Clarify the comparable assertion

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

@@ -6674,11 +6674,24 @@ message LockSpecV2 {
   string CreatedBy = 5 [(gogoproto.jsontag) = "created_by,omitempty"];
 }
 
-// LockTarget lists the attributes of interactions to be disabled.
+// LockTarget lists the attributes of interactions to be disabled. This type is
+// performance sensitive (as lock targets are checked during authz for just
+// about every interaction with the cluster) and it's used as a Go map key and
+// compared with Go equality, so care must be taken when adding fields to the
+// message. There is an assertion for the resulting Go message type to be
+// comparable, but some comparable types (like any pointer, or time.Time) don't
+// compare _correctly_ without additional steps. It's best to limit this message
+// to numbers and strings, if new additions are necessary.
 message LockTarget {
   option (gogoproto.goproto_stringer) = false;
   option (gogoproto.stringer) = false;
 
+  // these options remove extraneous fields from the resulting Go struct, making
+  // it possible to compare by equality
+  option (gogoproto.goproto_unrecognized) = false;
+  option (gogoproto.goproto_sizecache) = false;
+  option (gogoproto.goproto_unkeyed) = false;
+
   // User specifies the name of a Teleport user.
   string User = 1 [(gogoproto.jsontag) = "user,omitempty"];
 

api/types/lock.go

@@ -204,7 +204,7 @@ func (c *LockV2) CheckAndSetDefaults() error {
 		return trace.Wrap(err)
 	}
 
-	if c.Spec.Target.IsEmpty() {
+	if c.Spec.Target == (LockTarget{}) {
 		return trace.BadParameter("at least one target field must be set")
 	}
 	return nil
@@ -263,24 +263,14 @@ func (t *LockTarget) FromMap(m map[string]string) error {
 	return trace.Wrap(utils.ObjectToStruct(m, t))
 }
 
-// IsEmpty returns true if none of the target's fields is set.
-func (t LockTarget) IsEmpty() bool {
-	return t.User == "" &&
-		t.Role == "" &&
-		t.Login == "" &&
-		t.MFADevice == "" &&
-		t.WindowsDesktop == "" &&
-		t.LinuxDesktop == "" &&
-		t.AccessRequest == "" &&
-		t.Device == "" &&
-		t.ServerID == "" &&
-		t.BotInstanceID == "" &&
-		t.JoinToken == ""
+func _() {
+	type assertComparable[T comparable] struct{}
+	var _ assertComparable[LockTarget]
 }
 
 // Match returns true if the lock's target is matched by this target.
 func (t LockTarget) Match(lock Lock) bool {
-	if t.IsEmpty() {
+	if t == (LockTarget{}) {
 		return false
 	}
 	lockTarget := lock.Target()
@@ -302,17 +292,20 @@ func (t LockTarget) String() string {
 	return strings.TrimSpace(proto.CompactTextString(&t))
 }
 
+// TODO(espadolini): delete in v20
+
 // Equals returns true when the two lock targets are equal.
+// Deprecated: compare the two LockTargets directly instead
+//
+//go:fix inline
 func (t LockTarget) Equals(t2 LockTarget) bool {
-	return t.User == t2.User &&
-		t.Role == t2.Role &&
-		t.Login == t2.Login &&
-		t.MFADevice == t2.MFADevice &&
-		t.WindowsDesktop == t2.WindowsDesktop &&
-		t.LinuxDesktop == t2.LinuxDesktop &&
-		t.AccessRequest == t2.AccessRequest &&
-		t.Device == t2.Device &&
-		t.ServerID == t2.ServerID &&
-		t.BotInstanceID == t2.BotInstanceID &&
-		t.JoinToken == t2.JoinToken
+	return t == t2
+}
+
+// IsEmpty returns true if none of the target's fields is set.
+// Deprecated: compare the LockTarget against LockTarget{} instead.
+//
+//go:fix inline
+func (t LockTarget) IsEmpty() bool {
+	return t == LockTarget{}
 }

api/types/lock_test.go

@@ -18,7 +18,6 @@ package types
 
 import (
 	"reflect"
-	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -74,57 +73,27 @@ func TestLockTargetMatch(t *testing.T) {
 	require.False(t, emptyTarget.Match(lock))
 }
 
-// TestLockTargetIsEmpty checks that the implementation of [LockTarget.IsEmpty]
-// is correct by filling one field at a time and expecting IsEmpty to return
-// false. Only the public fields that don't start with `XXX_` are checked (as
-// those are gogoproto-internal fields).
-func TestLockTargetIsEmpty(t *testing.T) {
-	require.True(t, (LockTarget{}).IsEmpty())
-
-	for i, field := range reflect.VisibleFields(reflect.TypeOf(LockTarget{})) {
-		if strings.HasPrefix(field.Name, "XXX_") {
-			continue
-		}
-
-		var lt LockTarget
-		// if we add non-string fields to LockTarget we need a type switch here
-		reflect.ValueOf(&lt).Elem().Field(i).SetString("nonempty")
-		require.False(t, lt.IsEmpty(), "field name: %v", field.Name)
+func TestLockTargetIsSimple(t *testing.T) {
+	ty := reflect.TypeFor[LockTarget]()
+	for f := range ty.Fields() {
+		// A struct embedded by value that is also "simple" in this sense would
+		// work too, so if we need something like that we can extend this check.
+		// Arrays (of likewise "simple" types) would also work but outside of
+		// nasty gogoproto shenanigans (which we should not make use of) it's
+		// not possible to have an array in a protobuf message struct, so it's a
+		// moot point. Pointers are a no-no, since they are compared by address
+		// rather than by checking the value that they point to.
+		require.Containsf(t, []reflect.Kind{
+			reflect.Bool,
+			reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
+			reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64,
+			reflect.Uintptr,
+			reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128,
+			reflect.String,
+		}, f.Type.Kind(), "field %+q (%#d) is not of a scalar kind (%s)", f.Name, f.Index[0], f.Type.Kind())
+		require.NotEqualf(t, "_", f.Name, "field %+q (#%d) is padding", f.Name, f.Index[0])
+		require.Truef(t, f.IsExported(), "field %+q (#%d) is unexported", f.Name, f.Index[0])
+		// embedding scalar newtypes is weird but technically possible
+		require.Falsef(t, f.Anonymous, "field %+q (#%d) is embedded", f.Name, f.Index[0])
 	}
 }
-
-// TestLockTargetEquals checks that the implementation of [LockTarget.Equals]
-// is correct by filling one field at a time in for two LockTargets and expecting
-// Equals to return the appropriate value. Only the public fields that don't start with
-// `XXX_` are checked (as those are gogoproto-internal fields).
-func TestLockTargetEquals(t *testing.T) {
-	t.Run("equal", func(t *testing.T) {
-		require.True(t, (LockTarget{}).Equals(LockTarget{}), "empty targets equal")
-
-		for i, field := range reflect.VisibleFields(reflect.TypeOf(LockTarget{})) {
-			if strings.HasPrefix(field.Name, "XXX_") {
-				continue
-			}
-
-			var a, b LockTarget
-			// if we add non-string fields to LockTarget we need a type switch here
-			reflect.ValueOf(&a).Elem().Field(i).SetString("nonempty")
-			reflect.ValueOf(&b).Elem().Field(i).SetString("nonempty")
-			require.True(t, a.Equals(b), "field name: %v", field.Name)
-		}
-	})
-
-	t.Run("not equal", func(t *testing.T) {
-		for i, field := range reflect.VisibleFields(reflect.TypeOf(LockTarget{})) {
-			if strings.HasPrefix(field.Name, "XXX_") {
-				continue
-			}
-
-			var a, b LockTarget
-			// if we add non-string fields to LockTarget we need a type switch here
-			reflect.ValueOf(&a).Elem().Field(i).SetString("nonempty")
-			reflect.ValueOf(&b).Elem().Field(i).SetString("other")
-			require.False(t, a.Equals(b), "field name: %v", field.Name)
-		}
-	})
-}

lib/auth/auth.go

@@ -4258,16 +4258,12 @@ func (a *Server) verifyLocksForUserCerts(req verifyLocksForUserCertsReq) error {
 	}
 
 	if unscoped := req.checkerContext.CertParams().UnscopedCertParams(); unscoped != nil {
-		lockTargets = append(lockTargets,
-			services.RolesToLockTargets(unscoped.RoleNames())...,
-		)
+		lockTargets = slices.AppendSeq(lockTargets, services.RolesToLockTargets(slices.Values(unscoped.RoleNames())))
 	}
 
 	// TODO(fspmarshall/scopes): implement scoped role locking.
 
-	lockTargets = append(lockTargets,
-		services.AccessRequestsToLockTargets(req.activeAccessRequests)...,
-	)
+	lockTargets = slices.AppendSeq(lockTargets, services.AccessRequestsToLockTargets(slices.Values(req.activeAccessRequests)))
 	if req.botInstanceID != "" {
 		lockTargets = append(lockTargets, types.LockTarget{BotInstanceID: req.botInstanceID})
 	}

lib/auth/auth_test.go

@@ -31,6 +31,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	goslices "slices"
 	"sort"
 	"strings"
 	"sync"
@@ -3218,15 +3219,13 @@ func TestGenerateUserCertWithLocks(t *testing.T) {
 	_, _, err = p.a.GenerateUserTestCertsWithContext(ctx, certReq)
 	require.NoError(t, err)
 
-	testTargets := append(
-		[]types.LockTarget{
-			{User: user.GetName()},
-			{MFADevice: mfaID},
-			{AccessRequest: requestID},
-			{Device: deviceID},
-		},
-		services.RolesToLockTargets(user.GetRoles())...,
-	)
+	testTargets := []types.LockTarget{
+		{User: user.GetName()},
+		{MFADevice: mfaID},
+		{AccessRequest: requestID},
+		{Device: deviceID},
+	}
+	testTargets = goslices.AppendSeq(testTargets, services.RolesToLockTargets(goslices.Values(user.GetRoles())))
 	for _, target := range testTargets {
 		t.Run(fmt.Sprintf("lock targeting %v", target), func(t *testing.T) {
 			lockWatch, err := p.a.SubscribeToLockTarget(ctx, target)

lib/auth/auth_with_roles.go

@@ -9170,7 +9170,7 @@ func checkOktaLockTarget(ctx context.Context, authzCtx *authz.Context, users ser
 
 	target := lock.Target()
 	switch {
-	case !target.Equals(types.LockTarget{User: target.User}):
+	case target != (types.LockTarget{User: target.User}):
 		return trace.BadParameter("%s", errorMsg)
 
 	case target.User == "":

lib/authz/permissions.go

@@ -24,8 +24,10 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"maps"
 	"net"
 	"os"
+	"slices"
 	"strings"
 	"time"
 
@@ -310,44 +312,18 @@ func (c *Context) LockTargets() []types.LockTarget {
 	if _, ok := c.Identity.(UnauthenticatedRole); ok {
 		return nil
 	}
-	lockTargets := services.LockTargetsFromTLSIdentity(c.Identity.GetIdentity())
-Loop:
-	for _, unmappedTarget := range services.LockTargetsFromTLSIdentity(c.UnmappedIdentity.GetIdentity()) {
-		// Append a lock target from UnmappedIdentity only if it is not already
-		// known from Identity.
-		for _, knownTarget := range lockTargets {
-			if unmappedTarget.Equals(knownTarget) {
-				continue Loop
-			}
-		}
-		lockTargets = append(lockTargets, unmappedTarget)
+	lockTargets := make(map[types.LockTarget]struct{})
+	for lockTarget := range services.LockTargetsFromTLSIdentity(c.Identity.GetIdentity()) {
+		lockTargets[lockTarget] = struct{}{}
+	}
+	for lockTarget := range services.LockTargetsFromTLSIdentity(c.UnmappedIdentity.GetIdentity()) {
+		lockTargets[lockTarget] = struct{}{}
 	}
 	if r, ok := c.Identity.(BuiltinRole); ok {
-		switch r.Role {
-		// Node role is a special case because it was previously suported as a
-		// lock target that only locked the `ssh_service`. If the same Teleport server
-		// had multiple roles, Node lock would only lock the `ssh_service` while
-		// other roles would be able to authenticate into Teleport without a problem.
-		// To remove the ambiguity, we now lock the entire Teleport server for
-		// all roles using the LockTarget.ServerID field and `Node` field is
-		// deprecated.
-		// In order to support legacy behavior, we need fill in both `ServerID`
-		// and `Node` fields if the role is `Node` so that the previous behavior
-		// is preserved.
-		// This is a legacy behavior that we need to support for backwards compatibility.
-		case types.RoleNode:
-			lockTargets = append(lockTargets,
-				types.LockTarget{ServerID: r.GetServerID()},
-				types.LockTarget{ServerID: r.Identity.Username},
-			)
-		default:
-			lockTargets = append(lockTargets,
-				types.LockTarget{ServerID: r.GetServerID()},
-				types.LockTarget{ServerID: r.Identity.Username},
-			)
-		}
+		lockTargets[types.LockTarget{ServerID: r.GetServerID()}] = struct{}{}
+		lockTargets[types.LockTarget{ServerID: r.Identity.Username}] = struct{}{}
 	}
-	return lockTargets
+	return slices.AppendSeq(make([]types.LockTarget, 0, len(lockTargets)), maps.Keys(lockTargets))
 }
 
 // WithExtraRoles returns a shallow copy of [c], where the users roles have been

lib/authz/permissions_test.go

@@ -240,6 +240,28 @@ func TestContextLockTargets(t *testing.T) {
 	})
 }
 
+func BenchmarkLockTargets(b *testing.B) {
+	for _, roleCount := range []int{1, 10, 100, 1000} {
+		b.Run(fmt.Sprintf("role_count=%d", roleCount), func(b *testing.B) {
+			roles := make([]string, 0, roleCount)
+			for i := range roleCount {
+				roles = append(roles, fmt.Sprintf("load-test-%d", i))
+			}
+			id := tlsca.Identity{
+				Groups: roles,
+			}
+			var idGetter authz.IdentityGetter = authz.WrapIdentity(id)
+			ctx := &authz.Context{
+				Identity:         idGetter,
+				UnmappedIdentity: idGetter,
+			}
+			for b.Loop() {
+				_ = ctx.LockTargets()
+			}
+		})
+	}
+}
+
 func TestAuthorizeWithLocksForLocalUser(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()

lib/services/lock.go

@@ -20,11 +20,13 @@ package services
 
 import (
 	"fmt"
+	"iter"
+	"maps"
+	"slices"
 
 	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport/api/types"
-	apiutils "github.com/gravitational/teleport/api/utils"
 	"github.com/gravitational/teleport/lib/sshca"
 	"github.com/gravitational/teleport/lib/tlsca"
 	"github.com/gravitational/teleport/lib/utils"
@@ -50,32 +52,46 @@ var StrictLockingModeAccessDenied = trace.AccessDenied("preventive lock-out due
 func SSHAccessLockTargets(localClusterName, serverID, osLogin string, accessInfo *AccessInfo, unmappedIdentity *sshca.Identity) []types.LockTarget {
 	// ssh access lock targets are currently constructed identically to proxying lock targets,
 	// except that the os login associated with the ssh access attempt is also locked.
-	return append(ProxyingLockTargets(localClusterName, serverID, accessInfo, unmappedIdentity), types.LockTarget{Login: osLogin})
+	return proxyingLockTargets(localClusterName, serverID, &osLogin, accessInfo, unmappedIdentity)
 }
 
 // ProxyingLockTargets computes the full set of lock targets related to teleport proxying.
 func ProxyingLockTargets(localClusterName, serverID string, accessInfo *AccessInfo, unmappedIdentity *sshca.Identity) []types.LockTarget {
-	lockTargets := []types.LockTarget{
-		{User: accessInfo.Username},
-		{ServerID: serverID},
-		{ServerID: utils.HostFQDN(serverID, localClusterName)},
+	var noOSLogin *string
+	return proxyingLockTargets(localClusterName, serverID, noOSLogin, accessInfo, unmappedIdentity)
+}
+
+func proxyingLockTargets(localClusterName, serverID string, osLogin *string, accessInfo *AccessInfo, unmappedIdentity *sshca.Identity) []types.LockTarget {
+	lockTargets := map[types.LockTarget]struct{}{
+		{User: accessInfo.Username}:                            struct{}{},
+		{ServerID: serverID}:                                   struct{}{},
+		{ServerID: utils.HostFQDN(serverID, localClusterName)}: struct{}{},
 	}
 	if mfaDevice := unmappedIdentity.MFAVerified; mfaDevice != "" {
-		lockTargets = append(lockTargets, types.LockTarget{MFADevice: mfaDevice})
+		lockTargets[types.LockTarget{MFADevice: mfaDevice}] = struct{}{}
 	}
 	if trustedDevice := unmappedIdentity.DeviceID; trustedDevice != "" {
-		lockTargets = append(lockTargets, types.LockTarget{Device: trustedDevice})
+		lockTargets[types.LockTarget{Device: trustedDevice}] = struct{}{}
 	}
 	if joinToken := unmappedIdentity.JoinToken; joinToken != "" {
-		lockTargets = append(lockTargets, types.LockTarget{JoinToken: joinToken})
+		lockTargets[types.LockTarget{JoinToken: joinToken}] = struct{}{}
 	}
 	if botInstanceID := unmappedIdentity.BotInstanceID; botInstanceID != "" {
-		lockTargets = append(lockTargets, types.LockTarget{BotInstanceID: botInstanceID})
+		lockTargets[types.LockTarget{BotInstanceID: botInstanceID}] = struct{}{}
 	}
-	roles := apiutils.Deduplicate(append(accessInfo.Roles, unmappedIdentity.Roles...))
-	lockTargets = append(lockTargets, RolesToLockTargets(roles)...)
-	lockTargets = append(lockTargets, AccessRequestsToLockTargets(unmappedIdentity.ActiveRequests)...)
-	return lockTargets
+	for lockTarget := range RolesToLockTargets(slices.Values(accessInfo.Roles)) {
+		lockTargets[lockTarget] = struct{}{}
+	}
+	for lockTarget := range RolesToLockTargets(slices.Values(unmappedIdentity.Roles)) {
+		lockTargets[lockTarget] = struct{}{}
+	}
+	for lockTarget := range AccessRequestsToLockTargets(slices.Values(unmappedIdentity.ActiveRequests)) {
+		lockTargets[lockTarget] = struct{}{}
+	}
+	if osLogin != nil {
+		lockTargets[types.LockTarget{Login: *osLogin}] = struct{}{}
+	}
+	return slices.AppendSeq(make([]types.LockTarget, 0, len(lockTargets)), maps.Keys(lockTargets))
 }
 
 // GitForwardingLockTargets computes the full set of lock targets related to git forwarding.
@@ -85,42 +101,58 @@ func GitForwardingLockTargets(localClusterName, serverID string, accessInfo *Acc
 }
 
 // LockTargetsFromTLSIdentity infers a list of LockTargets from tlsca.Identity.
-func LockTargetsFromTLSIdentity(id tlsca.Identity) []types.LockTarget {
-	lockTargets := append(RolesToLockTargets(id.Groups), types.LockTarget{User: id.Username})
-	if id.MFAVerified != "" {
-		lockTargets = append(lockTargets, types.LockTarget{MFADevice: id.MFAVerified})
+func LockTargetsFromTLSIdentity(id tlsca.Identity) iter.Seq[types.LockTarget] {
+	return func(yield func(types.LockTarget) bool) {
+		for lockTarget := range RolesToLockTargets(slices.Values(id.Groups)) {
+			if !yield(lockTarget) {
+				return
+			}
+		}
+		if !yield(types.LockTarget{User: id.Username}) {
+			return
+		}
+		if id.MFAVerified != "" && !yield(types.LockTarget{MFADevice: id.MFAVerified}) {
+			return
+		}
+		if id.DeviceExtensions.DeviceID != "" && !yield(types.LockTarget{Device: id.DeviceExtensions.DeviceID}) {
+			return
+		}
+		if id.JoinToken != "" && !yield(types.LockTarget{JoinToken: id.JoinToken}) {
+			return
+		}
+		if id.BotInstanceID != "" && !yield(types.LockTarget{BotInstanceID: id.BotInstanceID}) {
+			return
+		}
+		for lockTarget := range AccessRequestsToLockTargets(slices.Values(id.ActiveRequests)) {
+			if !yield(lockTarget) {
+				return
+			}
+		}
 	}
-	if id.DeviceExtensions.DeviceID != "" {
-		lockTargets = append(lockTargets, types.LockTarget{Device: id.DeviceExtensions.DeviceID})
-	}
-	if id.JoinToken != "" {
-		lockTargets = append(lockTargets, types.LockTarget{JoinToken: id.JoinToken})
-	}
-	if id.BotInstanceID != "" {
-		lockTargets = append(lockTargets, types.LockTarget{BotInstanceID: id.BotInstanceID})
-	}
-	lockTargets = append(lockTargets, AccessRequestsToLockTargets(id.ActiveRequests)...)
-	return lockTargets
 }
 
 // RolesToLockTargets converts a list of roles to a list of LockTargets
 // (one LockTarget per role).
-func RolesToLockTargets(roles []string) []types.LockTarget {
-	lockTargets := make([]types.LockTarget, 0, len(roles))
-	for _, role := range roles {
-		lockTargets = append(lockTargets, types.LockTarget{Role: role})
+func RolesToLockTargets(roles iter.Seq[string]) iter.Seq[types.LockTarget] {
+	return func(yield func(types.LockTarget) bool) {
+		for role := range roles {
+			if !yield(types.LockTarget{Role: role}) {
+				return
+			}
+		}
 	}
-	return lockTargets
 }
 
 // AccessRequestsToLockTargets converts a list of access requests to a list of
 // LockTargets (one LockTarget per access request)
-func AccessRequestsToLockTargets(accessRequests []string) []types.LockTarget {
-	lockTargets := make([]types.LockTarget, 0, len(accessRequests))
-	for _, accessRequest := range accessRequests {
-		lockTargets = append(lockTargets, types.LockTarget{AccessRequest: accessRequest})
+func AccessRequestsToLockTargets(accessRequests iter.Seq[string]) iter.Seq[types.LockTarget] {
+	return func(yield func(types.LockTarget) bool) {
+		for accessRequest := range accessRequests {
+			if !yield(types.LockTarget{AccessRequest: accessRequest}) {
+				return
+			}
+		}
 	}
-	return lockTargets
 }
 
 // UnmarshalLock unmarshals the Lock resource from JSON.

lib/services/lock_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/testing/protocmp"
 
 	"github.com/gravitational/teleport/api/types"
@@ -45,25 +46,26 @@ func TestLockTargetsFromTLSIdentity(t *testing.T) {
 		}
 
 		// Test.
-		got := services.LockTargetsFromTLSIdentity(identity)
+		got := make(map[types.LockTarget]struct{})
+		for lockTarget := range services.LockTargetsFromTLSIdentity(identity) {
+			got[lockTarget] = struct{}{}
+		}
 
-		want := []types.LockTarget{
-			{User: identity.Username},
-			{MFADevice: identity.MFAVerified},
-			{Device: identity.DeviceExtensions.DeviceID},
-			{JoinToken: "example"},
-			{BotInstanceID: "a-b-c-d"},
+		want := map[types.LockTarget]struct{}{
+			{User: identity.Username}:                    struct{}{},
+			{MFADevice: identity.MFAVerified}:            struct{}{},
+			{Device: identity.DeviceExtensions.DeviceID}: struct{}{},
+			{JoinToken: "example"}:                       struct{}{},
+			{BotInstanceID: "a-b-c-d"}:                   struct{}{},
+		}
+		for _, role := range identity.Groups {
+			want[types.LockTarget{Role: role}] = struct{}{}
 		}
-		// Insert roles at the start to match `got`s order.
-		// The test itself doesn't care about the order, it's just easier to test
-		// this way.
-		want = append(services.RolesToLockTargets(identity.Groups), want...)
 		for _, request := range identity.ActiveRequests {
-			want = append(want, types.LockTarget{AccessRequest: request})
-		}
-		if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" {
-			t.Errorf("LockTargetsFromTLSIdentity mismatch (-want +got)\n%s", diff)
+			want[types.LockTarget{AccessRequest: request}] = struct{}{}
 		}
+
+		require.Empty(t, cmp.Diff(want, got, protocmp.Transform()), "LockTargetsFromTLSIdentity mismatch (-want +got)")
 	})
 }
 
@@ -96,30 +98,31 @@ func TestSSHAccessLockTargets(t *testing.T) {
 			Roles:    mappedRoles,
 		}
 
-		got := services.SSHAccessLockTargets(clusterName, serverID, osLogin, accessInfo, unmappedIdentity)
-		want := []types.LockTarget{
-			{User: teleportUser},
-			{ServerID: serverID},
-			{ServerID: serverID + "." + clusterName},
-			{MFADevice: mfaDevice},
-			{Device: trustedDevice},
-			{JoinToken: joinToken},
-			{BotInstanceID: botInstanceID},
+		got := make(map[types.LockTarget]struct{})
+		for _, lockTarget := range services.SSHAccessLockTargets(clusterName, serverID, osLogin, accessInfo, unmappedIdentity) {
+			got[lockTarget] = struct{}{}
+		}
+
+		want := map[types.LockTarget]struct{}{
+			{User: teleportUser}:                     struct{}{},
+			{ServerID: serverID}:                     struct{}{},
+			{ServerID: serverID + "." + clusterName}: struct{}{},
+			{MFADevice: mfaDevice}:                   struct{}{},
+			{Device: trustedDevice}:                  struct{}{},
+			{JoinToken: joinToken}:                   struct{}{},
+			{BotInstanceID: botInstanceID}:           struct{}{},
+			{Login: osLogin}:                         struct{}{},
 		}
 		for _, role := range mappedRoles {
-			want = append(want, types.LockTarget{Role: role})
+			want[types.LockTarget{Role: role}] = struct{}{}
 		}
-		for _, role := range unmappedRoles[:len(unmappedRoles)-1] /* skip duplicate role */ {
-			want = append(want, types.LockTarget{Role: role})
+		for _, role := range unmappedRoles {
+			want[types.LockTarget{Role: role}] = struct{}{}
 		}
 		for _, request := range accessRequests {
-			want = append(want, types.LockTarget{AccessRequest: request})
+			want[types.LockTarget{AccessRequest: request}] = struct{}{}
 		}
 
-		want = append(want, types.LockTarget{Login: osLogin})
-
-		if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" {
-			t.Errorf("SSHAccessLockTargets mismatch (-want +got)\n%s", diff)
-		}
+		require.Empty(t, cmp.Diff(want, got, protocmp.Transform()), "SSHAccessLockTargets mismatch (-want +got)")
 	})
 }

lib/services/watcher.go

@@ -1306,7 +1306,7 @@ func (p *lockCollector) notifyStale() {
 func lockTargetsToWatchKinds(targets []types.LockTarget) ([]types.WatchKind, error) {
 	watchKinds := make([]types.WatchKind, 0, len(targets))
 	for _, target := range targets {
-		if target.IsEmpty() {
+		if target == (types.LockTarget{}) {
 			continue
 		}
 		filter, err := target.IntoMap()

lib/srv/ctx_test.go

@@ -332,26 +332,29 @@ func TestSSHAccessLockTargets(t *testing.T) {
 			Roles:    mappedRoles,
 		}
 
-		got := services.SSHAccessLockTargets(clusterName, serverID, osLogin, accessInfo, unmappedIdentity)
-		want := []types.LockTarget{
-			{User: username},
-			{ServerID: serverID},
-			{ServerID: serverID + "." + clusterName},
-			{MFADevice: mfaDevice},
-			{Device: trustedDevice},
+		got := make(map[types.LockTarget]struct{})
+		for _, lockTarget := range services.SSHAccessLockTargets(clusterName, serverID, osLogin, accessInfo, unmappedIdentity) {
+			got[lockTarget] = struct{}{}
+		}
+
+		want := map[types.LockTarget]struct{}{
+			{User: username}:                         struct{}{},
+			{ServerID: serverID}:                     struct{}{},
+			{ServerID: serverID + "." + clusterName}: struct{}{},
+			{MFADevice: mfaDevice}:                   struct{}{},
+			{Device: trustedDevice}:                  struct{}{},
+			{Login: osLogin}:                         struct{}{},
 		}
 		for _, role := range mappedRoles {
-			want = append(want, types.LockTarget{Role: role})
+			want[types.LockTarget{Role: role}] = struct{}{}
 		}
-		for _, role := range unmappedRoles[:len(unmappedRoles)-1] /* skip duplicate role */ {
-			want = append(want, types.LockTarget{Role: role})
+		for _, role := range unmappedRoles {
+			want[types.LockTarget{Role: role}] = struct{}{}
 		}
 		for _, request := range accessRequests {
-			want = append(want, types.LockTarget{AccessRequest: request})
-		}
-		want = append(want, types.LockTarget{Login: osLogin})
-		if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" {
-			t.Errorf("SSHAccessLockTargets mismatch (-want +got)\n%s", diff)
+			want[types.LockTarget{AccessRequest: request}] = struct{}{}
 		}
+
+		require.Empty(t, cmp.Diff(want, got, protocmp.Transform()), "SSHAccessLockTargets mismatch (-want +got)")
 	})
 }

lib/srv/desktop/linux_server.go

@@ -563,7 +563,7 @@ func (sess *linuxSession) startMonitor() error {
 		EmitterContext:        s.closeCtx,
 		LockWatcher:           s.cfg.LockWatcher,
 		LockingMode:           sess.authCtx.Checker.LockingMode(sess.authPref.GetLockingMode()),
-		LockTargets:           append(services.LockTargetsFromTLSIdentity(sess.identity), types.LockTarget{LinuxDesktop: sess.desktop.GetMetadata().GetName()}),
+		LockTargets:           slices.AppendSeq([]types.LockTarget{{LinuxDesktop: sess.desktop.GetMetadata().GetName()}}, services.LockTargetsFromTLSIdentity(sess.identity)),
 		Tracker:               &sess.track,
 		TeleportUser:          sess.identity.Username,
 		UserOriginClusterName: sess.identity.OriginClusterName,

lib/srv/desktop/windows_server.go

@@ -30,6 +30,7 @@ import (
 	"maps"
 	"net"
 	"os"
+	goslices "slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -1001,7 +1002,7 @@ func (s *WindowsService) connectRDP(ctx context.Context, log *slog.Logger, tdpCo
 		EmitterContext:        s.closeCtx,
 		LockWatcher:           s.cfg.LockWatcher,
 		LockingMode:           authCtx.Checker.LockingMode(authPref.GetLockingMode()),
-		LockTargets:           append(services.LockTargetsFromTLSIdentity(identity), types.LockTarget{WindowsDesktop: desktop.GetName()}),
+		LockTargets:           goslices.AppendSeq([]types.LockTarget{{WindowsDesktop: desktop.GetName()}}, services.LockTargetsFromTLSIdentity(identity)),
 		Tracker:               rdpc,
 		TeleportUser:          identity.Username,
 		UserOriginClusterName: identity.OriginClusterName,