Skip to content

feat(identity): observe and escalate signature throttling - #2084

Open
HayimShaul wants to merge 1 commit into
mainfrom
1643_observe_and_escalate_signer_verifier_throttling
Open

feat(identity): observe and escalate signature throttling#2084
HayimShaul wants to merge 1 commit into
mainfrom
1643_observe_and_escalate_signer_verifier_throttling

Conversation

@HayimShaul

Copy link
Copy Markdown
Contributor

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:

  • metrics — operation counts by op/role/outcome, operation and GetSigner duration histograms, signer-cache lookups, throttle escalations, and a gauge 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 per-principal token bucket driving an escalating normal → soft → blocked state machine that de-escalates once the principal goes quiet

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. Denials surface as token.SignatureThrottled.

Hot-path cost

BenchmarkGetSignerAndSign, warm signer cache, GetSigner + Sign per iteration:

ns/op B/op allocs/op
observer off 1401 522 9
observer on (metrics + audit + policy) 2167 863 15

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 from docs/README.md, with the configuration block in docs/configuration.md.

Fixes #1643

@HayimShaul HayimShaul added this to the Q3/26 milestone Jul 30, 2026
@HayimShaul HayimShaul self-assigned this Jul 30, 2026
@github-actions

Copy link
Copy Markdown

📊 Token Validation Benchmark

Comparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.

Variant Benchmark Params Workers TPS (base → PR) Δ TPS
csp BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 118 → 117 ➖ -0.1%
csp BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 115 → 115 ➖ +0.0%
ipa BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 70 → 70 ➖ +0.1%
ipa BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 76 → 76 ➖ -0.3%

@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch from 1a67adc to d7f9b1b Compare August 4, 2026 09:11
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

📊 Token Validation Benchmark

Comparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.

Variant Benchmark Params Workers TPS (base → PR) Δ TPS
csp BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 117 → 117 ➖ -0.0%
csp BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 116 → 116 ➖ -0.2%
ipa BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 78 → 78 ➖ +0.2%
ipa BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 76 → 76 ➖ +0.0%

@AkramBitar
AkramBitar marked this pull request as ready for review August 5, 2026 09:28
@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch from d7f9b1b to 746cbae Compare August 5, 2026 09:57
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

📊 Token Validation Benchmark

Comparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.

Variant Benchmark Params Workers TPS (base → PR) Δ TPS
csp BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 106 → 106 ➖ -0.0%
csp BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 125 → 125 ➖ -0.0%
ipa BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 70 → 70 ➖ +0.1%
ipa BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 68 → 68 ➖ +0.1%

@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch 2 times, most recently from 2933b99 to c199202 Compare August 6, 2026 06:17

@AkramBitar AkramBitar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread token/services/identity/throttle/throttle.go
Comment thread token/services/identity/throttle/throttle.go
Comment thread token/sig.go Outdated
Comment thread token/services/identity/throttle/throttle.go
Comment thread token/services/ratelimit/bucket.go
Comment thread token/services/identity/sigobserve/decorator.go Outdated
Comment thread token/services/identity/provider.go Outdated
Comment thread token/services/identity/sigpolicy/stack.go Outdated
Comment thread token/core/fabtoken/v1/driver/driver.go
Comment thread token/services/identity/throttle/config.go Outdated
@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch from c199202 to 52bce9e Compare August 6, 2026 11:19
@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch from 52bce9e to 1e0b204 Compare August 18, 2026 09:20
@AkramBitar

Copy link
Copy Markdown
Contributor

@adecaro

I would like to ask your help to review this PR.

Regards,
Akram

@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch 3 times, most recently from 4768806 to 8d6dcc3 Compare August 23, 2026 07:56
@AkramBitar

Copy link
Copy Markdown
Contributor

@HayimShaul

Could you please fix the lint issues?

Regards,
Akram

@adecaro adecaro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: does this fully address #1643?

@HayimShaul

Requesting changes: CI's checks job is currently failing on a lint error at the exact line that fixes the escalation bug this review's earlier round found, so utest/itest/bench-check haven't run against this commit yet.

Akram already did a thorough pass on an earlier commit (12 inline threads, changes requested). I checked what's changed since then rather than re-reviewing from scratch.

go build ./...                                                    -> clean
go vet (throttle, ratelimit, sigobserve, sigpolicy, common, token) -> clean
go test -count=1 ./token/services/identity/... ./token/services/ratelimit/... ./token/core/common/...  -> ok, all packages
go test -race -run TestEscalator ./token/services/identity/throttle/...  -> ok, no data race
go test -coverprofile ./token/services/identity/throttle/...      -> 97.0% of statements
gofmt -l <touched files>                                           -> clean

I reverted the new escalate soft-duration guard (throttle.go:390-393) and re-ran
TestEscalatorSoftDurationIsHonouredBeforeBlocking: it fails exactly as expected ("request 12
must stay at soft" gets blocked instead), so the test pins the fix rather than just decorating
the diff. Restored, rebuilt clean.

CI's checks is red at this commit (run) on an nlreturn finding, and because checks gates the rest of the
pipeline here, utest/itest/bench-check/publish-coverage never ran against 8d6dcc33e
the green marks on the PR predate this commit's fix.

What it fixes

# Finding Verified
1643.1 Instrument Signer/Verifier operations with metrics sigobserve + identity.Metrics cover op/role/outcome counts, latency, cache hit/miss, escalation counters — read in full
1643.2 Automated defensive response (quota reduction / temporary block) throttle.Escalator's normal → soft → blocked state machine, now with the soft-duration guard fixed and test-pinned
1643.3 Audit logging of caller identity, operation, outcome sigobserve.AuditLogger, identity hashed rather than logged raw
1643.4 Configurable thresholds, not hardcoded throttle.Config + docs/configuration.md, docs now match the >1 disables code behavior

All four are substantively covered. The three HIGH defects from the earlier review round — the
escalate logic reaching blocked after one violation, the same defect via the unlocked
buckets.Take race, and AuditorVerifier/GetSigner turning the default quota into a per-node
throughput ceiling — are fixed and, for the first, test-pinned. Full detail in the write-up.

Blocking 1 — make lint fails on the line that fixes the escalation bug

throttle.go:392 trips nlreturn ("return with no blank line before"), which is why CI's
checks job is red at this commit and utest/itest never ran. It's a one-line fix, but it's
blocking merge right now, and I want to flag it rather than let it slip past on the strength of
the already-green Analyze/build/benchmark jobs.

Non-blocking

  • Still open, not re-raised: three of Akram's earlier MEDIUM findings remain unresolved in this
    commit and are worth another look before merge — unbounded retention of escalated principals
    that go silent (throttle.go evictIdle only reclaims LevelNormal), Stop() leaving the
    escalator wired as an observer with no eviction running in both ws.go factories, and
    OutcomeError/OutcomeThrottled logged at warn on what are, at two call sites, expected/routine
    outcomes.
  • Resolved since the earlier round: the sigpolicy/stack.go mode-off cost (now backed by a
    test asserting the observer collapses to Nop), the goroutine leak on NewTokenService init
    failure in both fabtoken and zkatdlog driver.go (the suggested transferred defer-guard is now
    in both), and the config.go docs/code mismatch on the disabling threshold.

Recommendation

Fix the nlreturn lint error so checks (and therefore utest/itest) actually run against this
commit, and decide whether the three still-open MEDIUM items from the earlier round need addressing
before merge or can be tracked as follow-up. Everything else checked out clean.

Comment thread token/services/identity/throttle/throttle.go
@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch from 8d6dcc3 to 36e7f3e Compare August 25, 2026 17:49
@AkramBitar

Copy link
Copy Markdown
Contributor

@HayimShaul

Could you please fix lint issues?

Regards,
Akram

@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch 5 times, most recently from 675a5db to 810e528 Compare August 27, 2026 12:49
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>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>

fix(metricsdoc): sync the metrics reference with the signature instruments

The signature-throttling change added five metrics to token/services/identity
and moved the wallet-service call in both token drivers, which left three
checks in token/services/metricsdoc failing:

- TestMetricsReference: testdata/metrics.golden did not list
  identity_signature_operations_total,
  identity_signature_operation_duration_seconds,
  identity_signer_cache_lookups_total, identity_throttle_escalations_total
  or identity_throttled_principals. Regenerated with UPDATE_GOLDEN=1.
- TestReferenceDocumentsEveryMetric: the same five were undocumented. Added
  them to the identity table in docs/development/metrics.md, pointing at
  docs/security/signature_observability.md for the label value sets.
- TestWiringSitesArePresent: both drivers now call
  "ws, sigStack, err := d.newWalletService(" - the zkatdlog side also lost
  its exported spelling - so the pinned wiring snippets no longer matched.

The five new metrics have no Grafana panel, so the two sentences claiming the
overview dashboard covers every exported metric are narrowed to say so instead
of being left wrong.

go test ./token/services/metricsdoc/... passes.

Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
@HayimShaul
HayimShaul force-pushed the 1643_observe_and_escalate_signer_verifier_throttling branch from 810e528 to 7f797c2 Compare August 30, 2026 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Observability, Alerting, and Automated Throttle Escalation [LOW]

3 participants