Feat/security hardening - #385
Conversation
… secure random API key
# Conflicts: # services/api/handlers/status_test.go # services/api/internal/httputil/errors.go
|
@DioChuks Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
| match |= subtle.ConstantTimeCompare([]byte(hash), []byte(target)) | ||
| } | ||
| } | ||
| return match == 1 |
There was a problem hiding this comment.
The timing-attack premise doesn't hold here. What's being compared is hmacKeyHash(key) — an HMAC-SHA256 digest of the secret, not the secret itself. Leaking how many leading bytes of a digest matched doesn't get an attacker closer to a valid key, because they'd have to invert the HMAC to use it.
So this trades an O(1) map lookup for an O(n) scan over every hash on each request, for no security gain. The crypto/subtle use in admin.go is different and correct — that one compares the raw ADMIN_API_KEY.
Happy to be shown otherwise if you've got a concrete attack in mind, but as written I'd leave the map lookup alone.
| // (as stored in API_KEY_HASHES or API_KEY) into a set for lookup. | ||
| func ParseKeyHashes(raw string) map[string]struct{} { | ||
| out := map[string]struct{}{} | ||
| if raw == "" { |
There was a problem hiding this comment.
This makes a parse function read the environment, which is surprising — ParseKeyHashes(raw) now ignores its own argument and reaches for API_KEY when raw is empty.
It also quietly widens auth: API_KEY isn't documented as an auth source, and the values in API_KEY_HASHES are HMAC digests, so anyone setting API_KEY to a plaintext key gets a silent no-match rather than an error.
If we want an API_KEY fallback it should be a deliberate change with its own docs and tests, resolved at the call site rather than inside the parser.
| @@ -0,0 +1,47 @@ | |||
| package main | |||
There was a problem hiding this comment.
This file is byte-identical to services/api/cmd/keygen/main.go in the same PR. There's no go.mod at the repo root, so this copy isn't in any module and won't build — worth deleting and keeping the one under services/api/.
|
Alright will resolve 'em |
ParseKeyHashes ignored its own argument and fell back to os.Getenv("API_KEY")
when raw was empty. That made a parse function depend on the environment and
quietly widened the auth surface: API_KEY is not documented as an auth source,
and API_KEY_HASHES holds HMAC digests, so a plaintext API_KEY could only ever
produce a silent no-match rather than a clear error.
An API_KEY fallback, if wanted, belongs at the call site with its own docs and
tests.
go vet and go test ./middleware/ pass.
Byte-identical to services/api/cmd/keygen/main.go. There is no go.mod at the repo root, so this copy belongs to no module and never built; the copy under services/api is the real one and still builds.
Addressed in this PR: the API_KEY environment fallback is removed from ParseKeyHashes, and the duplicate cmd/keygen at the repo root is deleted (it was byte-identical to services/api/cmd/keygen and, with no root go.mod, belonged to no module).
Not addressed: ConstantTimeContains still does an O(n) subtle.ConstantTimeCompare scan rather than a map lookup. That remains a performance point rather than a security one — the values compared are HMAC digests, not the raw key — so it is not holding up the merge. Filing it separately.
- database/migrations/0031 + schema.sql: both branches created a 0031 network-constraint migration. Kept dev's (via #593): #592's allowed only ('mainnet','testnet'), which would reject the futurenet and sandbox values the Network enum and the rest of the schema accept. - streamer: both implement reorg detection for #196. Kept dev's check_and_handle_reorg (from #535) — it enforces max_reorg_depth and has a passing integration test — and dropped #592's detect_and_repair_reorg, its end-to-end test, and its now-unused make_event/mock_ledger_hash helpers. - db/mod.rs: removed #592's recent_ledger_hashes and rewind_for_reorg with their tests. get_recent_ledger_metadata covers the first and the rollback inside check_and_handle_reorg covers the second. - metrics.rs: both defined REORGS_TOTAL and record_reorg. Kept #592's documented versions, dropped the terse duplicates. - .env.example: kept the idempotency encryption key and dropped the API_KEY entry, which documents a fallback #385 removed — auth.go now reads only API_KEY_HASHES and API_KEY_SALT. Corrected the stale comments too. cargo clippy -p trident-indexer --all-targets -- -D warnings, go build and go vet all pass.
2 participants
Summary
Successfully hardened API key validation in the Go REST API, added CLI/script key generation tooling, documented the complete API key lifecycle, updated environment configuration references, and verified all tests pass.
Changes Made
1. API Key Validation Middleware
crypto/subtleimport.ConstantTimeContains(validHashes, targetHash)helper usingsubtle.ConstantTimeCompareto eliminate timing side-channel vulnerabilities during hash comparisons.ParseKeyHashesto automatically fall back to single-key environment variableAPI_KEYwhenAPI_KEY_HASHESis not set.2. Key Generation Tooling
crypto/rand), hex-encodes it (64 characters), computes its HMAC-SHA256 digest usingAPI_KEY_SALT, and prints deployment configuration instructions.go run ./cmd/keygenfor quick key generation.3. Documentation & Environment References
API_KEY,API_KEY_HASHES, andAPI_KEY_SALT.4. Unit Tests
UNAUTHORIZED), invalid key (401 +UNAUTHORIZED), public/v1/healthbypass,API_KEYfallback, constant-time hash comparison logic, and raw key non-leakage.Verification Results
Unit Tests
Ran
go test ./...inservices/api:handlers,middleware,httputil,config,cursor,grpc,internal/*,validation,ws) passed 100%.Closes #48