From 5c34249b68ed187dd59aa5ee02355869ac328495 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 15:02:17 -0700 Subject: [PATCH 1/2] docs(runbooks): the connector-secret ring rotation order (append, flip, rewrap, drop) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit connector-ops found the gap (74229): the two-phase rotation order lives only in the header of `backend/scripts/rewrap-connector-secrets.ts`, which nobody opens before editing a Secret Manager value. Add it to the runbook that operators actually read before touching these values. New §5 gives the order, the one-line rule the rest follows from — the ring value is appended to, never replaced — and the in-pod dry-run/`--apply` invocation. Plus §4 gains a step pointing at it. Facts read before writing, none assumed: - `rewrap()` (`connectorSecrets.ts:164-172`) returns early when a row is already under the active key and compare-and-sets each rewrite — so re-running is safe, and the old key must stay in the ring until the count reaches zero. - `parseKeyRing()` (`:49-79`) refuses an unset var, a duplicate id, or an active id absent from the ring, which is what makes "flip before append" a failure. - `node dist/scripts/rewrap-connector-secrets.js` is real in the image: `tsconfig.build.json` includes `scripts/**` (`include: **/*.ts`, not excluded) and the Dockerfile runs `npx tsc -p tsconfig.build.json`. - Both ring env refs are `optional: true` (`templates/core/backend-deployment.yaml`), and `api-keys` maps both at `latest` with no `version:` — the two properties that make a dropped ring quiet at boot and a destroyed version irreversible. No code, no values; follows the page's existing `-n ` convention. --- docs/runbooks/connector-credentials-setup.md | 60 ++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/docs/runbooks/connector-credentials-setup.md b/docs/runbooks/connector-credentials-setup.md index fd84655aea..dba9fba4a4 100644 --- a/docs/runbooks/connector-credentials-setup.md +++ b/docs/runbooks/connector-credentials-setup.md @@ -389,3 +389,63 @@ For any connector credential that lands in `api-keys`: 6. Only then the provider-side step that depends on the deployed secret (Slack `request_url`; GitHub Connection create; Discord webhook enable). 7. Post outcomes to the pod record by name and length, never by value. +8. Rotating the ring is not a stand-up: the order is §5 — append, flip the + active id, rewrap to zero, then drop the old key. + +--- + +## 5. Rotating the connector-secret key ring + +**Use this when** the value behind `commonly-dev-connector-secret-keys` has to +change: a scheduled rotation, or a key you believe was exposed. §2.4 has the +format and the refusal modes; this section is the **order**, which is the part +that loses data. + +Every `ConnectorSecret` row is encrypted under the key id it was written with, +and it is decrypted with **the ring as it stands**. So the ring value is +**appended to, never replaced**: replace `k1:…` with `k2:…` and `k1` leaves the +ring while rows still name it, so those rows stop decrypting and whatever reads +one fails at use rather than at boot. `CONNECTOR_SECRET_ACTIVE_KEY` is the same +trap one step later — it must name a key present in the ring or `parseKeyRing` +refuses (§2.4), which is a failing connector instead of a failing start. + +Rotation is four steps in this order. It is the order the rewrap script's own +header states, which is why that header exists: + +1. **Append the new key.** Generate it as in §2.4 and set + `commonly-dev-connector-secret-keys` to `k1:,k2:`, leaving + `commonly-dev-connector-secret-active-key` on `k1`. Force-sync (§0.1) or wait + out the 1h ESO refresh, then confirm from the pod that the ring names two ids + — by count, never by value. +2. **Point the active key at the new id.** Set + `commonly-dev-connector-secret-active-key` to `k2` and force-sync again. New + writes encrypt under `k2`; existing rows still decrypt, because `k1` is still + in the ring. Nothing has been rewritten yet. +3. **Rewrap until it reports zero.** In the backend pod, which carries + `MONGO_URI` and the ring (the script exits if either is missing): + + ```bash + # dry run: lists the rows still under a non-active key + kubectl exec -n deploy/backend -- node dist/scripts/rewrap-connector-secrets.js + # re-encrypt them under the active key + kubectl exec -n deploy/backend -- node dist/scripts/rewrap-connector-secrets.js --apply + ``` + + The acceptance check is the script's last line — `remaining under non-active + keys: 0`. Re-running is safe: `rewrap()` returns early on a row already under + the active key, and compare-and-sets every rewrite it does make. +4. **Only now drop the old key.** Remove `k1` from + `commonly-dev-connector-secret-keys` and force-sync. Dropping it at step 1 or + 2 instead is the failure this section exists to prevent. + +Two properties of this surface are worth knowing before you start, because both +are quiet rather than loud: + +- **`optional: true` on both env refs** (`templates/core/backend-deployment.yaml`) + means a ring that disappears from the `api-keys` mapping still starts the pod — + with no ring. That is recoverable for as long as Secret Manager holds the + value, and invisible until something reads a connector credential. +- **`api-keys` maps both at `latest`** (no `version:` on the `remoteRef`), so the + one irreversible act is destroying the Secret Manager version carrying the old + id: the key material is then gone, and rows naming it can never be rewrapped. + Append, rewrap to zero, destroy — in that order. From 588149db17e541066044f4be19a544bd9f48c23f Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 15:05:21 -0700 Subject: [PATCH 2/2] docs(runbooks): the ring-rotation acceptance is two numbers, and step 2 needs a pod check (Vera's hold) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vera's hold at `5c34249b` found a window the first draft walked straight into: if step 3 runs while the pod still holds `ACTIVE_KEY=k1`, its selector `{ keyId: { $ne: activeKeyId } }` matches nothing, so the run prints `rewrapped 0; remaining under non-active keys: 0` — the documented acceptance line, satisfied by "nothing needed it" rather than by a completed rewrap. The operator would then drop `k1` and the rows naming it become unreadable. - Step 2 now ends with a pod-side confirmation (`printenv CONNECTOR_SECRET_ACTIVE_KEY` must read `k2`) and says why: until it does, step 3 reports success for that reason. - Step 3's acceptance is two numbers: the dry run's count is the pre-state, and the apply line must read `rewrapped ; remaining under non-active keys: 0`. A `rewrapped 0` against a non-zero count sends the operator back to step 2. - Residual from 74233 folded in: the dry run selects on `keyId` and never decrypts, so a row whose key has already left the ring still reads as merely "needing rewrap". It surfaces only at `--apply`, which decrypts before it writes (`getKey` throws `ConnectorSecretKeyMissing`) — fail-safe, nothing written, but not evidence the old key is present. - §0.1 contrast added as Vera asked: its freeze is for a *missing* `remoteRef`, not for an `optional: true` ref that resolves to nothing. Verified in `rewrap-connector-secrets.ts` before writing: the selector, the `.select(...).lean()` with no decrypt, both printed strings, and that `rewrap()` is the only thing that calls `decrypt`. Vera measured the piece I could not — `dist/scripts/rewrap-connector-secrets.js` is present in the serving pod and the dry run prints `connector secrets needing rewrap: 0`. --- docs/runbooks/connector-credentials-setup.md | 37 +++++++++++++++++--- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/docs/runbooks/connector-credentials-setup.md b/docs/runbooks/connector-credentials-setup.md index dba9fba4a4..4a6e9cd84c 100644 --- a/docs/runbooks/connector-credentials-setup.md +++ b/docs/runbooks/connector-credentials-setup.md @@ -421,6 +421,17 @@ header states, which is why that header exists: `commonly-dev-connector-secret-active-key` to `k2` and force-sync again. New writes encrypt under `k2`; existing rows still decrypt, because `k1` is still in the ring. Nothing has been rewritten yet. + + Before going on, confirm from the pod that the active id is now `k2`: + + ```bash + kubectl exec -n deploy/backend -- printenv CONNECTOR_SECRET_ACTIVE_KEY + ``` + + Until that reads `k2`, step 3 has nothing to do — and reports success for that + reason. (`CONNECTOR_SECRET_ACTIVE_KEY` is the key **id**, a label; + `CONNECTOR_SECRET_KEYS` holds the ring and is never printed, here or anywhere + else on this page.) 3. **Rewrap until it reports zero.** In the backend pod, which carries `MONGO_URI` and the ring (the script exits if either is missing): @@ -431,8 +442,24 @@ header states, which is why that header exists: kubectl exec -n deploy/backend -- node dist/scripts/rewrap-connector-secrets.js --apply ``` - The acceptance check is the script's last line — `remaining under non-active - keys: 0`. Re-running is safe: `rewrap()` returns early on a row already under + The dry run's count is the **pre-state** — note it. The acceptance check is + the apply line, and it is **two numbers**: `rewrapped ; + remaining under non-active keys: 0`. `remaining: 0` on its own does not + distinguish a completed rewrap from one that never started — it is satisfied + by "everything was rewrapped" and by "nothing needed it", and the second one + is the disaster: the operator goes on to step 4 and drops `k1` while the rows + still name it. A `rewrapped 0` against a non-zero dry-run count means the pod + is still on the old active id; go back to step 2. + + The dry run is also the weaker of the two instruments. It selects on `keyId` + and never decrypts, so a row whose key has already left the ring still reads + as merely "needing rewrap" — the reassuring output is not evidence that the + old key is still present. The loss surfaces only when `--apply` calls + `rewrap()`, which decrypts before it writes (`getKey` throws + `ConnectorSecretKeyMissing` first): fail-safe, nothing is written, but that is + also why step 1 confirms the ring names both ids. + + Re-running is safe: `rewrap()` returns early on a row already under the active key, and compare-and-sets every rewrite it does make. 4. **Only now drop the old key.** Remove `k1` from `commonly-dev-connector-secret-keys` and force-sync. Dropping it at step 1 or @@ -443,8 +470,10 @@ are quiet rather than loud: - **`optional: true` on both env refs** (`templates/core/backend-deployment.yaml`) means a ring that disappears from the `api-keys` mapping still starts the pod — - with no ring. That is recoverable for as long as Secret Manager holds the - value, and invisible until something reads a connector credential. + with no ring. §0.1's freeze does not cover this pair: it is a *missing* + `remoteRef` that fails the sync, and an `optional: true` ref that resolves to + nothing is simply absent. That is recoverable for as long as Secret Manager + holds the value, and invisible until something reads a connector credential. - **`api-keys` maps both at `latest`** (no `version:` on the `remoteRef`), so the one irreversible act is destroying the Secret Manager version carrying the old id: the key material is then gone, and rows naming it can never be rewrapped.