Skip to content

Allow resetting secrets - #77

Open
kevinmcconnell wants to merge 1 commit into
mainfrom
reset-secret-key
Open

Allow resetting secrets#77
kevinmcconnell wants to merge 1 commit into
mainfrom
reset-secret-key

Conversation

@kevinmcconnell

Copy link
Copy Markdown
Collaborator

If a credential like SECRET_KEY_BASE were ever leaked, we should provide a way to reset it. Previously it wasn't very clear how to do this -- technically you could use a custom env var to shadow the generated ones, but that's non-obvious, and leaves open the question of what to put in as the new one. Same story for the VAPID keys.

To address this, we do two things:

  • Move the key store away from the volume label, and into the regular application settings. Otherwise changing them requires replacing the volume, which is awkward.
  • Add a new reset-secrets command. This generates a new SECRET_KEY_BASE and/or VAPID key pair, and re-deploys the app to use them.

Copilot AI review requested due to automatic review settings July 29, 2026 16:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds first-class support for rotating application secrets (e.g., SECRET_KEY_BASE, VAPID keys) by moving key storage into application settings and introducing a reset-secrets command, while keeping backup/restore compatible with older Once versions.

Changes:

  • Introduce KeysSettings stored in ApplicationSettings, with generation/rotation helpers and updated env building.
  • Add once reset-secrets command to rotate SECRET_KEY_BASE and/or VAPID keys and redeploy.
  • Maintain backward-compatible backup/restore by duplicating keys into the legacy volume-settings backup entry and adopting legacy keys on restore.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/docker/volume.go Removes key generation/volume-based key access now that keys live in app settings.
internal/docker/volume_test.go Removes tests tied to volume-based key generation/storage.
internal/docker/namespace.go Updates backup parsing to return only app settings + data, adopting legacy keys into app settings when needed.
internal/docker/keys.go New key settings type plus key generation/rotation helpers.
internal/docker/keys_test.go Unit tests for key generation/rotation and VAPID encoding/uniqueness.
internal/docker/application.go Ensures keys exist before deploy; adds ResetSecrets API; switches env construction to use app settings keys.
internal/docker/application_settings.go Adds Keys to app settings and switches BuildEnv to use it.
internal/docker/application_settings_test.go Updates env tests and adds marshal/equality coverage for Keys.
internal/docker/application_backup.go Restores without volume key settings, duplicates keys into legacy backup entry for compatibility.
internal/docker/application_backup_test.go Adds coverage for legacy-key adoption and precedence during backup parsing.
internal/command/root.go Registers the new reset-secrets command.
internal/command/reset_secrets.go New CLI command for rotating secrets with confirmation/--yes.
internal/command/reset_secrets_test.go Unit tests for confirmation parsing and message formatting.
integration/docker_test.go Updates integration tests for new key storage and adds integration coverage for secret rotation + migration.
Comments suppressed due to low confidence (2)

internal/docker/keys.go:52

  • generateVAPIDKeyPair panics on GenerateKey error. This is production code in a CLI; panicking will crash the process and print a stack trace. Prefer returning an error and letting callers (e.g., deploy/reset-secrets) surface a clean message and abort safely.
func generateVAPIDKeyPair() (publicKey, privateKey string) {
	key, err := ecdh.P256().GenerateKey(rand.Reader)
	if err != nil {
		panic(err)
	}

internal/docker/application_backup.go:133

  • Restore runs the post-restore hook container using a.Settings.BuildEnv() before ensuring keys exist. If a.Settings.Keys is empty (e.g., malformed/partial backup metadata), the hook runs with blank SECRET_KEY_BASE/VAPID env vars. Ensuring keys immediately after creating the volume avoids that failure mode.
	vol, err := CreateVolume(ctx, a.namespace, a.Settings.Name, ApplicationVolumeSettings{})
	if err != nil {
		return fmt.Errorf("creating volume: %w", err)
	}


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/docker/keys.go
Comment thread internal/command/reset_secrets.go Outdated
Copilot AI review requested due to automatic review settings July 29, 2026 16:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/command/reset_secrets.go:35

  • MarkFlagsOneRequired returns an error (e.g., if a flag name is wrong). Ignoring it can trip errcheck/golangci-lint and would hide a misconfigured CLI flag setup until runtime.
	r.cmd.MarkFlagsOneRequired("base", "vapid")

internal/docker/keys.go:52

  • generateVAPIDKeyPair panics on key generation errors. A crypto/rand failure is rare, but if it happens this will crash the CLI instead of returning a normal error path (previously this code returned errors). Consider returning an error from key generation (and from GenerateKeys / rotation helpers) and plumb it up to callers so the user gets a helpful message and the app can roll back safely.
func generateVAPIDKeyPair() (publicKey, privateKey string) {
	key, err := ecdh.P256().GenerateKey(rand.Reader)
	if err != nil {
		panic(err) // Error can only happen on randomness failure
	}

Copilot AI review requested due to automatic review settings July 30, 2026 10:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/docker/keys.go:52

  • generateVAPIDKeyPair panics on RNG failure. Even if this is extremely rare, a panic will crash the CLI during deploy/reset and prevents callers from returning a meaningful error (and potentially from rolling back state cleanly). Consider changing key generation helpers (and GenerateKeys / rotation helpers) to return an error and have callers propagate it instead of panicking.
func generateVAPIDKeyPair() (publicKey, privateKey string) {
	key, err := ecdh.P256().GenerateKey(rand.Reader)
	if err != nil {
		panic(err) // Error can only happen on randomness failure
	}

internal/command/reset_secrets.go:36

  • The error return from MarkFlagsOneRequired is ignored. If the flag names change or registration fails, this will silently disable the intended validation. Since this is a programming/configuration-time error, it’s better to fail fast here.
	r.cmd.Flags().BoolVar(&r.secretKeyBase, "secret-key-base", false, "generate a new SECRET_KEY_BASE")
	r.cmd.Flags().BoolVar(&r.vapid, "vapid", false, "generate a new VAPID key pair")
	r.cmd.Flags().BoolVarP(&r.yes, "yes", "y", false, "skip the confirmation prompt")
	r.cmd.MarkFlagsOneRequired("secret-key-base", "vapid")
	return r

Copilot AI review requested due to automatic review settings July 30, 2026 11:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

internal/command/reset_secrets.go:35

  • MarkFlagsOneRequired returns an error (e.g. if a flag name changes). Ignoring it makes future refactors fail silently and can leave the command without the intended validation. Use cobra.CheckErr (or equivalent) to fail fast if the required flags can’t be marked.
	r.cmd.MarkFlagsOneRequired("secret-key-base", "vapid")

internal/docker/application.go:213

  • ResetSecrets redeploys but never pulls the image first. If the image was pruned/missing locally, this will fail at ContainerCreate even though Deploy/Update handle this by calling pullImage up-front. Consider pulling before mutating keys and redeploying so the reset command behaves reliably.
func (a *Application) ResetSecrets(ctx context.Context, resetBase, resetVAPID bool, progress DeployProgressCallback) error {
	vol, err := a.Volume(ctx)
	if err != nil {
		return fmt.Errorf("getting volume: %w", err)
	}
	a.ensureKeys(vol)

internal/docker/keys.go:55

  • generateVAPIDKeyPair discards the error from ecdh.P256().GenerateKey. If key generation ever fails (e.g. RNG failure), key will be nil and the subsequent key.Bytes() / key.PublicKey() calls will panic with an unclear nil dereference. Handle the error explicitly so failures are predictable and diagnosable.
	key, _ := ecdh.P256().GenerateKey(rand.Reader) // err is always nil on Go 1.26 and later

If a credential like `SECRET_KEY_BASE` were ever leaked, we should
provide a way to reset it. Previously it wasn't very clear how to do
this -- technically you could use a custom env var to shadow the
generated ones, but that's non-obvious, and leaves open the question of
what to put in as the new one. Same story for the VAPID keys.

To address this, we do two things:

- Move the key store away from the volume label, and into the regular
  application settings. Otherwise changing them requires replacing the
  volume, which is awkward.
- Add a new `reset-secrets` command. This generates a new
  `SECRET_KEY_BASE` and/or VAPID key pair, and re-deploys the app to use
  them.
Copilot AI review requested due to automatic review settings July 30, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/docker/keys.go:135

  • generateVAPIDKeyPair panics if key generation fails. This can crash the CLI during deploy/restore/reset paths (since GenerateKeys() is used for default key creation) instead of returning a user-actionable error. It would be more robust to return an error and plumb it up so callers can surface a friendly message (and so tests can cover the failure path).
	key, err := ecdh.P256().GenerateKey(rand.Reader)
	if err != nil {
		// Randomness failure; no fallback possible
		panic(fmt.Errorf("generating VAPID key pair: %w", err))
	}

internal/docker/keys.go:120

  • KeysSettings.WithReset can return a partially updated KeysSettings when applying the VAPID private key fails (e.g., if reset includes both SecretKeyBase/GenerateSecretKeyBase and an invalid VAPIDPrivateKey). That makes it easier for a caller to accidentally persist a half-applied reset if they don’t discard the returned value on error. Consider applying changes to a copy and returning the original k on any error.

This issue also appears on line 131 of the same file.

	case reset.VAPIDPrivateKey != "":
		var err error
		if k, err = k.WithVAPIDKeysFromPrivateKey(reset.VAPIDPrivateKey); err != nil {
			return k, err
		}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants