Skip to content

security: HMAC signature verification on token validate path (Step 1 — shadow mode)#7

Open
chitcommit wants to merge 2 commits into
mainfrom
fix/validate-signature-verification
Open

security: HMAC signature verification on token validate path (Step 1 — shadow mode)#7
chitcommit wants to merge 2 commits into
mainfrom
fix/validate-signature-verification

Conversation

@chitcommit
Copy link
Copy Markdown
Contributor

Summary

Step 1 of #4. Adds HMAC signature verification to TokenManager.validate() running in shadow mode (logs mismatches, does not reject). Step 2 will flip enforcement and update SECURITY.md / CHARTER.md.

Why

Today validate() looks up tokens by SHA-256 hash and trusts the row it gets back. The HMAC signature embedded in the wire token is generated at issue time but never re-checked at validate time. That means a D1 compromise alone (SQL injection, leaked credentials, lost backup) is enough to mint forged tokens — an attacker who can write a row whose token_hash, chitty_id, scope, and service_name they control can produce a matching plaintext token that the validator accepts. Verifying the signature raises the bar to "compromise D1 and steal the signing key", restoring the defense-in-depth the HMAC was always meant to provide.

This is documented as Known Limitation #3 in SECURITY.md and tracked as #4.

Changes

  • src/token-manager.js
    • New verifySignature(token, chittyId, service, expectedTokenId) helper. Rebuilds the canonical payload (chittyId/service come from the looked-up record — they are HMAC-committed, not in the wire token), peels the fixed-length 32-char base64url signature suffix, timing-safe compares. Never throws; returns false on any malformed input. Gotcha noted inline: base64url alphabet contains _, so the body cannot be split('_') — we right-peel by length instead.
    • validate() now calls verifySignature AFTER lookup, BEFORE expiry. Shadow mode by default: writes a signature_mismatch audit event but still returns valid. Enforce mode requires env.CHITTYAUTH_VERIFY_SIGNATURE === 'enforce' exactly — typo-safe so 'true'/'1' stay in shadow.
    • provisionToken INSERT now writes service_name so the column is available for signature reconstruction at validate time. (Schema already had the column; the INSERT was just dropping it.)
  • tests/token-manager.test.js29/29 passing, no mocks. Coverage:
    • Helper unit tests: happy path, single-byte mutation in each segment (sig/tokenId/timestamp), wrong chittyId/service in reconstruction, malformed body, null inputs, signature-length guard, 200-token reliability sweep (catches base64url-underscore split regressions).
    • Validate-path integration: shadow tampering still validates but emits signature_mismatch; enforce mode rejects with 'Invalid token signature' and emits both signature_mismatch + token_validation_failed; D1 fallback path exercises the new service_name INSERT; refresh chain works end-to-end under enforce.
  • plans/validate-signature-verification/{plan.md,implementation.md} — design + step-by-step rationale.

Why shadow first

If a deployment has been silently using the dev fallback signing key (#5), flipping enforcement immediately invalidates every outstanding token at deploy. Shadow lets operators see the real-world mismatch rate before flipping. Step 2 PR will land enforce + doc updates, gated on this soak.

Test plan

  • npm test — 29/29 passing
  • No vi.mock / jest.mock on DB or service modules (per global no-mocks rule)
  • Tamper-by-one-byte tests fail without verifySignature, pass with it
  • Forged-record path: tests construct a token with a different signing key and confirm it's rejected under enforce
  • Smoke test against wrangler dev --local after merge before Step 2
  • Operator soak in shadow mode → confirm signature_mismatch rate is ~0 before Step 2 enforce flip

Related

🤖 Generated with Claude Code

chitcommit and others added 2 commits May 2, 2026 22:46
…path)

Post-review fixes after auditing CHARTER/CHITTY/AGENTS/SECURITY against src/,
schema.sql, wrangler.toml, and package.json.

Critical corrections (docs were lying about code):
- CHARTER.md storage bindings: replace fictional `users`/`api_tokens`/
  `audit_logs`/`oauth_clients` with the real schema.sql tables (`tokens`,
  `service_credentials`, `auth_events`, `token_stats`, `service_health`,
  `registrations`).
- CHARTER.md scope: `audit_logs` -> `auth_events` (matches token-manager.js:419).
- SECURITY.md threat boundary: `api_tokens` -> `tokens`; audit section:
  `audit_logs` -> `auth_events`.
- SECURITY.md validate pipeline: REMOVE the false "signature verification with
  TOKEN_SIGNING_KEY" step. token-manager.js validate() hashes the bearer and
  does a hash lookup; the embedded HMAC signature is generated at issuance
  but never re-derived/compared. Documented as Known Limitation #3.
- SECURITY.md cryptographic design: caveat the signing claim — signing
  happens at issuance, but verification on the validate path is not enforced.
- SECURITY.md Known Limitation #4: document the
  CHITTYAUTH_ISSUED_MINT_API_KEY -> TOKEN_SIGNING_KEY ->
  'dev-signing-key-change-in-production' fallback chain in
  token-manager.js:11; production must fail closed instead.

Consistency fixes (post commit f500e76 canonical-secret migration):
- AGENTS.md: adopt canonical secret names
  (CHITTYAUTH_ISSUED_MINT_API_KEY/CHITTYAUTH_ISSUED_CONNECT_API_KEY) with
  legacy aliases noted; flag wrangler.toml CREATE_NEW_* placeholders as
  blockers; add `auth-provider.js` to file list (omitted in initial draft).
- AGENTS.md: mark `npm run test:unit/test:integration` and
  `npm run setup:kv` as aspirational/broken — directories and
  `scripts/setup-kv.js` do not exist today.
- SECURITY.md normative sections (threat model, crypto, validate, limits):
  switch to canonical secret names; legacy names retained only in the
  migration paragraph and the limitations note documenting the fallback chain.
- CHARTER.md & CHITTY.md: document `CHITTYAUTH_PROVIDER=local|neon` provider
  modes; add Neon OAuth facade (`src/auth-provider.js`) to architecture so
  the validation-flow diagram doesn't pretend to cover both modes.
- CHARTER.md health gate: match `api-router.js:392-403`'s actual response
  shape (`dependencies.chittyConnect`); flag richer `checks` object as
  future enhancement.
- SECURITY.md hardening checklist: add `svc_` to the token-prefix list.

Charter version 1.2.0 -> 1.3.0.

Code-side issues opened as follow-ups:
- Implement signature verification on validate path (or remove signing).
- Fail closed when both canonical and legacy signing-key secrets are unset.
- Resolve wrangler.toml/package.json drift (auth.chitty.cc route +
  name="chittyauth" collide with chittyauth-app standalone-app posture).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mode)

Step 1 of issue #4 — close the gap where validate() looked up tokens by
SHA-256 hash but never re-derived or compared the embedded HMAC
signature. A D1 compromise alone could otherwise let an attacker insert a
forged (token_hash, chitty_id, scope, service) row and produce a
matching plaintext token that the validator accepts.

Changes:
- Add verifySignature(token, chittyId, service, expectedTokenId) helper.
  Rebuilds the canonical payload from looked-up record fields (chittyId
  and service are NOT in the wire token, only HMAC-committed), peels the
  fixed-length 32-char base64url signature suffix, then timing-safe
  compares. Never throws; returns false on any malformed input.
- Wire it into validate() AFTER lookup, BEFORE expiry check. Shadow mode
  by default: log signature_mismatch audit events but do not reject.
  Enforce mode requires env.CHITTYAUTH_VERIFY_SIGNATURE === 'enforce'
  exactly (typo-safe — 'true'/'1' stay in shadow).
- Fix INSERT in provisionToken to include service_name column so the
  field is available for signature reconstruction at validate time.
- 29 tests, all real (no mocks): tamper detection on each segment,
  payload reconstruction with wrong chittyId/service, malformed body,
  null inputs, signature length guard, 200-token reliability sweep
  (catches base64url-underscore split bugs), shadow vs enforce gating,
  D1 fallback path exercising the new service_name column, and a full
  refresh-chain under enforce.

Step 2 (flip enforcement, update SECURITY.md/CHARTER.md) lands in a
follow-up commit on this branch — see plans/validate-signature-verification/plan.md.

Refs #4
Related: #5 (fail-closed signing key), #6 (wrangler/package drift)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8b4b306f-67b8-4cc3-80a1-1f8c0889dea5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)

✅ Unit Test PR creation complete.

  • Create PR with unit tests
  • Commit unit tests in branch fix/validate-signature-verification

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chitcommit chitcommit marked this pull request as ready for review May 9, 2026 02:48
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 9, 2026

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.


Generating unit tests... This may take up to 20 minutes.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 9, 2026

✅ Created PR with unit tests: #8

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant