Repository navigation
Conversation
95f3368 to
94e74b1
Compare
94e74b1 to
b84d67c
Compare
AkramBitar
left a comment
There was a problem hiding this comment.
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.
cf3c311 to
3c2fa1f
Compare
c5b0c90 to
106122f
Compare
106122f to
d3ea2f5
Compare
|
@AkramBitar I'm opening a separate PR for findings 1 and 2 |
…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>
d3ea2f5 to
2da77a3
Compare
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
Nopand the gate staysnil. No behaviour change.token/services/identity/throttle:Escalator(bothObserverandGate), per-principal windowed state, level transitions, memory bounding, andConfigwith validated defaults.token/services/identity/sigpolicy:Stackassembling metrics + audit + escalator behind theReporterinterface in one place.provider.go/deserializer.go:SetObserver+ instrumentation of signer/verifier resolution, with aNopfast path (zero cost when unwired).token/sig.go: the single gated surface: theSignatureThrottledsentinel,WithSignatureObserver/WithSignatureGate/WithPrincipalKeyResolver, and theallow()helper.common/tms.go,token/tms.go:SignatureInstrumentationplumbing so a driver can later install the stack.