diff --git a/cicd-mcp-server/cloudbuild/cloudbuild.go b/cicd-mcp-server/cloudbuild/cloudbuild.go index 104c02a..d721517 100644 --- a/cicd-mcp-server/cloudbuild/cloudbuild.go +++ b/cicd-mcp-server/cloudbuild/cloudbuild.go @@ -135,7 +135,6 @@ func setPermissionsForCloudBuildSA(ctx context.Context, projectID, serviceAccoun gcbSARoles := []string{ "roles/artifactregistry.writer", "roles/cloudbuild.builds.editor", - "roles/cloudbuild.workerpools.use", "roles/developerconnect.tokenAccessor", "roles/logging.logWriter", "roles/run.developer", @@ -155,7 +154,7 @@ func setPermissionsForCloudBuildSA(ctx context.Context, projectID, serviceAccoun } // 2. Roles for the Developer Connect Service Agent (Project Level) - dcP4sa := fmt.Sprintf("serviceAccount:service-%d@gcp-sa-developerconnect.iam.gserviceaccount.com", projectNumber) + dcP4sa := fmt.Sprintf("serviceAccount:service-%d@gcp-sa-devconnect.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) @@ -174,14 +173,21 @@ func setPermissionsForCloudBuildSA(ctx context.Context, projectID, serviceAccoun } } - // 4. Role for the Cloud Build Service Agent on the Cloud Run SA (Default Compute SA) + // 4. Grant Cloud Build Service Agent permissions to impersonate 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) + _, err = iamClient.AddServiceAccountRoleBinding(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) } + // 5. Grant Cloud Build Service Agent permissions to impersonate the SA passed in (SA that is going to run the build) + r := fmt.Sprintf("projects/%s/serviceAccounts/%s", projectID, strings.TrimPrefix(resolvedSA, "serviceAccount:")) + _, err = iamClient.AddServiceAccountRoleBinding(ctx, r, "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, r, err) + } + return resolvedSA, nil } diff --git a/cicd-mcp-server/cloudbuild/cloudbuild_test.go b/cicd-mcp-server/cloudbuild/cloudbuild_test.go index 0bdcfb6..bbdac22 100644 --- a/cicd-mcp-server/cloudbuild/cloudbuild_test.go +++ b/cicd-mcp-server/cloudbuild/cloudbuild_test.go @@ -42,14 +42,13 @@ func TestSetPermissionsForCloudBuildSA(t *testing.T) { 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) + dcP4sa := fmt.Sprintf("serviceAccount:service-%d@gcp-sa-devconnect.iam.gserviceaccount.com", projectNumber) gcbP4sa := fmt.Sprintf("serviceAccount:service-%d@gcp-sa-cloudbuild.iam.gserviceaccount.com", projectNumber) gcbP4saRoles := []string{ "roles/cloudbuild.serviceAgent", @@ -57,6 +56,7 @@ func TestSetPermissionsForCloudBuildSA(t *testing.T) { } defaultComputeSA := fmt.Sprintf("%d-compute@developer.gserviceaccount.com", projectNumber) saResource := fmt.Sprintf("projects/%s/serviceAccounts/%s", projectID, defaultComputeSA) + userSAResource := fmt.Sprintf("projects/%s/serviceAccounts/%s", projectID, "test-sa@example.com") t.Run("with service account", func(t *testing.T) { for _, r := range gcbSARoles { @@ -67,7 +67,8 @@ func TestSetPermissionsForCloudBuildSA(t *testing.T) { 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) + mockIAMClient.EXPECT().AddServiceAccountRoleBinding(ctx, saResource, "roles/iam.serviceAccountUser", gcbP4sa).Return(nil, nil) + mockIAMClient.EXPECT().AddServiceAccountRoleBinding(ctx, userSAResource, "roles/iam.serviceAccountUser", gcbP4sa).Return(nil, nil) resolvedSA, err := setPermissionsForCloudBuildSA(ctx, projectID, serviceAccount, mockRMClient, mockIAMClient) assert.NoError(t, err) @@ -83,7 +84,8 @@ func TestSetPermissionsForCloudBuildSA(t *testing.T) { 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) + mockIAMClient.EXPECT().AddServiceAccountRoleBinding(ctx, saResource, "roles/iam.serviceAccountUser", gcbP4sa).Return(nil, nil) + mockIAMClient.EXPECT().AddServiceAccountRoleBinding(ctx, userSAResource, "roles/iam.serviceAccountUser", gcbP4sa).Return(nil, nil) resolvedSA, err := setPermissionsForCloudBuildSA(ctx, projectID, serviceAccountWOPrefix, mockRMClient, mockIAMClient) assert.NoError(t, err) @@ -118,3 +120,27 @@ func TestSetPermissionsForCloudBuildSA(t *testing.T) { assert.Error(t, err) }) } + +func TestIsValidServiceAccount(t *testing.T) { + tests := []struct { + name string + sa string + want bool + }{ + {"valid sa", "serviceAccount:test-sa@project.iam.gserviceaccount.com", true}, + {"valid sa with dashes", "serviceAccount:my-sa-123@my-project-456.iam.gserviceaccount.com", true}, + {"missing prefix", "test-sa@project.iam.gserviceaccount.com", false}, + {"wrong prefix", "user:test-sa@project.iam.gserviceaccount.com", false}, + {"missing domain", "serviceAccount:test-sa@project", false}, + {"wrong domain", "serviceAccount:test-sa@project.com", false}, + {"empty string", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsValidServiceAccount(tt.sa); got != tt.want { + t.Errorf("IsValidServiceAccount() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/cicd-mcp-server/iam/client/iamclient.go b/cicd-mcp-server/iam/client/iamclient.go index a8e24b7..9681ba3 100644 --- a/cicd-mcp-server/iam/client/iamclient.go +++ b/cicd-mcp-server/iam/client/iamclient.go @@ -35,10 +35,11 @@ type RoleBindingList struct { // Client is an interface for interacting with the IAM API. type IAMClient interface { - CreateServiceAccount(ctx context.Context, projectID, displayName, accountID string) (*iamv1.ServiceAccount, error) - AddIAMRoleBinding(ctx context.Context, resourceID, role, member string) (*cloudresourcemanagerv1.Policy, error) - ListServiceAccounts(ctx context.Context, projectID string) (*ServiceAccountList, error) - GetIAMRoleBinding(ctx context.Context, projectID, serviceAccountEmail string) (*RoleBindingList, error) + CreateServiceAccount(ctx context.Context, projectID, displayName, accountID string) (*iamv1.ServiceAccount, error) + AddIAMRoleBinding(ctx context.Context, resourceID, role, member string) (*cloudresourcemanagerv1.Policy, error) // Project-level + AddServiceAccountRoleBinding(ctx context.Context, resourceID, role, member string) (*iamv1.Policy, error) // SA-level + ListServiceAccounts(ctx context.Context, projectID string) (*ServiceAccountList, error) + GetIAMRoleBinding(ctx context.Context, projectID, serviceAccountEmail string) (*RoleBindingList, error) } // clientImpl is a client for interacting with the IAM API. @@ -148,3 +149,46 @@ func (c *IAMClientImpl) GetIAMRoleBinding(ctx context.Context, projectID, servic return &RoleBindingList{Items: roles}, nil } + +// AddServiceAccountRoleBinding adds a member to a role on a specific SA and returns the updated Policy. +func (c *IAMClientImpl) AddServiceAccountRoleBinding(ctx context.Context, resourceID, role, member string) (*iamv1.Policy, error) { + // 1. Get the current policy (includes the ETag) + policy, err := c.iamService.Projects.ServiceAccounts.GetIamPolicy(resourceID).Context(ctx).Do() + if err != nil { + return nil, fmt.Errorf("failed to get iam policy: %w", err) + } + + // 2. Modify the policy + updated := false + for _, binding := range policy.Bindings { + if binding.Role == role { + for _, m := range binding.Members { + if m == member { + return policy, nil // Already exists, exit early to save an API call + } + } + binding.Members = append(binding.Members, member) + updated = true + break + } + } + + if !updated { + policy.Bindings = append(policy.Bindings, &iamv1.Binding{ + Role: role, + Members: []string{member}, + }) + } + + // 3. Set the policy (The ETag in 'policy' ensures we don't overwrite concurrent changes) + request := &iamv1.SetIamPolicyRequest{ + Policy: policy, + } + + updatedPolicy, err := c.iamService.Projects.ServiceAccounts.SetIamPolicy(resourceID, request).Context(ctx).Do() + if err != nil { + return nil, fmt.Errorf("failed to set iam policy: %w", err) + } + + return updatedPolicy, nil +} \ No newline at end of file diff --git a/cicd-mcp-server/iam/client/mocks/mock_iamclient.go b/cicd-mcp-server/iam/client/mocks/mock_iamclient.go index 08d0339..eb06d24 100644 --- a/cicd-mcp-server/iam/client/mocks/mock_iamclient.go +++ b/cicd-mcp-server/iam/client/mocks/mock_iamclient.go @@ -52,6 +52,21 @@ func (mr *MockIAMClientMockRecorder) AddIAMRoleBinding(ctx, resourceID, role, me return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddIAMRoleBinding", reflect.TypeOf((*MockIAMClient)(nil).AddIAMRoleBinding), ctx, resourceID, role, member) } +// AddServiceAccountRoleBinding mocks base method. +func (m *MockIAMClient) AddServiceAccountRoleBinding(ctx context.Context, resourceID, role, member string) (*iam.Policy, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddServiceAccountRoleBinding", ctx, resourceID, role, member) + ret0, _ := ret[0].(*iam.Policy) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// AddServiceAccountRoleBinding indicates an expected call of AddServiceAccountRoleBinding. +func (mr *MockIAMClientMockRecorder) AddServiceAccountRoleBinding(ctx, resourceID, role, member interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddServiceAccountRoleBinding", reflect.TypeOf((*MockIAMClient)(nil).AddServiceAccountRoleBinding), ctx, resourceID, role, member) +} + // CreateServiceAccount mocks base method. func (m *MockIAMClient) CreateServiceAccount(ctx context.Context, projectID, displayName, accountID string) (*iam.ServiceAccount, error) { m.ctrl.T.Helper() diff --git a/skills/google-cicd-pipeline-design/references/how_to_create_cloudbuild_trigger.md b/skills/google-cicd-pipeline-design/references/how_to_create_cloudbuild_trigger.md index f19f491..8341547 100644 --- a/skills/google-cicd-pipeline-design/references/how_to_create_cloudbuild_trigger.md +++ b/skills/google-cicd-pipeline-design/references/how_to_create_cloudbuild_trigger.md @@ -44,10 +44,8 @@ Cloud Build triggers connect to source code via Developer Connect. The entire co 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@.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. - +1. **Create Service Account**: Create one using `gcloud iam service-accounts create`. +2. **Required Roles (Ensured Automatically)**: Using the `create_build_trigger` MCP tool will **automatically ensure** the necessary roles are granted to the given service account. You **do not** need to grant these permissions manually. --- ## Final Step: Creating the Trigger