Security release/candidate 1 - #4109
Open
lionakhnazarov wants to merge 446 commits into
Open
Conversation
|
Important Review skippedToo many files! This PR contains 395 files, which is 295 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (395)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lionakhnazarov
marked this pull request as ready for review
June 30, 2026 18:26
lionakhnazarov
force-pushed
the
security-release/candidate-1
branch
2 times, most recently
from
July 12, 2026 15:16
ee628a3 to
cbf31e0
Compare
Member
|
The PR description says head is Worth double-checking |
lionakhnazarov
force-pushed
the
security-release/candidate-1
branch
from
July 16, 2026 13:53
dc42a85 to
971f603
Compare
Collaborator
Author
thanks Leo! Fix applied |
lrsaturnino
added a commit
to lionakhnazarov/keep-core
that referenced
this pull request
Jul 24, 2026
…ee RPCs, monitoring wiring (vet round 2) Addresses the round-2 vet report (findings 1, 3, 6 + three net-new). P0 — production discovery/identity is now authoritative, not optional (finding 1): - Completeness now REQUIRES service-discovery reconciliation and on-chain identity verification (CollectorConfig.RequireServiceDiscovery / RequireIdentityVerification, set true by the command). A missing feed blocks readiness instead of degrading to trusting inventory; the command warns that readiness cannot be certified. (isComplete, main.go) - Service discovery is keyed by network ID (per-instance) not operator-only, so multiple instances of one operator resolve to distinct discovered targets (ServiceDiscovery.byNetworkID, MetricsURLForInstance, InventoryInstance.NetworkID, applyDiscoveredTargets). - The metrics adapter validates the responding node's self-attested identity (diagnostics chain_address + network_id) instead of copying InstanceID/ OperatorAddress from inventory; a mismatch is rejected (reportadapter.go). P0 — reporter revision is durable across collector restart (net-new 1): - ReporterRevision now derives from the attestation timestamp (UnixNano) rather than a process-local counter that reset on restart and sat below the persisted high-water mark. Added TestCollector_ReportsAcceptedImmediatelyAfterRestart and TestMetricsReportSource_ReporterRevisionSurvivesRestart. P1 — remaining evidence gaps (finding 3): - StakingProvider must be a canonical, non-zero address; expected image digest must be sha256:<64 hex> (test fixture corrected to a full digest); cross-instance staking-provider contradictions are fail-closed (last claim no longer wins). P1 — no collector lock across network calls (net-new 2): - Identity verification runs BEFORE the central-state lock (verify first, apply under lock) so a degraded RPC never blocks readers; the RPC timeout derives from the passed-in context (CollectContext(ctx), IdentityVerifier now takes ctx, ethCall uses context.WithTimeout(ctx, ...)). P1 — monitoring deployment wiring (finding 6): - A non-loopback API bind without an allowlist is refused at startup. - cutover-readiness.json added to the Grafana ConfigMap; instance-reason text panel replaced with a real table sourced from the readiness API via a new Infinity datasource (+ plugin install). - Added a cutover-roster Deployment/Service/PVC and a Prometheus scrape job, plus an Alertmanager workload + routing tree matching the alerts' route_to label. (Image digest, allowlist CIDR, inventory ConfigMap/Secret, and receiver integrations are REPLACE_ placeholders — ops follow-up, documented in README.adoc.) P1 — 30-day resolved purge (net-new 3): resolution of the semantic conflict. - The fail-closed reopening of a vanished resolved operator (finding 2, RESOLVED) and the naive 30-day purge of a departed resolved operator are in genuine, irreconcilable tension for the "resolved operator departs" population: a departed operator MUST reopen offline_unknown and be retained indefinitely, so it can never age out via the resolved path, and an actively-resolved operator is continuously re-confirmed (ResolvedAt refreshed) and must not be dropped. purge therefore remains a bounded-store backstop for a resolved record no longer being re-confirmed. Restored independent coverage as a white-box test of the purge mechanism (TestCollector_PurgeResolvedAfter30Days): a resolved record older than the retention window is purged with its instances, while a fresh resolved record and any blocking record are retained. P1/P2 — harness: README/compose/script examples now use an immutable @sha256: digest instead of the mutable keep-client:candidate tag the harness itself rejects. P2 — draft PR body: corrected the inaccurate "wire-compatible / no session-ID/KDF change" claim to scope it to THIS diff only and explicitly note PR threshold-network#4109's Part A cryptographic changes are wire-breaking and not described here. Live PR/issue untouched (out of band). Local CI oracle (format, vet, staticcheck SA*, golangci-lint, gosec, go test ./..., race tier-2 subset, integration, race over new packages): PASS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lrsaturnino
added a commit
to lionakhnazarov/keep-core
that referenced
this pull request
Jul 24, 2026
…ee RPCs, monitoring wiring (vet round 2) Addresses the round-2 vet report (findings 1, 3, 6 + three net-new). P0 — production discovery/identity is now authoritative, not optional (finding 1): - Completeness now REQUIRES service-discovery reconciliation and on-chain identity verification (CollectorConfig.RequireServiceDiscovery / RequireIdentityVerification, set true by the command). A missing feed blocks readiness instead of degrading to trusting inventory; the command warns that readiness cannot be certified. (isComplete, main.go) - Service discovery is keyed by network ID (per-instance) not operator-only, so multiple instances of one operator resolve to distinct discovered targets (ServiceDiscovery.byNetworkID, MetricsURLForInstance, InventoryInstance.NetworkID, applyDiscoveredTargets). - The metrics adapter validates the responding node's self-attested identity (diagnostics chain_address + network_id) instead of copying InstanceID/ OperatorAddress from inventory; a mismatch is rejected (reportadapter.go). P0 — reporter revision is durable across collector restart (net-new 1): - ReporterRevision now derives from the attestation timestamp (UnixNano) rather than a process-local counter that reset on restart and sat below the persisted high-water mark. Added TestCollector_ReportsAcceptedImmediatelyAfterRestart and TestMetricsReportSource_ReporterRevisionSurvivesRestart. P1 — remaining evidence gaps (finding 3): - StakingProvider must be a canonical, non-zero address; expected image digest must be sha256:<64 hex> (test fixture corrected to a full digest); cross-instance staking-provider contradictions are fail-closed (last claim no longer wins). P1 — no collector lock across network calls (net-new 2): - Identity verification runs BEFORE the central-state lock (verify first, apply under lock) so a degraded RPC never blocks readers; the RPC timeout derives from the passed-in context (CollectContext(ctx), IdentityVerifier now takes ctx, ethCall uses context.WithTimeout(ctx, ...)). P1 — monitoring deployment wiring (finding 6): - A non-loopback API bind without an allowlist is refused at startup. - cutover-readiness.json added to the Grafana ConfigMap; instance-reason text panel replaced with a real table sourced from the readiness API via a new Infinity datasource (+ plugin install). - Added a cutover-roster Deployment/Service/PVC and a Prometheus scrape job, plus an Alertmanager workload + routing tree matching the alerts' route_to label. (Image digest, allowlist CIDR, inventory ConfigMap/Secret, and receiver integrations are REPLACE_ placeholders — ops follow-up, documented in README.adoc.) P1 — 30-day resolved purge (net-new 3): resolution of the semantic conflict. - The fail-closed reopening of a vanished resolved operator (finding 2, RESOLVED) and the naive 30-day purge of a departed resolved operator are in genuine, irreconcilable tension for the "resolved operator departs" population: a departed operator MUST reopen offline_unknown and be retained indefinitely, so it can never age out via the resolved path, and an actively-resolved operator is continuously re-confirmed (ResolvedAt refreshed) and must not be dropped. purge therefore remains a bounded-store backstop for a resolved record no longer being re-confirmed. Restored independent coverage as a white-box test of the purge mechanism (TestCollector_PurgeResolvedAfter30Days): a resolved record older than the retention window is purged with its instances, while a fresh resolved record and any blocking record are retained. P1/P2 — harness: README/compose/script examples now use an immutable @sha256: digest instead of the mutable keep-client:candidate tag the harness itself rejects. P2 — draft PR body: corrected the inaccurate "wire-compatible / no session-ID/KDF change" claim to scope it to THIS diff only and explicitly note PR threshold-network#4109's Part A cryptographic changes are wire-breaking and not described here. Live PR/issue untouched (out of band). Local CI oracle (format, vet, staticcheck SA*, golangci-lint, gosec, go test ./..., race tier-2 subset, integration, race over new packages): PASS.
lrsaturnino
force-pushed
the
security-release/candidate-1
branch
from
July 24, 2026 14:55
54cdc4a to
33808cb
Compare
Contributor
|
Two 1. The
|
| advisory | summary | fixed in |
|---|---|---|
GO-2022-1098 |
DoS in message decoding | 0.23.2 |
GHSA-2chg-86hq-7w38 |
mishandles witness size checking | 0.23.2 |
GO-2024-2818 |
consensus failures | 0.24.0 |
GHSA-3jgf-r68h-xfqm |
consensus failures | 0.24.0 |
GO-2024-3189 |
consensus failure | 0.24.2-beta.rc1 |
GHSA-27vh-h6mc-q6g8 |
incorrect re-implementation of FindAndDelete |
0.24.2-beta.rc1 |
The first two are already fixed at the version this repo requires — the replace reintroduces them.
Unlike the x/crypto case, this code is in the build: go list -deps ./... returns 16 btcsuite/btcd packages including blockchain, blockchain/indexers, btcec, btcjson and btcutil. Consensus-failure advisories in a Bitcoin bridge seem worth either fixing here or recording why the downgrade is deliberate — right now the replace carries no comment, unlike the keep-common and protobuf ones above it.
mswilkison
added a commit
that referenced
this pull request
Jul 26, 2026
All five workflows that build and test the contracts pin Node 18.15.0,
which reached end of life on 2025-04-30 and receives no further security
patches. `engines.node` in both solidity manifests and the two contract
Dockerfiles say the same.
Target is Node 22 (Jod), EOL 2027-04-30. Node 24 has more than twice the
remaining support and was tried first, but Hardhat's undici call is
incompatible with Node 24 until ~2.20, and @defi-wonderland/smock breaks on
hardhat >= 2.20 -- the two windows do not overlap. Node 22 is the ceiling
in both repos until smock is replaced, and smock is archived upstream
(wonderland-archive/smock).
No dependency changes. Verified against this branch, solidity/random-beacon:
Node v18.20.8 168 passing, 12 failing
Node v22.23.1 168 passing, 12 failing
The 12 are identical and pre-existing on both -- every one is
"No deployment found for: T", a missing external deployment artifact in a
local checkout, unrelated to the runtime. hardhat 2.10.0 and
@defi-wonderland/smock 2.0.7, the packages most likely to break on a newer
runtime, are clean.
infrastructure/ is deliberately untouched. Its Dockerfiles are not built
by CI -- `client.yml` lists `infrastructure/**` under `paths-ignore`, and
the only build contexts in the workflows are `.`, `./solidity/ecdsa` and
`./solidity/random-beacon`. The node:11 init container there wants a
decision about whether it is still deployed, not a bump to a file nothing
builds.
Stacked on #4109 -- carries that branch's commits and should merge after
it. #4109 changes no Node-runtime surface, so there is no overlap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
client_info now carries the three labels fleet reconciliation matches against — release version, exact source revision, and compiled protocol epoch — instead of version alone; the maintainer process exports the same identity. Diagnostics gain a protocol_participation object with the epoch, resolved cutover block and its source, live gate state, clock availability, and active per-mode ceremony counts, so a port-enabled node answers the readiness questions from one scrape.
…rmit The beacon DKG permit previously selected only a protocol mode: neither cancellation nor commit safety reached the running ceremony, so a clock failure or forced quiescence could cancel a permit while GJKR kept executing, submitted a result, and registered key material. The sync state machine now executes under the ceremony context, and the beacon DKG threads its permit end to end: the terminal result submission and the signer activation both pass a last-moment completion fence, with no implicit default guard. Interruption after key generation preserves the orphaned signer in a new quarantine namespace beside the active beacon keystore, together with an audit metadata record. The namespace is a sibling directory, so an active-group scan of any release cannot load quarantined outputs as active signers. An accepted result whose local activation the gate refuses is saved durably without cache activation, since dropping an accepted share would permanently reduce its group. Deterministic node-level tests cover a legacy permit completing after the cutover block, forced shutdown and chain-clock failure inside the publication window preserving signers only in quarantine, and mid-GJKR cancellation aborting cleanly with nothing persisted.
…ermit The lifecycle controller previously drained only beacon DKG permits: relay signing, restart resume, share forwarding, and timeout monitoring ran untracked, so quiescence could declare the process drained while real protocol work was still in flight, and a canceled gate could not reach any of those paths. Relay entry signing now issues one permit per local membership, with the restart path going through the gate's resume verification against the on-chain request start block. The terminal relay entry submission passes a completion fence. Timeout monitoring holds a permit anchored at the relay request block and fences the report as a penalty commit, so a legacy monitor cannot create new penalty state at or after the cutover block and no monitor can report once quiescence begins. BroadcastChannelForwarderFor returns a lifecycle handle with idempotent Close and a Done channel: the libp2p relay stops on TTL, provider shutdown, or explicit close, and the local provider returns an already-done no-op. Share forwarding runs under a forwarding permit that closes the handle when the gate cancels, and releases the permit when the relay ends on its own. Tests cover the suppressed legacy timeout report after the cutover, the normal report below it, and both directions of the forwarding lifecycle.
…kstop Signal capture was installed only after both applications started, leaving every protocol callback spawned during startup exposed to the default signal action. The channel is now armed before any component initializes; a signal arriving mid-startup is held and handled by the lifecycle controller once startup completes. The quiesce backstop previously multiplied the completion bound straight into a time.Duration with no overflow check and no block margin. It is now derived at startup from the maximum legacy completion bound plus a reviewed block margin, with every step overflow-checked; an overflowing deadline refuses startup instead of silently truncating the grace period. The startup log records the bound, margin, block-interval bound, and resulting backstop as the same inputs the release manifest derives the external termination grace from.
… smoke The release directory previously claimed the cutover gate was intentionally unimplemented and carried no Part A rehearsal structure. It now holds the rehearsal driver, fleet shell, and evidence schema for the two mandatory container rehearsals: the exact-image single-release rehearsal and the homogeneous rollback rehearsal. The driver runs the repository-local Go proofs of the gate today and validates the container-rehearsal inputs, requiring immutable image digests. The container stages refuse to run with an explicit BLOCKED report naming their missing inputs — a rehearsal chain with deployed contracts, prior and R1 digests, per-node keys, storage snapshots — rather than pretending to pass. Accepted runs must produce an evidence record conforming to the schema: exact SHAs, per-architecture digests, chain ID and cutover block, per-stage canonical and callback blocks, permit modes, gauges, transaction hashes, and state checksums.
… barrier The rollback barrier requires an offline classification of every stopped node's persisted protocol state before any prior binary may start. The new participation-state-audit command produces that manifest from a storage snapshot copy: a full file inventory of the beacon, beacon quarantine, and tBTC namespaces with checksums of the at-rest encrypted bytes, plus an interpreted view of the beacon active memberships and quarantined outputs when the storage password is supplied. Every inconsistency is a finding and fails the audit: quarantine metadata without its preserved membership, memberships without metadata, records that cannot be decrypted or decoded, and quarantine state surfacing in the active-group scan. Without the password the tool degrades to a raw inventory and refuses to classify the snapshot as consistent. Chain reconciliation is explicitly out of scope: the manifest records it as not performed and never authorizes activating quarantined material by itself.
…tion-safe A canceled ceremony could be held hostage by a stalled chain: the sync machine's start-block wait and its between-state delay waits were synchronous WaitForBlockHeight calls that ignored the machine context, so a permit canceled during a held wait could not return, surface its cancellation cause, or reach the signer recovery path. Both waits now select on the machine context and return the cancellation cause promptly; two held-wait tests pin the behavior before execution and between states. Beacon result and relay event subscriptions also sent on unbuffered channels, so a callback in flight when its consumer returned on cancellation or timeout blocked forever. The three remaining unbuffered subscription channels now carry the same one-slot buffer the relay timeout monitor already uses.
A termination signal was captured at the top of startup but acted on only after every component had initialized, so an operator's shutdown request during a slow startup left the gate issuing new permits until the end of initialization. The quiesce drive now runs in a dedicated lifecycle controller launched immediately after the gate and its backstop deadline are constructed — before the network provider, beacon, or tBTC can begin protocol work — so the first signal refuses all subsequent permits no matter when it arrives. The controller reports its shutdown cause before canceling the run context, so the main goroutine always prefers the signal report over a bare context end.
…ross-validated The offline state audit could race its own findings list — the persistence error-drain goroutines appended concurrently with the descriptor loops — and could classify a snapshot as consistent from namespace inventory alone, leaving every rollback-manifest question unasked. Findings now go through one mutex, and the manifest carries the snapshot identity (aggregate checksum, root access mode), an expected-layout scan that flags any entry the audit cannot classify, and explicit rollback blockers: the audit exits nonzero until chain reconciliation, Bitcoin reconciliation, the quiescence outcome report, and prior-reader compatibility evidence are supplied and recorded, so inventory alone can never read as rollback-ready. Interpretation now cross-validates what it decodes. Beacon quarantine metadata is checked against its schema version, this release's epoch, the ceremony class, the cutover arithmetic of its recorded mode and anchor, the directory and member file name it is stored under, the decrypted membership it pairs with, and the active namespace — a group present on both sides of the quarantine boundary is the exact ambiguity the quarantine exists to prevent. Active beacon records are checked against their storage location the active scan trusts. tBTC keystore records are decoded with the same full decode the wallet registry loader uses, via a new exported audit decoder proven against the loader fixtures, and the work namespace is classified with unclassifiable records reported.
The rehearsal fleet shell attached every node only to an internal network while requiring an external chain endpoint, so no node could ever reach ETH_WS_URL; the fleet now spans an internal protocol network plus a chain-egress network, and the mounted per-node inputs are actually wired into each node through --config, a read-only keystore mount, the storage volume, and the key password pass-through. Preflight verifies the per-node config exists before pulling images. The driver gains the missing halves of the rehearsal contract: a validate-evidence stage that checks every produced record against the evidence schema, local proofs extended to the signal lifecycle, held-wait cancellation, state audit, and audit decoder tests, and a manually dispatched workflow that runs the local proofs in the same build image the client CI uses and keeps the container stages BLOCKED-red until the fleet inputs exist. The port smoke harness asserted the participation gate metrics were not exposed, which stopped being true when the gate landed in this tree; the positive-response assertions now require the gate gauges and the participation diagnostics object. Harness and schema comments now describe the rehearsals in their own terms.
The block counters deliver exactly one notification per registered waiter with a blocking send on an unbuffered channel. A canceled execution that simply walked away from its waiter — the interruptible start-block and delay waits, or the machine select abandoning a state's end-block waiter — left that sender goroutine parked forever once the height was eventually reached. Ownership of an abandoned waiter now passes to a drain goroutine that performs the single receive, so the eventual sender terminates and the drain exits with it. The new tests drive a manually advanced counter that reproduces the production waiter contract and prove the sender completes after a cancellation, both for the bare wait helper and for the machine's abandoned end-block waiter.
Every tBTC ceremony choke point now runs under a permit from the shared release gate, so process quiescence, clock failure, and the active-mode accounting finally see tBTC work: - Each locally controlled DKG member acquires a permit anchored at the DKG started event block immediately before its goroutine; the retry loop, announcer classification, and execution context all derive from the permit, and a gate cancellation is no longer counted as an ordinary DKG failure. - Each wallet action acquires one permit before handler and dispatcher setup, anchored at the proposal-processing start block; the action owns the permit for its whole execution, and its signing and Bitcoin broadcast run on the permit context instead of unowned background contexts. - The coordination procedure runs under a tracking permit anchored at the window's coordination block; its wire format is shared by both releases, so the procedure itself runs in either mode. - The last-moment completion fences guard signer activation, DKG result submission, and every Bitcoin broadcast attempt; the penalty fence guards the heartbeat's consecutive-failure accounting and the terminal inactivity claim submission, so quiescence or a legacy-after-cutover result suppresses new penalty state instead of punishing the grace. - A refused activation preserves the generated share without activating it: durably in the active namespace when the wallet is registered on chain, otherwise in the new protected tbtc-quarantine storage namespace no release's active-wallet scan reads, with audit metadata recording mode, cutover arithmetic, and a seed hash but never raw seeds or shares. - registerSigner is split into durable save and cache activation so the non-activating save path exists. The pinned tss-lib revision has no reviewed per-party legacy mode, so a tECDSA ceremony cannot reproduce the legacy proof transcript. A legacy-mode permit for DKG, signing, heartbeat, or any wallet action is therefore refused outright — emitting the hardened transcript under a legacy permit would produce wire traffic compatible with neither release. The signing executor additionally hard-refuses any non-security-v2 mode as defense in depth. The tbtc block-height wait helper also drains its abandoned waiter on context cancellation, matching the protocol state machine fix.
The shell-analysis stage runs whatever shellcheck the runner image ships, and that is 0.9.0 while local runs are on 0.11.0. The two report the same false positive about helpers reached only through a dispatch they cannot follow under different codes: SC2317 on the function's body before 0.11.0, the release that added SC2329, and SC2329 on the definition from there on. The suppressions already at those sites named only the newer code, so 172 findings the local analyzer never shows failed the gate on the runner. Name both codes wherever one was already carried. That holds the gate green either side of the split rather than on the version the runner image happens to ship, which nothing in this path pins.
…ntext .dockerignore drops docs*/ and scripts/ wholesale, but tests under cmd and pkg/clientinfo open the release manifest, the three schemas its records are validated against, the deployment scaffolds its termination grace is compared to, and the participation metrics reference, all through paths relative to their own package. Those tests run inside the build image, so the files resolve in a working tree and are missing exactly where CI executes them. The scaffolds surfaced only by fixing the rest: each case stops at the first file it cannot read, so the run reported one missing input per test and hid the two behind it. Negate the eight paths back into the context — the files themselves, not the trees they sit under — and re-derive the scaffold's hand-written mirror of these rules, which verify_build_context_mirror holds to the real file on every push that touches either.
…test greps GNU grep 3.11 (the CI runner's grep) doesn't expand a literal \t inside a single-quoted -E/-oE pattern, so the three ceremony/case-line extraction greps in test-validate-evidence.sh matched nothing and returned 1. Under set -euo pipefail that silently aborted the self-test before it printed its own pass/fail tally, failing the scaffold-lint job with no visible error. It passed locally because BSD grep on macOS does expand \t there. Switching each pattern to $'...' ANSI-C quoting makes bash substitute a real tab byte before grep ever sees it, so both implementations match identically. Verified against GNU grep 3.11 in an Ubuntu 24.04 container: the self-test now completes with '642 passed, 0 failed' where it previously died silently.
…CI parallelism Updated the block time from 100ms to 200ms in the DKG cutover integration tests to ensure the real key-generation transcript completes within one attempt's protocol window under CI package parallelism. This change addresses potential race conditions during testing.
CI measured 345003 gas, which sat 3 over the old 330000±15000 band.
piotr-roslaniec
added a commit
that referenced
this pull request
Aug 7, 2026
## Summary `go.mod` replaces the whole `btcd` module with **v0.22.3** - *below* the `v0.23.2` the module itself requires - which keeps three already-fixed security advisories open against the Bitcoin consensus, wire, and script packages the bridge actually compiles. (Three vulnerabilities, six identifiers counting the GHSA aliases.) ``` go.mod github.com/btcsuite/btcd v0.23.2 // require go.mod github.com/btcsuite/btcd => github.com/btcsuite/btcd v0.22.3 // replace — wins ``` This is long-standing on `main` and not specific to any release branch. It surfaced during a dependency sweep of #4109; that PR's integration branch inherits it, and this fix is deliberately targeted here rather than stacked on the release candidate so it gets CI and normal review. Unlike an unused-code advisory, this code **is** in the build — `go list -deps ./...` returns 16 `btcsuite/btcd` packages including `blockchain`, `blockchain/indexers`, `btcjson`, `btcutil`, `txscript`, and `wire`. | Advisory | Package | Fixed in | Status before | After | |---|---|---|---|---| | GO-2022-1098 / GHSA-2chg-86hq-7w38 — DoS in message decoding, witness size checking | `wire` | 0.23.2 | **open** (below the repo's own require) | fixed | | GO-2024-2818 / GHSA-3jgf-r68h-xfqm — consensus failures | `blockchain`, `txscript` | 0.24.0 | **open** | fixed | | GO-2024-3189 / GHSA-27vh-h6mc-q6g8 — incorrect `FindAndDelete` re-implementation | `txscript` | 0.24.2-beta.rc1 | **open** | fixed | ## Why the downgrade was there, and why it can go btcd v0.23 extracted `btcd/btcec` into the separate module `btcd/btcec/v2`, and later versions don't ship the pre-split package at all. Two consumers still import the pre-split path and can't simply be edited: the pinned **tss-lib fork** (`crypto/ckd`, `tss/curve`) and **first-party key-handling code** (`pkg/crypto/ephemeral`, `pkg/crypto/secp256k1`, `pkg/net/...`, `pkg/chain/local_v1`, `pkg/internal/pbutils`), which relies on v1 `btcec` types being aliases of the `crypto/ecdsa` types. So one crypto package was holding the entire module — consensus code included - a minor version back. This PR serves *just that package* from an in-tree copy of the btcd v0.22.3 `btcec` sources (`third_party/btcsuite/btcec`) and moves the module itself to **v0.24.2**. The vendored copy is byte-identical to upstream apart from a mechanical `gofmt` pass; `VENDOR.md` records provenance, a reproducible `diff -r` verification recipe, and the exit path (delete the directory once tss-lib migrates to `btcec/v2`). The upstream btcec test suite ships with it and passes. ### Why v0.24.2 and not v0.26.2 **v0.24.2 is the lowest version that fixes all three advisories** (the last one lands in `0.24.2-beta.rc1`), and going higher isn't currently possible: - **v0.25.0 requires `btcec/v2` v2.3.5**, and `btcec/v2` **v2.3.4** changed `ecdsa.SignCompact` from `([]byte, error)` to `[]byte`. go-ethereum v1.13.15 calls the two-value form in `crypto/signature_nocgo.go`, so pulling `btcec/v2` that far forward breaks **every cgo-less build** - including the cross-compiled release binaries that `make release` produces. Raising btcd past v0.24.2 requires upgrading go-ethereum first, which is out of scope here. - **v0.26** additionally restructured btcd: `wire`, `txscript`, `chaincfg`, `chainhash`, and `btcutil` moved to separate `/v2` modules and the plain-path packages no longer exist. The go-electrum and tss-lib forks import the plain paths with import strings we don't control, so v0.26.x isn't reachable without forking both. So this is a compatibility ceiling, not a security gap - v0.24.2 is advisory-clean. Both constraints are recorded in the `go.mod` comment so the next person to touch this pin doesn't have to rediscover them. ### The `btcd/v2` alias goes too `replace github.com/btcsuite/btcd/v2 => github.com/btcsuite/btcd v0.23.4` only isolates *self-contained* packages. Any aliased package that imports a sibling btcd package does so by plain path, which resolved back to the **v0.22.3** module - so the "patched" alias was linking vulnerable `wire`/`txscript` underneath it. With the downgrade gone the alias has no purpose, and the two `pkg/bitcoin/electrum` files using it return to plain paths. ## First-party fallout - `txscript.NewTxSigHashes` takes a `PrevOutputFetcher` since v0.23 and (as of v0.24.2) requires the previous outputs of all inputs. `TransactionBuilder` now registers each input's locking script and value in a `MultiPrevOutFetcher` as inputs are added - it already fetched exactly that data for its sighash args. `ComputeSignatureHashes` also asserts the set is complete and returns an error, rather than letting a future gap reach the panicking path. - `mempool.GetTxVirtualSize` / `btcutil.Hash160` / `btcutil.NewTx` move from the archived `btcsuite/btcutil` to `btcd/btcutil` (same semantics). ### Deserialization cap and recover - required by the version jump btcd v0.24.2 rewrote `MsgTx.btcDecode` to slice every script of a transaction out of **one fixed-size 4 MiB buffer** (`scriptSlabSize = 1 << 22`), advancing it after each read. `readScriptBuf` bounds each individual script against `maxWitnessItemSize` but never against the *remaining* buffer, so once the cumulative script length of a single transaction passes 4 MiB the next slice **panics** instead of returning an error: ``` panic: runtime error: slice bounds out of range [:900000] with capacity 594304 btcd@v0.24.2/wire/msgtx.go:1041 readScriptBuf ... pkg/bitcoin/transaction.go (*Transaction).Deserialize ``` Reproduced directly: 4 × 900 KB output scripts decode cleanly, 5 panic. Both decoders this PR replaces (v0.22.3 and the v0.23.4 alias) allocate per script and handle the same bytes fine, so this would be a **regression introduced by the upgrade** - on a path fed by the untrusted Electrum backend. `decodeTransaction` runs *before* `GetTransaction`'s txid check, `GetTransactionConfirmations` performs no hash check at all, and no `recover()` covers the tbtc signing paths, so it would terminate the node. Upstream fixed it only in the split-out `wire/v2` module, part of the v0.26 layout the plain-path forks make unreachable. The fix is two layers, and **only the combination closes the panic**: a `MaxTransactionByteLength = 4_000_000` cap applied in `bitcoin.Transaction.Deserialize` and `electrum.decodeTransaction` (a transaction's weight is `3*base_size + total_size` and cannot exceed the 4,000,000 WU maximum block weight, so a consensus-valid transaction is always strictly shorter than the cap - nothing legitimate is rejected), **plus** a `recover()` that turns the slice-bounds panic that can still fire below the cap into a plain error. Two regression tests (`TestTransaction_DeserializeDeclaredNotDelivered` and `TestDecodeTransactionDeclaredNotDelivered`, both added in `626538d2`) construct a 194,378-byte (189.8 KiB, 4.86% of the cap) transaction that panics in unpatched btcd and confirm the `recover()` returns the error. The cap alone is *not* sufficient - without the `recover()`, payloads of that size terminate the process. The cap bounds grossly oversized input; the `recover()` is what actually turns the under-cap panic into an error. A subsequent commit also pre-checks `len(rawTx)/2` against `MaxTransactionByteLength` in `electrum.decodeTransaction` *before* `hex.DecodeString` allocates, so a malicious multi-GB hex string can't pay the full allocation before the guard runs. Note: the `recover()` covers slice-bounds panics from `readScriptBuf` only. It does **not** cover fatal runtime errors such as out-of-memory from attacker-declared varint counts that btcd's wire decoder honors before reading backing bytes. Closing that class of input requires either a pre-decode budget check on declared input/output counts or a future btcd release; both are out of scope here. The v0.22.3-to-v0.24.2 jump also changes btcd's script-decoding internals beyond the slab panic. Two material changes worth knowing for future triage: - The shared script free-list ceiling rises from 6.4 MB globally (12,500 × 512 B entries) to roughly 524 MB globally (125 × 4 MiB slabs) - a ~82× increase, not ~8000× (the earlier draft conflated bytes-per-entry with bytes-per-slab). Per-decode worst case *becomes* one 4 MiB slab (v0.22.3 allocated per script); only the global retention grows. - The per-script cap tightens from 32 MiB (`MaxMessagePayload`) to 4,000,000 (`maxWitnessItemSize`) - strictly safer, no legitimate transaction is affected. Neither is a security issue; both are documented in the corresponding `MaxTransactionByteLength` doc comment and in `VENDOR.md`. ### Docker `third_party/` is copied before `RUN go mod download`. The directory `replace` target must exist for the module graph to resolve, so the image build would otherwise fail at that step even though local builds pass. ## Verification - `go build ./...`, `gofmt -l .` clean; full `go test ./...` green. - `CGO_ENABLED=0 go build` for `darwin/amd64`, `darwin/arm64`, `linux/amd64`, `linux/arm64`, `windows/amd64` - the cgo-less cross-compile path `make release` uses, which selects go-ethereum's `signature_nocgo.go` and is where the `btcec/v2` incompatibility above shows up. - `go vet ./...` reports only the pre-existing `pkg/tecdsa/signing/protocol.go` copylock warning, byte-identical on unmodified `main`. - `TestTransactionBuilder_Signing` byte-vector fixtures pass: DER signatures, sighashes, and compressed public keys are **byte-identical** across the version jump. The signing path still uses v1 btcec (now vendored, byte-identical), so its implicit BIP-62 low-S normalization is preserved. - Vendored-copy identity verified via the `VENDOR.md` recipe: only the four added files (`go.mod`, `go.sum`, `LICENSE`, `VENDOR.md`) differ from gofmt'd upstream `btcec@v0.22.3`. - `docker build --target build-sources` passes, including the `go mod download` step that fails without the `third_party` COPY. - Panic reproduced before the fix and confirmed to return a clean error after. ## Known residuals - The archived `github.com/btcsuite/btcutil` (2019) is still linked, because the tss-lib fork's `crypto/ckd` imports its `base58` package - the same unchangeable-import-string constraint that motivates the vendoring. One pure codec package, no advisories against it; first-party code no longer touches it. MVS now selects an ~20-months-older `btcsuite/btcutil` pseudo-version than `main` did (the archived `v1.0.3-0.20201208143702` direct require is gone, replaced by the tss-lib fork's `v0.0.0-20190425235716-9e5f4b9a998d`); the only compiled package (`base58`) is functionally identical between the two commits, so this is benign. - `go.sum` keeps `/go.mod`-only hash lines for two old btcd pseudo-versions reached through transitive requires that MVS overrides. No `h1:` hashes, so no source is compiled; the vulnerable `v0.22.3`/`v0.23.4` full hashes are gone. Naive graph-level SCA tools may still surface them. - The vendored `btcec` sits at a module path OSV doesn't index, so a hypothetical *future* advisory against pre-split btcec code wouldn't be flagged automatically. None of the current three are in `btcec`. The vendored dir is also invisible to root-module `go test ./...` and has no automated advisory monitor; byte-identity relies on a human running the `VENDOR.md` recipe. - The versionless directory `replace` means external modules importing keep-core packages can't resolve the btcec requirement. Not a new blast radius: `main` already carries the same unresolvable-with-replace pattern for `google.golang.org/protobuf/dev`, which downstream consumers cannot resolve either. The btcec pseudo-version is new on this branch, but the underlying pattern is pre-existing. ## Test plan - [x] `go build ./...` / `go vet ./...` / `gofmt -l .` - [x] `go test ./...` (full suite) - [x] Vendored btcec upstream test suite, from the main module and standalone - [x] `VENDOR.md` byte-identity verification recipe - [x] `docker build --target build-sources` - [ ] CI green on this branch ## Note for #4109 The security-release integration branch carries the same downgrade. Once this lands, its next rebase picks the fix up; the `go.mod` resolution there is mechanical (that branch has a much newer dependency set, so the conflict is in surrounding lines rather than the btcd ones). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Bitcoin transaction decoding now rejects oversized data and returns errors instead of crashing on malformed input. * Signature-hash generation now reports missing previous transaction outputs. * Updated Bitcoin cryptography support improves compatibility and dependency resolution. * **Security** * Added secp256k1 key, signature, encryption, and decryption capabilities with validation safeguards. * **Tests** * Expanded coverage for transaction handling, cryptographic operations, key parsing, signatures, and error scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Collaborator
|
Checking in on this security release candidate — it conflicts with |
lionakhnazarov
added a commit
to lionakhnazarov/keep-core
that referenced
this pull request
Aug 10, 2026
Bring PR threshold-network#4109 current with threshold-network/keep-core main. Keep cutover participation permits and fold in main's stuck-transaction monitor plus the btcd/btcec third_party replace. Drop main's node.go file splits that duplicated the candidate's monolithic node (handlers stay permit-aware). Co-authored-by: Cursor <cursoragent@cursor.com>
lionakhnazarov
force-pushed
the
security-release/candidate-1
branch
from
August 10, 2026 13:51
6713089 to
2d858b7
Compare
lionakhnazarov
pushed a commit
to lionakhnazarov/keep-core
that referenced
this pull request
Aug 10, 2026
…ee RPCs, monitoring wiring (vet round 2) Addresses the round-2 vet report (findings 1, 3, 6 + three net-new). P0 — production discovery/identity is now authoritative, not optional (finding 1): - Completeness now REQUIRES service-discovery reconciliation and on-chain identity verification (CollectorConfig.RequireServiceDiscovery / RequireIdentityVerification, set true by the command). A missing feed blocks readiness instead of degrading to trusting inventory; the command warns that readiness cannot be certified. (isComplete, main.go) - Service discovery is keyed by network ID (per-instance) not operator-only, so multiple instances of one operator resolve to distinct discovered targets (ServiceDiscovery.byNetworkID, MetricsURLForInstance, InventoryInstance.NetworkID, applyDiscoveredTargets). - The metrics adapter validates the responding node's self-attested identity (diagnostics chain_address + network_id) instead of copying InstanceID/ OperatorAddress from inventory; a mismatch is rejected (reportadapter.go). P0 — reporter revision is durable across collector restart (net-new 1): - ReporterRevision now derives from the attestation timestamp (UnixNano) rather than a process-local counter that reset on restart and sat below the persisted high-water mark. Added TestCollector_ReportsAcceptedImmediatelyAfterRestart and TestMetricsReportSource_ReporterRevisionSurvivesRestart. P1 — remaining evidence gaps (finding 3): - StakingProvider must be a canonical, non-zero address; expected image digest must be sha256:<64 hex> (test fixture corrected to a full digest); cross-instance staking-provider contradictions are fail-closed (last claim no longer wins). P1 — no collector lock across network calls (net-new 2): - Identity verification runs BEFORE the central-state lock (verify first, apply under lock) so a degraded RPC never blocks readers; the RPC timeout derives from the passed-in context (CollectContext(ctx), IdentityVerifier now takes ctx, ethCall uses context.WithTimeout(ctx, ...)). P1 — monitoring deployment wiring (finding 6): - A non-loopback API bind without an allowlist is refused at startup. - cutover-readiness.json added to the Grafana ConfigMap; instance-reason text panel replaced with a real table sourced from the readiness API via a new Infinity datasource (+ plugin install). - Added a cutover-roster Deployment/Service/PVC and a Prometheus scrape job, plus an Alertmanager workload + routing tree matching the alerts' route_to label. (Image digest, allowlist CIDR, inventory ConfigMap/Secret, and receiver integrations are REPLACE_ placeholders — ops follow-up, documented in README.adoc.) P1 — 30-day resolved purge (net-new 3): resolution of the semantic conflict. - The fail-closed reopening of a vanished resolved operator (finding 2, RESOLVED) and the naive 30-day purge of a departed resolved operator are in genuine, irreconcilable tension for the "resolved operator departs" population: a departed operator MUST reopen offline_unknown and be retained indefinitely, so it can never age out via the resolved path, and an actively-resolved operator is continuously re-confirmed (ResolvedAt refreshed) and must not be dropped. purge therefore remains a bounded-store backstop for a resolved record no longer being re-confirmed. Restored independent coverage as a white-box test of the purge mechanism (TestCollector_PurgeResolvedAfter30Days): a resolved record older than the retention window is purged with its instances, while a fresh resolved record and any blocking record are retained. P1/P2 — harness: README/compose/script examples now use an immutable @sha256: digest instead of the mutable keep-client:candidate tag the harness itself rejects. P2 — draft PR body: corrected the inaccurate "wire-compatible / no session-ID/KDF change" claim to scope it to THIS diff only and explicitly note PR threshold-network#4109's Part A cryptographic changes are wire-breaking and not described here. Live PR/issue untouched (out of band). Local CI oracle (format, vet, staticcheck SA*, golangci-lint, gosec, go test ./..., race tier-2 subset, integration, race over new packages): PASS.
lionakhnazarov
added a commit
to lionakhnazarov/keep-core
that referenced
this pull request
Aug 10, 2026
Bring PR threshold-network#4109 current with threshold-network/keep-core main. Keep cutover participation permits and fold in main's stuck-transaction monitor plus the btcd/btcec third_party replace. Drop main's node.go file splits that duplicated the candidate's monolithic node (handlers stay permit-aware).
Bring PR threshold-network#4109 current with threshold-network/keep-core main. Keep cutover participation permits and fold in main's stuck-transaction monitor plus the btcd/btcec third_party replace. Drop main's node.go file splits that duplicated the candidate's monolithic node (handlers stay permit-aware).
lionakhnazarov
force-pushed
the
security-release/candidate-1
branch
from
August 10, 2026 14:07
2d858b7 to
da9de1b
Compare
Collaborator
Author
@piotr-roslaniec Thanks for the ping. I’ve synced security-release/candidate-1 with current main (merge commit on the PR tip) so the conflict set should be cleared for re-review. I’m aiming to keep this as a single PR and finish remaining review/CI follow-ups. I’d prefer not to split unless review becomes blocked on size/scope. The cutover/participation changes are tightly coupled; splitting would likely create awkward intermediate states |
Update GetTransactionConfirmations stub signature after main merge so cmd tests compile under lint/golangci/CI.
Treat IsDKGResultValid errors (e.g. high-s OpenZeppelin recover reverts) as invalid results and enter the challenge loop, matching on-chain challengeDkgResult's validation-reverted path.
…ionLayer The merge that ported this branch's monolithic node.go from main's split node files (da9de1b) carried over the transaction monitor's construction, metrics wiring, and action wiring, but dropped the five lines that actually started its polling goroutine. The monitor has been registering broadcast transactions since that merge without ever checking them: no stuck-warning, no 24h eviction, and the stuck/unmonitored counters read a healthy zero whether the loop is alive or not. Restore the single nil-guarded launch at the top of runCoordinationLayer, bound to its own context so it starts asynchronously and stops with the node. Add a private check-interval seam so tests don't wait out the production five-minute tick, and three liveness/progress metrics (running, check cycles, tracked count) as positive evidence an operator can gate on, since the existing counters can't distinguish an inert loop from a healthy one.
…oordinationLayer" This reverts commit 591b879.
Member
|
Transparency note for @lionakhnazarov: I accidentally pushed 591b879 directly to security-release/candidate-1 while setting up to open a PR for the transaction-monitor lifecycle-launch fix (a local clone had push.default=upstream, which redirected the push to the tracked ref instead of creating a new branch). I reverted it immediately with a forward commit (d2a95ee) that restores the branch tree to its exact prior state without rewriting shared history, then reopened the same verified patch properly at #15 from my fork. Sorry for the disruption. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Integration candidate for the coordinated security release (
sr-candidate-1).Merges:
tlabs-xyz/keep-core-security@epic-consolidation(ac63328) — private security epic (#41)main@4137b34be(testnet4 stack, chore(ci): align Yarn 4 workflows and solidity workspace tooling #3999–feat(go): add testnet4 mapping and harden runtime integration behavior #4002, fix(libp2p): bound Keep authentication handshake with SetDeadline #4008, Remove bootstrap nodes and replace with operator peers #3909, …)getProofInfomirrors Bridge difficulty logic (d07eb41)dd2c873)Head:
971f60358(rebased ontomain@038b7ced1; geth/Go CVE bumps restored)tss-lib pin:
github.com/threshold-network/tss-lib@v0.0.0-20260615180949-86bd1a375cc0(86bd1a3)Deps:
go-ethereum v1.17.3,keep-common v1.7.1-tlabs.1,go 1.25.7/ toolchaingo1.25.10Sepolia smoke on
security-candidate-2@cbf31e0e8(pre-geth-restore tip of refresh stack): PASS (SR-1, SR-2 groupSize=100, SR-4 live SPV proof, SR-5 rollback). Local short regression after geth restore: PASS. Details below.All operators in a DKG or signing ceremony must run the same build. Mixed old/new nodes will fail TSS rounds (session IDs, proof transcripts, wire formats differ).
Do not roll out node-by-node during an in-flight ceremony. Upgrade when idle; rollback only when idle.
Breaking changes (operator-visible)
G1HashToPoint,PrepareForSigningAPI changesconnected_wellknown_peers_count[clientinfo] Portexplicitly if scraping/metricsconnected_bootstrap_count→connected_wellknown_peers_count--network.bootstrap=truedeprecatedFull table: see epic
CHANGELOG.mdandSECURITY-BREAKING-CHANGES.mdonkeep-core-security.Conflict resolution (vs
main+ epic)applyHandshakeDeadline/ SetDeadline)getProofInfofix (testnet4 minimum-difficulty blocks)solidity-v1,dashboard,token-stakedrop86bd1a3viago.modreplaceDependencies
Local regression (pre-merge)
Sepolia validation (
security-candidate-1@dd2c87360)Droplet:
162.243.170.240· Image:keep-client:security-candidate-1[clientinfo]enabledWalletCreated@ Sepolia block 11130388; wallet PKH0xa8abdbaa…123c75b8…) →submitDepositSweepProof0x0d5de901…@ block 11136453 (status=1)Baseline replay: prior image
sepolia-dev-forklev-btcdiff-idle-flagvs candidate — no mesh/metrics/rollback regression; candidate proves new crypto + live SPV.Docker image
docker build --platform linux/amd64 -f Dockerfile --target runtime-docker \ --build-arg ENVIRONMENT=sepolia \ --build-arg VERSION=security-candidate-1 \ --build-arg REVISION=dd2c87360 \ -t keep-client:security-candidate-1 .Important: default Dockerfile final stage (
output-bins) is not runnable — use--target runtime-docker.Operator upgrade checklist
connected_wellknown_peers_count).LightRelayMaintainerProxy.authorize+authorizeSpvMaintainerafter fresh deploy; fund maintainer gas for btcdiff catch-up.Rollback: revert image tag when chain/ceremony is idle only.
Review focus
keep-core-security#41CHANGELOG86bd1a3matches intended releasegetProofInfoDIFF1 logic matches BridgeRelated
86bd1a3Test plan
go teston changed packages[Unreleased]vs epic