Enforce dedicated service account for Cloud Build triggers (#110)

* feat: enforce mandatory SA, slim down roles, and avoid using compute engine default SA
* feat: implement minimum required permissions for Cloud Build SA and Service Agents

Author: alex-doudou

cicd-mcp-server/cloudbuild/cloudbuild.go

@@ -91,7 +91,7 @@ type CreateTriggerArgs struct {
 	Location       string `json:"location" jsonschema:"The Google Cloud location for the trigger."`
 	TriggerID      string `json:"trigger_id" jsonschema:"The ID of the trigger."`
 	RepoLink       string `json:"repo_link" jsonschema:"The Developer Connect repository link, use dev connect setup repo to create a connect and repo link"`
-	ServiceAccount string `json:"service_account,omitempty" jsonschema:"The service account to use for the build. E.g. serviceAccount:name@project-id.iam.gserviceaccount.com optional"`
+	ServiceAccount string `json:"service_account" jsonschema:"The service account to use for the build. E.g. serviceAccount:name@project-id.iam.gserviceaccount.com. This MUST be a dedicated service account, not the default Compute Engine service account."`
 	Branch         string `json:"branch,omitempty" jsonschema:"Create builds on push to branch. Should be regex e.g. '^main$'"`
 	Tag            string `json:"tag,omitempty" jsonschema:"Create builds on new tag push. Should be regex e.g. '^nightly$'"`
 }
@@ -100,10 +100,13 @@ var createTriggerToolFunc func(ctx context.Context, req *mcp.CallToolRequest, ar
 
 func addCreateTriggerTool(server *mcp.Server, cbClient cloudbuildclient.CloudBuildClient, iamClient iamclient.IAMClient, rmClient resourcemanagerclient.ResourcemanagerClient) {
 	createTriggerToolFunc = func(ctx context.Context, req *mcp.CallToolRequest, args CreateTriggerArgs) (*mcp.CallToolResult, any, error) {
-		if args.ServiceAccount != "" && !strings.HasPrefix(args.ServiceAccount, "serviceAccount:") {
+		if args.ServiceAccount == "" {
+			return &mcp.CallToolResult{}, nil, fmt.Errorf("service_account is required and must be a dedicated service account")
+		}
+		if !strings.HasPrefix(args.ServiceAccount, "serviceAccount:") {
 			args.ServiceAccount = fmt.Sprintf("serviceAccount:%s", args.ServiceAccount)
 		}
-		if args.ServiceAccount != "" && !IsValidServiceAccount(args.ServiceAccount) {
+		if !IsValidServiceAccount(args.ServiceAccount) {
 			return &mcp.CallToolResult{}, nil, fmt.Errorf("service account needs to be of the form serviceAccount:name@project-id.iam.gserviceaccount.com")
 		}
 		resolvedSA, err := setPermissionsForCloudBuildSA(ctx, args.ProjectID, args.ServiceAccount, rmClient, iamClient)
@@ -119,30 +122,66 @@ func addCreateTriggerTool(server *mcp.Server, cbClient cloudbuildclient.CloudBui
 	mcp.AddTool(server, &mcp.Tool{Name: "create_build_trigger", Description: "Creates a new Cloud Build trigger."}, createTriggerToolFunc)
 }
 
-// setPermissionsForSA resolves the SA (if default) and grants it a role.
-// It creates and manages its own Resource Manager client.
+// setPermissionsForCloudBuildSA resolves the SA and grants it a role.
 func setPermissionsForCloudBuildSA(ctx context.Context, projectID, serviceAccount string, rmClient resourcemanagerclient.ResourcemanagerClient, iamClient iamclient.IAMClient) (string, error) {
-	// Construct the Compute Engine default service account email
 	resolvedSA := serviceAccount
-	if resolvedSA == "" {
-		projectNumber, err := rmClient.ToProjectNumber(ctx, projectID)
-		if err != nil {
-			return "", fmt.Errorf("unable to resolve project id to number: %w", err)
-		}
-		resolvedSA = fmt.Sprintf("serviceAccount:%d-compute@developer.gserviceaccount.com", projectNumber)
-	}
 
 	// If the serviceAccount prefix is not there, add it.
 	if !strings.HasPrefix(resolvedSA, "serviceAccount:") {
 		resolvedSA = fmt.Sprintf("serviceAccount:%s", resolvedSA)
 	}
-	roles := []string{"roles/developerconnect.tokenAccessor"}
-	for _, r := range roles {
+
+	// 1. Roles for the Cloud Build Service Account (Project Level)
+	gcbSARoles := []string{
+		"roles/artifactregistry.writer",
+		"roles/cloudbuild.builds.editor",
+		"roles/cloudbuild.workerpools.use",
+		"roles/developerconnect.tokenAccessor",
+		"roles/logging.logWriter",
+		"roles/run.developer",
+		"roles/serviceusage.serviceUsageConsumer",
+		"roles/storage.admin",
+	}
+	for _, r := range gcbSARoles {
 		_, err := iamClient.AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), r, resolvedSA)
 		if err != nil {
-			return "", fmt.Errorf("unable to add role %s to member %s on resource %s err: %w", r, resolvedSA, fmt.Sprintf("projects/%s", projectID), err)
+			return "", fmt.Errorf("unable to add role %s to member %s on project %s err: %w", r, resolvedSA, projectID, err)
+		}
+	}
+
+	projectNumber, err := rmClient.ToProjectNumber(ctx, projectID)
+	if err != nil {
+		return "", fmt.Errorf("unable to resolve project id to number: %w", err)
+	}
+
+	// 2. Roles for the Developer Connect Service Agent (Project Level)
+	dcP4sa := fmt.Sprintf("serviceAccount:service-%d@gcp-sa-developerconnect.iam.gserviceaccount.com", projectNumber)
+	_, err = iamClient.AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), "roles/secretmanager.admin", dcP4sa)
+	if err != nil {
+		return "", fmt.Errorf("unable to add secretmanager.admin role to Developer Connect P4SA %s: %w", dcP4sa, err)
+	}
+
+	// 3. Roles for the Cloud Build Service Agent (Project Level)
+	gcbP4sa := fmt.Sprintf("serviceAccount:service-%d@gcp-sa-cloudbuild.iam.gserviceaccount.com", projectNumber)
+	gcbP4saRoles := []string{
+		"roles/cloudbuild.serviceAgent",
+		"roles/developerconnect.tokenAccessor",
+	}
+	for _, r := range gcbP4saRoles {
+		_, err := iamClient.AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), r, gcbP4sa)
+		if err != nil {
+			return "", fmt.Errorf("unable to add role %s to Cloud Build P4SA %s on project %s err: %w", r, gcbP4sa, projectID, err)
 		}
 	}
+
+	// 4. Role for the Cloud Build Service Agent on the Cloud Run SA (Default Compute SA)
+	defaultComputeSA := fmt.Sprintf("serviceAccount:%d-compute@developer.gserviceaccount.com", projectNumber)
+	resource := fmt.Sprintf("projects/%s/serviceAccounts/%s", projectID, strings.TrimPrefix(defaultComputeSA, "serviceAccount:"))
+	_, err = iamClient.AddIAMRoleBinding(ctx, resource, "roles/iam.serviceAccountUser", gcbP4sa)
+	if err != nil {
+		return "", fmt.Errorf("unable to add iam.serviceAccountUser role to Cloud Build P4SA %s on SA %s err: %w", gcbP4sa, defaultComputeSA, err)
+	}
+
 	return resolvedSA, nil
 }
 

cicd-mcp-server/cloudbuild/cloudbuild_test.go

@@ -38,32 +38,65 @@ func TestSetPermissionsForCloudBuildSA(t *testing.T) {
 	serviceAccount := "serviceAccount:test-sa@example.com"
 	serviceAccountWOPrefix := "test-sa@example.com"
 
+	projectNumber := int64(12345)
+	gcbSARoles := []string{
+		"roles/artifactregistry.writer",
+		"roles/cloudbuild.builds.editor",
+		"roles/cloudbuild.workerpools.use",
+		"roles/developerconnect.tokenAccessor",
+		"roles/logging.logWriter",
+		"roles/run.developer",
+		"roles/serviceusage.serviceUsageConsumer",
+		"roles/storage.admin",
+	}
+	dcP4sa := fmt.Sprintf("serviceAccount:service-%d@gcp-sa-developerconnect.iam.gserviceaccount.com", projectNumber)
+	gcbP4sa := fmt.Sprintf("serviceAccount:service-%d@gcp-sa-cloudbuild.iam.gserviceaccount.com", projectNumber)
+	gcbP4saRoles := []string{
+		"roles/cloudbuild.serviceAgent",
+		"roles/developerconnect.tokenAccessor",
+	}
+	defaultComputeSA := fmt.Sprintf("%d-compute@developer.gserviceaccount.com", projectNumber)
+	saResource := fmt.Sprintf("projects/%s/serviceAccounts/%s", projectID, defaultComputeSA)
+
 	t.Run("with service account", func(t *testing.T) {
-		mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), "roles/developerconnect.tokenAccessor", serviceAccount).Return(nil, nil)
+		for _, r := range gcbSARoles {
+			mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), r, serviceAccount).Return(nil, nil)
+		}
+		mockRMClient.EXPECT().ToProjectNumber(ctx, projectID).Return(projectNumber, nil)
+		mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), "roles/secretmanager.admin", dcP4sa).Return(nil, nil)
+		for _, r := range gcbP4saRoles {
+			mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), r, gcbP4sa).Return(nil, nil)
+		}
+		mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, saResource, "roles/iam.serviceAccountUser", gcbP4sa).Return(nil, nil)
 
 		resolvedSA, err := setPermissionsForCloudBuildSA(ctx, projectID, serviceAccount, mockRMClient, mockIAMClient)
 		assert.NoError(t, err)
 		assert.Equal(t, serviceAccount, resolvedSA)
 	})
 
 	t.Run("with service account, no prefix", func(t *testing.T) {
-		mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), "roles/developerconnect.tokenAccessor", serviceAccount).Return(nil, nil)
+		for _, r := range gcbSARoles {
+			mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), r, serviceAccount).Return(nil, nil)
+		}
+		mockRMClient.EXPECT().ToProjectNumber(ctx, projectID).Return(projectNumber, nil)
+		mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), "roles/secretmanager.admin", dcP4sa).Return(nil, nil)
+		for _, r := range gcbP4saRoles {
+			mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), r, gcbP4sa).Return(nil, nil)
+		}
+		mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, saResource, "roles/iam.serviceAccountUser", gcbP4sa).Return(nil, nil)
 
 		resolvedSA, err := setPermissionsForCloudBuildSA(ctx, projectID, serviceAccountWOPrefix, mockRMClient, mockIAMClient)
 		assert.NoError(t, err)
 		assert.Equal(t, serviceAccount, resolvedSA)
 	})
 
-	t.Run("without service account", func(t *testing.T) {
-		projectNumber := int64(12345)
-		expectedSA := fmt.Sprintf("serviceAccount:%d-compute@developer.gserviceaccount.com", projectNumber)
-
-		mockRMClient.EXPECT().ToProjectNumber(ctx, projectID).Return(projectNumber, nil)
-		mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), "roles/developerconnect.tokenAccessor", expectedSA).Return(nil, nil)
+	t.Run("no service account provided fails", func(t *testing.T) {
+		// Since fallback is removed, providing an empty SA should result in an invalid SA prefix 
+		// and fail during IAM role binding.
+		mockIAMClient.EXPECT().AddIAMRoleBinding(gomock.Any(), gomock.Any(), gomock.Any(), "serviceAccount:").Return(nil, fmt.Errorf("invalid member"))
 
-		resolvedSA, err := setPermissionsForCloudBuildSA(ctx, projectID, "", mockRMClient, mockIAMClient)
-		assert.NoError(t, err)
-		assert.Equal(t, expectedSA, resolvedSA)
+		_, err := setPermissionsForCloudBuildSA(ctx, projectID, "", mockRMClient, mockIAMClient)
+		assert.Error(t, err)
 	})
 
 	t.Run("iam error", func(t *testing.T) {
@@ -74,9 +107,14 @@ func TestSetPermissionsForCloudBuildSA(t *testing.T) {
 	})
 
 	t.Run("resourcemanager error", func(t *testing.T) {
-		mockRMClient.EXPECT().ToProjectNumber(gomock.Any(), gomock.Any()).Return(int64(0), fmt.Errorf("some error"))
+		// Mock success for the first set of roles
+		for _, r := range gcbSARoles {
+			mockIAMClient.EXPECT().AddIAMRoleBinding(ctx, fmt.Sprintf("projects/%s", projectID), r, serviceAccount).Return(nil, nil)
+		}
+		// Mock failure for ToProjectNumber
+		mockRMClient.EXPECT().ToProjectNumber(ctx, projectID).Return(int64(0), fmt.Errorf("some error"))
 
-		_, err := setPermissionsForCloudBuildSA(ctx, projectID, "", mockRMClient, mockIAMClient)
+		_, err := setPermissionsForCloudBuildSA(ctx, projectID, serviceAccount, mockRMClient, mockIAMClient)
 		assert.Error(t, err)
 	})
 }

cicd-mcp-server/iam/client/iamclient.go

@@ -82,10 +82,32 @@ func (c *IAMClientImpl) AddIAMRoleBinding(ctx context.Context, resourceID, role,
 		return nil, fmt.Errorf("failed to get iam policy: %v", err)
 	}
 
-	policy.Bindings = append(policy.Bindings, &cloudresourcemanagerv1.Binding{
-		Role:    role,
-		Members: []string{member},
-	})
+	// Find the binding for the specific role
+	var binding *cloudresourcemanagerv1.Binding
+	for _, b := range policy.Bindings {
+		if b.Role == role {
+			binding = b
+			break
+		}
+	}
+
+	if binding != nil {
+		// Check if the member already exists in the binding
+		for _, m := range binding.Members {
+			if m == member {
+				// Member already exists, no need to update
+				return policy, nil
+			}
+		}
+		// Add member to existing binding
+		binding.Members = append(binding.Members, member)
+	} else {
+		// Create new binding for the role
+		policy.Bindings = append(policy.Bindings, &cloudresourcemanagerv1.Binding{
+			Role:    role,
+			Members: []string{member},
+		})
+	}
 
 	setPolicyRequest := &cloudresourcemanagerv1.SetIamPolicyRequest{
 		Policy: policy,

skills/google-cicd-pipeline-design/references/how_to_create_cloudbuild_trigger.md

@@ -4,24 +4,24 @@ This document outlines the standard, idempotent procedure for creating a Google
 
 ---
 
-## ## Core Principle: Idempotency
+## Core Principle: Idempotency
 
 Every step in this process must be **idempotent**. This means the agent must **always check if a resource already exists** before attempting to create it. This prevents errors and ensures the process can be run multiple times safely.
 
 ---
 
-## ## Prerequisite Checklist
+## Prerequisite Checklist
 
 The following dependencies must be satisfied in order before creating the trigger.
 
-### ### 1. Ensure `cloudbuild.yaml` Exists
+### 1. Ensure `cloudbuild.yaml` Exists
 
 The trigger needs a build configuration file to execute.
 
 * **Action**: Check for a `cloudbuild.yaml` file at the root of the source repository.
 * **If it does not exist**: Generate one by translating the user-approved plan. The steps in the generated YAML must be a direct translation of the components defined in the plan's `stages` object. The specifics of the steps (e.g., using `pytest` vs. `mvn test`) should be informed by discovering the application archetype (e.g., by finding a `pyproject.toml` or `pom.xml`).
 
-### ### 2. Ensure Artifact Registry Repository Exists
+### 2. Ensure Artifact Registry Repository Exists
 
 The `cloudbuild.yaml` file will reference an Artifact Registry repository to push container images. This repository must exist before a build can succeed.
 
@@ -30,7 +30,7 @@ The `cloudbuild.yaml` file will reference an Artifact Registry repository to pus
 * **Check** if this repository already exists in the target GCP project.
 * **If it does not exist**: Create it using the available tools.
 
-### ### 3. Ensure Developer Connect and Repository Link Exist
+### 3. Ensure Developer Connect and Repository Link Exist
 
 Cloud Build triggers connect to source code via Developer Connect. The entire connection and repository link must be in place.
 
@@ -40,8 +40,16 @@ Cloud Build triggers connect to source code via Developer Connect. The entire co
 4.  **Check for Repository Link**: Check if a repository link for that specific URI already exists within the Developer Connect connection.
 5.  **Create Repository Link (if needed)**: If the link does not exist, create it. This link is the resource that the Cloud Build trigger will formally point to.
 
+### 4. Ensure Dedicated Service Account Exists
+
+Cloud Build triggers should **always** use a dedicated, user-managed service account instead of the default Compute Engine service account to follow the principle of least privilege. **CRITICAL: NEVER use the default Compute Engine service account (typically formatted as `[PROJECT_NUMBER]-compute@developer.gserviceaccount.com`), even if you observe existing triggers in the project using it.**
+
+1.  **Check for Service Account**: Check if a dedicated service account for Cloud Build (e.g., `cloud-build-runner@<PROJECT_ID>.iam.gserviceaccount.com`) already exists.
+2.  **Create Service Account (if needed)**: If no dedicated service account exists, create one using `gcloud iam service-accounts create`.
+3.  **Required Roles (Ensured Automatically)**: The `create_build_trigger` MCP tool will **automatically ensure** the necessary roles (e.g., `roles/logging.logWriter`, `roles/artifactregistry.writer`, `roles/developerconnect.tokenAccessor`, `roles/run.developer`, `roles/storage.admin`, `roles/serviceusage.serviceUsageConsumer`, `roles/cloudbuild.builds.editor`, `roles/cloudbuild.workerpools.use`, and specific IAM delegation on the default Compute SA) are granted to the relevant service accounts. You **do not** need to grant these permissions manually.
+
 ---
 
-## ## Final Step: Creating the Trigger
+## Final Step: Creating the Trigger
 
-Once all prerequisites are met, the agent can create the trigger itself using the available tools.
+Once all prerequisites are met, the agent can create the trigger itself using the `create_build_trigger` tool. **You MUST provide the email of the dedicated service account identified or created in Step 4 to the `service_account` parameter.**