diff --git a/cmd/deploy/secrets.go b/cmd/deploy/secrets.go index e3137540..128a61e9 100644 --- a/cmd/deploy/secrets.go +++ b/cmd/deploy/secrets.go @@ -27,6 +27,7 @@ var ( secretsAllowMissing bool secretsYes bool secretsModules []string + secretsRotate []string ) // SecretsCmd seeds an environment's secret store with what its render reads. @@ -43,9 +44,9 @@ operator, and named). The store is the backend behind the SecretStore the render names, read from the environment's cluster; a render reading through several stores is planned and written store by store. -A stored value is never rotated: a property the store already holds is kept even -when a configuration file holds one too, and the two disagreeing is refused -rather than resolved. A configured key with an empty value supplies nothing, so +A stored value is never rotated unless --rotate names it: a property the store +already holds is kept even when a configuration file holds one too, and the two +disagreeing is refused rather than resolved. A configured key with an empty value supplies nothing, so a tracked placeholder file still leaves its properties to be supplied. Each property is resolved by the secret key the render reads out of it, not by @@ -60,7 +61,19 @@ reads a stored value: it knows which remote keys exist, not what they hold. --module limits the plan to the remote keys the named modules' services read. Every other remote key is still read, so a scoped key keeps agreeing with what -the store already holds, but nothing outside the scope is planned or written.`, +the store already holds, but nothing outside the scope is planned or written. + +--rotate //: (or : for a +workspace configuration, or the CODEFLY__… secret key) replaces a GENERATED key's +stored value with a new one, in every remote key holding it, and rewrites every +stored value derived from it in the same apply: a stored URL whose password is +the old value gets the new one. Any other stored value embedding the old one, +and any key service-secrets.generate does not declare, is refused by name. Each +write is a new version, refused if the key moved since the plan read it; a +rotation failing part-way restores what it wrote. The CLI changes the store +only: force-sync the ExternalSecrets, make the server accept the new credential +(a restart only does that for an agent that re-applies it on every start), then +restart its consumers. An identifier or signing key is never rotated.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { ctx, done := common.NewContext() @@ -92,6 +105,11 @@ the store already holds, but nothing outside the scope is planned or written.`, return fmt.Errorf("--module %s: not a module rendered for %s (rendered: %s)", module, env.Name, strings.Join(rendered.Modules, ", ")) } } + groups := renderedByStore(rendered) + rotate, err := rotationKeys(rendered, groups, secretsRotate) + if err != nil { + return err + } // The operator's own configuration files, read once for the whole // environment: they are a property of the workspace, not of a store, so // every per-store plan below resolves against the same index. @@ -103,7 +121,7 @@ the store already holds, but nothing outside the scope is planned or written.`, // per-key store such as an identity provider's) is planned and written // store by store: one plan writes one store, and each is confirmed on // its own. - for _, group := range renderedByStore(rendered) { + for _, group := range groups { store, err := resolveSecretStore(ctx, env, group) if err != nil { return err @@ -117,6 +135,7 @@ the store already holds, but nothing outside the scope is planned or written.`, Modules: secretsModules, MayBeEmpty: env.ServiceSecrets.MayBeEmpty, Configured: configured, + Rotate: readBy(group, rotate), }) if err != nil { return err @@ -176,12 +195,48 @@ func finishSecretsPlan(ctx context.Context, plan *deploysecrets.Plan, store depl return fmt.Errorf("write not confirmed") } written, err := plan.Apply(ctx, store, allowMissing) - for _, key := range written { - cli.Info("Wrote %s", key) + for _, write := range written { + verb := "Wrote" + if write.Restored { + verb = "Restored the previous document of" + } + cli.Info("%s %s as version %s (%s)", verb, write.Key, write.Version, strings.Join(write.Properties, ", ")) } return err } +// rotationKeys resolves every --rotate selector, refusing one no rendered +// ExternalSecret reads or whose configuration spans stores. +func rotationKeys(rendered gitops.RenderedEnvironment, groups []gitops.RenderedEnvironment, selectors []string) ([]string, error) { + keys := make([]string, 0, len(selectors)) + for _, selector := range selectors { + key, err := deploysecrets.RotationKey(selector) + if err != nil { + return nil, err + } + if !deploysecrets.ReadsKey(rendered, key) { + return nil, fmt.Errorf("--rotate %s: no rendered ExternalSecret reads %s", selector, key) + } + if err = deploysecrets.CheckRotationStores(groups, key); err != nil { + return nil, err + } + keys = append(keys, key) + } + return keys, nil +} + +// readBy keeps the rotated keys one store group reads; each is rotated by the +// plan of the store holding it. +func readBy(group gitops.RenderedEnvironment, keys []string) []string { + var read []string + for _, key := range keys { + if deploysecrets.ReadsKey(group, key) { + read = append(read, key) + } + } + return read +} + // resolveSecretStore resolves the one store every ExternalSecret of a // renderedByStore group reads through, from the environment's cluster. A mixed // group is a programming error and is refused by name. @@ -271,4 +326,5 @@ func init() { SecretsCmd.Flags().BoolVar(&secretsAllowMissing, "allow-missing", false, "Write what can be resolved even when some properties must still be supplied") SecretsCmd.Flags().BoolVarP(&secretsYes, "yes", "y", false, "Write without an interactive confirmation") SecretsCmd.Flags().StringSliceVar(&secretsModules, "module", nil, "Plan only these modules' remote keys (comma-separated or repeated)") + SecretsCmd.Flags().StringSliceVar(&secretsRotate, "rotate", nil, "Replace a generated key's stored value and every stored value derived from it: //:, :, or the CODEFLY__ key (repeatable)") } diff --git a/cmd/deploy/secrets_test.go b/cmd/deploy/secrets_test.go index 1ace2704..1abba0fd 100644 --- a/cmd/deploy/secrets_test.go +++ b/cmd/deploy/secrets_test.go @@ -125,9 +125,9 @@ func (store *outputSafetyStore) Describe(context.Context, string) (deploysecrets func (store *outputSafetyStore) Read(context.Context, string) (map[string]string, error) { return store.document, nil } -func (store *outputSafetyStore) Write(context.Context, string, map[string]string, bool) error { +func (store *outputSafetyStore) Write(context.Context, string, map[string]string, bool) (string, error) { store.writes++ - return nil + return "1", nil } func TestSecretsCommandRefusesMissingNoOpBeforeConfirmation(t *testing.T) { @@ -152,3 +152,22 @@ func TestSecretsCommandRefusesMissingNoOpBeforeConfirmation(t *testing.T) { t.Fatal("explicit allow-missing should permit a no-op") } } + +// A --rotate selector resolves to the key a rendered ExternalSecret reads, is +// refused when none reads it, and reaches only the store group reading it. +func TestRotationKeysResolveAgainstTheRender(t *testing.T) { + const key = "CODEFLY__SERVICE_SECRET_CONFIGURATION__TRACING__EVIDENCE__CLICKHOUSE__PASSWORD" + rendered := gitops.RenderedEnvironment{Secrets: []gitops.RenderedServiceSecret{{RemoteKey: "evidence", Services: []string{"tracing/evidence"}, + Properties: []gitops.RenderedSecretProperty{{Property: key, Keys: []string{key}}}}}} + groups := renderedByStore(rendered) + keys, err := rotationKeys(rendered, groups, []string{"tracing/evidence/clickhouse:PASSWORD"}) + if err != nil || len(keys) != 1 || keys[0] != key { + t.Fatalf("rotationKeys = %v, %v; want %s", keys, err, key) + } + if got := readBy(gitops.RenderedEnvironment{}, keys); len(got) != 0 { + t.Errorf("a group reading nothing was handed %v", got) + } + if _, err = rotationKeys(rendered, groups, []string{"tracing/evidence/clickhouse:USER"}); err == nil || !strings.Contains(err.Error(), "no rendered ExternalSecret reads") { + t.Errorf("an unread key = %v, want a refusal", err) + } +} diff --git a/docs/cli-reference.md b/docs/cli-reference.md index a3dacfe3..ddfdbcd5 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -2295,9 +2295,9 @@ operator, and named). The store is the backend behind the SecretStore the render names, read from the environment's cluster; a render reading through several stores is planned and written store by store. -A stored value is never rotated: a property the store already holds is kept even -when a configuration file holds one too, and the two disagreeing is refused -rather than resolved. A configured key with an empty value supplies nothing, so +A stored value is never rotated unless --rotate names it: a property the store +already holds is kept even when a configuration file holds one too, and the two +disagreeing is refused rather than resolved. A configured key with an empty value supplies nothing, so a tracked placeholder file still leaves its properties to be supplied. Each property is resolved by the secret key the render reads out of it, not by @@ -2313,6 +2313,18 @@ reads a stored value: it knows which remote keys exist, not what they hold. --module limits the plan to the remote keys the named modules' services read. Every other remote key is still read, so a scoped key keeps agreeing with what the store already holds, but nothing outside the scope is planned or written. + +--rotate //: (or : for a +workspace configuration, or the CODEFLY__… secret key) replaces a GENERATED key's +stored value with a new one, in every remote key holding it, and rewrites every +stored value derived from it in the same apply: a stored URL whose password is +the old value gets the new one. Any other stored value embedding the old one, +and any key service-secrets.generate does not declare, is refused by name. Each +write is a new version, refused if the key moved since the plan read it; a +rotation failing part-way restores what it wrote. The CLI changes the store +only: force-sync the ExternalSecrets, make the server accept the new credential +(a restart only does that for an agent that re-applies it on every start), then +restart its consumers. An identifier or signing key is never rotated. ``` ``` @@ -2327,6 +2339,7 @@ Flags: --env string Environment whose rendered ExternalSecrets to seed (default "local") --metadata-only With --dry-run: never read a stored value, only which remote keys exist --module strings Plan only these modules' remote keys (comma-separated or repeated) + --rotate strings Replace a generated key's stored value and every stored value derived from it: //:, :, or the CODEFLY__ key (repeatable) -y, --yes Write without an interactive confirmation ``` diff --git a/docs/commands.md b/docs/commands.md index ca23799e..8507d4d7 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -1214,6 +1214,7 @@ codefly deploy secrets --env staging --dry-run --metadata-only # which keys exi codefly deploy secrets --env staging --dry-run # full plan; reads the store in memory codefly deploy secrets --env staging # write it (confirms; --yes to skip) codefly deploy secrets --env staging --module runtime --dry-run # one module's keys only +codefly deploy secrets --env staging --rotate tracing/evidence/clickhouse:CLICKHOUSE_PASSWORD # rotate a generated key ``` It reads every `ExternalSecret` the render projected for `--env` under @@ -1224,11 +1225,12 @@ resolves every remote property to a source, never printing a value: | action | source | | --- | --- | -| `keep` | the store already holds it; nothing stored is ever rotated | +| `keep` | the store already holds it; nothing stored is rotated unless `--rotate` names it | | `propagate` | a configuration value (`CODEFLY__WORKSPACE_SECRET_CONFIGURATION__…`, `CODEFLY__SERVICE_SECRET_CONFIGURATION__…`) is one value however many services read it, so it is copied from the remote key that holds it | | `configured` | the environment's own `configurations//.secret.env` holds the value — the file `codefly config set` writes — and the plan seeds the store from it, naming the file as its source | | `generate` | declared random by `service-secrets.generate` (below) | | `require` | nothing produces it: the operator supplies it — an external credential, or a value its producing agent derives | +| `rotate` | `--rotate` names its generated key, or its stored value is derived from one that is ([below](#rotating-a-generated-key)) | **`configured`: the environment's own files are a source.** `codefly config set KEY=VALUE --env ` writes @@ -1251,8 +1253,9 @@ key reaches its value whichever way the group or key was spelled. Four rules bound it: -- **A stored value still wins and is never rotated.** A property the store - already holds is `keep`, file or no file. +- **A stored value still wins and is never rotated** (bar an explicit + `--rotate`, which refuses a configured key). A property the store already + holds is `keep`, file or no file. - **A stored value that differs from the configured one is refused**, like two remote keys disagreeing: keeping it would leave the deployed workload on one value and every local run on the other, reported as `keep`. Change the @@ -1356,6 +1359,83 @@ service already signs with is never replaced by a second run. It is one workload's key: a plan in which more than one service reads it is refused, and the remedy is to remove the extra reader, never to re-mint the key. +#### Rotating a generated key + +`--rotate` replaces a stored value. It is explicit and narrow: + +```bash +codefly deploy secrets --env blue --rotate tracing/evidence/clickhouse:CLICKHOUSE_PASSWORD --dry-run +codefly deploy secrets --env blue --rotate tracing/evidence/clickhouse:CLICKHOUSE_PASSWORD +``` + +The selector is spelled like the `service-secrets.generate` entry that declares +the key — `//:`, `:` +for a workspace configuration — or is the `CODEFLY__…_SECRET_CONFIGURATION__…` +key itself; it is repeatable. Only a key `service-secrets.generate` declares is +rotated. Each of these is refused with its reason: a supplied key (`require`); a +configured one (change the file instead); an `ed25519-pkcs8` signing key (its +public half is registered elsewhere); an `identifier` key (a new user or +database name in the store renames nothing on the server); and a key no rendered +ExternalSecret reads. + +The new value is minted as the generator would mint it and written to **every** +remote key holding the old one; a holder outside `--module` is refused rather +than left behind. Then every **derived** value moves in the same apply. What is +derived is read from the store, not guessed from a name: every stored property +of the store is examined, and one that parses as a URL whose **decoded** userinfo +password is the old value — the connection an agent exports without a template, +stored by hand because the plan reported it `require` — has that password +replaced in its userinfo and every other byte kept. Any other stored value +holding the old one, raw or percent-encoded, is refused, naming +`remote-key#property`, because codefly cannot recompute it and a stale derived +value is the failure rotation must not leave. A connection its producer declares +as a template (below) is not stored at all: External Secrets re-assembles it from +the rotated primitive on its next sync. A configuration read through more than +one store is refused: a plan sees one store. **What is not seen:** a copy of the +value in another store, or one that no rendered ExternalSecret reads (a value +outside codefly's render), is not examined; find those before rotating. + +`--dry-run` shows `rotate` rows by property name, no value; with +`--metadata-only` it shows the rotated key's rows only, since derived values are +found by reading the store. Each write adds a **new version** (nothing is +destroyed) and the result lists each remote key, the properties it set and the +version the store assigned. Before each write the key's newest version is +compared with the one the plan read, and a key that moved is refused by name +(the store has no conditional write, so this narrows the race; it does not close +it). A rotation failing part-way writes the previous document of every key it +wrote back as a new version — including the failing key when its write landed +anyway, and never a key someone else wrote since — on a context of its own, so +the source and its derived values still agree and the same command can be run +again. If a restore fails too, the error names the keys that may still hold the +rotated document. After success a plain `deploy secrets` re-run writes nothing; +running `--rotate` again rotates again. + +**The CLI changes the store only — not the server's credential.** Whether a new +value in the store changes the credential the server accepts is the producing +agent's behaviour, and the CLI cannot tell which an agent does: + +- An agent that **re-applies the credential from its environment on every + start** follows a restart. The ClickHouse agent is one: its entrypoint + rewrites `users.d` from `CLICKHOUSE_USER`/`CLICKHOUSE_PASSWORD` on every start + (service-clickhouse `main.go`). +- An agent whose image applies it **only when the data directory is first + created** (the official Postgres image, for one) does not: after a restart the + store and every consumer URL carry the new password while the database user + keeps the old one. Change the credential on the server yourself (for Postgres, + `ALTER ROLE … PASSWORD …` with the new value) before restarting consumers, or + do not rotate that key here. + +Then, in order: + +1. force-sync the affected ExternalSecrets (`kubectl annotate externalsecret force-sync=$(date +%s) --overwrite`, or wait for their refresh interval); +2. restart (or re-credential, above) the server that reads the rotated value; +3. restart its consumers, which read the derived connection. + +**Undo.** Every earlier version is kept. To go back, make each written key's +previous version current again by adding it as a new version, without printing +it — `gcloud secrets versions access --secret --project | gcloud secrets versions add --project --data-file=-` +for each key the result listed — then repeat the three steps above. + A key its producer declares as a template (core's `ConfigurationValue.template` — a Postgres agent's `READ_WRITE_CONNECTION`, say: role, address and database around a reference to its read-write password) is never read from the store. diff --git a/docs/deployment-secret-safety.md b/docs/deployment-secret-safety.md index c28c858b..62578697 100644 --- a/docs/deployment-secret-safety.md +++ b/docs/deployment-secret-safety.md @@ -22,7 +22,15 @@ Older rendered ExternalSecrets already contain the `secretKey` and `remoteRef` needed for resolution. There is no render-inventory migration. Valid stored values remain unchanged. A seed operation does not rotate them or -prove that a manually seeded value authenticates to its intended service. +prove that a manually seeded value authenticates to its intended service. Only +`--rotate` replaces a stored value, and only a generated one: it rewrites a +stored URL whose decoded password is the old value and refuses any other stored +value holding it, raw or percent-encoded, so no derived value in the store is +left on the old one. It does not change the credential the server accepts, and +it does not see a copy outside the store it plans. Its output names keys, +properties and store versions; the old and new values are never printed, and a +derived URL that fails to parse is refused without echoing it +([commands](commands.md#rotating-a-generated-key)). Configuration aliases must agree across every referenced property, including properties in one document. Aliases use one generated value when none exists, or reuse the consistent value already held anywhere in that alias group. diff --git a/docs/development-loop.md b/docs/development-loop.md index 25bf628c..45cb2310 100644 --- a/docs/development-loop.md +++ b/docs/development-loop.md @@ -25,7 +25,7 @@ edit ──▶ codefly run service local, no image build seconds |---|---|---|---| | Run it locally | `codefly run service ` | Starts the service and its dependency graph on this machine, injecting connection strings as env vars. No image is built — a docker-backed dependency runs from an image it already has. | Seconds. This is the inner loop; stay in it as long as you can. | | Render | `codefly deploy gitops render --env ` | Resolves the module, builds and pushes every service image, writes the promotable tree under `deployments/modules//` and records `.codefly-render.json`. | Minutes — the image builds. Images whose inputs did not change are kept, so a configuration-only edit costs the render and no build. | -| Seed secrets | `codefly deploy secrets --env ` | Reads the ExternalSecrets the render projected and resolves each remote property to a source — kept, derived, propagated, generated or required — then writes the store. Values are never printed. | One backend read per remote key, eight at a time: seconds for a small environment, longer for a large one. Run it after a render that added a service or a secret key. | +| Seed secrets | `codefly deploy secrets --env ` | Reads the ExternalSecrets the render projected and resolves each remote property to a source — kept, derived, propagated, generated or required — then writes the store; `--rotate` replaces a generated value and every stored value derived from it. Values are never printed. | One backend read per remote key, eight at a time: seconds for a small environment, longer for a large one. Run it after a render that added a service or a secret key. | | Plan | `codefly deploy gitops plan --env ` | Shows the exact publication diff — what publishing would change in the GitOps repository — and writes nothing. | Seconds. | | Publish | `codefly deploy gitops publish --env ` | Creates a signed promotion commit and opens or updates its pull request. The cluster changes when that lands and Argo CD syncs it, never from this command. | Seconds. | | Observe | `codefly deploy gitops observe --env ` | Verifies Argo CD reconciled the reviewed revision and stores the evidence. | As long as the rollout takes. See [deployment-completion.md](deployment-completion.md). | diff --git a/pkg/deploysecrets/plan.go b/pkg/deploysecrets/plan.go index 76f9c19c..e25b932c 100644 --- a/pkg/deploysecrets/plan.go +++ b/pkg/deploysecrets/plan.go @@ -7,7 +7,8 @@ // from, and leaves the store to hold them. This package reads those names back // from the render and resolves each one to a source, in order: // -// 1. keep — the store already holds it; a value is never rotated here; +// 1. keep — the store already holds it; a stored value is never rotated +// unless the operator names its generated key with --rotate (rotate.go); // 2. propagate — a configuration value is one value however many services read // it, so a key another remote secret already holds is copied from there; // 3. configured — the environment's own configuration holds the value, in the @@ -66,6 +67,9 @@ const ( // ActionUnverified: the remote key exists but was not read, so whether it // holds the property is unknown. Source says what would happen if it does not. ActionUnverified Action = "unverified" + // ActionRotate: the property is stored and replaced, because --rotate names + // its generated key or because its stored value is derived from one that is. + ActionRotate Action = "rotate" ) // PropertyPlan is the plan for one property of one remote key. It carries no @@ -104,12 +108,32 @@ type Plan struct { Secrets []SecretPlan writes []pendingWrite + // rotating makes Apply all-or-nothing: a rotation half-written is a source + // and its derived values disagreeing, so a failed write restores the rest. + rotating bool } type pendingWrite struct { key string document map[string]string create bool + // changed names the properties this write sets; previous is the document it + // replaces (empty for a key it creates), which a failed rotation restores. + changed []string + previous map[string]string + // version is the newest version the plan read; the write is refused when + // the key no longer has it. + version string +} + +// Written is one remote key an apply wrote: the properties it set and the +// version the store assigned. Restored marks the write of the previous document +// after a rotation failed part-way. It carries no value. +type Written struct { + Key string + Properties []string + Version string + Restored bool } // Changes lists the remote keys an apply would write, in the order it would @@ -175,6 +199,10 @@ type Inputs struct { // considered, and reported as ActionConfigured naming the file. Nil plans // without the operator's files, exactly as before they were read. Configured map[string]ConfiguredValue + // Rotate are generated secret keys (RotationKey) whose stored value is + // replaced with a newly generated one, together with every stored value + // derived from it. A key no generator declares is refused. + Rotate []string } // remoteState is one remote key as the plan sees it. @@ -225,6 +253,11 @@ func Build(ctx context.Context, in *Inputs) (*Plan, error) { // generated holds one value per stored configuration key, so every remote key // reading it receives the same one. generated := map[string]string{} + rotating, derived, err := planRotations(in, states, generators, generated, aliases) + if err != nil { + return nil, err + } + plan.rotating = len(rotating) > 0 for _, state := range states { if !planning(&state.secret, in.Modules) { continue @@ -238,11 +271,17 @@ func Build(ctx context.Context, in *Inputs) (*Plan, error) { } changes := map[string]string{} for _, property := range state.secret.Properties { + location := propertyLocation{state.secret.RemoteKey, property.Property} propertyPlan, value, err := planConfigured(state, property, states, generators, generated, in.Configured, - aliases[propertyLocation{state.secret.RemoteKey, property.Property}]) + aliases[location], rotating) if err != nil { return nil, err } + if rewrite, ok := derived[location]; ok { + propertyPlan = PropertyPlan{Property: property.Property, Keys: property.Keys, Action: ActionRotate, + Source: "derived: the stored URL's password follows rotated " + strings.Join(rewrite.from, ", ")} + value = rewrite.value + } if value != "" { changes[property.Property] = value } @@ -255,7 +294,8 @@ func Build(ctx context.Context, in *Inputs) (*Plan, error) { if len(changes) > 0 && state.known() { document := maps.Clone(state.document) maps.Copy(document, changes) - plan.writes = append(plan.writes, pendingWrite{key: state.secret.RemoteKey, document: document, create: !state.description.Exists}) + plan.writes = append(plan.writes, pendingWrite{key: state.secret.RemoteKey, document: document, create: !state.description.Exists, + changed: slices.Sorted(maps.Keys(changes)), previous: state.document, version: state.description.Version}) } } return plan, nil @@ -491,7 +531,7 @@ func refuseSharedSigningKey(state *remoteState, property gitops.RenderedSecretPr // only consulted for a property nothing holds. func planConfigured(state *remoteState, property gitops.RenderedSecretProperty, states []*remoteState, generators map[string]environments.EnvironmentSecretGenerator, generated map[string]string, - configured map[string]ConfiguredValue, keys []string) (PropertyPlan, string, error) { + configured map[string]ConfiguredValue, keys []string, rotating map[string]bool) (PropertyPlan, string, error) { lookup := property lookup.Keys = keys declaredKey, generator, err := declaredGenerator(lookup, generators) @@ -541,6 +581,12 @@ func planConfigured(state *remoteState, property gitops.RenderedSecretProperty, return PropertyPlan{}, "", fmt.Errorf("%s holds different values in %s and %s: one configuration value cannot be two, and a stored value is never rotated — change the configuration to match what is stored, or remove the stored property so the plan seeds it from the file", strings.Join(property.Keys, ", "), strings.Join(holders, ", "), configuredValue.File) } + if declaredKey != "" && rotating[declaredKey] { + if hasConfigured { + return PropertyPlan{}, "", fmt.Errorf("--rotate %s: the value is configured in %s, which the store must keep agreeing with; change the file instead", declaredKey, configuredValue.File) + } + return rotatedProperty(state, property, declaredKey, generated[declaredKey]) + } if state.known() { if stored, present := state.document[property.Property]; present { if len(holders) > 0 && stored != holderValue { @@ -652,25 +698,53 @@ func generatorIndex(generators []environments.EnvironmentSecretGenerator, servic return index, nil } -// Apply writes the plan's changes, in the render's order. It refuses a plan -// that did not read the store — it would overwrite properties it never saw — -// and, unless allowMissing, one that leaves a property the operator must -// supply. It returns the keys written, which on error are the ones written -// before it. -func (plan *Plan) Apply(ctx context.Context, store Store, allowMissing bool) ([]string, error) { +// Apply writes the plan's changes, in the render's order, each as a new +// version. It refuses a plan that did not read the store — it would overwrite +// properties it never saw — and, unless allowMissing, one that leaves a +// property the operator must supply. It returns what it wrote, which on error +// is what it wrote before it — and, for a rotation, the restores (restore). +func (plan *Plan) Apply(ctx context.Context, store Store, allowMissing bool) ([]Written, error) { if err := plan.ValidateApply(allowMissing); err != nil { return nil, err } - var written []string - for _, write := range plan.writes { - if err := store.Write(ctx, write.key, write.document, write.create); err != nil { - return written, fmt.Errorf("write %s: %w", write.key, err) + var written []Written + for i, write := range plan.writes { + if err := unchanged(ctx, store, &write); err != nil { + if plan.rotating { + return restore(ctx, store, written, plan.writes[:i], nil, err) + } + return written, err } - written = append(written, write.key) + version, err := store.Write(ctx, write.key, write.document, write.create) + if err != nil { + err = fmt.Errorf("write %s: %w", write.key, err) + if plan.rotating { + // The failed write may still have landed, so it is restored too. + return restore(ctx, store, written, plan.writes[:i], &plan.writes[i], err) + } + return written, err + } + written = append(written, Written{Key: write.key, Properties: write.changed, Version: version}) } return written, nil } +// unchanged refuses a write to a key that is no longer as the plan read it: a +// document built from what it used to hold would silently undo whatever was +// written since. The store offers no conditional version add, so this narrows +// the window to one describe; it does not close it. +func unchanged(ctx context.Context, store Store, write *pendingWrite) error { + current, err := store.Describe(ctx, write.key) + if err != nil { + return wrapKeyError("describe", write.key, err) + } + if current.Exists != !write.create || current.Version != write.version { + return fmt.Errorf("remote key %s changed after the plan read it (version %q, now %q): re-run to plan against what it holds now", + write.key, write.version, current.Version) + } + return nil +} + // ValidateApply checks completeness before confirmation, no-op success or writes. func (plan *Plan) ValidateApply(allowMissing bool) error { if plan.Unverified() { diff --git a/pkg/deploysecrets/plan_test.go b/pkg/deploysecrets/plan_test.go index 02dd6eb2..78abee51 100644 --- a/pkg/deploysecrets/plan_test.go +++ b/pkg/deploysecrets/plan_test.go @@ -29,6 +29,11 @@ type fakeStore struct { mutex sync.Mutex reads []string writes []string + // versions counts each key's versions; failWrite fails the write of a key, + // and landThenFail stores it and then reports a failure. + versions map[string]int + failWrite map[string]bool + landThenFail map[string]bool } func newFakeStore(documents map[string]map[string]string) *fakeStore { @@ -44,7 +49,11 @@ func (store *fakeStore) Describe(_ context.Context, key string) (Description, er return Description{Exists: true}, nil } _, exists := store.documents[key] - return Description{Exists: exists, HasVersion: exists}, nil + description := Description{Exists: exists, HasVersion: exists} + if exists { + description.Version = fmt.Sprint(store.versions[key]) + } + return description, nil } func (store *fakeStore) Read(_ context.Context, key string) (map[string]string, error) { @@ -58,15 +67,34 @@ func (store *fakeStore) Read(_ context.Context, key string) (map[string]string, return maps.Clone(document), nil } -func (store *fakeStore) Write(_ context.Context, key string, document map[string]string, create bool) error { +func (store *fakeStore) Write(_ context.Context, key string, document map[string]string, create bool) (string, error) { _, exists := store.documents[key] if create == exists && !store.empty[key] { - return fmt.Errorf("write %s with create=%v on a key that exists=%v", key, create, exists) + return "", fmt.Errorf("write %s with create=%v on a key that exists=%v", key, create, exists) + } + if store.failWrite[key] { + return "", fmt.Errorf("write %s refused", key) } store.writes = append(store.writes, key) store.documents[key] = maps.Clone(document) delete(store.empty, key) - return nil + if store.versions == nil { + store.versions = map[string]int{} + } + store.versions[key]++ + if store.landThenFail[key] { + delete(store.landThenFail, key) + return "", fmt.Errorf("write %s timed out", key) + } + return fmt.Sprint(store.versions[key]), nil +} + +func writtenKeys(written []Written) []string { + keys := make([]string, 0, len(written)) + for _, write := range written { + keys = append(keys, write.Key) + } + return keys } const ( @@ -187,7 +215,7 @@ func TestPlanSeedsANewModuleConsistentlyWithTheSeededCell(t *testing.T) { t.Fatalf("Apply: %v", err) } // Writes follow the render's order, and only keys with something to write. - if !slices.Equal(written, []string{"notes-backend", "tasks-store"}) { + if !slices.Equal(writtenKeys(written), []string{"notes-backend", "tasks-store"}) { t.Errorf("written %v, want notes-backend then tasks-store", written) } if store.documents["notes-backend"][internalToken] != "shared-internal-token" { diff --git a/pkg/deploysecrets/rotate.go b/pkg/deploysecrets/rotate.go new file mode 100644 index 00000000..70a2c3c3 --- /dev/null +++ b/pkg/deploysecrets/rotate.go @@ -0,0 +1,356 @@ +package deploysecrets + +// Rotation: `codefly deploy secrets --rotate`. +// +// A plan keeps every stored value. --rotate is the one exception, and it is +// narrow: it names a key the environment GENERATES (service-secrets.generate), +// mints a new value for it the way the generator would, writes that value to +// every remote key holding it, and rewrites every stored value derived from the +// old one — in the same apply, so a source and what is derived from it never +// disagree. A supplied, configured or propagated-only key is refused: codefly +// does not produce it, so it cannot produce the next one either. +// +// What "derived" means here is what the store can show, not what a name +// suggests. A producer that declares its connection as a template over its own +// secrets (core's ConfigurationValue.template, see gitops/configuration_templates.go) +// has it assembled by External Secrets in the cluster: nothing is stored, so a +// rotated primitive is picked up on the next sync and there is nothing to +// rewrite. A connection an operator stored instead — the one agents export +// without a template, which the plan reports as `require` — embeds the old +// value. Every stored property of the store is examined: one that parses as a +// URL whose DECODED userinfo password is the old value is recomputed by +// replacing the password in its userinfo and nothing else. Any other value +// holding the old one, raw or percent-encoded, is refused, naming the property, +// because codefly cannot recompute it and leaving it on the old value is the +// failure rotation exists to prevent. + +import ( + "context" + "errors" + "fmt" + "maps" + "net/url" + "slices" + "strings" + "time" + + "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/gitops" + "github.com/codefly-dev/cli/pkg/secretgen" + "github.com/codefly-dev/core/resources" +) + +// RotationKey resolves a --rotate selector to the secret key it names. The +// selector is spelled like the service-secrets.generate entry that declares the +// key: `//:` for a service configuration, +// `:` for a workspace one. A CODEFLY__…_SECRET_CONFIGURATION +// key is taken as written. +func RotationKey(selector string) (string, error) { + selector = strings.TrimSpace(selector) + if configurationKey(selector) { + return selector, nil + } + cut := strings.LastIndex(selector, ":") + if cut <= 0 || cut == len(selector)-1 { + return "", fmt.Errorf("--rotate %q: want //:, :, or a CODEFLY__…_SECRET_CONFIGURATION__ key", selector) + } + parts, key := strings.Split(selector[:cut], "/"), selector[cut+1:] + if slices.Contains(parts, "") { + return "", fmt.Errorf("--rotate %q: an empty module, service or configuration", selector) + } + switch len(parts) { + case 1: + return resources.WorkspaceSecretConfigurationPrefix + "__" + resources.NameToKey(parts[0]) + "__" + resources.NameToKey(key), nil + case 3: + return resources.ServiceSecretConfigurationEnvironmentKeyPrefixFromUnique(parts[0]+"/"+parts[1]) + + "__" + resources.NameToKey(parts[2]) + "__" + resources.NameToKey(key), nil + default: + return "", fmt.Errorf("--rotate %q: want //: or :", selector) + } +} + +// ReadsKey reports whether any rendered ExternalSecret reads key. +func ReadsKey(rendered gitops.RenderedEnvironment, key string) bool { + for _, secret := range rendered.Secrets { + for _, property := range secret.Properties { + if slices.Contains(property.Keys, key) { + return true + } + } + } + return false +} + +// CheckRotationStores refuses a rotation whose configuration is read through +// more than one store. A plan reads and writes one store, so a value of the +// same configuration filed in another — the connection a consumer reads +// through its own store — would be invisible to the rotation and left on the +// old value. +func CheckRotationStores(groups []gitops.RenderedEnvironment, key string) error { + cut := strings.LastIndex(key, "__") + if cut < 0 { + return nil + } + configuration := key[:cut+2] + var stores []string + for _, group := range groups { + for _, secret := range group.Secrets { + for _, property := range secret.Properties { + for _, read := range property.Keys { + if strings.HasPrefix(read, configuration) && !slices.Contains(stores, secret.Store.Name) { + stores = append(stores, secret.Store.Name) + } + } + } + } + } + if len(stores) > 1 { + return fmt.Errorf("--rotate %s: its configuration is read through %d stores (%s), and a plan rotates within one store; a value derived from it in another would be left on the old one", + key, len(stores), strings.Join(stores, ", ")) + } + return nil +} + +// derivedRewrite is a stored value recomputed from rotated keys. +type derivedRewrite struct { + value string + from []string +} + +// planRotations validates every --rotate key, mints its new value into +// generated, and finds every stored value derived from its old one. It returns +// the rotated keys and the derived rewrites, by location. +func planRotations(in *Inputs, states []*remoteState, generators map[string]environments.EnvironmentSecretGenerator, + generated map[string]string, aliases map[propertyLocation][]string) (map[string]bool, map[propertyLocation]derivedRewrite, error) { + rotating := map[string]bool{} + derived := map[propertyLocation]derivedRewrite{} + for _, key := range in.Rotate { + if rotating[key] { + continue + } + generator, declared := generators[key] + if !declared { + return nil, nil, refuseRotation(in.Rendered, key) + } + switch generator.Format { + case environments.SecretGeneratorFormatEd25519PKCS8: + return nil, nil, fmt.Errorf("--rotate %s: it is an ed25519-pkcs8 signing key, whose public half is registered outside this store; rotating it here would break that registration silently", key) + case environments.SecretGeneratorFormatIdentifier: + return nil, nil, fmt.Errorf("--rotate %s: it is an identifier (a user or database name, say); a new one in the store renames nothing on the server that owns it", key) + } + format, size := generatorFormat(&generator) + value, err := secretgen.Generate(format, size) + if err != nil { + return nil, nil, err + } + rotating[key], generated[key] = true, value + + // Every holder rotates together, so every holder must be in scope. + var old string + for _, state := range states { + for _, property := range state.secret.Properties { + if !slices.Contains(aliases[propertyLocation{state.secret.RemoteKey, property.Property}], key) { + continue + } + if !planning(&state.secret, in.Modules) { + return nil, nil, fmt.Errorf("--rotate %s: it is also held in %s#%s, outside --module %s; widen the scope so every holder rotates together", + key, state.secret.RemoteKey, property.Property, strings.Join(in.Modules, ",")) + } + if stored, present := state.document[property.Property]; present { + old = stored + } + } + } + if old == "" { + // Stored nowhere (or not read): there is nothing to rotate away from, + // so nothing can be derived from it either. + continue + } + for _, state := range states { + for _, property := range state.secret.Properties { + location := propertyLocation{state.secret.RemoteKey, property.Property} + if slices.Contains(aliases[location], key) { + continue + } + current, present := state.document[property.Property] + if rewrite, ok := derived[location]; ok { + current = rewrite.value + } + if !present { + continue + } + rewritten, matched := rewriteURLPassword(current, old, value) + if !matched && !embeds(current, old) { + continue + } + where := fmt.Sprintf("%s#%s (%s)", state.secret.RemoteKey, property.Property, strings.Join(property.Keys, ", ")) + if rewritten == "" { + return nil, nil, fmt.Errorf("--rotate %s: %s embeds its value other than as a URL's password codefly can rewrite in place, the one derivation it recomputes; rotating would leave it stale — declare the producer's connection as a template, or rotate both by hand", + key, where) + } + if !planning(&state.secret, in.Modules) { + return nil, nil, fmt.Errorf("--rotate %s: %s is derived from it and lies outside --module %s; widen the scope so it is rewritten in the same apply", + key, where, strings.Join(in.Modules, ",")) + } + derived[location] = derivedRewrite{value: rewritten, from: append(derived[location].from, key)} + } + } + } + return rotating, derived, nil +} + +// refuseRotation says why a key no generator declares cannot be rotated. +func refuseRotation(rendered gitops.RenderedEnvironment, key string) error { + if !ReadsKey(rendered, key) { + return fmt.Errorf("--rotate %s: no rendered ExternalSecret reads it", key) + } + return fmt.Errorf("--rotate %s: service-secrets.generate does not declare it, so codefly does not produce its value — it is supplied (configured or by the operator) and is rotated where it is supplied", key) +} + +// rotatedProperty plans one property of a rotated key: the new value replaces a +// stored one, or seeds an absent one exactly as the generator would. +func rotatedProperty(state *remoteState, property gitops.RenderedSecretProperty, key, value string) (PropertyPlan, string, error) { + plan := PropertyPlan{Property: property.Property, Keys: property.Keys, Action: ActionRotate, + Source: "--rotate: a new service-secrets.generate value for " + key} + if !state.known() { + plan.Source = "--rotate " + key + " if stored; values derived from it are found when the store is read" + return plan, "", nil + } + if _, present := state.document[property.Property]; !present { + plan.Action, plan.Source = ActionGenerate, "--rotate: not stored here yet, generated for "+key + } + return plan, value, nil +} + +// embeds reports whether value holds old raw or in any percent-encoding a URL +// would give it. +func embeds(value, old string) bool { + for _, form := range []string{old, url.QueryEscape(old), url.PathEscape(old), escapedPassword(old)} { + if strings.Contains(value, form) { + return true + } + } + return false +} + +func escapedPassword(password string) string { + return strings.TrimPrefix(url.UserPassword("", password).String(), ":") +} + +// rewriteURLPassword matches a URL whose decoded userinfo password is old and +// returns it with that password replaced and every other byte as stored: the +// userinfo is spliced, never re-serialized from the parsed URL. matched with an +// empty result is a match codefly cannot rewrite safely — the caller refuses +// it. Neither the value nor a parse error is ever returned to a caller that +// prints. +func rewriteURLPassword(value, old, replacement string) (string, bool) { + parsed, err := url.Parse(value) + if err != nil || parsed.User == nil { + return "", false + } + if password, set := parsed.User.Password(); !set || password != old { + return "", false + } + scheme := strings.Index(value, "://") + if scheme < 0 { + return "", true + } + authority := value[scheme+3:] + if end := strings.IndexAny(authority, "/?#"); end >= 0 { + authority = authority[:end] + } + at := strings.LastIndex(authority, "@") + colon := strings.Index(authority[:max(at, 0)], ":") + if at < 0 || colon < 0 { + return "", true + } + userinfoStart := scheme + 3 + rewritten := value[:userinfoStart+colon+1] + escapedPassword(replacement) + value[userinfoStart+at:] + check, err := url.Parse(rewritten) + if err != nil || check.User == nil || check.User.Username() != parsed.User.Username() || check.Host != parsed.Host || + check.Path != parsed.Path || check.RawQuery != parsed.RawQuery || check.Fragment != parsed.Fragment || embeds(rewritten, old) { + return "", true + } + if password, _ := check.User.Password(); password != replacement { + return "", true + } + return rewritten, true +} + +// restoreTimeout bounds the undo of a failed rotation. It runs on a context of +// its own: the failure that triggered it is often the caller's context ending, +// and an undo that inherits that cancellation undoes nothing. +const restoreTimeout = 2 * time.Minute + +// restore returns a rotation that failed part-way to where it started: every +// key it wrote gets its previous document back as a new version (a key it +// created gets an empty one), so no source is left disagreeing with what is +// derived from it. failing is the write that errored, which may have landed +// anyway: it is restored when the store holds exactly the document this +// rotation sent. A key that moved since this rotation wrote it is someone +// else's write and is never overwritten. The store keeps every version. +func restore(ctx context.Context, store Store, written []Written, done []pendingWrite, failing *pendingWrite, cause error) ([]Written, error) { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), restoreTimeout) + defer cancel() + ours := map[string]string{} + for _, write := range written { + ours[write.Key] = write.Version + } + var failed []error + var stranded []string + restored := 0 + restoreOne := func(write *pendingWrite, version string, known bool) { + strand := func(err error) { + failed, stranded = append(failed, fmt.Errorf("restore %s: %w", write.key, err)), append(stranded, write.key) + } + current, err := store.Describe(ctx, write.key) + if err != nil { + strand(err) + return + } + switch { + case !known && current.Exists == !write.create && current.Version == write.version: + return // the failed write did not land + case !known: + document, readErr := store.Read(ctx, write.key) + if readErr != nil { + strand(readErr) + return + } + if !maps.Equal(document, write.document) { + return // not this rotation's document: someone else's, left alone + } + case current.Version != version: + strand(fmt.Errorf("version %q is no longer the newest (now %q): it was written after this rotation", version, current.Version)) + return + } + previous := write.previous + if previous == nil { + previous = map[string]string{} + } + restoredVersion, err := store.Write(ctx, write.key, previous, false) + if err != nil { + strand(err) + return + } + restored++ + written = append(written, Written{Key: write.key, Properties: write.changed, Version: restoredVersion, Restored: true}) + } + if failing != nil { + restoreOne(failing, "", false) + } + for i := len(done) - 1; i >= 0; i-- { + version, known := ours[done[i].key] + restoreOne(&done[i], version, known && version != "") + } + if len(failed) > 0 { + // A re-run would not converge from here: the rotated value is no longer + // the old one the derived values embed. The previous versions are kept. + return written, fmt.Errorf("%w; %s may still hold this rotation's documents and could not be restored — re-add each one's previous version as its current version before rotating again: %w", + cause, strings.Join(stranded, ", "), errors.Join(failed...)) + } + if restored == 0 { + return written, fmt.Errorf("%w; nothing this rotation wrote is left in the store", cause) + } + return written, fmt.Errorf("%w; the %d remote keys this rotation wrote were restored to their previous documents as new versions, so the store holds no half-rotated value", cause, restored) +} diff --git a/pkg/deploysecrets/rotate_test.go b/pkg/deploysecrets/rotate_test.go new file mode 100644 index 00000000..98e5dca3 --- /dev/null +++ b/pkg/deploysecrets/rotate_test.go @@ -0,0 +1,338 @@ +package deploysecrets + +import ( + "context" + "fmt" + "maps" + "net/url" + "slices" + "strings" + "testing" + + "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/gitops" +) + +// The obin-blue shape: a ClickHouse server whose generated password and user +// are filed in its own remote key, and a consumer whose remote key holds the +// connection URL an operator stored by hand (no producer template), which the +// plan alone reports as `require`. +const ( + evidencePassword = "CODEFLY__SERVICE_SECRET_CONFIGURATION__TRACING__EVIDENCE__CLICKHOUSE__CLICKHOUSE_PASSWORD" + evidenceUser = "CODEFLY__SERVICE_SECRET_CONFIGURATION__TRACING__EVIDENCE__CLICKHOUSE__CLICKHOUSE_USER" + evidenceConnection = "CODEFLY__SERVICE_SECRET_CONFIGURATION__TRACING__EVIDENCE__CLICKHOUSE__CONNECTION" + evidenceKey = "blue-tracing-evidence" + consumerKey = "blue-tracing-tracing" + oldPassword = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +) + +func rotationFixture(consumerServices ...string) (gitops.RenderedEnvironment, []environments.EnvironmentSecretGenerator, *fakeStore) { + if len(consumerServices) == 0 { + consumerServices = []string{"tracing/tracing"} + } + store := environments.EnvironmentSecretStoreReference{Name: "cell-secrets", Kind: "ClusterSecretStore"} + rendered := gitops.RenderedEnvironment{Secrets: []gitops.RenderedServiceSecret{ + {Store: store, RemoteKey: evidenceKey, Services: []string{"tracing/evidence"}, Properties: filed(evidencePassword, evidenceUser)}, + {Store: store, RemoteKey: consumerKey, Services: consumerServices, Properties: filed(evidenceConnection)}, + }} + generators := []environments.EnvironmentSecretGenerator{{Scope: environments.SecretGeneratorScopeService, + Configuration: "clickhouse", Services: []string{"tracing/evidence"}, Keys: []string{"CLICKHOUSE_PASSWORD"}}, {Scope: environments.SecretGeneratorScopeService, + Configuration: "clickhouse", Services: []string{"tracing/evidence"}, Keys: []string{"CLICKHOUSE_USER"}, Format: environments.SecretGeneratorFormatIdentifier}} + return rendered, generators, newFakeStore(map[string]map[string]string{ + evidenceKey: {evidencePassword: oldPassword, evidenceUser: "evidence"}, + consumerKey: {evidenceConnection: "clickhouse://evidence:" + oldPassword + "@evidence.tracing.svc:9000/evidence?secure=false"}, + }) +} + +func rotationPlan(t *testing.T, rendered gitops.RenderedEnvironment, generators []environments.EnvironmentSecretGenerator, store Store, rotate ...string) (*Plan, error) { + t.Helper() + return Build(context.Background(), &Inputs{Rendered: rendered, Generators: generators, Store: store, ReadPayloads: true, Rotate: rotate}) +} + +// assertConsistent checks, without printing either value, that the stored +// connection's password is the stored password and everything else is as before. +func assertConsistent(t *testing.T, store *fakeStore) { + t.Helper() + parsed, err := url.Parse(store.documents[consumerKey][evidenceConnection]) + if err != nil { + t.Fatal("the stored connection is no longer a URL") + } + password, _ := parsed.User.Password() + if password != store.documents[evidenceKey][evidencePassword] { + t.Error("the stored connection's password is not the stored password") + } + if parsed.User.Username() != "evidence" || parsed.Host != "evidence.tracing.svc:9000" || parsed.Path != "/evidence" || parsed.RawQuery != "secure=false" { + t.Error("rewriting the connection changed more than its password") + } +} + +func TestRotationKeyResolvesTheGeneratorSpelling(t *testing.T) { + for selector, want := range map[string]string{ + "tracing/evidence/clickhouse:CLICKHOUSE_PASSWORD": evidencePassword, + "tracing/evidence/clickhouse:clickhouse-password": evidencePassword, + "internal-auth:codefly-internal-token": internalToken, + evidencePassword: evidencePassword, + "CODEFLY__WORKSPACE_SECRET_CONFIGURATION__IDENTITY__SECRET": "CODEFLY__WORKSPACE_SECRET_CONFIGURATION__IDENTITY__SECRET", + } { + if got, err := RotationKey(selector); err != nil || got != want { + t.Errorf("RotationKey(%q) = %q, %v; want %q", selector, got, err, want) + } + } + for _, selector := range []string{"", "PASSWORD", "tracing/evidence:PASSWORD", "tracing//clickhouse:PASSWORD", "clickhouse:"} { + if _, err := RotationKey(selector); err == nil { + t.Errorf("RotationKey(%q) accepted a malformed selector", selector) + } + } +} + +// A generated key rotates in every holder, the connection derived from it is +// rewritten in the same apply, the result names versions and properties only, +// and a plain re-run afterwards writes nothing. +func TestRotateReplacesAGeneratedKeyAndTheConnectionDerivedFromIt(t *testing.T) { + ctx := context.Background() + rendered, generators, store := rotationFixture() + plan, err := rotationPlan(t, rendered, generators, store, evidencePassword) + if err != nil { + t.Fatalf("Build: %v", err) + } + for _, check := range []struct { + key, property string + action Action + }{ + {evidenceKey, evidencePassword, ActionRotate}, + {evidenceKey, evidenceUser, ActionKeep}, + {consumerKey, evidenceConnection, ActionRotate}, + } { + if got := propertyPlan(t, plan, check.key, check.property); got.Action != check.action { + t.Errorf("%s#%s = %s (%s), want %s", check.key, check.property, got.Action, got.Source, check.action) + } + } + reported := fmt.Sprintf("%+v %v %v", plan.Secrets, plan.Changes(), plan.Required()) + written, err := plan.Apply(ctx, store, false) + if err != nil { + t.Fatalf("Apply: %v", err) + } + rotated := store.documents[evidenceKey][evidencePassword] + if rotated == oldPassword || len(rotated) != 64 { + t.Fatal("the password was not replaced by a new 32-byte hex value") + } + if strings.Contains(reported, rotated) || strings.Contains(reported, oldPassword) { + t.Fatal("the plan's report carries a value") + } + assertConsistent(t, store) + want := []Written{ + {Key: evidenceKey, Properties: []string{evidencePassword}, Version: "1"}, + {Key: consumerKey, Properties: []string{evidenceConnection}, Version: "1"}, + } + if !slices.EqualFunc(written, want, func(a, b Written) bool { + return a.Key == b.Key && a.Version == b.Version && !a.Restored && slices.Equal(a.Properties, b.Properties) + }) { + t.Errorf("written = %+v, want %+v", written, want) + } + + again, err := rotationPlan(t, rendered, generators, store) + if err != nil { + t.Fatal(err) + } + if changes := again.Changes(); len(changes) != 0 { + t.Errorf("a plain re-run after the rotation would write %v", changes) + } +} + +// Only a key codefly generates is rotated: a supplied key, a configured one and +// a signing key are each refused with their reason, before anything is written. +func TestRotateRefusesAKeyCodeflyDoesNotProduce(t *testing.T) { + rendered, generators, store := rotationFixture() + if _, err := rotationPlan(t, rendered, generators, store, evidenceConnection); err == nil || + !strings.Contains(err.Error(), "service-secrets.generate does not declare it") { + t.Errorf("rotating a supplied key = %v, want a refusal saying it is not generated", err) + } + _, err := Build(context.Background(), &Inputs{Rendered: rendered, Generators: generators, Store: store, ReadPayloads: true, + Rotate: []string{evidencePassword}, + Configured: map[string]ConfiguredValue{evidencePassword: {File: "configurations/blue/clickhouse.secret.env", value: oldPassword}}}) + if err == nil || !strings.Contains(err.Error(), "configurations/blue/clickhouse.secret.env") { + t.Errorf("rotating a configured key = %v, want a refusal naming the file", err) + } + signing := []environments.EnvironmentSecretGenerator{{Scope: environments.SecretGeneratorScopeService, Configuration: "clickhouse", + Services: []string{"tracing/evidence"}, Keys: []string{"CLICKHOUSE_PASSWORD"}, Format: environments.SecretGeneratorFormatEd25519PKCS8}} + if _, err := rotationPlan(t, rendered, signing, store, evidencePassword); err == nil || !strings.Contains(err.Error(), "signing key") { + t.Errorf("rotating a signing key = %v, want a refusal", err) + } + if _, err := rotationPlan(t, rendered, generators, store, evidenceUser); err == nil || !strings.Contains(err.Error(), "identifier") { + t.Errorf("rotating an identifier = %v, want a refusal: a new user name in the store renames nobody", err) + } + if len(store.writes) != 0 { + t.Errorf("a refused rotation wrote %v", store.writes) + } +} + +// A stored value embedding the old one in a shape codefly cannot recompute is +// refused by name instead of being left stale; so is a derived value outside +// --module. +func TestRotateRefusesADerivedValueItCannotKeepConsistent(t *testing.T) { + rendered, generators, store := rotationFixture() + store.documents[consumerKey][evidenceConnection] = "host=evidence password=" + oldPassword + _, err := rotationPlan(t, rendered, generators, store, evidencePassword) + if err == nil || !strings.Contains(err.Error(), consumerKey+"#"+evidenceConnection) { + t.Errorf("an unrecomputable derived value = %v, want a refusal naming it", err) + } + if err != nil && strings.Contains(err.Error(), oldPassword) { + t.Fatal("the refusal carries the value") + } + + rendered, generators, store = rotationFixture("observer/api") + _, err = Build(context.Background(), &Inputs{Rendered: rendered, Generators: generators, Store: store, ReadPayloads: true, + Rotate: []string{evidencePassword}, Modules: []string{"tracing"}}) + if err == nil || !strings.Contains(err.Error(), "outside --module") { + t.Errorf("a derived value outside the scope = %v, want a refusal", err) + } +} + +// --dry-run --metadata-only reads no value and shows the rotation by property +// name; it cannot be applied. +func TestRotateMetadataOnlyShowsRowsWithoutReading(t *testing.T) { + rendered, generators, store := rotationFixture() + plan, err := Build(context.Background(), &Inputs{Rendered: rendered, Generators: generators, Store: store, Rotate: []string{evidencePassword}}) + if err != nil { + t.Fatal(err) + } + if len(store.reads) != 0 { + t.Errorf("a metadata-only plan read %v", store.reads) + } + if got := propertyPlan(t, plan, evidenceKey, evidencePassword); got.Action != ActionRotate { + t.Errorf("rotated property = %s, want rotate", got.Action) + } + if !plan.Unverified() || len(plan.writes) != 0 { + t.Error("a metadata-only rotation is applicable") + } + if strings.Contains(fmt.Sprintf("%+v", plan.Secrets), oldPassword) { + t.Fatal("the plan carries a value") + } +} + +// A rotation failing part-way restores what it already wrote as new versions, +// so the source and its derived connection still agree; the same command then +// converges. +func TestRotateFailingPartWayLeavesNothingHalfRotated(t *testing.T) { + ctx := context.Background() + rendered, generators, store := rotationFixture() + before := maps.Clone(store.documents[evidenceKey]) + store.failWrite = map[string]bool{consumerKey: true} + plan, err := rotationPlan(t, rendered, generators, store, evidencePassword) + if err != nil { + t.Fatal(err) + } + written, err := plan.Apply(ctx, store, false) + if err == nil || !strings.Contains(err.Error(), "restored") { + t.Fatalf("Apply = %v, want the failure and the restore reported", err) + } + if !maps.Equal(store.documents[evidenceKey], before) { + t.Error("the rotated password was left in place after its connection failed to write") + } + if len(written) != 2 || !written[1].Restored || written[1].Key != evidenceKey || written[1].Version != "2" { + t.Errorf("written = %+v, want the rotation then its restore as version 2", written) + } + assertConsistent(t, store) + + store.failWrite = nil + retry, err := rotationPlan(t, rendered, generators, store, evidencePassword) + if err != nil { + t.Fatal(err) + } + if _, err = retry.Apply(ctx, store, false); err != nil { + t.Fatal(err) + } + if store.documents[evidenceKey][evidencePassword] == oldPassword { + t.Error("the retried rotation did not rotate") + } + assertConsistent(t, store) +} + +// A configuration read through two stores cannot be rotated by one plan. +func TestCheckRotationStoresRefusesAConfigurationSpanningStores(t *testing.T) { + rendered, _, _ := rotationFixture() + other := rendered + other.Secrets = []gitops.RenderedServiceSecret{rendered.Secrets[1]} + other.Secrets[0].Store.Name = "consumer-secrets" + one := rendered + one.Secrets = rendered.Secrets[:1] + if err := CheckRotationStores([]gitops.RenderedEnvironment{one, other}, evidencePassword); err == nil { + t.Error("a configuration spanning two stores was accepted") + } + if err := CheckRotationStores([]gitops.RenderedEnvironment{rendered}, evidencePassword); err != nil { + t.Errorf("one store: %v", err) + } +} + +// A base64 password holds '/' and '+', which a URL's userinfo percent-encodes. +// The derived connection is found by its DECODED password, rewritten with the +// new one encoded the same way, and found again by the next rotation. +func TestRotateFindsAPercentEncodedPassword(t *testing.T) { + const encoded = "ab/cd+ef/gh+ij==" + for name, stored := range map[string]string{ + "userinfo escaping": "clickhouse://evidence:" + escapedPassword(encoded) + "@evidence.tracing.svc:9000/evidence?secure=false", + "query escaping": "clickhouse://evidence:" + url.QueryEscape(encoded) + "@evidence.tracing.svc:9000/evidence?secure=false", + } { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + rendered, generators, store := rotationFixture() + generators[0].Format = environments.SecretGeneratorFormatBase64 + store.documents[evidenceKey][evidencePassword] = encoded + store.documents[consumerKey][evidenceConnection] = stored + for range 2 { + plan, err := rotationPlan(t, rendered, generators, store, evidencePassword) + if err != nil { + t.Fatalf("Build: %v", err) + } + if got := propertyPlan(t, plan, consumerKey, evidenceConnection); got.Action != ActionRotate { + t.Fatalf("the percent-encoded connection = %s, want rotate", got.Action) + } + if _, err = plan.Apply(ctx, store, false); err != nil { + t.Fatal(err) + } + assertConsistent(t, store) + } + if store.documents[evidenceKey][evidencePassword] == encoded { + t.Error("the password was not rotated") + } + }) + } +} + +// A write that reports failure after it landed is restored too, and the error +// says what the store holds. +func TestRotateRestoresAFailedWriteThatLanded(t *testing.T) { + rendered, generators, store := rotationFixture() + before := maps.Clone(store.documents[consumerKey]) + store.landThenFail = map[string]bool{consumerKey: true} + plan, err := rotationPlan(t, rendered, generators, store, evidencePassword) + if err != nil { + t.Fatal(err) + } + _, err = plan.Apply(context.Background(), store, false) + if err == nil || !strings.Contains(err.Error(), "the 2 remote keys this rotation wrote were restored") { + t.Fatalf("Apply = %v, want both keys reported restored", err) + } + if !maps.Equal(store.documents[consumerKey], before) || store.documents[evidenceKey][evidencePassword] != oldPassword { + t.Error("a landed write was left rotated") + } + assertConsistent(t, store) +} + +// A key someone wrote after the plan read it is never overwritten: the apply +// refuses by name, and what the rotation already wrote is restored. +func TestRotateRefusesAKeyThatMovedSinceThePlan(t *testing.T) { + rendered, generators, store := rotationFixture() + plan, err := rotationPlan(t, rendered, generators, store, evidencePassword) + if err != nil { + t.Fatal(err) + } + store.versions = map[string]int{consumerKey: 5} + _, err = plan.Apply(context.Background(), store, false) + if err == nil || !strings.Contains(err.Error(), consumerKey+" changed after the plan read it") { + t.Fatalf("Apply = %v, want the moved key refused by name", err) + } + if store.documents[evidenceKey][evidencePassword] != oldPassword || slices.Contains(store.writes[:1], consumerKey) { + t.Error("the rotation was left half-applied or overwrote the moved key") + } +} diff --git a/pkg/deploysecrets/store.go b/pkg/deploysecrets/store.go index 4c496430..c233aa84 100644 --- a/pkg/deploysecrets/store.go +++ b/pkg/deploysecrets/store.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "os/exec" + "path" "strings" "github.com/codefly-dev/cli/pkg/environments" @@ -26,8 +27,9 @@ type Store interface { // Read returns the remote key's current document. Read(ctx context.Context, key string) (map[string]string, error) // Write stores document as the remote key's new current version, creating - // the key when Describe reported it absent. - Write(ctx context.Context, key string, document map[string]string, create bool) error + // the key when Describe reported it absent, and returns that version's + // identifier. A write never destroys an earlier version. + Write(ctx context.Context, key string, document map[string]string, create bool) (string, error) // Name identifies the store in output: its backend and location, never a // credential. Name() string @@ -39,6 +41,10 @@ type Description struct { // HasVersion is false for a key created with no enabled version: reading it // fails, so it is planned as an empty document. HasVersion bool + // Version identifies the newest enabled version. Apply compares it before + // each write, so a key someone changed after the plan read it is refused + // rather than overwritten with a document built from what it used to hold. + Version string } // Runner executes a command with stdin and returns its standard output. The @@ -183,11 +189,16 @@ func (store *GoogleSecretManager) Describe(ctx context.Context, key string) (Des return Description{}, err } versions, err := store.Run(ctx, nil, "gcloud", "secrets", "versions", "list", key, "--project", store.Project, - "--filter=state:ENABLED", "--limit=1", "--format=value(name)") + "--filter=state:ENABLED", "--sort-by=~createTime", "--limit=1", "--format=value(name)") if err != nil { return Description{}, err } - return Description{Exists: true, HasVersion: strings.TrimSpace(string(versions)) != ""}, nil + newest := strings.TrimSpace(string(versions)) + description := Description{Exists: true, HasVersion: newest != ""} + if description.HasVersion { + description.Version = path.Base(newest) + } + return description, nil } func (store *GoogleSecretManager) Read(ctx context.Context, key string) (map[string]string, error) { @@ -198,22 +209,25 @@ func (store *GoogleSecretManager) Read(ctx context.Context, key string) (map[str return decodeDocument(key, payload) } -func (store *GoogleSecretManager) Write(ctx context.Context, key string, document map[string]string, create bool) error { +func (store *GoogleSecretManager) Write(ctx context.Context, key string, document map[string]string, create bool) (string, error) { payload, err := json.Marshal(document) if err != nil { - return err + return "", err } if create { // The replication policy is Secret Manager's default; the store's other // secrets use it too, and nothing in the environment declares otherwise. if _, err = store.Run(ctx, nil, "gcloud", "secrets", "create", key, "--project", store.Project, "--replication-policy=automatic"); err != nil { - return err + return "", err } } // The value travels on stdin, never on the command line where the process - // table would show it. - _, err = store.Run(ctx, payload, "gcloud", "secrets", "versions", "add", key, "--project", store.Project, "--data-file=-") - return err + // table would show it. The output is the new version's resource name only. + name, err := store.Run(ctx, payload, "gcloud", "secrets", "versions", "add", key, "--project", store.Project, "--data-file=-", "--format=value(name)") + if err != nil { + return "", err + } + return path.Base(strings.TrimSpace(string(name))), nil } // notFound reports whether a backend command failed because the remote key does diff --git a/pkg/deploysecrets/store_test.go b/pkg/deploysecrets/store_test.go index 5fe88cd7..72a503eb 100644 --- a/pkg/deploysecrets/store_test.go +++ b/pkg/deploysecrets/store_test.go @@ -89,6 +89,21 @@ func TestGoogleSecretManagerDescribesAMissingKey(t *testing.T) { } } +func TestGoogleSecretManagerDescribesTheNewestEnabledVersion(t *testing.T) { + runner := &scriptedRunner{answers: map[string]func() ([]byte, error){ + "gcloud secrets describe key": func() ([]byte, error) { return []byte("projects/p/secrets/key\n"), nil }, + "gcloud secrets versions list": func() ([]byte, error) { return []byte("projects/p/secrets/key/versions/12\n"), nil }, + }} + store := &GoogleSecretManager{Project: "p", Run: runner.run} + description, err := store.Describe(context.Background(), "key") + if err != nil || !description.HasVersion || description.Version != "12" { + t.Fatalf("Describe = %+v, %v, want version 12", description, err) + } + if !slices.Contains(runner.calls[1].args, "--sort-by=~createTime") { + t.Error("the version listing is not newest first") + } +} + func TestGoogleSecretManagerDescribesAKeyWithoutReadingIt(t *testing.T) { runner := &scriptedRunner{answers: map[string]func() ([]byte, error){ "gcloud secrets describe key": func() ([]byte, error) { return []byte("projects/p/secrets/key\n"), nil }, @@ -111,12 +126,16 @@ func TestGoogleSecretManagerDescribesAKeyWithoutReadingIt(t *testing.T) { func TestGoogleSecretManagerWritesThroughStdin(t *testing.T) { runner := &scriptedRunner{answers: map[string]func() ([]byte, error){ "gcloud secrets create key": func() ([]byte, error) { return nil, nil }, - "gcloud secrets versions add": func() ([]byte, error) { return nil, nil }, + "gcloud secrets versions add": func() ([]byte, error) { return []byte("projects/p/secrets/key/versions/7\n"), nil }, }} store := &GoogleSecretManager{Project: "p", Run: runner.run} - if err := store.Write(context.Background(), "key", map[string]string{"A": "value-a"}, true); err != nil { + version, err := store.Write(context.Background(), "key", map[string]string{"A": "value-a"}, true) + if err != nil { t.Fatal(err) } + if version != "7" { + t.Errorf("version = %q, want the basename of the version gcloud reports", version) + } if len(runner.calls) != 2 || runner.calls[0].args[1] != "create" { t.Fatalf("calls = %+v, want create then versions add", runner.calls) }