Fix Assume Roles switch back, don't delete role if access list is using it. (#33746)

* Fix Assume Roles switch back, don't delete role if access list is using it. When switching back to the regular user permissions after assuming roles via an access request, Teleport will now use the user login state to ensure that access list permissions are taken into account. Additionally, users will not be able to delete roles if they are in use by an access list. Finally, when refreshing the user while extending a web session, the user login state will be regenerated and used for permissions. * Use sentinel errors. * Trace sentinel error values. * Use errors.New instead.
2026-09-24 16:17:11 +08:00 · 2023-10-23 16:20:35 +00:00
parent 761f73740d
commit 6aa69d459b
4 changed files with 171 additions and 15 deletions
@@ -18,10 +18,12 @@ package auth

 import (
 	"context"
+	"errors"

 	"github.com/gravitational/trace"

 	"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/lib/authz"
 	"github.com/gravitational/teleport/lib/events"
@@ -97,6 +99,12 @@ func (a *Server) UpsertRole(ctx context.Context, role types.Role) (types.Role, e
 	return upserted, nil
 }

+var (
+	errDeleteRoleUser       = errors.New("failed to delete a role that is still in use by a user, check the system server logs for more details")
+	errDeleteRoleCA         = errors.New("failed to delete a role that is still in use by a certificate authority, check the system server logs for more details")
+	errDeleteRoleAccessList = errors.New("failed to delete a role that is still in use by an access list, check the system server logs for more details")
+)
+
 // DeleteRole deletes a role and emits a related audit event.
 func (a *Server) DeleteRole(ctx context.Context, name string) error {
 	// check if this role is used by CA or Users
@@ -110,7 +118,7 @@ func (a *Server) DeleteRole(ctx context.Context, name string) error {
 				// Mask the actual error here as it could be used to enumerate users
 				// within the system.
 				log.Warnf("Failed to delete role: role %v is used by user %v.", name, u.GetName())
-				return trace.BadParameter("failed to delete a role that is still in use by a user, check the system server logs for more details")
+				return trace.Wrap(errDeleteRoleUser)
 			}
 		}
 	}
@@ -126,11 +134,48 @@ func (a *Server) DeleteRole(ctx context.Context, name string) error {
 				// Mask the actual error here as it could be used to enumerate users
 				// within the system.
 				log.Warnf("Failed to delete role: role %v is used by user cert authority %v", name, a.GetClusterName())
-				return trace.BadParameter("failed to delete a role that is still in use by a user, check the system server logs for more details")
+				return trace.Wrap(errDeleteRoleCA)
 			}
 		}
 	}

+	var nextToken string
+	for {
+		var accessLists []*accesslist.AccessList
+		var err error
+		accessLists, nextToken, err = a.Services.AccessListClient().ListAccessLists(ctx, 0 /* default page size */, nextToken)
+		if err != nil {
+			return trace.Wrap(err)
+		}
+
+		for _, accessList := range accessLists {
+			for _, r := range accessList.Spec.Grants.Roles {
+				if r == name {
+					log.Warnf("Failed to delete role: role %v is granted by access list %s", name, accessList.GetName())
+					return trace.Wrap(errDeleteRoleAccessList)
+				}
+			}
+
+			for _, r := range accessList.Spec.MembershipRequires.Roles {
+				if r == name {
+					log.Warnf("Failed to delete role: role %v is required by members of access list %s", name, accessList.GetName())
+					return trace.Wrap(errDeleteRoleAccessList)
+				}
+			}
+
+			for _, r := range accessList.Spec.OwnershipRequires.Roles {
+				if r == name {
+					log.Warnf("Failed to delete role: role %v is required by owners of access list %s", name, accessList.GetName())
+					return trace.Wrap(errDeleteRoleAccessList)
+				}
+			}
+		}
+
+		if nextToken == "" {
+			break
+		}
+	}
+
 	if err := a.Services.DeleteRole(ctx, name); err != nil {
 		return trace.Wrap(err)
 	}
@@ -19,12 +19,15 @@ package auth
 import (
 	"context"
 	"testing"
+	"time"

 	"github.com/gravitational/trace"
 	"github.com/stretchr/testify/require"

 	"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/lib/events"
 )

@@ -68,6 +71,98 @@ func TestUpsertDeleteRoleEventsEmitted(t *testing.T) {
 	require.Nil(t, p.mockEmitter.LastEvent())
 }

+func TestUpsertDeleteDependentRoles(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	p, err := newTestPack(ctx, t.TempDir())
+	require.NoError(t, err)
+
+	// test create new role
+	role, err := types.NewRole("test-role", types.RoleSpecV6{
+		Options: types.RoleOptions{},
+		Allow:   types.RoleConditions{},
+	})
+	require.NoError(t, err)
+
+	// Create a role and assign it to a user.
+	role, err = p.a.UpsertRole(ctx, role)
+	require.NoError(t, err)
+	user, err := types.NewUser("test-user")
+	require.NoError(t, err)
+	user.AddRole(role.GetName())
+	_, err = p.a.CreateUser(ctx, user)
+	require.NoError(t, err)
+
+	// Deletion should fail.
+	require.ErrorIs(t, p.a.DeleteRole(ctx, role.GetName()), errDeleteRoleUser)
+	require.NoError(t, p.a.DeleteUser(ctx, user.GetName()))
+
+	clusterName, err := p.a.GetClusterName()
+	require.NoError(t, err)
+
+	// Update the user CA with the role.
+	ca, err := p.a.GetCertAuthority(ctx, types.CertAuthID{Type: types.UserCA, DomainName: clusterName.GetClusterName()}, true)
+	require.NoError(t, err)
+	ca.AddRole(role.GetName())
+	require.NoError(t, p.a.UpsertCertAuthority(ctx, ca))
+
+	// Deletion should fail.
+	require.ErrorIs(t, p.a.DeleteRole(ctx, role.GetName()), errDeleteRoleCA)
+
+	// Clear out the roles for the CA.
+	ca.SetRoles([]string{})
+	require.NoError(t, p.a.UpsertCertAuthority(ctx, ca))
+
+	// Create an access list that references the role.
+	accessList, err := accesslist.NewAccessList(header.Metadata{
+		Name: "test-access-list",
+	}, accesslist.Spec{
+		Title: "simple",
+		Owners: []accesslist.Owner{
+			{Name: "some-user"},
+		},
+		Grants: accesslist.Grants{
+			Roles: []string{role.GetName()},
+		},
+		Audit: accesslist.Audit{
+			NextAuditDate: time.Now(),
+		},
+		MembershipRequires: accesslist.Requires{
+			Roles: []string{role.GetName()},
+		},
+		OwnershipRequires: accesslist.Requires{
+			Roles: []string{role.GetName()},
+		},
+	})
+	require.NoError(t, err)
+	_, err = p.a.UpsertAccessList(ctx, accessList)
+	require.NoError(t, err)
+
+	// Deletion should fail due to the grant.
+	require.ErrorIs(t, p.a.DeleteRole(ctx, role.GetName()), errDeleteRoleAccessList)
+
+	accessList.Spec.Grants.Roles = []string{"non-existent-role"}
+	_, err = p.a.UpsertAccessList(ctx, accessList)
+	require.NoError(t, err)
+
+	// Deletion should fail due to membership requires.
+	require.ErrorIs(t, p.a.DeleteRole(ctx, role.GetName()), errDeleteRoleAccessList)
+
+	accessList.Spec.MembershipRequires.Roles = []string{"non-existent-role"}
+	_, err = p.a.UpsertAccessList(ctx, accessList)
+	require.NoError(t, err)
+
+	// Deletion should fail due to ownership requires.
+	require.ErrorIs(t, p.a.DeleteRole(ctx, role.GetName()), errDeleteRoleAccessList)
+
+	accessList.Spec.OwnershipRequires.Roles = []string{"non-existent-role"}
+	_, err = p.a.UpsertAccessList(ctx, accessList)
+	require.NoError(t, err)
+
+	// Deletion should succeed
+	require.NoError(t, p.a.DeleteRole(ctx, role.GetName()))
+}
+
 func TestUpsertDeleteLockEventsEmitted(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
@@ -431,7 +431,7 @@ func NewServer(cfg *InitConfig, opts ...ServerOption) (*Server, error) {
 	}

 	// Add in a login hook for generating state during user login.
-	ulsGenerator, err := userloginstate.NewGenerator(userloginstate.GeneratorConfig{
+	as.ulsGenerator, err = userloginstate.NewGenerator(userloginstate.GeneratorConfig{
 		Log:         log,
 		AccessLists: services,
 		Access:      services,
@@ -442,7 +442,7 @@ func NewServer(cfg *InitConfig, opts ...ServerOption) (*Server, error) {
 		return nil, trace.Wrap(err)
 	}

-	as.RegisterLoginHook(ulsGenerator.LoginHook(services.UserLoginStates))
+	as.RegisterLoginHook(as.ulsGenerator.LoginHook(services.UserLoginStates))

 	return &as, nil
 }
@@ -791,6 +791,9 @@ type Server struct {

 	// accessMonitoringEnabled is a flag that indicates whether access monitoring is enabled.
 	accessMonitoringEnabled bool
+
+	// ulsGenerator is the user login state generator.
+	ulsGenerator *userloginstate.Generator
 }

 // SetSAMLService registers svc as the SAMLService that provides the SAML
@@ -3453,10 +3456,17 @@ func (a *Server) ExtendWebSession(ctx context.Context, req WebSessionReq, identi
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}
+
+		// Make sure to refresh the user login state.
+		userState, err := a.ulsGenerator.Refresh(ctx, user, a.UserLoginStates)
+		if err != nil {
+			return nil, trace.Wrap(err)
+		}
+
 		// Updating traits is needed for guided SSH flow in Discover.
-		traits = user.GetTraits()
+		traits = userState.GetTraits()
 		// Updating roles is needed for guided Connect My Computer flow in Discover.
-		roles = user.GetRoles()
+		roles = userState.GetRoles()

 	} else if req.AccessRequestID != "" {
 		accessRequest, err := a.getValidatedAccessRequest(ctx, identity, req.User, req.AccessRequestID)
@@ -3490,7 +3500,7 @@ func (a *Server) ExtendWebSession(ctx context.Context, req WebSessionReq, identi
 		}

 		// Get default/static roles.
-		user, err := a.GetUser(ctx, req.User, false)
+		userState, err := a.GetUserOrLoginState(ctx, req.User)
 		if err != nil {
 			return nil, trace.Wrap(err, "failed to switchback")
 		}
@@ -3499,7 +3509,7 @@ func (a *Server) ExtendWebSession(ctx context.Context, req WebSessionReq, identi
 		allowedResourceIDs = nil

 		// Calculate expiry time.
-		roleSet, err := services.FetchRoles(user.GetRoles(), a, user.GetTraits())
+		roleSet, err := services.FetchRoles(userState.GetRoles(), a, userState.GetTraits())
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}
@@ -3508,7 +3518,7 @@ func (a *Server) ExtendWebSession(ctx context.Context, req WebSessionReq, identi

 		// Set default roles and expiration.
 		expiresAt = prevSession.GetLoginTime().UTC().Add(sessionTTL)
-		roles = user.GetRoles()
+		roles = userState.GetRoles()
 		accessRequests = nil
 	}

@@ -266,15 +266,21 @@ func (g *Generator) emitUsageEvent(ctx context.Context, user types.User, state *
 	return nil
 }

+// Refresh will take the user and update the user login state in the backend.
+func (g *Generator) Refresh(ctx context.Context, user types.User, ulsService services.UserLoginStates) (*userloginstate.UserLoginState, error) {
+	uls, err := g.Generate(ctx, user)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	uls, err = ulsService.UpsertUserLoginState(ctx, uls)
+	return uls, trace.Wrap(err)
+}
+
 // LoginHook creates a login hook from the Generator and the user login state service.
 func (g *Generator) LoginHook(ulsService services.UserLoginStates) func(context.Context, types.User) error {
 	return func(ctx context.Context, user types.User) error {
-		uls, err := g.Generate(ctx, user)
-		if err != nil {
-			return trace.Wrap(err)
-		}
-
-		_, err = ulsService.UpsertUserLoginState(ctx, uls)
+		_, err := g.Refresh(ctx, user, ulsService)
 		return trace.Wrap(err)
 	}
 }