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.