From 67b45cbf18994a6062d08f007766d4a64c2988aa Mon Sep 17 00:00:00 2001 From: Greg Allen Date: Tue, 4 Aug 2026 17:53:39 -0400 Subject: [PATCH] fix(#5912): GitLab WIF install/uninstall cleanup and edge cases Signed-off-by: Claude Opus 4.6 Signed-off-by: Greg Allen Co-Authored-By: Claude Opus 4.6 Signed-off-by: Greg Allen Co-Authored-By: Claude Opus 4.6 Signed-off-by: Greg Allen Co-Authored-By: Claude Opus 4.6 Signed-off-by: Greg Allen Co-Authored-By: Claude Opus 4.6 Signed-off-by: Greg Allen Co-Authored-By: Claude Opus 4.6 --- docs/cli/repos.md | 2 +- docs/guides/getting-started/operations.md | 1 + internal/cli/repos.go | 61 +++++- internal/cli/repos_gitlab.go | 90 +++++++- internal/cli/repos_gitlab_test.go | 237 +++++++++++++++++++++- internal/cli/repos_test.go | 36 ++++ internal/dispatch/gcf/fakeclient.go | 14 ++ internal/dispatch/gcf/fakeclient_test.go | 2 + internal/dispatch/gcf/gcp.go | 50 ++++- internal/dispatch/gcf/gcp_test.go | 56 +++++ 10 files changed, 519 insertions(+), 30 deletions(-) diff --git a/docs/cli/repos.md b/docs/cli/repos.md index 3899aa903e..e2c9f072cf 100644 --- a/docs/cli/repos.md +++ b/docs/cli/repos.md @@ -224,7 +224,7 @@ Requires a GitHub token via `GH_TOKEN`, `GITHUB_TOKEN`, or `gh auth token`. For Tear down fullsend from the specified repos and remove them from the manifest. By default, the command tears down first (deleting workflow files, variables, and secrets), then removes successfully-torn-down repos from the manifest. Partial failures leave the manifest entry intact so the user can retry. -GCP WIF cleanup is handled separately via `inference deprovision`. +GCP WIF pool/provider cleanup is handled separately via `inference deprovision`. For GitLab WIF-mode repos, `repos uninstall` performs best-effort deletion of the bot token Secret Manager secret. When multiple repos are targeted (via globs or explicit bulk lists), the command prompts for confirmation unless `--yes` is set. diff --git a/docs/guides/getting-started/operations.md b/docs/guides/getting-started/operations.md index dc7f9ae3f3..2745c83b8d 100644 --- a/docs/guides/getting-started/operations.md +++ b/docs/guides/getting-started/operations.md @@ -82,6 +82,7 @@ To remove fullsend from a single repository: 2. Delete all CI/CD variables prefixed with `FULLSEND_` 3. Revoke the `fullsend-bot` project access token (Settings → Access Tokens) 4. Delete fullsend pipeline schedules +5. For WIF-mode repos: delete the bot token Secret Manager secret (named `fullsend-bot-token---`) from the GCP project If you manage your own self-hosted mint, run `fullsend mint unenroll "$OWNER/$REPO"` instead of GitHub step 3. See the [standalone commands](#standalone-commands) table for details. diff --git a/internal/cli/repos.go b/internal/cli/repos.go index 43c99e9084..c7cc903710 100644 --- a/internal/cli/repos.go +++ b/internal/cli/repos.go @@ -886,7 +886,8 @@ type reposUninstallConfig struct { uninstallOnly bool gitlabToken string - testClient forge.Client + testClient forge.Client + testGCPClientFactory func(projectID string) gcf.GCFClient } func newReposUninstallCmd() *cobra.Command { @@ -1003,6 +1004,36 @@ func runReposUninstall(ctx context.Context, opts *reposUninstallConfig, repoArgs return err } + // Pre-uninstall: gather GCP project IDs for GitLab WIF repos + // so we can delete Secret Manager secrets after teardown. The + // FULLSEND_SA variable is deleted during uninstall, so we read + // it now. + gcpProjectByRepo := make(map[string]string) + if !opts.dryRun { + for _, repoName := range concreteRepos { + parts := strings.SplitN(repoName, "/", 2) + if len(parts) != 2 { + continue + } + owner, repo := parts[0], parts[1] + rc, ok := manifest.ResolveConfigWithGlobs(owner, repo) + if !ok || rc.Forge != repos.ForgeGitLab { + continue + } + fc, fcErr := clients.ConfigFor(repos.ForgeGitLab) + if fcErr != nil { + continue + } + sa, found, readErr := fc.Client.GetRepoVariable(ctx, owner, repo, "FULLSEND_SA") + if readErr != nil || !found { + continue + } + if projectID := projectIDFromSAEmail(sa); projectID != "" { + gcpProjectByRepo[repoName] = projectID + } + } + } + teardownCfg := repos.UninstallConfig{ Manifest: manifest, Repos: concreteRepos, @@ -1031,11 +1062,14 @@ func runReposUninstall(ctx context.Context, opts *reposUninstallConfig, repoArgs } } - // GitLab post-uninstall: clean up pipeline schedules and bot tokens. - // Note: if the CLI's --gitlab-token lacks permission to list/revoke - // project access tokens, the bot token will be orphaned. The user - // must manually revoke it via Settings → Access Tokens. + // GitLab post-uninstall: clean up pipeline schedules, bot tokens, + // and Secret Manager secrets. if !opts.dryRun { + newGCPClient := opts.testGCPClientFactory + if newGCPClient == nil { + newGCPClient = func(pid string) gcf.GCFClient { return gcf.NewLiveGCFClient(pid) } + } + for _, r := range results { if !r.Success { continue @@ -1053,15 +1087,20 @@ func runReposUninstall(ctx context.Context, opts *reposUninstallConfig, repoArgs printer.StepWarn(fmt.Sprintf("[%s] Could not get GitLab client: %v", repoFullName, fcErr)) continue } - glClient, ok := fc.Client.(*gl.LiveClient) - if !ok { + _ = cleanupGitLabPipelineSchedules(ctx, fc.Client, printer, r.Owner, r.Repo) + + if glClient, ok := fc.Client.(*gl.LiveClient); ok { + _ = cleanupGitLabBotToken(ctx, glClient, printer, r.Owner, r.Repo) + } else { printer.StepWarn(fmt.Sprintf("[%s] GitLab client type assertion failed — bot token cleanup skipped", repoFullName)) - _ = cleanupGitLabPipelineSchedules(ctx, fc.Client, printer, r.Owner, r.Repo) - continue } - _ = cleanupGitLabPipelineSchedules(ctx, fc.Client, printer, r.Owner, r.Repo) - _ = cleanupGitLabBotToken(ctx, glClient, printer, r.Owner, r.Repo) + // Best-effort: delete the bot token Secret Manager + // secret if we know the GCP project from the pre- + // uninstall variable read. + if projectID, ok := gcpProjectByRepo[repoFullName]; ok { + cleanupGitLabBotTokenSecret(ctx, newGCPClient(projectID), printer, projectID, r.Owner, r.Repo) + } } } } else { diff --git a/internal/cli/repos_gitlab.go b/internal/cli/repos_gitlab.go index 11aa6d99aa..a4acd08a11 100644 --- a/internal/cli/repos_gitlab.go +++ b/internal/cli/repos_gitlab.go @@ -36,10 +36,13 @@ var secretIDSanitizer = regexp.MustCompile(`[^a-zA-Z0-9_\-]`) const secretIDMaxLen = 255 // botTokenSecretID returns the Secret Manager secret ID for a repo's bot token. -// Slashes in GitLab subgroup paths are mapped to double underscores so that -// "group/sub" and "group-sub" produce distinct IDs. +// Slashes in GitLab subgroup paths are mapped to double underscores and dots +// are mapped to "_dot_" so that "group/sub", "group-sub", "my.group", and +// "my-group" all produce distinct IDs. Note: a literal "_dot_" in a name would +// collide with a dot-mapped name; this is accepted as extremely unlikely. func botTokenSecretID(owner, repo string) (string, error) { combined := strings.ReplaceAll(owner, "/", "__") + "--" + repo + combined = strings.ReplaceAll(combined, ".", "_dot_") sanitized := secretIDSanitizer.ReplaceAllString(combined, "-") id := "fullsend-bot-token-" + sanitized if len(id) > secretIDMaxLen { @@ -48,6 +51,15 @@ func botTokenSecretID(owner, repo string) (string, error) { return id, nil } +// legacyBotTokenSecretID returns the pre-_dot_ secret ID for migration. +// Before the _dot_ mapping was added, dots were mapped to hyphens by the +// sanitizer. This is used during cleanup to delete secrets created under +// the old naming scheme. +func legacyBotTokenSecretID(owner, repo string) string { + combined := strings.ReplaceAll(owner, "/", "__") + "--" + repo + return "fullsend-bot-token-" + secretIDSanitizer.ReplaceAllString(combined, "-") +} + // setupGitLabBotToken creates a project access token for the fullsend bot // identity and stores it appropriately based on the credential mode. // @@ -66,6 +78,7 @@ func botTokenSecretID(owner, repo string) (string, error) { func setupGitLabBotToken(ctx context.Context, client forge.Client, glClient *gitlab.LiveClient, printer *ui.Printer, owner, repo, fallbackToken string, wifCfg *botTokenWIFConfig) (string, error) { printer.StepStart("Creating project access token") var botPAT string + var botTokenID int if glClient != nil { // Revoke any existing fullsend-bot tokens to avoid duplicates on re-install. existing, listErr := glClient.ListProjectAccessTokens(ctx, owner, repo) @@ -98,6 +111,7 @@ func setupGitLabBotToken(ctx context.Context, client forge.Client, glClient *git } } else { botPAT = token.Token + botTokenID = token.ID printer.StepDone(fmt.Sprintf("Created project access token %q (ID: %d)", gitlabBotTokenName, token.ID)) } } else if fallbackToken != "" { @@ -125,8 +139,17 @@ func setupGitLabBotToken(ctx context.Context, client forge.Client, glClient *git // Grant the WIF service account access to read the secret. saEmail := gcf.MintServiceAccountEmail(wifCfg.ProjectID) secretResource := fmt.Sprintf("projects/%s/secrets/%s", wifCfg.ProjectID, secretID) - if err := wifCfg.GCPClient.SetSecretIAMBinding(ctx, secretResource, + if err := wifCfg.GCPClient.ReplaceSecretIAMBinding(ctx, secretResource, "serviceAccount:"+saEmail, "roles/secretmanager.secretAccessor"); err != nil { + // Best-effort cleanup: delete the orphaned secret and revoke the PAT. + if delErr := wifCfg.GCPClient.DeleteSecret(ctx, wifCfg.ProjectID, secretID); delErr != nil { + printer.StepWarn(fmt.Sprintf("Failed to clean up secret %s: %v", secretID, delErr)) + } + if botTokenID != 0 && glClient != nil { + if revErr := glClient.RevokeProjectAccessToken(ctx, owner, repo, botTokenID); revErr != nil { + printer.StepWarn(fmt.Sprintf("Failed to revoke bot PAT (ID %d): %v", botTokenID, revErr)) + } + } printer.StepFail("Failed to grant secret access") return "", fmt.Errorf("granting secret access for %s: %w", secretID, err) } @@ -134,10 +157,27 @@ func setupGitLabBotToken(ctx context.Context, client forge.Client, glClient *git // Set FULLSEND_BOT_TOKEN_SECRET as a protected CI/CD variable // so the scaffold knows which secret to read from Secret Manager. if err := client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_BOT_TOKEN_SECRET", secretID); err != nil { + // Best-effort cleanup: delete the orphaned secret and revoke the PAT. + if delErr := wifCfg.GCPClient.DeleteSecret(ctx, wifCfg.ProjectID, secretID); delErr != nil { + printer.StepWarn(fmt.Sprintf("Failed to clean up secret %s: %v", secretID, delErr)) + } + if botTokenID != 0 && glClient != nil { + if revErr := glClient.RevokeProjectAccessToken(ctx, owner, repo, botTokenID); revErr != nil { + printer.StepWarn(fmt.Sprintf("Failed to revoke bot PAT (ID %d): %v", botTokenID, revErr)) + } + } printer.StepFail("Failed to set FULLSEND_BOT_TOKEN_SECRET") return "", fmt.Errorf("setting FULLSEND_BOT_TOKEN_SECRET: %w", err) } printer.StepDone("Bot credentials stored in Secret Manager") + + // Best-effort: delete any legacy-named secret left by + // a previous install that used dot-to-hyphen mapping. + if legacyID := legacyBotTokenSecretID(owner, repo); legacyID != secretID { + if err := wifCfg.GCPClient.DeleteSecret(ctx, wifCfg.ProjectID, legacyID); err == nil { + printer.StepDone(fmt.Sprintf("Deleted legacy secret %s", legacyID)) + } + } } else { // Variable mode: store bot PAT directly as a protected CI/CD variable. printer.StepStart("Storing bot credentials") @@ -262,6 +302,50 @@ func cleanupGitLabPipelineSchedules(ctx context.Context, client forge.Client, pr return nil } +// cleanupGitLabBotTokenSecret deletes the bot token Secret Manager secret +// and is a best-effort operation — errors are logged but not returned. +// This handles the GCP side of cleanup; the GitLab side (CI/CD variables, +// PAT revocation) is handled by the main uninstall path and +// cleanupGitLabBotToken. +// +// Tries both the current naming scheme (_dot_ for dots) and the legacy +// scheme (dots mapped to hyphens by the sanitizer) to handle secrets +// created before the _dot_ mapping was introduced. +func cleanupGitLabBotTokenSecret(ctx context.Context, gcpClient gcf.GCFClient, printer *ui.Printer, projectID, owner, repo string) { + secretID, err := botTokenSecretID(owner, repo) + if err != nil { + printer.StepWarn(fmt.Sprintf("Failed to derive secret ID for %s/%s: %v", owner, repo, err)) + return + } + if err := gcpClient.DeleteSecret(ctx, projectID, secretID); err != nil { + printer.StepWarn(fmt.Sprintf("Failed to delete Secret Manager secret %s: %v", secretID, err)) + } else { + printer.StepDone(fmt.Sprintf("Deleted Secret Manager secret %s", secretID)) + } + + legacyID := legacyBotTokenSecretID(owner, repo) + if legacyID != secretID { + if err := gcpClient.DeleteSecret(ctx, projectID, legacyID); err == nil { + printer.StepDone(fmt.Sprintf("Deleted legacy Secret Manager secret %s", legacyID)) + } + } +} + +// projectIDFromSAEmail extracts the GCP project ID from a service account +// email in the standard format: name@{projectID}.iam.gserviceaccount.com. +// Returns an empty string if the email doesn't match the expected format. +func projectIDFromSAEmail(email string) string { + parts := strings.SplitN(email, "@", 2) + if len(parts) != 2 { + return "" + } + const suffix = ".iam.gserviceaccount.com" + if !strings.HasSuffix(parts[1], suffix) { + return "" + } + return strings.TrimSuffix(parts[1], suffix) +} + // cleanupGitLabBotToken revokes any active fullsend bot project access // tokens from a GitLab project. func cleanupGitLabBotToken(ctx context.Context, glClient *gitlab.LiveClient, printer *ui.Printer, owner, repo string) error { diff --git a/internal/cli/repos_gitlab_test.go b/internal/cli/repos_gitlab_test.go index a9d03f42da..ad442d9d12 100644 --- a/internal/cli/repos_gitlab_test.go +++ b/internal/cli/repos_gitlab_test.go @@ -74,6 +74,15 @@ func (f *fakeSecretManagerClient) SetSecretIAMBinding(_ context.Context, resourc return nil } +func (f *fakeSecretManagerClient) ReplaceSecretIAMBinding(_ context.Context, resource, member, role string) error { + f.calls = append(f.calls, "ReplaceSecretIAMBinding") + if err := f.errs["ReplaceSecretIAMBinding"]; err != nil { + return err + } + f.iamBindings = append(f.iamBindings, fmt.Sprintf("%s:%s:%s", resource, member, role)) + return nil +} + // Stub methods required by GCFClient interface but unused in bot token tests. func (f *fakeSecretManagerClient) CreateServiceAccount(context.Context, string, string, string) error { return nil @@ -109,7 +118,14 @@ func (f *fakeSecretManagerClient) DisableSecretVersion(_ context.Context, _, _ s func (f *fakeSecretManagerClient) EnableSecretVersion(context.Context, string, string) error { return nil } -func (f *fakeSecretManagerClient) DeleteSecret(context.Context, string, string) error { return nil } +func (f *fakeSecretManagerClient) DeleteSecret(_ context.Context, _, sid string) error { + f.calls = append(f.calls, "DeleteSecret") + if err := f.errs["DeleteSecret"]; err != nil { + return err + } + delete(f.secrets, sid) + return nil +} func (f *fakeSecretManagerClient) SetProjectIAMBinding(context.Context, string, string, string) error { return nil } @@ -648,8 +664,8 @@ func TestSetupGitLabBotToken_WIFMode(t *testing.T) { assert.Contains(t, smClient.calls, "AddSecretVersion") assert.Equal(t, []byte("glpat-wif-token"), smClient.secretVersions[expectedSecretID]) - // Verify IAM binding was set. - assert.Contains(t, smClient.calls, "SetSecretIAMBinding") + // Verify IAM binding was set with replace semantics. + assert.Contains(t, smClient.calls, "ReplaceSecretIAMBinding") expectedBinding := fmt.Sprintf("projects/my-gcp-project/secrets/%s:serviceAccount:fullsend-mint@my-gcp-project.iam.gserviceaccount.com:roles/secretmanager.secretAccessor", expectedSecretID) require.Len(t, smClient.iamBindings, 1) assert.Equal(t, expectedBinding, smClient.iamBindings[0]) @@ -663,6 +679,39 @@ func TestSetupGitLabBotToken_WIFMode(t *testing.T) { assert.Empty(t, fake.CreatedSecrets, "WIF mode should not store FULLSEND_FORGE_TOKEN as CI/CD variable") }) + t.Run("cleans up legacy secret for dotted names on install", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v4/projects/", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "id": 1, "name": "fullsend-bot", "token": "glpat-wif-dot", "active": true, + }) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + glClient, err := gitlab.New("test-token", gitlab.WithBaseURL(srv.URL)) + require.NoError(t, err) + + fc := &forge.FakeClient{} + smClient := newFakeSecretManagerClient() + smClient.secrets["fullsend-bot-token-my-group--repo"] = true + var buf bytes.Buffer + printer := ui.New(&buf) + + wifCfg := &botTokenWIFConfig{ + GCPClient: smClient, + ProjectID: "my-gcp-project", + } + + _, err = setupGitLabBotToken(ctx, fc, glClient, printer, "my.group", "repo", "", wifCfg) + require.NoError(t, err) + + assert.NotContains(t, smClient.secrets, "fullsend-bot-token-my-group--repo", + "should delete legacy-named secret during install") + assert.Contains(t, buf.String(), "Deleted legacy secret") + }) + t.Run("Secret Manager create failure", func(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("/api/v4/projects/", func(w http.ResponseWriter, _ *http.Request) { @@ -693,7 +742,7 @@ func TestSetupGitLabBotToken_WIFMode(t *testing.T) { assert.Contains(t, err.Error(), "Secret Manager") }) - t.Run("IAM binding failure", func(t *testing.T) { + t.Run("IAM binding failure cleans up secret", func(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("/api/v4/projects/", func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -709,7 +758,7 @@ func TestSetupGitLabBotToken_WIFMode(t *testing.T) { fake := &forge.FakeClient{} smClient := newFakeSecretManagerClient() - smClient.errs["SetSecretIAMBinding"] = fmt.Errorf("iam error") + smClient.errs["ReplaceSecretIAMBinding"] = fmt.Errorf("iam error") var buf bytes.Buffer printer := ui.New(&buf) @@ -721,9 +770,12 @@ func TestSetupGitLabBotToken_WIFMode(t *testing.T) { _, err = setupGitLabBotToken(ctx, fake, glClient, printer, "group", "project", "", wifCfg) require.Error(t, err) assert.Contains(t, err.Error(), "granting secret access") + + // Verify best-effort cleanup deleted the orphaned secret. + assert.Contains(t, smClient.calls, "DeleteSecret") }) - t.Run("CreateProtectedCIVariable failure", func(t *testing.T) { + t.Run("CreateProtectedCIVariable failure cleans up secret", func(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("/api/v4/projects/", func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -751,6 +803,73 @@ func TestSetupGitLabBotToken_WIFMode(t *testing.T) { _, err = setupGitLabBotToken(ctx, fake, glClient, printer, "group", "project", "", wifCfg) require.Error(t, err) assert.Contains(t, err.Error(), "setting FULLSEND_BOT_TOKEN_SECRET") + + // Verify best-effort cleanup deleted the orphaned secret. + assert.Contains(t, smClient.calls, "DeleteSecret") + }) + + t.Run("IAM binding failure warns when cleanup also fails", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v4/projects/", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "id": 1, "name": "fullsend-bot", "token": "glpat-wif", "active": true, + }) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + glClient, err := gitlab.New("test-token", gitlab.WithBaseURL(srv.URL)) + require.NoError(t, err) + + fake := &forge.FakeClient{} + smClient := newFakeSecretManagerClient() + smClient.errs["ReplaceSecretIAMBinding"] = fmt.Errorf("iam error") + smClient.errs["DeleteSecret"] = fmt.Errorf("delete denied") + var buf bytes.Buffer + printer := ui.New(&buf) + + wifCfg := &botTokenWIFConfig{ + GCPClient: smClient, + ProjectID: "my-gcp-project", + } + + _, err = setupGitLabBotToken(ctx, fake, glClient, printer, "group", "project", "", wifCfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "granting secret access") + assert.Contains(t, buf.String(), "Failed to clean up secret") + }) + + t.Run("CreateProtectedCIVariable failure warns when cleanup also fails", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v4/projects/", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "id": 1, "name": "fullsend-bot", "token": "glpat-wif", "active": true, + }) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + glClient, err := gitlab.New("test-token", gitlab.WithBaseURL(srv.URL)) + require.NoError(t, err) + + fake := forge.NewFakeClient() + fake.Errors["CreateProtectedCIVariable"] = fmt.Errorf("variable exists") + smClient := newFakeSecretManagerClient() + smClient.errs["DeleteSecret"] = fmt.Errorf("delete denied") + var buf bytes.Buffer + printer := ui.New(&buf) + + wifCfg := &botTokenWIFConfig{ + GCPClient: smClient, + ProjectID: "my-gcp-project", + } + + _, err = setupGitLabBotToken(ctx, fake, glClient, printer, "group", "project", "", wifCfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "setting FULLSEND_BOT_TOKEN_SECRET") + assert.Contains(t, buf.String(), "Failed to clean up secret") }) } @@ -822,9 +941,10 @@ func TestBotTokenSecretID(t *testing.T) { want string }{ {"acme", "widgets", "fullsend-bot-token-acme--widgets"}, - {"my.group", "my/repo", "fullsend-bot-token-my-group--my-repo"}, + {"my.group", "my/repo", "fullsend-bot-token-my_dot_group--my-repo"}, {"group/subgroup", "repo", "fullsend-bot-token-group__subgroup--repo"}, {"org", "repo-name", "fullsend-bot-token-org--repo-name"}, + {"org", "repo.name", "fullsend-bot-token-org--repo_dot_name"}, } for _, tt := range tests { got, err := botTokenSecretID(tt.owner, tt.repo) @@ -850,4 +970,107 @@ func TestBotTokenSecretID_Collision(t *testing.T) { assert.NotEqual(t, id1, id2, "subgroup slash and hyphen should produce different secret IDs") }) + t.Run("dot vs hyphen in owner", func(t *testing.T) { + idDot, err := botTokenSecretID("my.group", "repo") + require.NoError(t, err) + idHyphen, err := botTokenSecretID("my-group", "repo") + require.NoError(t, err) + assert.NotEqual(t, idDot, idHyphen, + "dot and hyphen in owner should produce distinct secret IDs") + }) + + t.Run("dot vs hyphen in repo", func(t *testing.T) { + idDot, err := botTokenSecretID("owner", "my.repo") + require.NoError(t, err) + idHyphen, err := botTokenSecretID("owner", "my-repo") + require.NoError(t, err) + assert.NotEqual(t, idDot, idHyphen, + "dot and hyphen in repo should produce distinct secret IDs") + }) +} + +func TestLegacyBotTokenSecretID(t *testing.T) { + t.Run("no dots produces same as current", func(t *testing.T) { + current, err := botTokenSecretID("group", "repo") + require.NoError(t, err) + legacy := legacyBotTokenSecretID("group", "repo") + assert.Equal(t, current, legacy) + }) + + t.Run("dots produce different IDs", func(t *testing.T) { + current, err := botTokenSecretID("my.group", "repo") + require.NoError(t, err) + legacy := legacyBotTokenSecretID("my.group", "repo") + assert.NotEqual(t, current, legacy) + assert.Equal(t, "fullsend-bot-token-my_dot_group--repo", current) + assert.Equal(t, "fullsend-bot-token-my-group--repo", legacy) + }) +} + +func TestProjectIDFromSAEmail(t *testing.T) { + tests := []struct { + email string + want string + }{ + {"fullsend-mint@my-project.iam.gserviceaccount.com", "my-project"}, + {"sa@another-project-123.iam.gserviceaccount.com", "another-project-123"}, + {"bad-email", ""}, + {"user@gmail.com", ""}, + {"", ""}, + } + for _, tt := range tests { + got := projectIDFromSAEmail(tt.email) + assert.Equal(t, tt.want, got, "projectIDFromSAEmail(%q)", tt.email) + } +} + +func TestCleanupGitLabBotTokenSecret(t *testing.T) { + ctx := context.Background() + + t.Run("deletes secret successfully", func(t *testing.T) { + smClient := newFakeSecretManagerClient() + smClient.secrets["fullsend-bot-token-group--project"] = true + var buf bytes.Buffer + printer := ui.New(&buf) + + cleanupGitLabBotTokenSecret(ctx, smClient, printer, "my-project", "group", "project") + assert.Contains(t, smClient.calls, "DeleteSecret") + assert.Contains(t, buf.String(), "Deleted Secret Manager secret") + }) + + t.Run("warns on delete failure", func(t *testing.T) { + smClient := newFakeSecretManagerClient() + smClient.errs["DeleteSecret"] = fmt.Errorf("permission denied") + var buf bytes.Buffer + printer := ui.New(&buf) + + cleanupGitLabBotTokenSecret(ctx, smClient, printer, "my-project", "group", "project") + assert.Contains(t, buf.String(), "Failed to delete Secret Manager secret") + }) + + t.Run("deletes legacy secret for dotted names", func(t *testing.T) { + smClient := newFakeSecretManagerClient() + smClient.secrets["fullsend-bot-token-my-group--repo"] = true + var buf bytes.Buffer + printer := ui.New(&buf) + + cleanupGitLabBotTokenSecret(ctx, smClient, printer, "my-project", "my.group", "repo") + assert.Contains(t, buf.String(), "Deleted legacy Secret Manager secret fullsend-bot-token-my-group--repo") + }) + + t.Run("skips legacy when names match", func(t *testing.T) { + smClient := newFakeSecretManagerClient() + smClient.secrets["fullsend-bot-token-group--project"] = true + var buf bytes.Buffer + printer := ui.New(&buf) + + cleanupGitLabBotTokenSecret(ctx, smClient, printer, "my-project", "group", "project") + deleteCount := 0 + for _, c := range smClient.calls { + if c == "DeleteSecret" { + deleteCount++ + } + } + assert.Equal(t, 1, deleteCount, "should only call DeleteSecret once when legacy ID matches current") + }) } diff --git a/internal/cli/repos_test.go b/internal/cli/repos_test.go index e34a85966c..c0a3662f40 100644 --- a/internal/cli/repos_test.go +++ b/internal/cli/repos_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/fullsend-ai/fullsend/internal/dispatch/gcf" "github.com/fullsend-ai/fullsend/internal/forge" "github.com/fullsend-ai/fullsend/internal/repos" "github.com/fullsend-ai/fullsend/internal/ui" @@ -1679,3 +1680,38 @@ repos: require.Error(t, err) assert.Contains(t, err.Error(), "failed to uninstall") } + +func TestRunReposUninstall_GitLabWIFSecretPreRead(t *testing.T) { + m := `version: 1 +forge: + github: + mint_url: https://mint.example.com + inference_project_number: "123456789" + gitlab: + url: https://gitlab.example.com +defaults: + forge: gitlab +repos: + - acme/repo +` + manifestPath := writeTestManifest(t, m) + fc := newInstalledFakeClientCLI("acme/repo") + fc.VariableValues["acme/repo/FULLSEND_SA"] = "fullsend-mint@my-gcp-project.iam.gserviceaccount.com" + + fakeGCP := gcf.NewFakeGCFClient() + err := runReposUninstall(context.Background(), &reposUninstallConfig{ + manifest: manifestPath, + yes: true, + concurrency: 4, + testClient: fc, + testGCPClientFactory: func(projectID string) gcf.GCFClient { + require.Equal(t, "my-gcp-project", projectID) + return fakeGCP + }, + }, []string{"acme/repo"}) + require.NoError(t, err) + + deleted := gcf.DeletedSecretIDs(fakeGCP) + require.Len(t, deleted, 1) + assert.Equal(t, "fullsend-bot-token-acme--repo", deleted[0]) +} diff --git a/internal/dispatch/gcf/fakeclient.go b/internal/dispatch/gcf/fakeclient.go index f86b8bfcb6..06fa6c1fcc 100644 --- a/internal/dispatch/gcf/fakeclient.go +++ b/internal/dispatch/gcf/fakeclient.go @@ -162,6 +162,9 @@ func (f *fakeGCFClient) DeleteWIFProvider(_ context.Context, _, _, _ string) err func (f *fakeGCFClient) SetSecretIAMBinding(_ context.Context, _, _, _ string) error { return f.record("SetSecretIAMBinding") } +func (f *fakeGCFClient) ReplaceSecretIAMBinding(_ context.Context, _, _, _ string) error { + return f.record("ReplaceSecretIAMBinding") +} func (f *fakeGCFClient) SetProjectIAMBinding(_ context.Context, projectID, member, role string) error { f.projectIAMBindings = append(f.projectIAMBindings, projectIAMBinding{projectID, member, role}) return f.record("SetProjectIAMBinding") @@ -329,3 +332,14 @@ func ProjectIAMBindingCount(client GCFClient) int { } return len(f.projectIAMBindings) } + +// DeletedSecretIDs returns the secret IDs passed to DeleteSecret calls on a +// fake client, for cross-package test assertions. Returns nil if client isn't +// a fake. +func DeletedSecretIDs(client GCFClient) []string { + f, ok := client.(*fakeGCFClient) + if !ok { + return nil + } + return f.deletedSecretIDs +} diff --git a/internal/dispatch/gcf/fakeclient_test.go b/internal/dispatch/gcf/fakeclient_test.go index a7e7039fff..9c212cb2a7 100644 --- a/internal/dispatch/gcf/fakeclient_test.go +++ b/internal/dispatch/gcf/fakeclient_test.go @@ -52,10 +52,12 @@ func TestNewFakeGCFClient_OptionsAndMethods(t *testing.T) { require.Error(t, err) require.NoError(t, client.EnableSecretVersion(ctx, "p", "fullsend-coder-app-pem")) require.NoError(t, client.DeleteSecret(ctx, "p", "new-secret")) + assert.Equal(t, []string{"new-secret"}, DeletedSecretIDs(client)) require.NoError(t, client.DisableWIFProvider(ctx, "p", "pool", "prov")) require.NoError(t, client.DeleteWIFProvider(ctx, "p", "pool", "prov")) require.NoError(t, client.SetSecretIAMBinding(ctx, "p", "s", "m")) + require.NoError(t, client.ReplaceSecretIAMBinding(ctx, "p", "s", "m")) require.NoError(t, client.SetProjectIAMBinding(ctx, "p", "m", "r")) require.NoError(t, client.SetCloudRunInvoker(ctx, "p", "s", "m")) diff --git a/internal/dispatch/gcf/gcp.go b/internal/dispatch/gcf/gcp.go index c95e189fcf..2d35ba5067 100644 --- a/internal/dispatch/gcf/gcp.go +++ b/internal/dispatch/gcf/gcp.go @@ -118,6 +118,16 @@ type GCFClient interface { // IAM bindings SetSecretIAMBinding(ctx context.Context, resource, member, role string) error + // ReplaceSecretIAMBinding sets the IAM binding for a role on a Secret + // Manager resource, replacing all existing members for that role with + // the specified member. This is destructive: any other members bound + // to the same role on the resource are removed. Use this instead of + // SetSecretIAMBinding when re-install with a different service account + // should revoke the old account's access. + ReplaceSecretIAMBinding(ctx context.Context, resource, member, role string) error + // SetProjectIAMBinding adds an IAM binding on a Cloud Resource Manager + // project. This is intentionally additive (unlike ReplaceSecretIAMBinding) + // because project-level roles may have multiple legitimate members. SetProjectIAMBinding(ctx context.Context, projectID, member, role string) error // Cloud Run IAM (for function invoker policy) @@ -709,7 +719,23 @@ func (c *LiveGCFClient) DeleteWIFProvider(ctx context.Context, projectNumber, po // SetSecretIAMBinding sets an IAM binding on a Secret Manager resource. // Uses read-modify-write with retry on 409 Conflict (etag mismatch). +// The member is added to the existing binding for the role (additive). func (c *LiveGCFClient) SetSecretIAMBinding(ctx context.Context, resource, member, role string) error { + return c.setSecretIAMBindingWithMode(ctx, resource, member, role, false) +} + +// ReplaceSecretIAMBinding sets the IAM binding for a role on a Secret +// Manager resource, replacing all existing members for that role with +// the specified member. This is destructive: any other members bound +// to the same role are silently removed. Safe for fullsend-managed bot +// token secrets which have a single expected accessor. +func (c *LiveGCFClient) ReplaceSecretIAMBinding(ctx context.Context, resource, member, role string) error { + return c.setSecretIAMBindingWithMode(ctx, resource, member, role, true) +} + +// setSecretIAMBindingWithMode is the shared implementation for both additive +// (replace=false) and replace (replace=true) Secret Manager IAM operations. +func (c *LiveGCFClient) setSecretIAMBindingWithMode(ctx context.Context, resource, member, role string, replace bool) error { if !secretResourcePattern.MatchString(resource) { return fmt.Errorf("invalid secret resource path %q", resource) } @@ -718,7 +744,7 @@ func (c *LiveGCFClient) SetSecretIAMBinding(ctx context.Context, resource, membe setURL := fmt.Sprintf("https://secretmanager.googleapis.com/v1/%s:setIamPolicy", resource) for attempt := range maxRetries { - err := c.trySetIAMBinding(ctx, http.MethodGet, "", getURL, setURL, member, role) + err := c.trySetIAMBinding(ctx, http.MethodGet, "", getURL, setURL, member, role, replace) if err == nil { return nil } @@ -778,7 +804,7 @@ func (c *LiveGCFClient) SetProjectIAMBinding(ctx context.Context, projectID, mem url.PathEscape(projectID)) for attempt := range maxRetries { - err := c.trySetIAMBinding(ctx, http.MethodPost, "{}", getURL, setURL, member, role) + err := c.trySetIAMBinding(ctx, http.MethodPost, "{}", getURL, setURL, member, role, false) if err == nil { return nil } @@ -797,7 +823,9 @@ func (c *LiveGCFClient) SetProjectIAMBinding(ctx context.Context, projectID, mem // trySetIAMBinding performs a single read-modify-write IAM policy update. // getMethod/getBody control the getIamPolicy request (GET+"" for Secret Manager, // POST+"{}" for Cloud Resource Manager). setIamPolicy always uses POST. -func (c *LiveGCFClient) trySetIAMBinding(ctx context.Context, getMethod, getBody, getURL, setURL, member, role string) error { +// When replace is true, all existing members for the role are replaced with +// the specified member instead of appending. +func (c *LiveGCFClient) trySetIAMBinding(ctx context.Context, getMethod, getBody, getURL, setURL, member, role string, replace bool) error { resp, err := c.Client.DoRequest(ctx, getMethod, getURL, getBody) if err != nil { return fmt.Errorf("getting IAM policy: %w", err) @@ -825,13 +853,19 @@ func (c *LiveGCFClient) trySetIAMBinding(ctx context.Context, getMethod, getBody if binding["role"] != role { continue } - members, _ := binding["members"].([]interface{}) - for _, m := range members { - if m == member { - return nil + if replace { + // Replace mode: set members to only the specified member. + binding["members"] = []string{member} + } else { + // Additive mode: append the member if not already present. + members, _ := binding["members"].([]interface{}) + for _, m := range members { + if m == member { + return nil + } } + binding["members"] = append(members, member) } - binding["members"] = append(members, member) found = true break } diff --git a/internal/dispatch/gcf/gcp_test.go b/internal/dispatch/gcf/gcp_test.go index ba27bcaa6b..52c7ecec86 100644 --- a/internal/dispatch/gcf/gcp_test.go +++ b/internal/dispatch/gcf/gcp_test.go @@ -475,6 +475,62 @@ func TestLiveGCFClient_SetSecretIAMBinding(t *testing.T) { }) } +// --- ReplaceSecretIAMBinding --- + +func TestLiveGCFClient_ReplaceSecretIAMBinding(t *testing.T) { + t.Run("replaces existing members", func(t *testing.T) { + callCount := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + if callCount == 1 { + assert.Contains(t, r.URL.Path, ":getIamPolicy") + w.WriteHeader(http.StatusOK) + fmt.Fprintln(w, `{"bindings":[{"role":"roles/secretmanager.secretAccessor","members":["serviceAccount:old@proj.iam.gserviceaccount.com"]}],"etag":"abc"}`) + return + } + assert.Contains(t, r.URL.Path, ":setIamPolicy") + var body map[string]interface{} + json.NewDecoder(r.Body).Decode(&body) + policy := body["policy"].(map[string]interface{}) + bindings := policy["bindings"].([]interface{}) + require.Len(t, bindings, 1) + binding := bindings[0].(map[string]interface{}) + members := binding["members"].([]interface{}) + assert.Equal(t, []interface{}{"serviceAccount:new@proj.iam.gserviceaccount.com"}, members) + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + err := newTestClient(srv).ReplaceSecretIAMBinding(context.Background(), + "projects/proj/secrets/s", "serviceAccount:new@proj.iam.gserviceaccount.com", "roles/secretmanager.secretAccessor") + require.NoError(t, err) + assert.Equal(t, 2, callCount) + }) + + t.Run("adds binding when role not present", func(t *testing.T) { + callCount := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + if callCount == 1 { + w.WriteHeader(http.StatusOK) + fmt.Fprintln(w, `{"bindings":[],"etag":"abc"}`) + return + } + var body map[string]interface{} + json.NewDecoder(r.Body).Decode(&body) + policy := body["policy"].(map[string]interface{}) + bindings := policy["bindings"].([]interface{}) + require.Len(t, bindings, 1) + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + err := newTestClient(srv).ReplaceSecretIAMBinding(context.Background(), + "projects/proj/secrets/s", "serviceAccount:sa@proj.iam.gserviceaccount.com", "roles/secretmanager.secretAccessor") + require.NoError(t, err) + }) +} + // --- iamRetryDelay --- func TestIAMRetryDelay(t *testing.T) {