From 1c0420f06e7c2d99357bc7f350fdcc22e702cc93 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 22:23:19 +0200 Subject: [PATCH 1/2] sec(cli): remove the minted GCP service-account key after upload The GCP setup wizard wrote a never-expiring service-account key to ~/cudly-gcp-key.json and left it there after uploading it to Secrets Manager, where backups and file sync pick it up. The key is now written 0600 into a private 0700 os.MkdirTemp directory and removed (file and directory) after the upload succeeds and on every error path. A removal failure is returned, not ignored. An operator- supplied --credentials-file is never deleted. The AWS config now loads before the wizard runs, so a config error cannot strand a minted key. Closes #1947 --- cmd/configure_gcp.go | 111 +++++++++++++++++++++++++++----------- cmd/configure_gcp_test.go | 87 ++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 30 deletions(-) diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index a22452348..9cd2a6cc4 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -5,7 +5,9 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "fmt" + "io/fs" "log" "os" "os/exec" @@ -141,25 +143,21 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { fmt.Println("===================================================") fmt.Println() - credsFile, err := getGCPCredentialsFilePath(ctx, reader) - if err != nil { - return err - } - + // Load the AWS config before the wizard can mint a key, so a config error + // never strands a freshly minted key. cfg, err := loadAWSConfigForGCP(ctx) if err != nil { return err } + store := NewAWSSecretsStore(secretsmanager.NewFromConfig(cfg)) - creds, credsData, err := loadAndUpdateGCPCredentials(credsFile) + credsFile, minted, err := getGCPCredentialsFilePath(ctx, reader) if err != nil { return err } - smClient := secretsmanager.NewFromConfig(cfg) - store := NewAWSSecretsStore(smClient) - - if err := storeGCPCredentials(ctx, store, gcpOpts.StackName, string(credsData)); err != nil { + creds, err := uploadGCPCredentialsFile(ctx, store, gcpOpts.StackName, credsFile, minted) + if err != nil { return err } @@ -167,34 +165,63 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { return nil } -// getGCPCredentialsFilePath determines the credentials file path from options or user input. -func getGCPCredentialsFilePath(ctx context.Context, reader *bufio.Reader) (string, error) { - var credsFile string +// uploadGCPCredentialsFile stores credsFile in the secrets store. When minted +// is true the file is a key this run created, so it is removed on every path, +// and a removal failure is returned rather than ignored. +func uploadGCPCredentialsFile(ctx context.Context, store SecretsStore, stackName, credsFile string, minted bool) (creds GCPCredentials, err error) { + if minted { + defer func() { + if rmErr := removeMintedGCPKey(credsFile); rmErr != nil { + err = errors.Join(err, rmErr) + } else if err == nil { + fmt.Println("Removed the local copy of the minted key.") + } + }() + } + creds, credsData, err := loadAndUpdateGCPCredentials(credsFile) + if err != nil { + return GCPCredentials{}, err + } + + if err := storeGCPCredentials(ctx, store, stackName, string(credsData)); err != nil { + if minted { + err = fmt.Errorf("%w (the key minted for %s is still active in GCP; delete it with 'gcloud iam service-accounts keys delete' and re-run)", err, creds.ClientEmail) + } + return GCPCredentials{}, err + } + + return creds, nil +} + +// getGCPCredentialsFilePath determines the credentials file path from options +// or user input. minted reports whether the setup wizard created the file. +func getGCPCredentialsFilePath(ctx context.Context, reader *bufio.Reader) (credsFile string, minted bool, err error) { if gcpOpts.CredentialsFile != "" { credsFile = gcpOpts.CredentialsFile } else if !gcpOpts.SkipSetup { - var err error credsFile, err = runGCPSetupCommands(ctx, reader) if err != nil { - return "", err + return "", false, err + } + if credsFile != "" { + return credsFile, true, nil } } if credsFile == "" { fmt.Print("Path to GCP service account JSON key file: ") - var readErr error - credsFile, readErr = readTrimmedLine(reader) - if readErr != nil { - return "", fmt.Errorf("failed to read credentials file path: %w", readErr) + credsFile, err = readTrimmedLine(reader) + if err != nil { + return "", false, fmt.Errorf("failed to read credentials file path: %w", err) } } if credsFile == "" { - return "", fmt.Errorf("credentials file is required") + return "", false, fmt.Errorf("credentials file is required") } - return credsFile, nil + return credsFile, false, nil } // loadAWSConfigForGCP loads AWS configuration with optional profile. @@ -484,7 +511,7 @@ func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) er func writeServiceAccountKey(ctx context.Context, p gcpKeyProvisioner, saEmail, keyFile string) error { // Reserve the destination file first (fails if it already exists), so we // never mint a remote key we cannot persist locally. - // #nosec G304 -- keyFile is the sole caller's fixed path filepath.Join(os.UserHomeDir(), "cudly-gcp-key.json"); a constant filename under the operator's own home dir, program-controlled and not attacker input + // #nosec G304 -- keyFile is the sole caller's fixed filename inside a private os.MkdirTemp dir (newMintedGCPKeyPath); program-controlled and not attacker input f, err := os.OpenFile(keyFile, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) if err != nil { return fmt.Errorf("failed to reserve key file %s: %w", keyFile, err) @@ -715,29 +742,28 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm // an unknown choice it returns an empty string so the caller knows to prompt // for an existing credentials file instead of assuming one was written. func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) (string, error) { - home, err := os.UserHomeDir() + keyFile, err := newMintedGCPKeyPath() if err != nil { - return "", fmt.Errorf("failed to get home directory: %w", err) + return "", err } - keyFile := filepath.Join(home, "cudly-gcp-key.json") fmt.Println() fmt.Println("Step 5: Create and Download Key") fmt.Println("-------------------------------") fmt.Println("Create a JSON key file for the service account.") fmt.Println() - fmt.Printf("[R]un, [S]kip? (creates key for %s, writes to %s via SDK) ", saEmail, keyFile) + fmt.Printf("[R]un, [S]kip? (creates key for %s via SDK; the local copy is removed after upload) ", saEmail) choice, err := reader.ReadString('\n') if err != nil { - return "", fmt.Errorf("failed to read create-key choice: %w", err) + return "", errors.Join(fmt.Errorf("failed to read create-key choice: %w", err), removeMintedGCPKey(keyFile)) } switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": if keyErr := createGCPServiceAccountKey(ctx, saEmail, keyFile); keyErr != nil { - return "", keyErr + return "", errors.Join(keyErr, removeMintedGCPKey(keyFile)) } - fmt.Printf("Key file written to: %s\n", keyFile) + fmt.Printf("Key written to temporary file: %s\n", keyFile) fmt.Println() return keyFile, nil case "s", "skip": @@ -748,7 +774,32 @@ func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) // No key file was written; the caller will prompt for an existing one. fmt.Println() - return "", nil + return "", removeMintedGCPKey(keyFile) +} + +// mintedGCPKeyFilename is the key file's name inside its private temp dir. +const mintedGCPKeyFilename = "key.json" + +// newMintedGCPKeyPath returns a key path inside a fresh 0700 temp dir, so the +// minted key is never readable by other users or left in a synced home dir. +func newMintedGCPKeyPath() (string, error) { + dir, err := os.MkdirTemp("", "cudly-gcp-key-") + if err != nil { + return "", fmt.Errorf("failed to create private directory for the key: %w", err) + } + return filepath.Join(dir, mintedGCPKeyFilename), nil +} + +// removeMintedGCPKey deletes a key path from newMintedGCPKeyPath and its dir. +// It removes non-recursively so it can never delete anything else. +func removeMintedGCPKey(keyFile string) error { + if err := os.Remove(keyFile); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("failed to remove the minted key file %s; delete it manually: %w", keyFile, err) + } + if err := os.Remove(filepath.Dir(keyFile)); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("failed to remove the minted key directory %s: %w", filepath.Dir(keyFile), err) + } + return nil } // readRequiredInputLine prints prompt, reads a line, trims whitespace, and diff --git a/cmd/configure_gcp_test.go b/cmd/configure_gcp_test.go index d3a606cdd..8213a4cc7 100644 --- a/cmd/configure_gcp_test.go +++ b/cmd/configure_gcp_test.go @@ -248,3 +248,90 @@ func TestWriteServiceAccountKey_ReserveFailureNoMint(t *testing.T) { require.NoError(t, readErr) assert.Equal(t, []byte("pre-existing"), got) } + +// --- uploadGCPCredentialsFile (minted key cleanup, #1947) -------------------- + +// mintTestGCPKey mints a key into a newMintedGCPKeyPath location through the +// real writeServiceAccountKey flow and asserts it was created with 0600. +func mintTestGCPKey(t *testing.T) string { + t.Helper() + keyMaterial := []byte(`{"type":"service_account","project_id":"proj","client_email":"sa@proj.iam.gserviceaccount.com","private_key":"-----BEGIN PRIVATE KEY-----\nx\n-----END PRIVATE KEY-----\n"}`) + m := &mockGCPKeyProvisioner{ + keyName: "projects/-/serviceAccounts/sa@proj.iam.gserviceaccount.com/keys/abc123", + privateKeyData: base64.StdEncoding.EncodeToString(keyMaterial), + } + keyFile, err := newMintedGCPKeyPath() + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(filepath.Dir(keyFile)) }) + + require.NoError(t, writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile)) + info, err := os.Stat(keyFile) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0600), info.Mode().Perm(), "minted key must be 0600") + dirInfo, err := os.Stat(filepath.Dir(keyFile)) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0700), dirInfo.Mode().Perm(), "minted key dir must be 0700") + return keyFile +} + +func assertMintedKeyGone(t *testing.T, keyFile string) { + t.Helper() + _, err := os.Stat(keyFile) + assert.ErrorIs(t, err, os.ErrNotExist, "minted key file must be removed") + _, err = os.Stat(filepath.Dir(keyFile)) + assert.ErrorIs(t, err, os.ErrNotExist, "minted key dir must be removed") +} + +func TestUploadGCPCredentialsFile_MintedKeyRemovedAfterSuccess(t *testing.T) { + keyFile := mintTestGCPKey(t) + store := NewMockSecretsStore() + + creds, err := uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, true) + require.NoError(t, err) + assert.Equal(t, "sa@proj.iam.gserviceaccount.com", creds.ClientEmail) + assert.Contains(t, store.updatedSecrets, "stack-GCPCredentials") + assertMintedKeyGone(t, keyFile) +} + +func TestUploadGCPCredentialsFile_MintedKeyRemovedAfterUploadFailure(t *testing.T) { + keyFile := mintTestGCPKey(t) + store := NewMockSecretsStore() + store.updateSecretFunc = func(context.Context, string, string) error { return errors.New("access denied") } + + _, err := uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "access denied") + assert.Contains(t, err.Error(), "still active in GCP") + assertMintedKeyGone(t, keyFile) +} + +func TestUploadGCPCredentialsFile_MintedKeyRemovedAfterParseFailure(t *testing.T) { + keyFile, err := newMintedGCPKeyPath() + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(filepath.Dir(keyFile)) }) + require.NoError(t, os.WriteFile(keyFile, []byte("not json"), 0600)) + + _, err = uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, true) + require.Error(t, err) + assertMintedKeyGone(t, keyFile) +} + +func TestUploadGCPCredentialsFile_RemovalFailureReported(t *testing.T) { + keyFile := mintTestGCPKey(t) + // A read-only parent dir makes the unlink fail. + require.NoError(t, os.Chmod(filepath.Dir(keyFile), 0500)) + t.Cleanup(func() { _ = os.Chmod(filepath.Dir(keyFile), 0700) }) + + _, err := uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to remove the minted key file") +} + +func TestUploadGCPCredentialsFile_OperatorFileKept(t *testing.T) { + keyFile := mintTestGCPKey(t) + + _, err := uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, false) + require.NoError(t, err) + _, err = os.Stat(keyFile) + assert.NoError(t, err, "an operator-supplied credentials file must not be deleted") +} From 238844f5f5e59dd916c7d31c09b80b5a044d8ea6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 29 Sep 2026 00:18:19 +0200 Subject: [PATCH 2/2] sec(cli): delete the minted GCP key remotely when the upload fails A key minted by the configure-gcp wizard stayed active in GCP when the Secrets Manager upload (or parsing of the minted file) failed, and the error named no key ID. The wizard now threads the IAM key resource name returned by CreateKey through to the upload, and on any upload failure deletes the key via gcpKeyProvisioner.DeleteKey on a fresh context. If that delete fails too, the error names the key and prints the exact gcloud iam service-accounts keys delete command. The upload also runs under signal.NotifyContext, so SIGINT/SIGTERM cancels the Secrets Manager call and both the local and remote cleanup run instead of the process being killed. The signal scope covers only the upload, so Ctrl-C at the wizard's stdin prompts still exits. --- cmd/configure_gcp.go | 155 +++++++++++++++++++++++++------------- cmd/configure_gcp_test.go | 103 +++++++++++++++++++------ 2 files changed, 184 insertions(+), 74 deletions(-) diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 9cd2a6cc4..05e1bbaba 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -11,9 +11,11 @@ import ( "log" "os" "os/exec" + "os/signal" "path/filepath" "regexp" "strings" + "syscall" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -151,12 +153,16 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { } store := NewAWSSecretsStore(secretsmanager.NewFromConfig(cfg)) - credsFile, minted, err := getGCPCredentialsFilePath(ctx, reader) + credsFile, mintedKeyName, err := getGCPCredentialsFilePath(ctx, reader) if err != nil { return err } - creds, err := uploadGCPCredentialsFile(ctx, store, gcpOpts.StackName, credsFile, minted) + // Scoped to the upload (no stdin reads) so an interrupt cancels the + // Secrets Manager call and the minted-key cleanup still runs. + uploadCtx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) + defer stop() + creds, err := uploadGCPCredentialsFile(uploadCtx, store, gcpOpts.StackName, credsFile, mintedKeyName, deleteGCPServiceAccountKey) if err != nil { return err } @@ -165,12 +171,16 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { return nil } -// uploadGCPCredentialsFile stores credsFile in the secrets store. When minted -// is true the file is a key this run created, so it is removed on every path, -// and a removal failure is returned rather than ignored. -func uploadGCPCredentialsFile(ctx context.Context, store SecretsStore, stackName, credsFile string, minted bool) (creds GCPCredentials, err error) { - if minted { +// uploadGCPCredentialsFile stores credsFile in the secrets store. A non-empty +// mintedKeyName means the file is a key this run created: the local copy is +// removed on every path, and on failure the remote key is deleted via +// deleteKey so it does not stay active. Cleanup failures are returned. +func uploadGCPCredentialsFile(ctx context.Context, store SecretsStore, stackName, credsFile, mintedKeyName string, deleteKey func(context.Context, string) error) (creds GCPCredentials, err error) { + if mintedKeyName != "" { defer func() { + if err != nil { + err = deleteMintedGCPKeyRemotely(err, mintedKeyName, deleteKey) + } if rmErr := removeMintedGCPKey(credsFile); rmErr != nil { err = errors.Join(err, rmErr) } else if err == nil { @@ -185,27 +195,47 @@ func uploadGCPCredentialsFile(ctx context.Context, store SecretsStore, stackName } if err := storeGCPCredentials(ctx, store, stackName, string(credsData)); err != nil { - if minted { - err = fmt.Errorf("%w (the key minted for %s is still active in GCP; delete it with 'gcloud iam service-accounts keys delete' and re-run)", err, creds.ClientEmail) - } return GCPCredentials{}, err } return creds, nil } +// deleteMintedGCPKeyRemotely deletes the minted key after cause aborted the +// upload. It uses a fresh context so a canceled parent does not skip it. +func deleteMintedGCPKeyRemotely(cause error, keyName string, deleteKey func(context.Context, string) error) error { + ctx, cancel := context.WithTimeout(context.Background(), gcpSDKCallTimeout) + defer cancel() + if err := deleteKey(ctx, keyName); err != nil { + return fmt.Errorf("%w; the minted key is still active in GCP and deleting it failed (%w); delete it with: %s", cause, err, gcloudDeleteKeyCommand(keyName)) + } + fmt.Println("Deleted the minted key from GCP.") + return cause +} + +// gcloudDeleteKeyCommand renders the manual delete command for an IAM key +// resource name (projects/

/serviceAccounts//keys/). +func gcloudDeleteKeyCommand(keyName string) string { + parts := strings.Split(keyName, "/") + if len(parts) == 6 && parts[2] == "serviceAccounts" && parts[4] == "keys" { + return fmt.Sprintf("gcloud iam service-accounts keys delete %s --iam-account=%s", parts[5], parts[3]) + } + return fmt.Sprintf("gcloud iam service-accounts keys delete ", keyName) +} + // getGCPCredentialsFilePath determines the credentials file path from options -// or user input. minted reports whether the setup wizard created the file. -func getGCPCredentialsFilePath(ctx context.Context, reader *bufio.Reader) (credsFile string, minted bool, err error) { +// or user input. mintedKeyName is the IAM key resource name when the setup +// wizard created the file, and empty otherwise. +func getGCPCredentialsFilePath(ctx context.Context, reader *bufio.Reader) (credsFile, mintedKeyName string, err error) { if gcpOpts.CredentialsFile != "" { credsFile = gcpOpts.CredentialsFile } else if !gcpOpts.SkipSetup { - credsFile, err = runGCPSetupCommands(ctx, reader) + credsFile, mintedKeyName, err = runGCPSetupCommands(ctx, reader) if err != nil { - return "", false, err + return "", "", err } if credsFile != "" { - return credsFile, true, nil + return credsFile, mintedKeyName, nil } } @@ -213,15 +243,15 @@ func getGCPCredentialsFilePath(ctx context.Context, reader *bufio.Reader) (creds fmt.Print("Path to GCP service account JSON key file: ") credsFile, err = readTrimmedLine(reader) if err != nil { - return "", false, fmt.Errorf("failed to read credentials file path: %w", err) + return "", "", fmt.Errorf("failed to read credentials file path: %w", err) } } if credsFile == "" { - return "", false, fmt.Errorf("credentials file is required") + return "", "", fmt.Errorf("credentials file is required") } - return credsFile, false, nil + return credsFile, "", nil } // loadAWSConfigForGCP loads AWS configuration with optional profile. @@ -481,40 +511,57 @@ func (k *iamKeyProvisioner) DeleteKey(ctx context.Context, keyName string) error return err } -// createGCPServiceAccountKey creates a JSON key for the given service account -// and writes it to keyFile. This replaces +// newIAMKeyProvisioner builds an iamKeyProvisioner authenticated via ADC. +func newIAMKeyProvisioner(ctx context.Context) (*iamKeyProvisioner, error) { + opt, err := newGCPAPIOption(ctx) + if err != nil { + return nil, err + } + + svc, err := iamv1.NewService(ctx, opt) + if err != nil { + return nil, fmt.Errorf("failed to create IAM client: %w", err) + } + return &iamKeyProvisioner{svc: svc}, nil +} + +// createGCPServiceAccountKey creates a JSON key for the given service account, +// writes it to keyFile and returns the key resource name. This replaces // "gcloud iam service-accounts keys create --iam-account=". -func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) error { +func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) (string, error) { ctx, cancel := context.WithTimeout(ctx, gcpSDKCallTimeout) defer cancel() - opt, err := newGCPAPIOption(ctx) + p, err := newIAMKeyProvisioner(ctx) if err != nil { - return err + return "", err } + return writeServiceAccountKey(ctx, p, saEmail, keyFile) +} - svc, err := iamv1.NewService(ctx, opt) +// deleteGCPServiceAccountKey deletes the IAM key keyName via ADC. +func deleteGCPServiceAccountKey(ctx context.Context, keyName string) error { + p, err := newIAMKeyProvisioner(ctx) if err != nil { - return fmt.Errorf("failed to create IAM client: %w", err) + return err } - - return writeServiceAccountKey(ctx, &iamKeyProvisioner{svc: svc}, saEmail, keyFile) + return p.DeleteKey(ctx, keyName) } // writeServiceAccountKey reserves keyFile with exclusive-create semantics // BEFORE minting the remote key (so it never mints a key it cannot persist // locally), then mints the key via p, decodes the base64 material and writes it -// to keyFile. If decoding or writing fails after the remote key is minted it +// to keyFile, returning the key resource name. If decoding or writing fails after the remote key is minted it // deletes the remote key so it does not linger as an active, unused credential. // Extracted from createGCPServiceAccountKey so the reserve / mint / rollback // flow is unit-testable with a mock (no GCP credentials). -func writeServiceAccountKey(ctx context.Context, p gcpKeyProvisioner, saEmail, keyFile string) error { +func writeServiceAccountKey(ctx context.Context, p gcpKeyProvisioner, saEmail, keyFile string) (string, error) { // Reserve the destination file first (fails if it already exists), so we // never mint a remote key we cannot persist locally. // #nosec G304 -- keyFile is the sole caller's fixed filename inside a private os.MkdirTemp dir (newMintedGCPKeyPath); program-controlled and not attacker input f, err := os.OpenFile(keyFile, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) if err != nil { - return fmt.Errorf("failed to reserve key file %s: %w", keyFile, err) + return "", fmt.Errorf("failed to reserve key file %s: %w", keyFile, err) } // Best-effort: remove the reserved file if we return before writing it. wrote := false @@ -527,7 +574,7 @@ func writeServiceAccountKey(ctx context.Context, p gcpKeyProvisioner, saEmail, k keyName, privateKeyData, err := p.CreateKey(ctx, saEmail) if err != nil { - return fmt.Errorf("failed to create service account key: %w", err) + return "", fmt.Errorf("failed to create service account key: %w", err) } // From here on, any failure must delete the newly minted remote key so it @@ -545,14 +592,14 @@ func writeServiceAccountKey(ctx context.Context, p gcpKeyProvisioner, saEmail, k // PrivateKeyData is base64-encoded JSON. decoded, err := base64.StdEncoding.DecodeString(privateKeyData) if err != nil { - return deleteRemoteKey(fmt.Errorf("failed to decode key data: %w", err)) + return "", deleteRemoteKey(fmt.Errorf("failed to decode key data: %w", err)) } if _, err := f.Write(decoded); err != nil { - return deleteRemoteKey(fmt.Errorf("failed to write key file %s: %w", keyFile, err)) + return "", deleteRemoteKey(fmt.Errorf("failed to write key file %s: %w", keyFile, err)) } wrote = true - return nil + return keyName, nil } // runGCPSetupCommands guides the operator through GCP setup. @@ -576,23 +623,25 @@ func writeServiceAccountKey(ctx context.Context, p gcpKeyProvisioner, saEmail, k // Steps 4-6 (create SA, grant role, create key): performed via GCP IAM and // Cloud Resource Manager SDK v1 APIs using ADC. Fail loud on any SDK error // (no CLI fallback). -func runGCPSetupCommands(ctx context.Context, reader *bufio.Reader) (string, error) { - if err := gcpStepLogin(reader); err != nil { - return "", err +func runGCPSetupCommands(ctx context.Context, reader *bufio.Reader) (keyFile, keyName string, err error) { + err = gcpStepLogin(reader) + if err != nil { + return "", "", err } projectID, err := gcpStepSelectProject(ctx, reader) if err != nil { - return "", err + return "", "", err } saEmail, err := gcpStepCreateServiceAccount(ctx, reader, projectID) if err != nil { - return "", err + return "", "", err } - if err := gcpStepGrantRole(ctx, reader, projectID, saEmail); err != nil { - return "", err + err = gcpStepGrantRole(ctx, reader, projectID, saEmail) + if err != nil { + return "", "", err } return gcpStepCreateKey(ctx, reader, saEmail) @@ -738,13 +787,14 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm } // gcpStepCreateKey creates a JSON key file for the service account. It returns -// the written key-file path only when a key was actually created; on skip or -// an unknown choice it returns an empty string so the caller knows to prompt -// for an existing credentials file instead of assuming one was written. -func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) (string, error) { - keyFile, err := newMintedGCPKeyPath() +// the written key-file path and the key resource name only when a key was +// actually created; on skip or an unknown choice it returns empty strings so +// the caller knows to prompt for an existing credentials file instead of +// assuming one was written. +func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) (keyFile, keyName string, err error) { + keyFile, err = newMintedGCPKeyPath() if err != nil { - return "", err + return "", "", err } fmt.Println() @@ -756,16 +806,17 @@ func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) choice, err := reader.ReadString('\n') if err != nil { - return "", errors.Join(fmt.Errorf("failed to read create-key choice: %w", err), removeMintedGCPKey(keyFile)) + return "", "", errors.Join(fmt.Errorf("failed to read create-key choice: %w", err), removeMintedGCPKey(keyFile)) } switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - if keyErr := createGCPServiceAccountKey(ctx, saEmail, keyFile); keyErr != nil { - return "", errors.Join(keyErr, removeMintedGCPKey(keyFile)) + keyName, err = createGCPServiceAccountKey(ctx, saEmail, keyFile) + if err != nil { + return "", "", errors.Join(err, removeMintedGCPKey(keyFile)) } fmt.Printf("Key written to temporary file: %s\n", keyFile) fmt.Println() - return keyFile, nil + return keyFile, keyName, nil case "s", "skip": fmt.Println("Skipping Create Key") default: @@ -774,7 +825,7 @@ func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) // No key file was written; the caller will prompt for an existing one. fmt.Println() - return "", removeMintedGCPKey(keyFile) + return "", "", removeMintedGCPKey(keyFile) } // mintedGCPKeyFilename is the key file's name inside its private temp dir. diff --git a/cmd/configure_gcp_test.go b/cmd/configure_gcp_test.go index 8213a4cc7..f50939fbe 100644 --- a/cmd/configure_gcp_test.go +++ b/cmd/configure_gcp_test.go @@ -152,7 +152,7 @@ func TestWriteServiceAccountKey_Success(t *testing.T) { } keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") - err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + _, err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) require.NoError(t, err) assert.True(t, m.createCalled) @@ -176,7 +176,7 @@ func TestWriteServiceAccountKey_DecodeFailureRollsBack(t *testing.T) { } keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") - err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + _, err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) require.Error(t, err) assert.Contains(t, err.Error(), "failed to decode key data") @@ -200,7 +200,7 @@ func TestWriteServiceAccountKey_RollbackFailureSurfaced(t *testing.T) { } keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") - err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + _, err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) require.Error(t, err) assert.True(t, m.deleteCalled) assert.Contains(t, err.Error(), "failed to decode key data", "the original cause must be surfaced") @@ -217,7 +217,7 @@ func TestWriteServiceAccountKey_CreateFailureNoOrphan(t *testing.T) { } keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") - err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + _, err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) require.Error(t, err) assert.Contains(t, err.Error(), "failed to create service account key") assert.False(t, m.deleteCalled, "no remote key was minted, so DeleteKey must not be called") @@ -238,7 +238,7 @@ func TestWriteServiceAccountKey_ReserveFailureNoMint(t *testing.T) { privateKeyData: base64.StdEncoding.EncodeToString([]byte("{}")), } - err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + _, err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) require.Error(t, err) assert.Contains(t, err.Error(), "failed to reserve key file") assert.False(t, m.createCalled, "the remote key must not be minted when the file cannot be reserved") @@ -251,27 +251,32 @@ func TestWriteServiceAccountKey_ReserveFailureNoMint(t *testing.T) { // --- uploadGCPCredentialsFile (minted key cleanup, #1947) -------------------- +const testMintedKeyName = "projects/proj/serviceAccounts/sa@proj.iam.gserviceaccount.com/keys/abc123" + // mintTestGCPKey mints a key into a newMintedGCPKeyPath location through the -// real writeServiceAccountKey flow and asserts it was created with 0600. -func mintTestGCPKey(t *testing.T) string { +// real writeServiceAccountKey flow, asserts it was created with 0600, and +// returns the file and the key resource name. +func mintTestGCPKey(t *testing.T) (string, string) { t.Helper() keyMaterial := []byte(`{"type":"service_account","project_id":"proj","client_email":"sa@proj.iam.gserviceaccount.com","private_key":"-----BEGIN PRIVATE KEY-----\nx\n-----END PRIVATE KEY-----\n"}`) m := &mockGCPKeyProvisioner{ - keyName: "projects/-/serviceAccounts/sa@proj.iam.gserviceaccount.com/keys/abc123", + keyName: testMintedKeyName, privateKeyData: base64.StdEncoding.EncodeToString(keyMaterial), } keyFile, err := newMintedGCPKeyPath() require.NoError(t, err) t.Cleanup(func() { _ = os.RemoveAll(filepath.Dir(keyFile)) }) - require.NoError(t, writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile)) + keyName, err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + require.NoError(t, err) + require.Equal(t, testMintedKeyName, keyName) info, err := os.Stat(keyFile) require.NoError(t, err) assert.Equal(t, os.FileMode(0600), info.Mode().Perm(), "minted key must be 0600") dirInfo, err := os.Stat(filepath.Dir(keyFile)) require.NoError(t, err) assert.Equal(t, os.FileMode(0700), dirInfo.Mode().Perm(), "minted key dir must be 0700") - return keyFile + return keyFile, keyName } func assertMintedKeyGone(t *testing.T, keyFile string) { @@ -283,55 +288,109 @@ func assertMintedKeyGone(t *testing.T, keyFile string) { } func TestUploadGCPCredentialsFile_MintedKeyRemovedAfterSuccess(t *testing.T) { - keyFile := mintTestGCPKey(t) + keyFile, keyName := mintTestGCPKey(t) store := NewMockSecretsStore() + m := &mockGCPKeyProvisioner{} - creds, err := uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, true) + creds, err := uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, keyName, m.DeleteKey) require.NoError(t, err) assert.Equal(t, "sa@proj.iam.gserviceaccount.com", creds.ClientEmail) assert.Contains(t, store.updatedSecrets, "stack-GCPCredentials") + assert.False(t, m.deleteCalled, "a successfully uploaded key must stay active in GCP") assertMintedKeyGone(t, keyFile) } -func TestUploadGCPCredentialsFile_MintedKeyRemovedAfterUploadFailure(t *testing.T) { - keyFile := mintTestGCPKey(t) +func TestUploadGCPCredentialsFile_UploadFailureDeletesRemoteKey(t *testing.T) { + keyFile, keyName := mintTestGCPKey(t) store := NewMockSecretsStore() store.updateSecretFunc = func(context.Context, string, string) error { return errors.New("access denied") } + m := &mockGCPKeyProvisioner{} - _, err := uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, true) + _, err := uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, keyName, m.DeleteKey) require.Error(t, err) assert.Contains(t, err.Error(), "access denied") - assert.Contains(t, err.Error(), "still active in GCP") + assert.True(t, m.deleteCalled, "the minted key must be deleted from GCP when the upload fails") + assert.Equal(t, testMintedKeyName, m.deletedKeyName) assertMintedKeyGone(t, keyFile) } -func TestUploadGCPCredentialsFile_MintedKeyRemovedAfterParseFailure(t *testing.T) { +func TestUploadGCPCredentialsFile_RemoteDeleteFailureNamesGcloudCommand(t *testing.T) { + keyFile, keyName := mintTestGCPKey(t) + store := NewMockSecretsStore() + store.updateSecretFunc = func(context.Context, string, string) error { return errors.New("access denied") } + m := &mockGCPKeyProvisioner{deleteErr: errors.New("permission denied")} + + _, err := uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, keyName, m.DeleteKey) + require.Error(t, err) + assert.Contains(t, err.Error(), "access denied") + assert.Contains(t, err.Error(), "permission denied") + assert.Contains(t, err.Error(), "gcloud iam service-accounts keys delete abc123 --iam-account=sa@proj.iam.gserviceaccount.com") + assert.NotContains(t, err.Error(), "PRIVATE KEY", "the error must never carry key material") + assertMintedKeyGone(t, keyFile) +} + +// TestUploadGCPCredentialsFile_CanceledContextStillCleansUp models an +// interrupt during the Secrets Manager call: the remote delete must run on a +// live context even though the upload context is canceled. +func TestUploadGCPCredentialsFile_CanceledContextStillCleansUp(t *testing.T) { + keyFile, keyName := mintTestGCPKey(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + store := NewMockSecretsStore() + store.updateSecretFunc = func(ctx context.Context, _, _ string) error { return ctx.Err() } + var deleteCtxErr error + deleted := false + deleteKey := func(ctx context.Context, name string) error { + deleted = true + deleteCtxErr = ctx.Err() + return nil + } + + _, err := uploadGCPCredentialsFile(ctx, store, "stack", keyFile, keyName, deleteKey) + require.ErrorIs(t, err, context.Canceled) + assert.True(t, deleted, "the minted key must be deleted from GCP after an interrupt") + assert.NoError(t, deleteCtxErr, "the remote delete must not inherit the canceled context") + assertMintedKeyGone(t, keyFile) +} + +func TestUploadGCPCredentialsFile_ParseFailureDeletesRemoteKey(t *testing.T) { keyFile, err := newMintedGCPKeyPath() require.NoError(t, err) t.Cleanup(func() { _ = os.RemoveAll(filepath.Dir(keyFile)) }) require.NoError(t, os.WriteFile(keyFile, []byte("not json"), 0600)) + m := &mockGCPKeyProvisioner{} - _, err = uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, true) + _, err = uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, testMintedKeyName, m.DeleteKey) require.Error(t, err) + assert.Equal(t, testMintedKeyName, m.deletedKeyName, "a minted key that cannot be parsed must still be deleted from GCP") assertMintedKeyGone(t, keyFile) } func TestUploadGCPCredentialsFile_RemovalFailureReported(t *testing.T) { - keyFile := mintTestGCPKey(t) + keyFile, keyName := mintTestGCPKey(t) // A read-only parent dir makes the unlink fail. require.NoError(t, os.Chmod(filepath.Dir(keyFile), 0500)) t.Cleanup(func() { _ = os.Chmod(filepath.Dir(keyFile), 0700) }) + m := &mockGCPKeyProvisioner{} - _, err := uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, true) + _, err := uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, keyName, m.DeleteKey) require.Error(t, err) assert.Contains(t, err.Error(), "failed to remove the minted key file") } func TestUploadGCPCredentialsFile_OperatorFileKept(t *testing.T) { - keyFile := mintTestGCPKey(t) + keyFile, _ := mintTestGCPKey(t) + m := &mockGCPKeyProvisioner{} - _, err := uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, false) + _, err := uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, "", m.DeleteKey) require.NoError(t, err) _, err = os.Stat(keyFile) assert.NoError(t, err, "an operator-supplied credentials file must not be deleted") + assert.False(t, m.deleteCalled, "an operator-supplied key must never be deleted from GCP") +} + +func TestGcloudDeleteKeyCommand(t *testing.T) { + assert.Equal(t, + "gcloud iam service-accounts keys delete abc123 --iam-account=sa@proj.iam.gserviceaccount.com", + gcloudDeleteKeyCommand(testMintedKeyName)) }