feat(security): DB-backed rotating OIDC global secret (SecretRotator)

[SamTV12345]

What

Replaces the hard-coded fosite GlobalSecret in the OIDC authenticator ("some-cool-secret-that-is-32bytes") with a randomly generated, database-persisted secret that rotates on the configured cookie key-rotation interval. Older secrets stay valid for verification for the session lifetime, so in-flight artifacts keep working across a rotation. Port of etherpad-lite's SecretRotator.

How

• lib/security/secretrotator.go — HKDF-derived rotating secrets: array of active secrets (first = used to sign, all = accepted for verification), interval rotation + overlapping lifetime window, optional legacy-static-secret migration.
• Storage — dedicated secret_rotation table (migration 006) + SecretMethods on DataStore, implemented across SQLite/MySQL/Postgres/Memory via squirrel. MySQL declares the prefix index inline in CREATE TABLE (no CREATE INDEX IF NOT EXISTS).
• Wiring — lib/api/oidc/authenticator.go: each rotation rebuilds the fosite provider under an RWMutex with a fresh *fosite.Config (never mutated in place → race-free); endpoints read via currentProvider(); Stop() on shutdown.

Testing

White-box tests in lib/security (rotation, overlapping window, multi-instance, expiry, legacy secret). go build ./..., go vet, and DB/OIDC tests green (incl. MySQL/Postgres containers).

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

1. Unvalidated params can panic 🐞 Bug ☼ Reliability

Description

SecretRotator derives secrets from DB-persisted JSON without validating AlgID or Interval, so a
corrupted row (AlgID out of range or Interval=0) can trigger slice-bounds or divide-by-zero panics
during update()/scheduled rotation and crash the process.

Code

lib/security/secretrotator.go[R199-215]

+func mod(a, n int64) int64 { return ((a % n) + n) % n }
+
+func intervalStart(t, interval int64) int64 { return t - mod(t, interval) }
+
+// deriveSecrets derives all relevant secrets for one parameter set as of now.
+func (r *SecretRotator) deriveSecrets(p secretParams, now int64) ([][]byte, error) {
+	alg := algorithms[p.AlgID]
+	if p.Interval == nil {
+		s, err := alg.derive(p.AlgParams, "")
+		if err != nil {
+			return nil, err
+		}
+		return [][]byte{s}, nil
+	}
+	iv := *p.Interval
+	t0 := intervalStart(now, iv)
+	// Start of the first interval covered by these params, backdated by iv to

Evidence

deriveSecrets() indexes algorithms[p.AlgID] without bounds checks and calls `intervalStart(now,
iv) where iv is taken from persisted JSON; intervalStart() performs modulo by interval` (panic
if 0). update() unmarshals DB payloads and forwards them into deriveSecrets() without validating
AlgID/Interval first, so a single bad DB row can crash rotations.

lib/security/secretrotator.go[199-240]
lib/security/secretrotator.go[271-393]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SecretRotator` consumes persisted `secretParams` rows and uses `AlgID` and `Interval` directly. If a row is corrupted/unexpected (e.g., `algId` out of range, `interval` is 0/negative, or `keyLen` is nonsensical), the rotator can panic (index out of range, divide-by-zero in modulo, or an infinite loop risk when stepping by `iv`). Because `update()` runs on a timer, this can crash the service at runtime.
## Issue Context
The rotator reads JSON payloads from `secret_rotation` and attempts to derive secrets for all rows on every update.
## Fix Focus Areas
- Add defensive validation before using persisted params:
- Reject `AlgID < 0 || AlgID >= len(algorithms)`.
- If `Interval != nil`, require `*Interval > 0`.
- Consider clamping/validating HKDF `KeyLen` to an expected safe range (e.g., 32) to avoid empty secrets or huge allocations.
- On invalid rows: log a warning and skip deriving; optionally delete the bad row.
- Validate constructor inputs too (e.g., `r.interval > 0`) and return an error from `Start()` if invalid.
### Code pointers
- file: `lib/security/secretrotator.go`
- Validate interval/alg usage in `deriveSecrets()` and `intervalStart()` call sites.
- Validate DB-loaded params in `update()` before appending to `allParams` / calling `deriveSecrets()`.
#### Fix Focus Areas (exact)
- lib/security/secretrotator.go[199-241]
- lib/security/secretrotator.go[271-393]
- lib/security/secretrotator.go[151-165]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Secrets slice is mutable ✓ Resolved 🐞 Bug ⛨ Security

Description

SecretRotator.Secrets() only shallow-copies the outer [][]byte, so callers receive references to the
internal secret byte slices and can mutate the rotator’s in-memory signing/verification secrets.

Code

lib/security/secretrotator.go[R171-180]

+// Secrets returns a snapshot of the active secrets. The first entry is the one
+// to use for generating new MACs/signatures; all entries are valid for
+// verification.
+func (r *SecretRotator) Secrets() [][]byte {
+	r.mu.RLock()
+	defer r.mu.RUnlock()
+	out := make([][]byte, len(r.secrets))
+	copy(out, r.secrets)
+	return out
+}

Evidence

Secrets() copies only the outer slice, so the internal []byte secrets are returned by reference.
Those bytes are then used as GlobalSecret/RotatedGlobalSecrets in the OIDC provider, so mutating
them would directly affect signing/verification behavior.

lib/security/secretrotator.go[171-180]
lib/api/oidc/authenticator.go[130-146]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Secrets()` returns a shallow copy of `[][]byte`, which means the returned inner `[]byte` values alias the rotator’s internal secrets. Any caller that modifies the returned bytes will mutate the internal secrets used for signing/verification.
## Issue Context
Today `Authenticator.rebuildProvider()` consumes `rotator.Secrets()` to populate `fosite.Config.GlobalSecret` and `RotatedGlobalSecrets`. Even if current call sites don’t mutate the bytes, the exported API makes accidental mutation easy and future call sites could introduce subtle bugs.
## Fix Focus Areas
- In `Secrets()`, deep-copy each inner `[]byte` (e.g., allocate and `copy`, or `bytes.Clone` if available) before returning.
- Consider documenting immutability expectations in the method comment.
### Fix Focus Areas (exact)
- lib/security/secretrotator.go[171-180]
- lib/api/oidc/authenticator.go[134-146]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Rotate OIDC fosite GlobalSecret via DB-backed SecretRotator
✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Walkthroughs

Description

• Replace hard-coded OIDC HMAC secret with DB-persisted rotating secrets.
• Add secret_rotation table + datastore methods across SQLite/MySQL/Postgres/Memory.
• Rebuild fosite provider on rotation under RWMutex; stop rotator on shutdown; add tests.

Diagram

graph TD
  R["OIDC Endpoints"] --> A["Authenticator"] --> P{{"fosite Provider"}} --> GS["Rotated secrets"]
  SR(["SecretRotator"]) --> RB["rebuildProvider()"] --> A
  SR --> DS["SecretMethods"] --> DB[("secret_rotation")]

  subgraph Legend
    direction LR
    _m["Module"] ~~~ _p(["Background task"]) ~~~ _e{{"External"}} ~~~ _d[("Database")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Persist full derived secrets (not HKDF params)

• ➕ Simpler mental model (rows directly represent active secrets)
• ➕ Avoids subtle HKDF/info derivation differences across implementations
• ➖ More sensitive data stored at rest (larger blast radius on DB leak)
• ➖ More frequent writes and more rows over time vs compact param sets

2. Mutate a shared fosite.Config instead of rebuilding provider

• ➕ Avoids reconstructing provider on each rotation
• ➕ Less churn in composed provider dependencies
• ➖ Higher risk of data races and inconsistent reads during requests
• ➖ Harder to reason about handler-level consistency without per-request snapshotting

3. Delegate rotation to external KMS/HSM-managed key versions

• ➕ Centralized key management, auditing, and rotation policies
• ➕ Potentially stronger operational controls than app-managed rotation
• ➖ Adds infrastructure dependency and complexity
• ➖ Harder to run in local/dev and simple self-hosted deployments

Recommendation: Keep the PR’s approach (HKDF-derived secrets with DB-persisted params + provider rebuild under RWMutex). It strikes a good balance between operational simplicity (single table, multi-instance cooperation), security (no hard-coded secret; overlapping verification window), and runtime safety (fresh fosite.Config per rotation avoids in-place mutation races). The main alternatives either increase secret exposure (persisting derived secrets) or increase concurrency risk (mutating shared config).

File Changes

Enhancement (3)

authenticator.go Rebuild fosite provider using DB-backed rotating GlobalSecret +90/-24

Rebuild fosite provider using DB-backed rotating GlobalSecret

• Removes the hard-coded fosite GlobalSecret and wires in SecretRotator using cookie key-rotation interval and session lifetime. Adds RWMutex-guarded provider swapping via rebuildProvider() and per-request currentProvider() snapshots to avoid races, plus Stop() for shutdown.

lib/api/oidc/authenticator.go

---

secretrotator.go Add SecretRotator for HKDF-derived rotating secrets with overlap window +426/-0

Add SecretRotator for HKDF-derived rotating secrets with overlap window

• Implements a DB-coordinated secret rotation mechanism (ported from upstream Etherpad) that publishes derivation parameters, derives per-interval secrets via HKDF, keeps an overlapping verification window, and optionally supports legacy static-secret migration. Includes scheduling via timers, multi-instance reconciliation, and an OnRotate callback hook.

lib/security/secretrotator.go

---

server.go Stop OIDC SecretRotator during server shutdown +1/-0

Stop OIDC SecretRotator during server shutdown

• Invokes authenticator.Stop() on shutdown to halt background secret rotation and prevent timer goroutines from running after server termination.

lib/server/server.go

---

Tests (1)

secretrotator_test.go Add unit tests for rotation behavior, sharing, expiry, and legacy secret +204/-0

Add unit tests for rotation behavior, sharing, expiry, and legacy secret

• Adds deterministic tests using a fake SecretStore to validate first-run generation, stability within interval, rotation with overlap, multi-instance adoption, expired params cleanup, and legacy static secret inclusion.

lib/security/secretrotator_test.go

---

Other (7)

DataStore.go Add SecretMethods interface for secret rotation persistence +14/-0

Add SecretMethods interface for secret rotation persistence

• Introduces SecretMethods (Save/List/DeleteSecretParams) and embeds it into DataStore so the rotator can persist and reconcile rotation parameter sets across backends.

lib/db/DataStore.go

---

MemoryDataStore.go Implement SecretMethods in MemoryDataStore +29/-0

Implement SecretMethods in MemoryDataStore

• Adds an in-memory map to store secret rotation parameter rows and implements Save/List/DeleteSecretParams for tests and non-persistent runtime modes.

lib/db/MemoryDataStore.go

---

MySQLDB.go Implement SecretMethods for MySQL with upsert semantics +53/-0

Implement SecretMethods for MySQL with upsert semantics

• Adds MySQL-backed Save/List/DeleteSecretParams using squirrel and ON DUPLICATE KEY UPDATE to upsert rotation parameters into secret_rotation.

lib/db/MySQLDB.go

---

PostgresDB.go Implement SecretMethods for Postgres with ON CONFLICT upsert +59/-0

Implement SecretMethods for Postgres with ON CONFLICT upsert

• Adds Postgres-backed Save/List/DeleteSecretParams using squirrel with ON CONFLICT(id) DO UPDATE and updated_at refresh on writes.

lib/db/PostgresDB.go

---

SQLiteDB.go Implement SecretMethods for SQLite with ON CONFLICT upsert +56/-0

Implement SecretMethods for SQLite with ON CONFLICT upsert

• Adds SQLite-backed Save/List/DeleteSecretParams using ON CONFLICT(id) DO UPDATE and updated_at refresh, plus list/delete queries by prefix/id.

lib/db/SQLiteDB.go

---

001_initial_schema.go Register migration 006 for secret rotation table +1/-0

Register migration 006 for secret rotation table

• Adds the new secret rotation migration to the migration list so it runs during schema setup/upgrade.

lib/db/migrations/001_initial_schema.go

---

006_secret_rotation.go Create secret_rotation table and prefix index across dialects +51/-0

Create secret_rotation table and prefix index across dialects

• Introduces a new migration creating secret_rotation with id/prefix/payload/updated_at columns and an index on prefix; MySQL declares the index inline due to lack of CREATE INDEX IF NOT EXISTS.

lib/db/migrations/006_secret_rotation.go

---

[qodo-free-for-open-source-projects]

1. Unvalidated params can panic 🐞 Bug ☼ Reliability

SecretRotator derives secrets from DB-persisted JSON without validating AlgID or Interval, so a
corrupted row (AlgID out of range or Interval=0) can trigger slice-bounds or divide-by-zero panics
during update()/scheduled rotation and crash the process.

Agent Prompt

## Issue description
`SecretRotator` consumes persisted `secretParams` rows and uses `AlgID` and `Interval` directly. If a row is corrupted/unexpected (e.g., `algId` out of range, `interval` is 0/negative, or `keyLen` is nonsensical), the rotator can panic (index out of range, divide-by-zero in modulo, or an infinite loop risk when stepping by `iv`). Because `update()` runs on a timer, this can crash the service at runtime.

## Issue Context
The rotator reads JSON payloads from `secret_rotation` and attempts to derive secrets for all rows on every update.

## Fix Focus Areas
- Add defensive validation before using persisted params:
  - Reject `AlgID < 0 || AlgID >= len(algorithms)`.
  - If `Interval != nil`, require `*Interval > 0`.
  - Consider clamping/validating HKDF `KeyLen` to an expected safe range (e.g., 32) to avoid empty secrets or huge allocations.
  - On invalid rows: log a warning and skip deriving; optionally delete the bad row.
- Validate constructor inputs too (e.g., `r.interval > 0`) and return an error from `Start()` if invalid.

### Code pointers
- file: `lib/security/secretrotator.go`
  - Validate interval/alg usage in `deriveSecrets()` and `intervalStart()` call sites.
  - Validate DB-loaded params in `update()` before appending to `allParams` / calling `deriveSecrets()`.

#### Fix Focus Areas (exact)
- lib/security/secretrotator.go[199-241]
- lib/security/secretrotator.go[271-393]
- lib/security/secretrotator.go[151-165]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[SamTV12345]

Addressed the Qodo review in b534ed6:

1. Unvalidated params can panic — update() now validates each DB-loaded params set (validParams) and skips + deletes rows with an out-of-range AlgID or non-positive Interval before they reach deriveSecrets()/intervalStart(). HKDF keyLen is bounds-checked, and Start() rejects a non-positive interval. New test: TestSecretRotator_SkipsInvalidParams.
2. Secrets slice is mutable — Secrets() now bytes.Clones each inner []byte, so callers can no longer mutate the rotator's internal signing/verification material. New test: TestSecretRotator_SecretsAreDeepCopied.