feat(reporting): TAM-6862: auto-rotate the reporting role secret#10148
feat(reporting): TAM-6862: auto-rotate the reporting role secret#10148dannash100 wants to merge 1 commit into
Conversation
The reporting/raw role passwords derive from a per-server secret that was generated once and never changed. Rotate it automatically: getReportingSecret regenerates the secret once it passes db.reportingSecretRotationDays (default 90), recording the rotation time in a local system fact. Coordinated by an advisory lock so concurrently-starting central processes converge on one secret; the new passwords take effect as each process restarts (existing pooled connections keep working). 0 disables age-based rotation.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9c94dbf. Configure here.
| const existing = await models.LocalSystemSecret.get(FACT_REPORTING_ROLE_SECRET); | ||
| const rotatedAt = await models.LocalSystemFact.get(FACT_REPORTING_SECRET_ROTATED_AT); | ||
| const rotationDays = config.db?.reportingSecretRotationDays ?? 0; | ||
| if (existing && !isReportingSecretStale(rotatedAt, rotationDays)) return existing; |
There was a problem hiding this comment.
Missing rotation timestamp disables forever
Medium Severity
After upgrade, servers that already have a reportingRoleSecret but no reportingSecretRotatedAt fact never age-rotate: isReportingSecretStale treats a missing timestamp as not stale, and the early return keeps the old secret without backfilling the fact, so the default 90-day policy never applies on existing deployments.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9c94dbf. Configure here.
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
| const existing = await models.LocalSystemSecret.get(FACT_REPORTING_ROLE_SECRET); | ||
| const rotatedAt = await models.LocalSystemFact.get(FACT_REPORTING_SECRET_ROTATED_AT); | ||
| const rotationDays = config.db?.reportingSecretRotationDays ?? 0; | ||
| if (existing && !isReportingSecretStale(rotatedAt, rotationDays)) return existing; |
There was a problem hiding this comment.
[Bugs & Correctness] critical
After upgrading from a version that had no FACT_REPORTING_SECRET_ROTATED_AT fact, rotatedAt is null. isReportingSecretStale(null, 90) returns false, so the early-return fires and FACT_REPORTING_SECRET_ROTATED_AT is never written. Every subsequent startup follows the same path — the rotation timestamp is never seeded and auto-rotation silently never activates for any deployment that already had a secret before this feature was deployed.
Fix: when existing is truthy but rotatedAt is null, write the current time as the baseline timestamp before the staleness check:
if (existing) {
if (!rotatedAt) {
await models.LocalSystemFact.set(FACT_REPORTING_SECRET_ROTATED_AT, getCurrentDateTimeString());
rotatedAt = /* the value you just wrote */;
}
if (!isReportingSecretStale(rotatedAt, rotationDays)) return existing;
}Alternatively, treat null rotatedAt as stale (force a one-time rotation on first startup after upgrade) by removing the !rotatedAt guard from isReportingSecretStale.
| // keep their cached secret until they restart. | ||
| const getReportingSecret = async ({ models, sequelize }) => | ||
| sequelize.transaction(async () => { | ||
| await sequelize.query(`SELECT pg_advisory_xact_lock(hashtext('tamanu:reporting-secret'));`); |
There was a problem hiding this comment.
[Security] suggestion
pg_advisory_xact_lock(hashtext(...)) uses a 32-bit hash space (~4 billion values). If any other advisory lock in the system happens to hash to the same int4 value as 'tamanu:reporting-secret', the two will accidentally serialise against each other. Because advisory locks are cluster-global and other packages/extensions may use them, prefer a hardcoded int8 literal (e.g. SELECT pg_advisory_xact_lock(7829301042::bigint)) to guarantee uniqueness with no collision risk. Same concern applies to the existing 'tamanu:reporting-roles' lock on line 67.
|
🦸 Review Hero Summary Below consensus threshold (3 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
Fix: when
|
1 participant
Changes
Stacked on the reporting-roles PR (#10082). Auto-rotates the per-server reporting/raw role secret:
getReportingSecretregenerates it once it passesdb.reportingSecretRotationDays(default 90; 0 disables), recording the time in a local system fact. Coordinated by an advisory lock so concurrently-starting central processes converge on one secret; new passwords take effect as each process restarts (existing pooled connections keep working). Mid-run rotation is deliberately avoided — passwords derive from a secret each process caches for its pool, so live rotation would break other processes' new connections until restart.Auto-Deploy
Options
Tests
Review Hero
.github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.Remember to...