From 1272e721b8d5185634840c9aa9fc7698dd7cb124 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 30 Sep 2026 00:59:08 +0200 Subject: [PATCH] fix(gcp): grant only compute reads and commitment purchases Replace the setup wizard's Compute Admin grant with Compute Viewer and an exact project custom role for compute.commitments.create. Preserve conditional access and fail rather than silently widening existing grants. Cover the real SDK policy requests and wizard failure exits with local HTTP fixtures. Existing broad grants require separate operator review. --- cmd/configure_gcp.go | 86 +++++++++--- cmd/configure_gcp_iam_test.go | 243 ++++++++++++++++++++++++++++++++++ cmd/configure_gcp_test.go | 12 +- docs/cli/cloud-setup.md | 16 ++- 4 files changed, 334 insertions(+), 23 deletions(-) create mode 100644 cmd/configure_gcp_iam_test.go diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 9264083d1..ac9616e59 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -14,6 +14,7 @@ import ( "os/signal" "path/filepath" "regexp" + "slices" "strings" "syscall" "time" @@ -24,6 +25,7 @@ import ( "github.com/spf13/cobra" "golang.org/x/oauth2/google" "google.golang.org/api/cloudresourcemanager/v1" + "google.golang.org/api/googleapi" iamv1 "google.golang.org/api/iam/v1" "google.golang.org/api/option" ) @@ -445,7 +447,11 @@ func grantGCPIAMRole(ctx context.Context, projectID, member, role string) error return fmt.Errorf("failed to get IAM policy for project %s: %w", projectID, err) } - if !addMemberToPolicyBinding(policy, member, role) { + changed, err := addMemberToPolicyBinding(policy, member, role) + if err != nil { + return err + } + if !changed { // Member already bound to the role; nothing to write. return nil } @@ -463,27 +469,38 @@ func grantGCPIAMRole(ctx context.Context, projectID, member, role string) error return nil } -// addMemberToPolicyBinding adds member to the binding for role in policy, -// creating the binding if absent. It returns false if member is already bound -// (no change needed) and true if the policy was modified. -func addMemberToPolicyBinding(policy *cloudresourcemanager.Policy, member, role string) bool { +// Existing conditions must not be silently widened or treated as unconditional grants. +func addMemberToPolicyBinding(policy *cloudresourcemanager.Policy, member, role string) (bool, error) { + conditionalMember := false + var unconditional *cloudresourcemanager.Binding for _, b := range policy.Bindings { if b.Role != role { continue } for _, m := range b.Members { if m == member { - return false + if b.Condition == nil { + return false, nil + } + conditionalMember = true } } - b.Members = append(b.Members, member) - return true + if b.Condition == nil { + unconditional = b + } + } + if conditionalMember { + return false, fmt.Errorf("%s has only conditional access to %s; review the existing IAM condition before granting unconditional access", member, role) + } + if unconditional != nil { + unconditional.Members = append(unconditional.Members, member) + return true, nil } policy.Bindings = append(policy.Bindings, &cloudresourcemanager.Binding{ Role: role, Members: []string{member}, }) - return true + return true, nil } // gcpKeyProvisioner abstracts the IAM service-account key operations used by @@ -763,19 +780,48 @@ func gcpStepCreateServiceAccount(ctx context.Context, reader *bufio.Reader, proj return saEmail, nil } -// gcpStepGrantRole grants the compute.admin role to the service account via -// the Cloud Resource Manager SDK. It fails loud on any SDK error (no CLI -// fallback). +const gcpPurchaserRoleID = "cudlyCommitmentPurchaser" + +func ensureGCPPurchaserRole(ctx context.Context, projectID string) (string, error) { + ctx, cancel := context.WithTimeout(ctx, gcpSDKCallTimeout) + defer cancel() + opt, err := newGCPAPIOption(ctx) + if err != nil { + return "", err + } + svc, err := iamv1.NewService(ctx, opt) + if err != nil { + return "", fmt.Errorf("failed to create IAM client: %w", err) + } + parent := "projects/" + projectID + name := parent + "/roles/" + gcpPurchaserRoleID + role, err := svc.Projects.Roles.Get(name).Context(ctx).Do() + var apiErr *googleapi.Error + if errors.As(err, &apiErr) && apiErr.Code == 404 { + role, err = svc.Projects.Roles.Create(parent, &iamv1.CreateRoleRequest{ + RoleId: gcpPurchaserRoleID, + Role: &iamv1.Role{Title: "CUDly Commitment Purchaser", Stage: "GA", + IncludedPermissions: []string{"compute.commitments.create"}}, + }).Context(ctx).Do() + } + if err != nil { + return "", fmt.Errorf("failed to provision custom role %s; review the role and setup permissions before retrying: %w", name, err) + } + if role.Name != name || role.Deleted || role.Stage == "DISABLED" || !slices.Equal(role.IncludedPermissions, []string{"compute.commitments.create"}) { + return "", fmt.Errorf("unsafe custom role %s: expected an enabled role containing only compute.commitments.create; review it before retrying", name) + } + return name, nil +} + func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEmail string) error { member := fmt.Sprintf("serviceAccount:%s", saEmail) - role := "roles/compute.admin" fmt.Println() fmt.Println("Step 4: Grant IAM Roles") fmt.Println("-----------------------") fmt.Println("Grant the required roles to the service account.") fmt.Println() - fmt.Printf("[R]un, [S]kip? (grants %s to %s on project %s via SDK) ", role, saEmail, projectID) + fmt.Printf("[R]un, [S]kip? (creates or validates %s with compute.commitments.create, then grants it and roles/compute.viewer to %s on project %s via SDK) ", gcpPurchaserRoleID, saEmail, projectID) choice, err := reader.ReadString('\n') if err != nil { @@ -783,10 +829,16 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm } switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - if grantErr := grantGCPIAMRole(ctx, projectID, member, role); grantErr != nil { - return grantErr + role, roleErr := ensureGCPPurchaserRole(ctx, projectID) + if roleErr != nil { + return roleErr + } + for _, grant := range []string{"roles/compute.viewer", role} { + if grantErr := grantGCPIAMRole(ctx, projectID, member, grant); grantErr != nil { + return grantErr + } + fmt.Printf("Role %s granted to %s on project %s.\n", grant, saEmail, projectID) } - fmt.Printf("Role %s granted to %s on project %s.\n", role, saEmail, projectID) case "s", "skip": fmt.Println("Skipping Grant IAM Roles") default: diff --git a/cmd/configure_gcp_iam_test.go b/cmd/configure_gcp_iam_test.go new file mode 100644 index 000000000..01dedf145 --- /dev/null +++ b/cmd/configure_gcp_iam_test.go @@ -0,0 +1,243 @@ +package main + +import ( + "bufio" + "context" + "encoding/json" + "io" + "net" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + crm "google.golang.org/api/cloudresourcemanager/v1" + iam "google.golang.org/api/iam/v1" +) + +func TestGCPStepGrantRole(t *testing.T) { + if scenario := os.Getenv("CUDLY_IAM_TEST_CHILD"); scenario != "" { + runGCPIAMFixture(t, scenario) + return + } + for _, scenario := range []string{"create", "empty", "reuse", "extra-permission", "deleted", "disabled", "wrong-name", "bad-created", "forbidden", "write-error", "skip", "conditional-other", "conditional-member", "already-unconditional", "existing-admin", "coordinator", "coordinator-error", "coordinator-conflict", "coordinator-second-write"} { + t.Run(scenario, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + child := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestGCPStepGrantRole$") + child.Env = append(os.Environ(), "CUDLY_IAM_TEST_CHILD="+scenario) + output, err := child.CombinedOutput() + require.NoError(t, err, "%s", output) + }) + } +} + +func runGCPIAMFixture(t *testing.T, scenario string) { + t.Helper() + const roleName = "projects/fixture-project/roles/cudlyCommitmentPurchaser" + const member = "serviceAccount:cudly-service-account@fixture-project.iam.gserviceaccount.com" + role := &iam.Role{Name: roleName, Stage: "GA", IncludedPermissions: []string{"compute.commitments.create"}} + wantError := "" + switch scenario { + case "extra-permission", "bad-created": + role.IncludedPermissions = append(role.IncludedPermissions, "compute.instances.delete") + wantError = "unsafe custom role" + case "deleted": + role.Deleted = true + wantError = "unsafe custom role" + case "disabled": + role.Stage = "DISABLED" + wantError = "unsafe custom role" + case "wrong-name": + role.Name = "projects/fixture-project/roles/other" + wantError = "unsafe custom role" + case "forbidden", "coordinator-error": + wantError = "403" + case "write-error", "coordinator-second-write": + wantError = "failed to set IAM policy" + case "coordinator-conflict": + wantError = "409" + case "conditional-member": + wantError = "conditional" + } + conditional := &crm.Binding{Role: "roles/compute.viewer", Members: []string{"user:other@example.com"}, Condition: &crm.Expr{Title: "restricted", Expression: "request.time < timestamp('2030-01-01T00:00:00Z')"}} + if scenario == "conditional-member" || scenario == "already-unconditional" { + conditional.Members = []string{member} + } + policy := &crm.Policy{Version: 3, Etag: "fixture-etag", Bindings: []*crm.Binding{conditional}} + if scenario == "already-unconditional" { + policy.Bindings = append(policy.Bindings, &crm.Binding{Role: "roles/compute.viewer", Members: []string{member}}) + } + if scenario == "existing-admin" { + policy.Bindings = append(policy.Bindings, &crm.Binding{Role: "roles/compute.admin", Members: []string{member, "user:admin@example.com"}}) + } + var writes []*crm.SetIamPolicyRequest + var creates []*iam.CreateRoleRequest + var roleReads, keyCalls int + var mu sync.Mutex + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + defer mu.Unlock() + w.Header().Set("Content-Type", "application/json") + switch { + case r.URL.Path == "/token": + _, _ = io.WriteString(w, `{"access_token":"fixture","token_type":"Bearer","expires_in":3600}`) + case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/roles/cudlyCommitmentPurchaser"): + roleReads++ + switch scenario { + case "create", "empty", "bad-created", "coordinator", "coordinator-conflict": + http.Error(w, `{"error":{"code":404,"message":"missing"}}`, http.StatusNotFound) + case "forbidden", "coordinator-error": + http.Error(w, `{"error":{"code":403,"message":"denied"}}`, http.StatusForbidden) + default: + _ = json.NewEncoder(w).Encode(role) + } + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/roles"): + var request iam.CreateRoleRequest + require.NoError(t, json.NewDecoder(r.Body).Decode(&request)) + creates = append(creates, &request) + if scenario == "coordinator-conflict" { + http.Error(w, "conflict", http.StatusConflict) + return + } + _ = json.NewEncoder(w).Encode(role) + case strings.HasSuffix(r.URL.Path, ":getIamPolicy"): + var request crm.GetIamPolicyRequest + require.NoError(t, json.NewDecoder(r.Body).Decode(&request)) + require.EqualValues(t, 3, request.Options.RequestedPolicyVersion) + _ = json.NewEncoder(w).Encode(policy) + case strings.HasSuffix(r.URL.Path, ":setIamPolicy"): + var request crm.SetIamPolicyRequest + require.NoError(t, json.NewDecoder(r.Body).Decode(&request)) + writes = append(writes, &request) + if scenario == "write-error" || (scenario == "coordinator-second-write" && len(writes) == 2) { + http.Error(w, "denied", http.StatusForbidden) + return + } + policy = request.Policy + _ = json.NewEncoder(w).Encode(policy) + default: + keyCalls++ + http.Error(w, "unexpected fixture request", http.StatusBadRequest) + } + })) + defer server.Close() + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.Proxy = nil + transport.DialTLSContext = func(ctx context.Context, _, _ string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, "tcp", strings.TrimPrefix(server.URL, "http://")) + } + http.DefaultTransport = transport + t.Cleanup(transport.CloseIdleConnections) + dir := t.TempDir() + adc := filepath.Join(dir, "adc.json") + require.NoError(t, os.WriteFile(adc, []byte(`{"type":"authorized_user","client_id":"fixture","client_secret":"fixture","refresh_token":"fixture"}`), 0600)) + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", adc) + input := "r\n" + if scenario == "empty" { + input = "\n" + } + if scenario == "skip" { + input = "s\n" + } + var err error + if strings.HasPrefix(scenario, "coordinator") { + require.NoError(t, os.WriteFile(filepath.Join(dir, "gcloud"), []byte("#!/bin/sh\n[ \"$*\" = \"config set project fixture-project\" ]\n"), 0700)) + t.Setenv("PATH", dir) + t.Setenv("AWS_ACCESS_KEY_ID", "fixture") + t.Setenv("AWS_SECRET_ACCESS_KEY", "fixture") + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_REGION", "us-east-1") + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + stdin, writeErr := os.Create(filepath.Join(dir, "stdin")) + require.NoError(t, writeErr) + defer stdin.Close() + _, writeErr = io.WriteString(stdin, "s\ns\ns\nfixture-project\ns\nr\n") + require.NoError(t, writeErr) + _, writeErr = stdin.Seek(0, io.SeekStart) + require.NoError(t, writeErr) + os.Stdin = stdin + err = runConfigureGCP(nil, nil) + if scenario == "coordinator" { + wantError = "failed to read create-key choice" + } + } else { + err = gcpStepGrantRole(context.Background(), bufio.NewReader(strings.NewReader(input)), "fixture-project", strings.TrimPrefix(member, "serviceAccount:")) + } + mu.Lock() + defer mu.Unlock() + for _, write := range writes { + for _, binding := range write.Policy.Bindings { + if scenario == "existing-admin" && binding.Role == "roles/compute.admin" { + require.Equal(t, []string{member, "user:admin@example.com"}, binding.Members) + continue + } + require.NotEqual(t, "roles/compute.admin", binding.Role, "wizard must not introduce a Compute Admin grant") + } + require.Equal(t, "fixture-etag", write.Policy.Etag) + require.Equal(t, conditional, write.Policy.Bindings[0]) + } + if wantError != "" { + require.ErrorContains(t, err, wantError) + } else { + require.NoError(t, err) + } + require.Zero(t, keyCalls, "role setup must never mint a key or call unexpected endpoints") + wantCreates := 0 + switch scenario { + case "create", "empty", "bad-created", "coordinator", "coordinator-conflict": + wantCreates = 1 + } + require.Len(t, creates, wantCreates) + if scenario == "skip" { + require.Zero(t, roleReads) + require.Empty(t, writes) + return + } + require.Equal(t, 1, roleReads) + for _, create := range creates { + require.Equal(t, "cudlyCommitmentPurchaser", create.RoleId) + require.Empty(t, create.Role.Name) + require.Equal(t, []string{"compute.commitments.create"}, create.Role.IncludedPermissions) + } + if scenario == "coordinator-second-write" { + require.Len(t, writes, 2) + require.Len(t, policy.Bindings, 2, "failed second write must leave only the successful viewer grant") + require.Equal(t, "roles/compute.viewer", policy.Bindings[1].Role) + return + } + if wantError != "" && scenario != "coordinator" && scenario != "write-error" { + require.Empty(t, writes) + return + } + if scenario == "write-error" { + require.Len(t, writes, 1) + return + } + wantWrites := 2 + if scenario == "already-unconditional" { + wantWrites = 1 + } + require.Len(t, writes, wantWrites) + granted := map[string]bool{} + for _, binding := range policy.Bindings { + if binding.Condition == nil { + for _, m := range binding.Members { + if m == member { + granted[binding.Role] = true + } + } + } + } + expected := map[string]bool{"roles/compute.viewer": true, roleName: true} + if scenario == "existing-admin" { + expected["roles/compute.admin"] = true + } + require.Equal(t, expected, granted) +} diff --git a/cmd/configure_gcp_test.go b/cmd/configure_gcp_test.go index f50939fbe..1e8f7ca48 100644 --- a/cmd/configure_gcp_test.go +++ b/cmd/configure_gcp_test.go @@ -25,8 +25,9 @@ func TestAddMemberToPolicyBinding_AppendsToExistingBinding(t *testing.T) { }, } - changed := addMemberToPolicyBinding(policy, "serviceAccount:sa@proj.iam.gserviceaccount.com", "roles/viewer") + changed, err := addMemberToPolicyBinding(policy, "serviceAccount:sa@proj.iam.gserviceaccount.com", "roles/viewer") + require.NoError(t, err) require.True(t, changed, "adding a new member to an existing role binding must report a change") require.Len(t, policy.Bindings, 1) assert.Equal(t, []string{ @@ -45,8 +46,9 @@ func TestAddMemberToPolicyBinding_AlreadyBoundNoChange(t *testing.T) { }, } - changed := addMemberToPolicyBinding(policy, member, "roles/viewer") + changed, err := addMemberToPolicyBinding(policy, member, "roles/viewer") + require.NoError(t, err) require.False(t, changed, "re-adding a member already bound to the role must report no change") require.Len(t, policy.Bindings, 1) assert.Equal(t, []string{member}, policy.Bindings[0].Members, @@ -63,8 +65,9 @@ func TestAddMemberToPolicyBinding_CreatesMissingBinding(t *testing.T) { } member := "serviceAccount:sa@proj.iam.gserviceaccount.com" - changed := addMemberToPolicyBinding(policy, member, "roles/billing.projectManager") + changed, err := addMemberToPolicyBinding(policy, member, "roles/billing.projectManager") + require.NoError(t, err) require.True(t, changed, "adding a member to an absent role must create the binding and report a change") require.Len(t, policy.Bindings, 2) newBinding := policy.Bindings[1] @@ -94,7 +97,8 @@ func TestAddMemberToPolicyBinding_PreservesConditionalBindings(t *testing.T) { } member := "serviceAccount:sa@proj.iam.gserviceaccount.com" - changed := addMemberToPolicyBinding(policy, member, "roles/viewer") + changed, err := addMemberToPolicyBinding(policy, member, "roles/viewer") + require.NoError(t, err) require.True(t, changed) // The conditional binding must still be present, unchanged. diff --git a/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index 2d657912a..3c26212c1 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -107,7 +107,7 @@ When `--skip-setup` is not set, the command runs an interactive guided flow: 2. **`gcloud projects list`** - lists projects so you can identify your Project ID. 3. **`gcloud config set project `** - sets the active project. 4. **Creates a `cudly-service-account` Service Account** with the display name "CUDly Service Account". -5. **`gcloud projects add-iam-policy-binding`** - grants `roles/compute.admin` to the new Service Account. +5. **Grant IAM roles via SDK** - creates or validates the project custom role `cudlyCommitmentPurchaser`, then grants it and `roles/compute.viewer` to the Service Account. 6. **`gcloud iam service-accounts keys create ~/cudly-gcp-key.json`** - downloads a JSON key to your home directory. After the guided steps (or with `--skip-setup --credentials-file `), the command reads and validates the JSON file and writes it to the `-GCPCredentials` secret. @@ -136,7 +136,19 @@ The Service Account needs the following roles: | Role | Purpose | |------|---------| -| `roles/compute.admin` | Manage Compute Engine Committed Use Discounts | +| `roles/compute.viewer` | Read Compute Engine resources and commitment operations | +| `projects/PROJECT_ID/roles/cudlyCommitmentPurchaser` | Purchase commitments with only `compute.commitments.create` | + +The setup operator needs `iam.roles.get`, `iam.roles.create`, +`resourcemanager.projects.getIamPolicy`, and `resourcemanager.projects.setIamPolicy` +for this step. These setup permissions are not granted to the Service Account. +An existing custom role must have exactly the purchase permission and be enabled; +the wizard refuses incompatible roles rather than changing them. It also refuses +to widen an existing conditional-only grant for this Service Account. + +Rerunning the wizard does not remove broad grants from older installations. +Review existing `roles/compute.admin` grants separately. Recommendation access +requires additional Recommender permissions; neither role above provides them. If you manage Cloud SQL or Memorystore commitments, you may need additional roles. Check the GCP documentation for the minimum required permissions per commitment type.