Skip to content

feat(identity): escalating signature-throttle policy and instrumentation seam (2/3 of #2084) - #2440

Open
Effi-S wants to merge 1 commit into
split-2084/1-mechanismsfrom
split-2084/2-policy-seam
Open

Effi-S wants to merge 1 commit into
split-2084/1-mechanismsfrom
split-2084/2-policy-seam

Conversation

@Effi-S

@Effi-S Effi-S commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Split 2/3 of #2084

Adds the policy engine and makes the signature services instrumentable, while leaving the feature dormant: no driver wires it, so the observer stays Nop and the gate stays nil. No behaviour change.

  • token/services/identity/throttle: Escalator (both Observer and Gate), per-principal windowed state, level transitions, memory bounding, and Config with validated defaults.
  • token/services/identity/sigpolicy: Stack assembling metrics + audit + escalator behind the Reporter interface in one place.
  • provider.go / deserializer.go: SetObserver + instrumentation of signer/verifier resolution, with a Nop fast path (zero cost when unwired).
  • token/sig.go: the single gated surface: the SignatureThrottled sentinel, WithSignatureObserver / WithSignatureGate / WithPrincipalKeyResolver, and the allow() helper.
  • common/tms.go, token/tms.go: SignatureInstrumentation plumbing so a driver can later install the stack.

@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 — escalating signature-throttle policy (2/3 of #2084)

The policy engine itself is carefully built and well documented, and the dormancy story holds up: nothing wires it yet, so there is no behaviour change in this PR. The branch builds clean and go test -race -count=2 passes on all new packages; findings 1, 3 and 4 below were each confirmed with a throwaway probe test or benchmark.

The blocking pair is findings 1 and 2 together: the gate keys on rotating pseudonyms because no PrincipalKeyResolver is wired (2), and those fresh keys are exactly what drives the per-request O(MaxPrincipals) scan under a global mutex (1). Each is survivable alone; together they turn the defensive mechanism into an amplifier for the traffic it exists to stop, so they are worth resolving before 3/3 switches the feature on.

Nine findings are inline. One more has no file in the diff:

Finding 10 — no docs/ page for the new config surface — low

The PR adds a user-facing configuration surface — token.tms.<tms>.identity.throttle with 13 keys (ConfigKey, token/services/identity/throttle/config.go:93) — and no docs/ page, contrary to AGENTS.md's "Documentation Updates (Workflow Rule)". The non-obvious interactions found below (#3's idleTTL/deescalateAfter coupling, burst being silently raised to rate, mode: monitor still narrowing buckets) are exactly what an operator would need documented.

Checked and cleared

No deadlock between e.mu and BucketSet.mu (consistent lock order, no callbacks); the self-referential-observer guard in Observe correctly precedes e.mu.Lock(), so the documented "degrades to a dropped metric" claim holds; interface == comparisons against Nop cannot panic (multiObserver is a slice but never compared against itself); InstrumentSigner/InstrumentVerifier preserve the nil-interface contract that GetSigner's nil check depends on, and no call site type-asserts signers or verifiers beyond the driver.SigningIdentity case the wrapper handles; validators construct their own Deserializer (*/driver/validator.go:40), so the un-observed fast path genuinely applies there; evictToMakeRoom's normal-over-soft preference logic is correct; time.Duration config fields decode correctly (FSC's EnhancedExactUnmarshal uses Tag: "yaml" plus StringToTimeDurationHookFunc) and mode: off survives YAML parsing as a string. A suspected self-DoS on the endorsement path via the gated IssuerVerifier (network/fabric/endorsement/fsc/responder.go:557) was dropped: that call site is setupBehaviour.validatePublicParams, which runs on public-params setup, not per transaction.

Comment thread token/services/identity/throttle/throttle.go
Comment thread token/tms.go
Comment thread token/services/identity/throttle/throttle.go Outdated
Comment thread token/services/ratelimit/bucket.go
Comment thread token/services/identity/provider.go Outdated
Comment thread token/services/identity/sigobserve/audit.go
Comment thread token/services/ratelimit/bucket.go Outdated
Comment thread token/services/identity/provider.go
Comment thread token/services/identity/sigpolicy/stack.go Outdated
@Effi-S
Effi-S force-pushed the split-2084/2-policy-seam branch 2 times, most recently from cf3c311 to 3c2fa1f Compare October 6, 2026 14:51
@Effi-S
Effi-S force-pushed the split-2084/2-policy-seam branch 2 times, most recently from c5b0c90 to 106122f Compare October 7, 2026 09:39
@Effi-S
Effi-S force-pushed the split-2084/2-policy-seam branch from 106122f to d3ea2f5 Compare October 8, 2026 06:47
@Effi-S

Effi-S commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

@AkramBitar I'm opening a separate PR for findings 1 and 2

@Effi-S
Effi-S requested a review from AkramBitar October 8, 2026 07:28
…ion seam

Split 2/3 of PR #2084. Adds the policy engine and makes the signature
Signed-off-by: Effi-S <effi.szt@gmail.com>
@Effi-S
Effi-S force-pushed the split-2084/2-policy-seam branch from d3ea2f5 to 2da77a3 Compare October 8, 2026 12:27
@Effi-S Effi-S mentioned this pull request Oct 8, 2026

This branch has not been deployed

No deployments
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