From 70b02b343ba55550aeb4883c272136226996b9fc Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Tue, 17 Oct 2023 11:33:22 +0100 Subject: [PATCH] [v14] Allow Bots to submit access request reviews (#33509) * Guesstimate towards a solution * Remove unrelated comment * Roughly try a different way of supporting this * Revert line that was necessary to change * Change signature of exported func to match client * Fix test invocation * Add test for Bot reviewing access request * Fix missing err check in test * Use single context * Use client rather than unexported method in test * Check err before defer --- lib/auth/access_request_test.go | 65 +++++++++++++++++++++++++++++ lib/auth/auth.go | 25 ++++++++++- lib/auth/auth_with_roles.go | 12 ++++-- lib/auth/helpers.go | 2 +- lib/services/access_request.go | 58 ++++++++++++++++++++++++- lib/services/access_request_test.go | 8 ++-- 6 files changed, 160 insertions(+), 10 deletions(-) diff --git a/lib/auth/access_request_test.go b/lib/auth/access_request_test.go index b79e8d607df5d..989a19ebf3c76 100644 --- a/lib/auth/access_request_test.go +++ b/lib/auth/access_request_test.go @@ -180,6 +180,7 @@ func TestAccessRequest(t *testing.T) { t.Run("single", func(t *testing.T) { testSingleAccessRequests(t, testPack) }) t.Run("multi", func(t *testing.T) { testMultiAccessRequests(t, testPack) }) t.Run("role refresh with bogus request ID", func(t *testing.T) { testRoleRefreshWithBogusRequestID(t, testPack) }) + t.Run("bot user approver", func(t *testing.T) { testBotAccessRequestReview(t, testPack) }) } func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) { @@ -421,6 +422,70 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) { } } +// testBotAccessRequestReview specifically ensures that a bots output cert +// can be used to review a access request. This is because there's a special +// case to handle their role impersonated certs correctly. +func testBotAccessRequestReview(t *testing.T, testPack *accessRequestTestPack) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + // Create the bot + adminClient, err := testPack.tlsServer.NewClient(TestAdmin()) + require.NoError(t, err) + defer adminClient.Close() + bot, err := adminClient.CreateBot(ctx, &proto.CreateBotRequest{ + Name: "request-approver", + Roles: []string{ + // Grants the ability to approve requests + "admins", + }, + }) + require.NoError(t, err) + + // Use the bot user to generate some certs using role impersonation. + // This mimics what the bot actually does. + botClient, err := testPack.tlsServer.NewClient(TestUser(bot.UserName)) + require.NoError(t, err) + defer botClient.Close() + certRes, err := botClient.GenerateUserCerts(ctx, proto.UserCertsRequest{ + Username: bot.UserName, + PublicKey: testPack.pubKey, + Expires: time.Now().Add(time.Hour), + + RoleRequests: []string{"admins"}, + UseRoleRequests: true, + }) + require.NoError(t, err) + tlsCert, err := tls.X509KeyPair(certRes.TLS, testPack.privKey) + require.NoError(t, err) + impersonatedBotClient := testPack.tlsServer.NewClientWithCert(tlsCert) + defer impersonatedBotClient.Close() + + // Create an access request for the bot to approve + requesterClient, err := testPack.tlsServer.NewClient(TestUser("requester")) + require.NoError(t, err) + defer requesterClient.Close() + accessRequest, err := services.NewAccessRequest("requester", "admins") + require.NoError(t, err) + accessRequest, err = requesterClient.CreateAccessRequestV2(ctx, accessRequest) + require.NoError(t, err) + + // Approve the access request with the bot + accessRequest, err = impersonatedBotClient.SubmitAccessReview(ctx, types.AccessReviewSubmission{ + RequestID: accessRequest.GetName(), + Review: types.AccessReview{ + ProposedState: types.RequestState_APPROVED, + }, + }) + require.NoError(t, err) + + // Check the final state of the request + require.Equal(t, bot.UserName, accessRequest.GetReviews()[0].Author) + require.Equal(t, types.RequestState_APPROVED, accessRequest.GetState()) +} + func testMultiAccessRequests(t *testing.T, testPack *accessRequestTestPack) { t.Parallel() diff --git a/lib/auth/auth.go b/lib/auth/auth.go index bcedf225b479c..dc17539d347c1 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -4301,7 +4301,28 @@ func (a *Server) SetAccessRequestState(ctx context.Context, params types.AccessR return trace.Wrap(err) } -func (a *Server) SubmitAccessReview(ctx context.Context, params types.AccessReviewSubmission) (types.AccessRequest, error) { +// SubmitAccessReview is used to process a review of an Access Request. +// This is implemented by Server.submitAccessRequest but this method exists +// to provide a matching signature with the auth client. This allows the +// hosted plugins to use the Server struct directly as a client. +func (a *Server) SubmitAccessReview( + ctx context.Context, + params types.AccessReviewSubmission, +) (types.AccessRequest, error) { + // identity is passed as nil as we do not know which user has triggered + // this action. + return a.submitAccessReview(ctx, params, nil) +} + +// submitAccessReview implements submitting a review of an Access Request. +// The `identity` parameter should be the identity of the user that has called +// an RPC that has invoked this, if applicable. It may be nil if this is +// unknown. +func (a *Server) submitAccessReview( + ctx context.Context, + params types.AccessReviewSubmission, + identity *tlsca.Identity, +) (types.AccessRequest, error) { // When promoting a request, the access list name must be set. if params.Review.ProposedState.IsPromoted() && params.Review.GetAccessListName() == "" { return nil, trace.BadParameter("promoted access list can be only set when promoting access requests") @@ -4313,7 +4334,7 @@ func (a *Server) SubmitAccessReview(ctx context.Context, params types.AccessRevi } // set up a checker for the review author - checker, err := services.NewReviewPermissionChecker(ctx, a, params.Review.Author) + checker, err := services.NewReviewPermissionChecker(ctx, a, params.Review.Author, identity) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 297be4a6b6c4d..686ed531e4e50 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -2594,8 +2594,13 @@ func (a *ServerWithRoles) GetAccessRequests(ctx context.Context, filter types.Ac // their own requests. we therefore subselect the filter results to show only those requests // that the user *is* allowed to see (specifically, their own requests + requests that they // are allowed to review). - - checker, err := services.NewReviewPermissionChecker(ctx, a.authServer, a.context.User.GetName()) + identity := a.context.Identity.GetIdentity() + checker, err := services.NewReviewPermissionChecker( + ctx, + a.authServer, + a.context.User.GetName(), + &identity, + ) if err != nil { return nil, trace.Wrap(err) } @@ -2705,7 +2710,8 @@ func (a *ServerWithRoles) SubmitAccessReview(ctx context.Context, submission typ // under optimistic locking at the level of the backend service. the correctness of the // author field is all that needs to be enforced at this level. - return a.authServer.SubmitAccessReview(ctx, submission) + identity := a.context.Identity.GetIdentity() + return a.authServer.submitAccessReview(ctx, submission, &identity) } func (a *ServerWithRoles) GetAccessCapabilities(ctx context.Context, req types.AccessCapabilitiesRequest) (*types.AccessCapabilities, error) { diff --git a/lib/auth/helpers.go b/lib/auth/helpers.go index 1a09f8d97aa08..61d57f3257804 100644 --- a/lib/auth/helpers.go +++ b/lib/auth/helpers.go @@ -805,7 +805,7 @@ func TestUserWithDeviceExtensions(username string, exts tlsca.DeviceExtensions) } } -// TestUser returns a TestIdentity for a local user +// TestRenewableUser returns a TestIdentity for a local user // with renewable credentials. func TestRenewableUser(username string, generation uint64) TestIdentity { return TestIdentity{ diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 59f798ba956e1..00ed7614afda9 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -902,12 +902,68 @@ Outer: return len(needsAllow) == 0, nil } -func NewReviewPermissionChecker(ctx context.Context, getter RequestValidatorGetter, username string) (ReviewPermissionChecker, error) { +type userStateRoleOverride struct { + UserState + Roles []string +} + +func (u userStateRoleOverride) GetRoles() []string { + return u.Roles +} + +func NewReviewPermissionChecker( + ctx context.Context, + getter RequestValidatorGetter, + username string, + identity *tlsca.Identity, +) (ReviewPermissionChecker, error) { uls, err := GetUserOrLoginState(ctx, getter, username) if err != nil { return ReviewPermissionChecker{}, trace.Wrap(err) } + // By default, the users freshly fetched roles are used rather than the + // roles on the x509 identity. This prevents recursive access request + // review. + // + // For bots, however, the roles on the identity must be used. This is + // because the certs output by a bot always use role impersonation and the + // role directly assigned to a bot has minimal permissions. + if uls.IsBot() { + if identity == nil { + // Handle an edge case where SubmitAccessReview is being invoked + // in-memory but as a bot user. + return ReviewPermissionChecker{}, trace.BadParameter( + "bot user provided but identity parameter is nil", + ) + } + if identity.Username != username { + // It should not be possible for these to be different as a + // guard in AuthorizeAccessReviewRequest prevents submitting a + // request as another user unless you have the admin role. This + // safeguard protects against that regressing and creating an + // inconsistent state. + return ReviewPermissionChecker{}, trace.BadParameter( + "bot identity username and review author mismatch", + ) + } + if len(identity.ActiveRequests) > 0 { + // It should not be possible for a bot's output certificates to + // have active requests - but this additional check safeguards us + // against a regression elsewhere and prevents recursive access + // requests occurring. + return ReviewPermissionChecker{}, trace.BadParameter( + "bot should not have active requests", + ) + } + + // Override list of roles to roles currently present on the x509 ident. + uls = userStateRoleOverride{ + UserState: uls, + Roles: identity.Groups, + } + } + c := ReviewPermissionChecker{ UserState: uls, } diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index ded225e321538..c38a66a5255e7 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -146,6 +146,8 @@ func (m *mockGetter) GetClusterName(opts ...MarshalOption) (types.ClusterName, e // TestReviewThresholds tests various review threshold scenarios func TestReviewThresholds(t *testing.T) { + ctx := context.Background() + // describes a collection of roles with various approval/review // permissions. roleDesc := map[string]types.RoleConditions{ @@ -641,10 +643,10 @@ func TestReviewThresholds(t *testing.T) { // perform request validation (necessary in order to initialize internal // request variables like annotations and thresholds). - validator, err := NewRequestValidator(context.Background(), clock, g, tt.requestor, ExpandVars(true)) + validator, err := NewRequestValidator(ctx, clock, g, tt.requestor, ExpandVars(true)) require.NoError(t, err, "scenario=%q", tt.desc) - require.NoError(t, validator.Validate(context.Background(), req, identity), "scenario=%q", tt.desc) + require.NoError(t, validator.Validate(ctx, req, identity), "scenario=%q", tt.desc) Inner: for ri, rt := range tt.reviews { @@ -652,7 +654,7 @@ func TestReviewThresholds(t *testing.T) { rt.expect = types.RequestState_PENDING } - checker, err := NewReviewPermissionChecker(context.TODO(), g, rt.author) + checker, err := NewReviewPermissionChecker(ctx, g, rt.author, nil) require.NoError(t, err, "scenario=%q, rev=%d", tt.desc, ri) canReview, err := checker.CanReviewRequest(req)