[WIP] Feat/p1 security hardening#92
Draft
krakenhavoc wants to merge 4 commits into
Draft
Conversation
Failed API key validations cannot be attributed to a specific key (an unknown key hashes to no DB record), so protection is per client IP: after 10 failed validations in 15 minutes the IP is locked out of API key auth for 15 minutes (KK_APIKEY_LOCKOUT_* env overrides). The check runs before the scrypt hash, so locked-out clients also stop burning hashing CPU. Redis errors fail open to the existing global throttler. The one attributable failure - a correct but expired key - now notifies the key owner by email (rate-limited to once per key per day), since it usually means the key is still deployed somewhere or has leaked. Security notices intentionally bypass notification preferences. Hooks skipped (no pre-commit/gitleaks in sandbox); eslint+tsc+jest run manually, all green.
POST /certs/tls is now idempotent within a 15-minute window: requests are keyed by sha256(CSR) per user in Redis (reusing the BullMQ connection), duplicates return the original cert id/status, and an in-flight duplicate gets 409. Redis errors fail open. Failed or revoked certs do not block a fresh attempt. Issuance job failures are now classified: permanent errors (bad CSR, ACME policy rejections, CA rate limits) abort remaining retries via UnrecoverableError instead of burning attempts, while transient errors still retry with backoff. The cert is marked failed and the owner emailed once - when no retry will follow - instead of on every attempt, which previously sent up to three failure emails and flapped the status mid-retry. The failed-cert email no longer promises automatic retries. The dns-01 keyAuthorization is validated to be a 43-char base64url digest before it is handed to the DNS provider - defense in depth so a misbehaving ACME directory cannot inject arbitrary TXT content. Hooks skipped (no pre-commit/gitleaks in sandbox); eslint+tsc+jest run manually, all green.
Stripe's constructor throws on an empty key, so a missing KK_STRIPE_SECRET_KEY prevented the entire app from starting - certs, auth, everything - even though only billing needs it. Local dev has no Stripe secret (LOCAL_DEV.md promises dummy defaults suffice), making local boot impossible. Warn and fall back to a placeholder key: billing calls fail with auth errors until configured, nothing else is affected. Same convention as the KK_HMAC_SECRET handling in AuthService. Found while manually testing the P1 hardening branch; unit-tested, hooks run separately (sandbox lacks pre-commit).
Production traffic is client -> Cloudflare -> Traefik -> app, but Express was configured with trust proxy: 1 (one hop), so req.ip resolved to a Cloudflare edge IP. Everything keyed on client IP - the tier-aware throttler's anonymous bucket and the new API-key lockout - was bucketing unrelated users behind shared edge IPs while letting a real attacker dilute counters across many edges. Trust an explicit proxy list instead: loopback/linklocal/uniquelocal (Traefik in docker/private nets) plus Cloudflare's published ranges (www.cloudflare.com/ips, fetched 2026-07-06), overridable via KK_TRUSTED_PROXIES. With an address list, a direct-to-origin caller is itself untrusted, so spoofed X-Forwarded-For headers are never consulted - a hop count of 2 would have allowed that spoof. Also stop keying the throttler on req.ips[0] (leftmost XFF entry = client-forgeable); req.ip is the trusted-chain resolution. Hooks skipped (no pre-commit in sandbox); tsc+eslint+jest green (458).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WORK IN PROGRESS