feat(identity): observe and escalate signature throttling - #2084
feat(identity): observe and escalate signature throttling#2084HayimShaul wants to merge 8 commits into
Conversation
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
1a67adc to
d7f9b1b
Compare
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
d7f9b1b to
746cbae
Compare
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
2933b99 to
c199202
Compare
AkramBitar
left a comment
There was a problem hiding this comment.
Code review — signature observability & throttling
Reviewed at c199202. Overall the layering is clean (observer chain → escalator → gate) and the test coverage on the new packages is good. The findings below are mostly about the escalation state machine and the lifetime/keying of per-principal state, which is where this feature can turn into an availability problem rather than a defence against one.
Two findings I'd consider blocking:
| # | Severity | Where | What |
|---|---|---|---|
| 1 | HIGH | throttle.go:378 |
escalate ignores levelUntil, so a principal goes normal → blocked in two consecutive requests |
| 2 | HIGH | sig.go:89 |
Gated call sites key on a single fixed identity, making rate: 200 a hard per-TMS throughput ceiling |
| 3 | MEDIUM | throttle.go:500, bucket.go:252 |
Escalated principals are never evicted → attacker-controlled unbounded memory growth |
| 4 | MEDIUM | ws.go (both drivers) |
sigStack.Stop() kills eviction but leaves the escalator wired as an observer |
| 5 | MEDIUM | audit.go:94 |
Warn-level audit lines for routine control flow, and one per throttled request |
| 6 | MEDIUM | decorator.go:30, provider.go:363 |
Wrapped signer pins the resolution-time ctx for its whole lifetime |
| 7 | LOW | stack.go:77 |
mode: off does not restore the zero-cost path |
| 8 | LOW | driver.go:192 (both drivers) |
sigStack goroutines leak on every NewTokenService failure |
| 9 | LOW | config.go:99 |
Documented way to disable the error-rate trigger is off by one comparison |
Findings 1 and 2 compound: with the default config, a single momentary spike past burst on the auditing path blocks the auditor for blockDuration, and AuditView then fails every transaction with failed verifying auditor signature. Finding 1 is reproducible — details inline.
Details are in the inline comments.
| e.mu.Unlock() | ||
|
|
||
| if e.buckets.Take(principalID) { | ||
| return false, "" | ||
| } | ||
|
|
||
| e.mu.Lock() | ||
| e.escalate(ctx, principalID, p, ReasonRate) | ||
| level := p.level | ||
| e.mu.Unlock() |
There was a problem hiding this comment.
HIGH (same defect as the escalate comment, second route into it) — e.mu is dropped around buckets.Take, so concurrent over-quota requests each escalate a level.
The lock is released at line 220, Take runs unlocked, then the lock is re-acquired at 226 to mutate p. Two goroutines that both fail Take for the same principal will therefore both call escalate, and the second one observes p.level == LevelSoft set by the first — producing the same normal → blocked jump from a single bucket-exhaustion event, with no second round-trip needed.
At the default rate: 200 / burst: 400 on a concurrent call site this is the likely path in practice, not the sequential one.
Fixing escalate to honour levelUntil closes this too, since the second caller would find levelUntil in the future. If you'd rather fix it here, re-check the bucket under the lock (or have Take return the level-relevant decision) instead of trusting the pre-unlock read.
| for id, p := range e.principals { | ||
| if p.level == LevelNormal && p.lastSeen.Before(cutoff) { | ||
| delete(e.principals, id) | ||
| } |
There was a problem hiding this comment.
MEDIUM — escalated principals are retained forever, so the DoS defence is itself an attacker-controlled unbounded allocation.
The p.level == LevelNormal guard means any principal that has ever escalated is never deleted, regardless of IdleTTL. Its companion in ratelimit/bucket.go:252 skips overridden buckets — which is exactly the flag transition(LevelSoft) sets — so both halves of the state survive indefinitely.
A principal that escalates once and never calls again therefore keeps its principal struct (the slot ring + map key) and its bucket for the process lifetime, and e.counts / the identity_throttled_principals gauge never return to zero — so the gauge reads as a persistent incident rather than a current one.
The cost to an attacker is low: minting fresh identities and spending either burst+1 requests or ~MinSamples (50) failing operations per identity permanently pins an entry. There is no cap on len(e.principals). Since the identities are unverified at this point (that's the point of gating verification), nothing bounds the key space.
Worth doing both: evict escalated principals whose levelUntil has passed and which have been idle for IdleTTL (the block has expired, so there is no state left worth keeping), and add a hard cap on len(e.principals) with an eviction policy for when it's hit, so the worst case is bounded regardless.
c199202 to
52bce9e
Compare
Signing, verification and signer resolution had no observability: an operator could not see how often they happen, how long they take, or how often they fail, and an identity making a flood of invalid-signature calls looked no different from a healthy one. Add a leaf observer package, sigobserve, that the signature surface reports to, and three sinks for it: - metrics: operation counts by op/role/outcome, operation and GetSigner duration histograms, signer-cache lookups, throttle escalations and the number of currently throttled principals - audit log: one greppable record per operation, naming the principal by identity hash only so identity material never reaches a log file - throttle policy: a token bucket per principal driving an escalating normal -> soft -> blocked state machine that de-escalates once the principal is quiet again identity.Provider, common.Deserializer and the signers/verifiers they hand out are instrumented; the throttle gate is consulted only at token.SignatureService. Driver validators are instrumented but never gated, because per-node call history would make transaction validation non-deterministic across nodes. AreMe/IsMe are ungated too: refusing them would return a wrong answer rather than an error. Configured per TMS under token.tms.<name>.identity.throttle and defaulting to monitor mode, so a deployment gets the metrics and the audit trail without any identity being blocked until enforcement is switched on deliberately. Fixes #1643 Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
52bce9e to
1e0b204
Compare
Signature operations had no observability, and an identity flooding the node with invalid-signature calls looked no different from a healthy one.
This adds a leaf observer package,
sigobserve, that the signature surface reports to, plus three sinks:op/role/outcome, operation andGetSignerduration histograms, signer-cache lookups, throttle escalations, and a gauge of currently throttled principalsnormal → soft → blockedstate machine that de-escalates once the principal goes quietidentity.Provider,common.Deserializerand the signers/verifiers they hand out are instrumented. The throttle gate is consulted only attoken.SignatureService: driver validators are instrumented but never gated, because per-node call history would make transaction validation non-deterministic across nodes.AreMe/IsMeare ungated too — refusing them would return a wrong answer rather than an error.Configured per TMS under
token.tms.<name>.identity.throttleand defaulting to monitor mode, so a deployment gets the metrics and the audit trail without any identity being blocked until enforcement is switched on deliberately. Denials surface astoken.SignatureThrottled.Hot-path cost
BenchmarkGetSignerAndSign, warm signer cache,GetSigner+Signper iteration:The audit logger asks the logger whether the level is enabled before rendering a record, which keeps the string build off every routine operation at a production log level.
Docs
docs/security/signature_observability.md(metrics reference, audit format, escalation policy, rollout), linked fromdocs/README.md, with the configuration block indocs/configuration.md.Fixes #1643