Skip to content

network: reject already-processed or stale approval/setup requests - #1967

Open
adecaro wants to merge 6 commits into
mainfrom
1958-request-approval-check-if-the-request-was-already-approved
Open

network: reject already-processed or stale approval/setup requests#1967
adecaro wants to merge 6 commits into
mainfrom
1958-request-approval-check-if-the-request-was-already-approved

Conversation

@adecaro

@adecaro adecaro commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a pluggable replay.Guard (token/services/network/common/replay) checked in fsc.ResponderView.Call right after receive, before any crypto verification runs — rejects a duplicate or replayed proposal before paying the cost of validating it.
  • Dedup key = (TxID, Creator, Nonce, Timestamp) from the proposal itself; a symmetric freshness window also rejects requests whose timestamp has drifted too far from the guard's clock (ErrOutOfWindow, distinct from ErrAlreadyProcessed).
  • Default backend is in-memory (TTL + bounded LRU, atomic check-and-set), selected via a Config/factory.New pair so a distributed backend can be added later without touching callers — reusable/driver-agnostic so the WIP Ethereum endorser flow (ethereum driver: endorser-based approval flow on top of TTX #1669) can adopt it too.
  • Guard is constructed per-TMS (from fsc_endorsement.replay config, shared by the Fabric and FabricX loaders) rather than one instance per node, so tuning/freshness windows can differ across TMSs. Absent config falls back to replay.DefaultConfig() — fully backwards compatible.

Fixes #1958

Test plan

  • make lint-auto-fix — 0 issues
  • make checks — license/fmt/vet/staticcheck/tidy all pass
  • go test -race ./token/services/network/common/replay/... ./token/services/network/fabric/... ./token/services/network/fabricx/... — all pass
  • go build ./...
  • Docs updated: docs/configuration.md, docs/services/network-fabric.md, cross-linked from network-fabricx.md/network-ethereum.md

@adecaro adecaro added this to the Q3/26 milestone Jul 21, 2026
@adecaro adecaro self-assigned this Jul 21, 2026
@adecaro
adecaro force-pushed the 1958-request-approval-check-if-the-request-was-already-approved branch from 7b97d0e to ca33805 Compare July 21, 2026 10:31
Comment thread docs/services/network-fabric.md
@adecaro
adecaro force-pushed the 1958-request-approval-check-if-the-request-was-already-approved branch from 1af12ea to 7574d0c Compare July 27, 2026 16:58

@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.

LGTM

@adecaro
adecaro force-pushed the 1958-request-approval-check-if-the-request-was-already-approved branch 3 times, most recently from e6b6071 to d3bbba3 Compare July 29, 2026 05:40
@adecaro

adecaro commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@atharrva01 , would like to have a look at this one? It touches the endorsement part something that you also need for the EVM-related work stream you are handling 😄 🙏

@atharrva01 atharrva01 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.

Went through this properly since it sits right next to the EVM endorser I'm building. The guard reads well and the tests are better than most, 11 cases in memory_test.go covering concurrency, eviction, TTL and a moving clock. Using Contains instead of Get is right too.

One thing I think needs a call before u merge.

The cache gets written from unauthenticated input. ReceiveTx just deserializes the session bytes, FSC's builder.go does no signature check at all. The guard runs at responder.go:152 and the signature isn't verified until responder.go:342, so cache.Add happens on bytes nobody has checked.

I saw the thread with @AkramBitar and the answer that an honest client can regenerate. That holds while the party whose key gets burned is the one who submitted it. It stops holding when nothing is authenticated before the write, because then someone else can burn your key.

Concretely: the initiator sends the same signed proposal to every endorser. A malicious endorser, which a threshold policy already assumes exists, can forward it to the honest ones and burn the key before the initiator's own copy lands. Regenerating doesn't help, they can just do it again to the next one. That's a targeted denial of endorsement rather than a wasted slot.

Same cause, second effect: MaxEntries defaults to 100000 and the LRU drops the oldest on overflow (expirable_lru.go:140), so 100k unsigned junk proposals evict real keys and make them replayable again inside their window.

Would verifying the signature before Add work? It keeps the guard ahead of the expensive parts (MSP, ACL, translate, endorse), and one verify is cheap next to those and unavoidable anyway.

Second, the EVM guidance in network-ethereum.md doesn't work yet. It says build a replay.Key from txID/creator/nonce/timestamp, but my EndorseRequest only carries TokenRequest, TMSID, Anchor and Metadata. Only Anchor maps to TxID, so Timestamp would be the zero value, and I checked, that's outside the default 5 minute window. An EVM endorser following this doc would reject every request with ErrOutOfWindow.

Also, nonce and timestamp only mean anything if a signature covers them, otherwise a replayer just varies them and walks past the dedup. The EVM envelope isn't signed at that level today.

Happy to take the EVM side as follow-up, I'd just rather the doc not promise a shape the driver can't produce.

Two small things inline.

Window time.Duration `yaml:"window"`
// TTL is how long a seen key is remembered before it can be forgotten. Only meaningful
// for backends whose entries expire (e.g. BackendMemory). Must be at least 2*Window so an
// entry survives its entire potential-replay lifetime; backends enforce this floor.

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.

This says backends enforce the floor, but memory.New doesn't, only factory.New clamps it (factory.go:23). Anyone calling memory.New directly, tests or a driver following the ethereum doc, silently gets ttl < 2*window and an entry that can be forgotten while it's still replayable. Either move the clamp into memory.New or reword this to say the factory enforces it.

// FabricX reuses the same fsc_endorsement config namespace.
func NewReplayGuard(configuration tdriver.Configuration, tmsID token2.TMSID) (replay.Guard, error) {
replayCfg := replay.DefaultConfig()
_ = configuration.UnmarshalKey(ReplayKey, &replayCfg)

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.

The error is dropped, so a malformed replay block silently falls back to defaults or a half-applied config. Someone who sets a 30 minute window and typos it runs 5 without knowing. AGENTS.md asks for errors to be handled rather than blanked, and for a security control I'd fail loudly.

adecaro added 6 commits July 30, 2026 07:01
Add a pluggable replay.Guard (token/services/network/common/replay), checked
in fsc.ResponderView.Call right after receive and before any crypto
verification runs, so a duplicate or replayed proposal is rejected before
paying the cost of validating it. The dedup key is derived from the
proposal's TxID, Creator, Nonce, and Timestamp; a symmetric freshness window
also rejects requests whose timestamp has drifted too far from the guard's
clock. The default backend is in-memory (TTL + bounded LRU), selected via a
Config/factory pair so a distributed backend can be added later without
touching callers.

The guard is constructed per-TMS from the fsc_endorsement.replay config
block (shared by the Fabric and FabricX loaders), rather than one instance
per node, so tuning and freshness windows can differ across TMSs.

Fixes #1958

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
The heading generator drops the literal <name> placeholder from
docs/configuration.md headings instead of keeping the word "name",
so the anchor for the fsc_endorsement.replay section has no "name"
in it (matching the existing token.tms.<name>.auditor.locker link).
The new cross-reference included "name", which linkspector flagged
as unreachable.

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
fabric-smart-client v0.15.1's transaction.UnpackProposal now rejects a
ChaincodeInvocationSpec with an empty Input.Args slice. The fsc endorsement
tests' newValidProposalBytes helper built proposals with an empty
ChaincodeInput, which proposalKey's replay-key extraction unpacks on every
call, so every responder test in the package failed.

Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro
adecaro force-pushed the 1958-request-approval-check-if-the-request-was-already-approved branch from d3bbba3 to 1bcdb58 Compare July 30, 2026 05:01
@atharrva01

Copy link
Copy Markdown
Contributor

@adecaro gentle nudge on this one, mainly so the first point doesn't get lost since it's approved
and can go in any time.

The only thing I'd really like a call on is whether the proposal signature should be verified before
cache.Add. Right now the guard runs at responder.go:152 and the signature isn't checked until
responder.go:342, so anything that reaches the session can write keys. The bit I'd want your read
on is that the initiator sends the same signed proposal to every endorser, so a malicious one can
forward it to the honest endorsers and burn the key before the real request lands, and regenerating
doesn't help because it can just do it again. Fine either way, I just didn't want it to merge without
someone having decided it.

The EVM doc paragraph I'll take as a follow-up PR, no need to hold this up for it. One thing that
would help me though: are you expecting the EVM envelope to carry a signed nonce and timestamp? That
changes the message format on my side, so it'd be good to know before week 6 rather than after.

The two inline ones are tiny, happy for them to be ignored or picked up whenever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

request approval: check if the request was already approved

3 participants