From cf1dee4a2973c9c0b2b0c7bab3c88702934de113 Mon Sep 17 00:00:00 2001 From: Brian Joerger Date: Tue, 19 Nov 2024 10:56:32 -0800 Subject: [PATCH] Update several admin actions to allow reusable MFA challenges (#49095) * Allow reused MFA for more admin action endpoints; Add comment to AuthorizeAdminAction." * Add update to when to extend reuse section of RFD 155. --- lib/auth/auth_with_roles.go | 44 +++++++++---------- lib/auth/autoupdate/autoupdatev1/service.go | 6 +-- .../autoupdate/autoupdatev1/service_test.go | 32 +++++++++----- .../clusterconfig/clusterconfigv1/service.go | 6 +-- lib/auth/crownjewel/crownjewelv1/service.go | 10 ++--- .../crownjewel/crownjewelv1/service_test.go | 40 ++++++++++++----- lib/auth/dbobject/dbobjectv1/service.go | 3 +- lib/auth/dbobject/dbobjectv1/service_test.go | 40 ++++++++++++----- .../dbobjectimportrulev1/service.go | 3 +- .../dbobjectimportrulev1/service_test.go | 40 ++++++++++++----- .../dynamicwindowsv1/service.go | 8 ++-- .../dynamicwindowsv1/service_test.go | 40 ++++++++++++----- .../machineidv1/bot_instance_service.go | 2 +- .../machineidv1/bot_instance_service_test.go | 10 +++-- lib/auth/presence/presencev1/service.go | 4 +- .../userprovisioningv2/service.go | 12 ++--- lib/auth/users/usersv1/service.go | 2 +- lib/auth/vnetconfig/vnetconfigv1/service.go | 6 +-- .../vnetconfig/vnetconfigv1/service_test.go | 30 +++++++++---- lib/authz/permissions.go | 15 ++++++- rfd/0155-scoped-webauthn-credentials.md | 6 +++ 21 files changed, 233 insertions(+), 126 deletions(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 25ed29cd3ef71..70078ee0ff737 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -2135,7 +2135,7 @@ func (a *ServerWithRoles) DeleteToken(ctx context.Context, token string) error { return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -2736,7 +2736,7 @@ func (a *ServerWithRoles) DeleteAccessRequest(ctx context.Context, name string) return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -3501,7 +3501,7 @@ func (a *ServerWithRoles) UpdateOIDCConnector(ctx context.Context, connector typ return nil, trace.AccessDenied("OIDC is only available in Teleport Enterprise") } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -3623,7 +3623,7 @@ func (a *ServerWithRoles) DeleteOIDCConnector(ctx context.Context, connectorID s return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -3681,7 +3681,7 @@ func (a *ServerWithRoles) UpdateSAMLConnector(ctx context.Context, connector typ return nil, trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -3810,7 +3810,7 @@ func (a *ServerWithRoles) DeleteSAMLConnector(ctx context.Context, connectorID s return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -3884,7 +3884,7 @@ func (a *ServerWithRoles) UpdateGithubConnector(ctx context.Context, connector t return nil, trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -3929,7 +3929,7 @@ func (a *ServerWithRoles) DeleteGithubConnector(ctx context.Context, connectorID return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -4238,7 +4238,7 @@ func (a *ServerWithRoles) CreateRole(ctx context.Context, role types.Role) (type return nil, trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -4270,7 +4270,7 @@ func (a *ServerWithRoles) UpdateRole(ctx context.Context, role types.Role) (type return nil, trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -4388,7 +4388,7 @@ func (a *ServerWithRoles) DeleteRole(ctx context.Context, name string) error { return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -4586,7 +4586,7 @@ func (a *ServerWithRoles) ResetAuthPreference(ctx context.Context) error { return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -4729,7 +4729,7 @@ func (a *ServerWithRoles) ResetClusterNetworkingConfig(ctx context.Context) erro return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -4827,7 +4827,7 @@ func (a *ServerWithRoles) ResetSessionRecordingConfig(ctx context.Context) error } } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -4929,7 +4929,7 @@ func (a *ServerWithRoles) UpsertTrustedCluster(ctx context.Context, tc types.Tru return nil, trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -4952,7 +4952,7 @@ func (a *ServerWithRoles) DeleteTrustedCluster(ctx context.Context, name string) return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -5722,7 +5722,7 @@ func (a *ServerWithRoles) DeleteNetworkRestrictions(ctx context.Context) error { return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -6997,7 +6997,7 @@ func (a *ServerWithRoles) DeleteSAMLIdPServiceProvider(ctx context.Context, name return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -7147,7 +7147,7 @@ func (a *ServerWithRoles) CreateUserGroup(ctx context.Context, userGroup types.U return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -7164,7 +7164,7 @@ func (a *ServerWithRoles) UpdateUserGroup(ctx context.Context, userGroup types.U return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -7186,7 +7186,7 @@ func (a *ServerWithRoles) DeleteUserGroup(ctx context.Context, name string) erro return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } @@ -7208,7 +7208,7 @@ func (a *ServerWithRoles) DeleteAllUserGroups(ctx context.Context) error { return trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return trace.Wrap(err) } diff --git a/lib/auth/autoupdate/autoupdatev1/service.go b/lib/auth/autoupdate/autoupdatev1/service.go index 993860203746f..aa9e29f2fabea 100644 --- a/lib/auth/autoupdate/autoupdatev1/service.go +++ b/lib/auth/autoupdate/autoupdatev1/service.go @@ -237,7 +237,7 @@ func (s *Service) DeleteAutoUpdateConfig(ctx context.Context, req *autoupdate.De return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -419,7 +419,7 @@ func (s *Service) DeleteAutoUpdateVersion(ctx context.Context, req *autoupdate.D return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -571,7 +571,7 @@ func (s *Service) DeleteAutoUpdateAgentRollout(ctx context.Context, req *autoupd return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/autoupdate/autoupdatev1/service_test.go b/lib/auth/autoupdate/autoupdatev1/service_test.go index eb3220792b939..be71b976d698a 100644 --- a/lib/auth/autoupdate/autoupdatev1/service_test.go +++ b/lib/auth/autoupdate/autoupdatev1/service_test.go @@ -125,9 +125,13 @@ func TestServiceAccess(t *testing.T) { allowedVerbs: []string{types.VerbRead}, }, { - name: "DeleteAutoUpdateConfig", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, + name: "DeleteAutoUpdateConfig", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, }, // AutoUpdate version check. { @@ -168,9 +172,13 @@ func TestServiceAccess(t *testing.T) { allowedVerbs: []string{types.VerbRead}, }, { - name: "DeleteAutoUpdateVersion", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, + name: "DeleteAutoUpdateVersion", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, }, // AutoUpdate agent rollout check. { @@ -214,10 +222,14 @@ func TestServiceAccess(t *testing.T) { builtinRole: &authz.BuiltinRole{Role: types.RoleAuth}, }, { - name: "DeleteAutoUpdateAgentRollout", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, - builtinRole: &authz.BuiltinRole{Role: types.RoleAuth}, + name: "DeleteAutoUpdateAgentRollout", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, + builtinRole: &authz.BuiltinRole{Role: types.RoleAuth}, }, } diff --git a/lib/auth/clusterconfig/clusterconfigv1/service.go b/lib/auth/clusterconfig/clusterconfigv1/service.go index fcecf3d0700dc..5194db3a3adcf 100644 --- a/lib/auth/clusterconfig/clusterconfigv1/service.go +++ b/lib/auth/clusterconfig/clusterconfigv1/service.go @@ -357,7 +357,7 @@ func (s *Service) ResetAuthPreference(ctx context.Context, _ *clusterconfigpb.Re return nil, trace.Wrap(err) } - if err := authzCtx.AuthorizeAdminAction(); err != nil { + if err := authzCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -627,7 +627,7 @@ func (s *Service) ResetClusterNetworkingConfig(ctx context.Context, _ *clusterco } } - if err := authzCtx.AuthorizeAdminAction(); err != nil { + if err := authzCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -881,7 +881,7 @@ func (s *Service) ResetSessionRecordingConfig(ctx context.Context, _ *clustercon } } - if err := authzCtx.AuthorizeAdminAction(); err != nil { + if err := authzCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } defaultConfig := types.DefaultSessionRecordingConfig() diff --git a/lib/auth/crownjewel/crownjewelv1/service.go b/lib/auth/crownjewel/crownjewelv1/service.go index fa61478600800..b78cfa29f3129 100644 --- a/lib/auth/crownjewel/crownjewelv1/service.go +++ b/lib/auth/crownjewel/crownjewelv1/service.go @@ -112,7 +112,7 @@ func (s *Service) CreateCrownJewel(ctx context.Context, req *crownjewelv1.Create return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -186,7 +186,6 @@ func (s *Service) GetCrownJewel(ctx context.Context, req *crownjewelv1.GetCrownJ } return rsp, nil - } // UpdateCrownJewel updates crown jewel resource. @@ -200,7 +199,7 @@ func (s *Service) UpdateCrownJewel(ctx context.Context, req *crownjewelv1.Update return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -252,7 +251,7 @@ func (s *Service) UpsertCrownJewel(ctx context.Context, req *crownjewelv1.Upsert return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -274,7 +273,6 @@ func (s *Service) UpsertCrownJewel(ctx context.Context, req *crownjewelv1.Upsert } return rsp, nil - } func (s *Service) emitUpsertAuditEvent(ctx context.Context, old, new *crownjewelv1.CrownJewel, authCtx *authz.Context, err error) { @@ -296,7 +294,7 @@ func (s *Service) DeleteCrownJewel(ctx context.Context, req *crownjewelv1.Delete return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/crownjewel/crownjewelv1/service_test.go b/lib/auth/crownjewel/crownjewelv1/service_test.go index 734325a2e8eec..f975ff68a1a7e 100644 --- a/lib/auth/crownjewel/crownjewelv1/service_test.go +++ b/lib/auth/crownjewel/crownjewelv1/service_test.go @@ -47,24 +47,40 @@ func TestServiceAccess(t *testing.T) { allowedStates []authz.AdminActionAuthState }{ { - name: "CreateCrownJewel", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbCreate}, + name: "CreateCrownJewel", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbCreate}, }, { - name: "UpdateCrownJewel", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbUpdate}, + name: "UpdateCrownJewel", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbUpdate}, }, { - name: "DeleteCrownJewel", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, + name: "DeleteCrownJewel", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, }, { - name: "UpsertCrownJewel", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbCreate, types.VerbUpdate}, + name: "UpsertCrownJewel", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbCreate, types.VerbUpdate}, }, { name: "ListCrownJewels", diff --git a/lib/auth/dbobject/dbobjectv1/service.go b/lib/auth/dbobject/dbobjectv1/service.go index 39b04d07a1c46..43ecd7a6a8dc9 100644 --- a/lib/auth/dbobject/dbobjectv1/service.go +++ b/lib/auth/dbobject/dbobjectv1/service.go @@ -87,7 +87,7 @@ func (rs *DatabaseObjectService) authorize(ctx context.Context, adminAction bool } if adminAction { - err = authCtx.AuthorizeAdminAction() + err = authCtx.AuthorizeAdminActionAllowReusedMFA() if err != nil { return trace.Wrap(err) } @@ -153,7 +153,6 @@ func (rs *DatabaseObjectService) CreateDatabaseObject( return nil, trace.Wrap(err) } return out, nil - } // UpsertDatabaseObject creates a new DatabaseObject or forcefully updates an existing DatabaseObject. diff --git a/lib/auth/dbobject/dbobjectv1/service_test.go b/lib/auth/dbobject/dbobjectv1/service_test.go index 49a737e1b1986..b9e06117500a0 100644 --- a/lib/auth/dbobject/dbobjectv1/service_test.go +++ b/lib/auth/dbobject/dbobjectv1/service_test.go @@ -82,24 +82,40 @@ func TestServiceAccess(t *testing.T) { allowedStates []authz.AdminActionAuthState }{ { - name: "UpsertDatabaseObject", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbUpdate, types.VerbCreate}, + name: "UpsertDatabaseObject", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbUpdate, types.VerbCreate}, }, { - name: "CreateDatabaseObject", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbCreate}, + name: "CreateDatabaseObject", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbCreate}, }, { - name: "UpdateDatabaseObject", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbUpdate}, + name: "UpdateDatabaseObject", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbUpdate}, }, { - name: "DeleteDatabaseObject", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, + name: "DeleteDatabaseObject", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, }, { name: "GetDatabaseObject", diff --git a/lib/auth/dbobjectimportrule/dbobjectimportrulev1/service.go b/lib/auth/dbobjectimportrule/dbobjectimportrulev1/service.go index 03e169994dcc5..a10b1fc7a5529 100644 --- a/lib/auth/dbobjectimportrule/dbobjectimportrulev1/service.go +++ b/lib/auth/dbobjectimportrule/dbobjectimportrulev1/service.go @@ -87,7 +87,7 @@ func (rs *DatabaseObjectImportRuleService) authorize(ctx context.Context, adminA } if adminAction { - err = authCtx.AuthorizeAdminAction() + err = authCtx.AuthorizeAdminActionAllowReusedMFA() if err != nil { return trace.Wrap(err) } @@ -153,7 +153,6 @@ func (rs *DatabaseObjectImportRuleService) CreateDatabaseObjectImportRule( return nil, trace.Wrap(err) } return out, nil - } // UpsertDatabaseObjectImportRule creates a new DatabaseObjectImportRule or forcefully updates an existing DatabaseObjectImportRule. diff --git a/lib/auth/dbobjectimportrule/dbobjectimportrulev1/service_test.go b/lib/auth/dbobjectimportrule/dbobjectimportrulev1/service_test.go index ebd624737de71..5fcbf95099d0e 100644 --- a/lib/auth/dbobjectimportrule/dbobjectimportrulev1/service_test.go +++ b/lib/auth/dbobjectimportrule/dbobjectimportrulev1/service_test.go @@ -82,24 +82,40 @@ func TestServiceAccess(t *testing.T) { allowedStates []authz.AdminActionAuthState }{ { - name: "UpsertDatabaseObjectImportRule", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbUpdate, types.VerbCreate}, + name: "UpsertDatabaseObjectImportRule", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbUpdate, types.VerbCreate}, }, { - name: "CreateDatabaseObjectImportRule", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbCreate}, + name: "CreateDatabaseObjectImportRule", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbCreate}, }, { - name: "UpdateDatabaseObjectImportRule", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbUpdate}, + name: "UpdateDatabaseObjectImportRule", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbUpdate}, }, { - name: "DeleteDatabaseObjectImportRule", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, + name: "DeleteDatabaseObjectImportRule", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, }, { name: "GetDatabaseObjectImportRule", diff --git a/lib/auth/dynamicwindows/dynamicwindowsv1/service.go b/lib/auth/dynamicwindows/dynamicwindowsv1/service.go index b8ca1c8ac09f5..dac5b72ca3351 100644 --- a/lib/auth/dynamicwindows/dynamicwindowsv1/service.go +++ b/lib/auth/dynamicwindows/dynamicwindowsv1/service.go @@ -159,7 +159,7 @@ func (s *Service) CreateDynamicWindowsDesktop(ctx context.Context, req *dynamicw if err != nil { return nil, trace.Wrap(err) } - if err := auth.AuthorizeAdminAction(); err != nil { + if err := auth.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } if err := auth.CheckAccessToKind(types.KindDynamicWindowsDesktop, types.VerbCreate); err != nil { @@ -191,7 +191,7 @@ func (s *Service) UpdateDynamicWindowsDesktop(ctx context.Context, req *dynamicw if err != nil { return nil, trace.Wrap(err) } - if err := auth.AuthorizeAdminAction(); err != nil { + if err := auth.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } if err := auth.CheckAccessToKind(types.KindDynamicWindowsDesktop, types.VerbUpdate); err != nil { @@ -226,7 +226,7 @@ func (s *Service) UpsertDynamicWindowsDesktop(ctx context.Context, req *dynamicw if err != nil { return nil, trace.Wrap(err) } - if err := auth.AuthorizeAdminAction(); err != nil { + if err := auth.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } if err := auth.CheckAccessToKind(types.KindDynamicWindowsDesktop, types.VerbCreate, types.VerbUpdate); err != nil { @@ -263,7 +263,7 @@ func (s *Service) DeleteDynamicWindowsDesktop(ctx context.Context, req *dynamicw if err != nil { return nil, trace.Wrap(err) } - if err := auth.AuthorizeAdminAction(); err != nil { + if err := auth.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } if err := auth.CheckAccessToKind(types.KindDynamicWindowsDesktop, types.VerbDelete); err != nil { diff --git a/lib/auth/dynamicwindows/dynamicwindowsv1/service_test.go b/lib/auth/dynamicwindows/dynamicwindowsv1/service_test.go index 8ae55acf040a0..efc0f9a1fb142 100644 --- a/lib/auth/dynamicwindows/dynamicwindowsv1/service_test.go +++ b/lib/auth/dynamicwindows/dynamicwindowsv1/service_test.go @@ -83,24 +83,40 @@ func TestServiceAccess(t *testing.T) { allowedStates []authz.AdminActionAuthState }{ { - name: "CreateDynamicWindowsDesktop", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbCreate}, + name: "CreateDynamicWindowsDesktop", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbCreate}, }, { - name: "UpdateDynamicWindowsDesktop", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbUpdate}, + name: "UpdateDynamicWindowsDesktop", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbUpdate}, }, { - name: "UpsertDynamicWindowsDesktop", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbCreate, types.VerbUpdate}, + name: "UpsertDynamicWindowsDesktop", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbCreate, types.VerbUpdate}, }, { - name: "DeleteDynamicWindowsDesktop", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, + name: "DeleteDynamicWindowsDesktop", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, }, { name: "ListDynamicWindowsDesktops", diff --git a/lib/auth/machineid/machineidv1/bot_instance_service.go b/lib/auth/machineid/machineidv1/bot_instance_service.go index 025c5b0e64827..2e44015aeba16 100644 --- a/lib/auth/machineid/machineidv1/bot_instance_service.go +++ b/lib/auth/machineid/machineidv1/bot_instance_service.go @@ -102,7 +102,7 @@ func (b *BotInstanceService) DeleteBotInstance(ctx context.Context, req *pb.Dele return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/machineid/machineidv1/bot_instance_service_test.go b/lib/auth/machineid/machineidv1/bot_instance_service_test.go index 69dc11f54a620..5b79a70cd0e9c 100644 --- a/lib/auth/machineid/machineidv1/bot_instance_service_test.go +++ b/lib/auth/machineid/machineidv1/bot_instance_service_test.go @@ -70,9 +70,13 @@ func TestBotInstanceServiceAccess(t *testing.T) { allowedVerbs: []string{types.VerbRead, types.VerbList}, }, { - name: "DeleteBotInstance", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, + name: "DeleteBotInstance", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, }, { name: "SubmitHeartbeat", diff --git a/lib/auth/presence/presencev1/service.go b/lib/auth/presence/presencev1/service.go index 8bb5f1511a4ee..c83e36fc453e3 100644 --- a/lib/auth/presence/presencev1/service.go +++ b/lib/auth/presence/presencev1/service.go @@ -222,7 +222,7 @@ func (s *Service) UpdateRemoteCluster( if err := authCtx.CheckAccessToKind(types.KindRemoteCluster, types.VerbUpdate); err != nil { return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -295,7 +295,7 @@ func (s *Service) DeleteRemoteCluster( ); err != nil { return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/userprovisioning/userprovisioningv2/service.go b/lib/auth/userprovisioning/userprovisioningv2/service.go index 4de5ade2ab901..78339a2586c93 100644 --- a/lib/auth/userprovisioning/userprovisioningv2/service.go +++ b/lib/auth/userprovisioning/userprovisioningv2/service.go @@ -91,7 +91,7 @@ func (s *Service) ListStaticHostUsers(ctx context.Context, req *userprovisioning if err := authCtx.CheckAccessToKind(types.KindStaticHostUser, types.VerbList, types.VerbRead); err != nil { return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -117,7 +117,7 @@ func (s *Service) GetStaticHostUser(ctx context.Context, req *userprovisioningpb if err := authCtx.CheckAccessToKind(types.KindStaticHostUser, types.VerbRead); err != nil { return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -147,7 +147,7 @@ func (s *Service) CreateStaticHostUser(ctx context.Context, req *userprovisionin if err := authCtx.CheckAccessToKind(types.KindStaticHostUser, types.VerbCreate); err != nil { return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -180,7 +180,7 @@ func (s *Service) UpdateStaticHostUser(ctx context.Context, req *userprovisionin if err := authCtx.CheckAccessToKind(types.KindStaticHostUser, types.VerbUpdate); err != nil { return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -213,7 +213,7 @@ func (s *Service) UpsertStaticHostUser(ctx context.Context, req *userprovisionin if err := authCtx.CheckAccessToKind(types.KindStaticHostUser, types.VerbCreate, types.VerbUpdate); err != nil { return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -249,7 +249,7 @@ func (s *Service) DeleteStaticHostUser(ctx context.Context, req *userprovisionin if err := authCtx.CheckAccessToKind(types.KindStaticHostUser, types.VerbDelete); err != nil { return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/users/usersv1/service.go b/lib/auth/users/usersv1/service.go index 75098db371aa2..217a258002392 100644 --- a/lib/auth/users/usersv1/service.go +++ b/lib/auth/users/usersv1/service.go @@ -472,7 +472,7 @@ func (s *Service) DeleteUser(ctx context.Context, req *userspb.DeleteUserRequest return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/vnetconfig/vnetconfigv1/service.go b/lib/auth/vnetconfig/vnetconfigv1/service.go index 0c861c0d1fddb..c64609e7470b1 100644 --- a/lib/auth/vnetconfig/vnetconfigv1/service.go +++ b/lib/auth/vnetconfig/vnetconfigv1/service.go @@ -77,7 +77,7 @@ func (s *Service) CreateVnetConfig(ctx context.Context, req *vnet.CreateVnetConf return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -96,7 +96,7 @@ func (s *Service) UpdateVnetConfig(ctx context.Context, req *vnet.UpdateVnetConf return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -135,7 +135,7 @@ func (s *Service) DeleteVnetConfig(ctx context.Context, _ *vnet.DeleteVnetConfig return nil, trace.Wrap(err) } - if err := authCtx.AuthorizeAdminAction(); err != nil { + if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/vnetconfig/vnetconfigv1/service_test.go b/lib/auth/vnetconfig/vnetconfigv1/service_test.go index 7babff02fe006..7bffb42c0df1c 100644 --- a/lib/auth/vnetconfig/vnetconfigv1/service_test.go +++ b/lib/auth/vnetconfig/vnetconfigv1/service_test.go @@ -60,18 +60,26 @@ func TestServiceAccess(t *testing.T) { } testCases := []testCase{ { - name: "CreateVnetConfig", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbCreate}, + name: "CreateVnetConfig", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbCreate}, action: func(service *Service) error { _, err := service.CreateVnetConfig(ctx, &vnet.CreateVnetConfigRequest{VnetConfig: vnetConfig}) return trace.Wrap(err) }, }, { - name: "UpdateVnetConfig", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbUpdate}, + name: "UpdateVnetConfig", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbUpdate}, action: func(service *Service) error { if _, err := service.storage.CreateVnetConfig(ctx, vnetConfig); err != nil { return trace.Wrap(err, "creating vnet_config as pre-req for Update test") @@ -81,9 +89,13 @@ func TestServiceAccess(t *testing.T) { }, }, { - name: "DeleteVnetConfig", - allowedStates: []authz.AdminActionAuthState{authz.AdminActionAuthNotRequired, authz.AdminActionAuthMFAVerified}, - allowedVerbs: []string{types.VerbDelete}, + name: "DeleteVnetConfig", + allowedStates: []authz.AdminActionAuthState{ + authz.AdminActionAuthNotRequired, + authz.AdminActionAuthMFAVerified, + authz.AdminActionAuthMFAVerifiedWithReuse, + }, + allowedVerbs: []string{types.VerbDelete}, action: func(service *Service) error { if _, err := service.storage.CreateVnetConfig(ctx, vnetConfig); err != nil { return trace.Wrap(err, "creating vnet_config as pre-req for Delete test") diff --git a/lib/authz/permissions.go b/lib/authz/permissions.go index f8fcf6f810c8d..d5fff2f945474 100644 --- a/lib/authz/permissions.go +++ b/lib/authz/permissions.go @@ -1507,6 +1507,13 @@ func (c *Context) CheckAccessToRule(ruleCtx *services.Context, kind string, verb } // AuthorizeAdminAction will ensure that the user is authorized to perform admin actions. +// MFA challenges that allow reuse will not be accepted. +// +// In the majority of cases, allowing reuse is ok and can result in better UX. Forbidding +// reuse should be reserved for critical actions (e.g. CA rotation, cert generation) and +// other actions that are not expected to be performed in bulk (e.g. access request reviews). +// +// See https://github.com/gravitational/teleport/blob/master/rfd/0155-scoped-webauthn-credentials.md#when-to-extend-reuse func (c *Context) AuthorizeAdminAction() error { switch c.AdminActionAuthState { case AdminActionAuthMFAVerified, AdminActionAuthNotRequired: @@ -1516,7 +1523,13 @@ func (c *Context) AuthorizeAdminAction() error { } // AuthorizeAdminActionAllowReusedMFA will ensure that the user is authorized to perform -// admin actions. Additionally, MFA challenges that allow reuse will be accepted. +// admin actions. MFA challenges that allow reuse will be accepted. +// +// In the majority of cases, allowing reuse is ok and can result in better UX. Forbidding +// reuse should be reserved for critical actions (e.g. CA rotation, cert generation) and +// other actions that are not expected to be performed in bulk (e.g. access request reviews). +// +// See https://github.com/gravitational/teleport/blob/master/rfd/0155-scoped-webauthn-credentials.md#when-to-extend-reuse func (c *Context) AuthorizeAdminActionAllowReusedMFA() error { if c.AdminActionAuthState == AdminActionAuthMFAVerifiedWithReuse { return nil diff --git a/rfd/0155-scoped-webauthn-credentials.md b/rfd/0155-scoped-webauthn-credentials.md index 251094bedbd63..c661d3a45ad6b 100644 --- a/rfd/0155-scoped-webauthn-credentials.md +++ b/rfd/0155-scoped-webauthn-credentials.md @@ -245,6 +245,12 @@ endpoints: - `http createWebSession` - `http deleteWebSession` +Update: Minimizing which admin actions allow reuse has caused several issues +in new bulk admin actions, most notably the new Discover flows. Other than the +critical admin action endpoints listed above, most now allow reuse. It is +instead left up to the client to be reasonable, only requesting a reusable MFA +challenge in preparation for a bulk admin action. + #### Expiration Webauthn challenges are always set to expire after 5 minutes. However, as we've