Allow Bots to submit access request reviews (gravitational#33375)

* 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

@@ -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()
 

lib/auth/auth.go

@@ -4400,7 +4400,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")
@@ -4412,7 +4433,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)
 	}

lib/auth/auth_with_roles.go

@@ -2485,8 +2485,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)
 	}
@@ -2596,7 +2601,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) {

lib/auth/helpers.go

@@ -821,7 +821,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{

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,
 	}

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,18 +643,18 @@ 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 {
 				if rt.expect.IsNone() {
 					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)