From 8db6aa58833ca5af93f0cdcae56b4dd862c81dfe Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Mon, 3 Jan 2022 19:43:42 +0100 Subject: [PATCH] Only allow access request deletion through static roles' permissions (#9540) * Only allow access request deletion through static roles' permissions * Test coverage for access request deletion --- lib/auth/auth_with_roles.go | 29 ++++++++++- lib/auth/auth_with_roles_test.go | 89 ++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 7843397c52076..7066fd0e8b542 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -93,6 +93,23 @@ func (a *ServerWithRoles) withOptions(opts ...actionOption) actionConfig { return cfg } +func (a *ServerWithRoles) withStaticRoles() (actionConfig, error) { + user, err := a.authServer.GetUser(a.context.User.GetName(), false) + if err != nil { + return actionConfig{}, trace.Wrap(err) + } + + checker, err := services.FetchRoles(user.GetRoles(), a.authServer, user.GetTraits()) + if err != nil { + return actionConfig{}, trace.Wrap(err) + } + + return actionConfig{context: Context{ + User: user, + Checker: checker, + }}, nil +} + func (c actionConfig) action(namespace, resource string, verbs ...string) error { if len(verbs) == 0 { return trace.BadParameter("no verbs provided for authorization check on resource %q", resource) @@ -1385,7 +1402,17 @@ func (a *ServerWithRoles) getProxyPublicAddr() string { } func (a *ServerWithRoles) DeleteAccessRequest(ctx context.Context, name string) error { - if err := a.action(apidefaults.Namespace, types.KindAccessRequest, types.VerbDelete); err != nil { + cfg, err := a.withStaticRoles() + if err != nil { + return err + } + if err := cfg.action(apidefaults.Namespace, types.KindAccessRequest, types.VerbDelete); err != nil { + if trace.IsAccessDenied(err) { + if a.withOptions(quietAction(true)).action(apidefaults.Namespace, types.KindAccessRequest, types.VerbDelete) == nil { + // the user would've had permission with the roles granted by access requests + return trace.WrapWithMessage(err, "access request deletion through elevated roles is not allowed") + } + } return trace.Wrap(err) } return a.authServer.DeleteAccessRequest(ctx, name) diff --git a/lib/auth/auth_with_roles_test.go b/lib/auth/auth_with_roles_test.go index 2355814bebb94..5f60ba44a2d9e 100644 --- a/lib/auth/auth_with_roles_test.go +++ b/lib/auth/auth_with_roles_test.go @@ -28,6 +28,7 @@ import ( "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/types" libdefaults "github.com/gravitational/teleport/lib/defaults" + "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/tlsca" "github.com/google/go-cmp/cmp" @@ -1233,3 +1234,91 @@ func TestKindClusterConfig(t *testing.T) { } }) } + +func TestNoElevatedAccessRequestDeletion(t *testing.T) { + t.Parallel() + ctx := context.Background() + + srv, err := NewTestAuthServer(TestAuthServerConfig{Dir: t.TempDir()}) + require.NoError(t, err) + t.Cleanup(func() { srv.Close() }) + + deleterRole, err := types.NewRole("deleter", types.RoleSpecV4{ + Allow: types.RoleConditions{ + Rules: []types.Rule{{ + Resources: []string{"access_request"}, + Verbs: []string{"delete"}, + }}, + }, + }) + require.NoError(t, err) + deleterUser, err := CreateUser(srv.AuthServer, "deletey", deleterRole) + require.NoError(t, err) + + requesterRole, err := types.NewRole("requester", types.RoleSpecV4{ + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{deleterRole.GetName()}, + }, + }, + }) + require.NoError(t, err) + requesterUser, err := CreateUser(srv.AuthServer, "requesty", requesterRole) + require.NoError(t, err) + + request, err := services.NewAccessRequest(requesterUser.GetName(), deleterRole.GetName()) + require.NoError(t, err) + // the request must be for an allowed user/role combination or it will get rejected + err = srv.AuthServer.CreateAccessRequest(ctx, request) + require.NoError(t, err) + + // requesty has used some other unspecified access request to get the + // deleter role in this identity + requesterAuthContext, err := srv.Authorizer.Authorize(context.WithValue(ctx, + ContextUser, + LocalUser{ + Username: requesterUser.GetName(), + Identity: tlsca.Identity{ + Username: requesterUser.GetName(), + Groups: []string{requesterRole.GetName(), deleterRole.GetName()}, + // a tlsca.Identity must have a nonempty Traits field or the + // roles will be reloaded from the backend during Authorize + Traits: map[string][]string{"nonempty": {}}, + }, + }, + )) + require.NoError(t, err) + requesterAuth := &ServerWithRoles{ + authServer: srv.AuthServer, + sessions: srv.SessionServer, + alog: srv.AuditLog, + context: *requesterAuthContext, + } + + err = requesterAuth.DeleteAccessRequest(ctx, request.GetName()) + require.True(t, trace.IsAccessDenied(err)) + // matches the message in lib/auth/auth_with_roles.go:(*ServerWithRoles).DeleteAccessRequest() + require.Contains(t, err.Error(), "deletion through elevated roles") + + deleterAuthContext, err := srv.Authorizer.Authorize(context.WithValue(ctx, + ContextUser, + LocalUser{ + Username: deleterUser.GetName(), + Identity: tlsca.Identity{ + Username: deleterUser.GetName(), + Groups: []string{deleterRole.GetName()}, + Traits: map[string][]string{"nonempty": {}}, + }, + }, + )) + require.NoError(t, err) + deleterAuth := &ServerWithRoles{ + authServer: srv.AuthServer, + sessions: srv.SessionServer, + alog: srv.AuditLog, + context: *deleterAuthContext, + } + + err = deleterAuth.DeleteAccessRequest(ctx, request.GetName()) + require.NoError(t, err) +}