feat(backend): Stellar key custody & transaction signing service with KMS abstraction, sequence management & fee-bump retries - #48
Conversation
Adds server-side signing and submission of Stellar/Soroban transactions. Callers hand over the operations to execute and nothing else — no source account, no sequence number, no fee, no key — and the service sees the transaction through to a terminal state. Custody sits behind a SigningProvider interface with no method that can return a key. LocalSigningProvider parses configured seeds once at startup and holds only KeyPairs; KmsSigningProvider delegates to an external gateway so the secret key never enters this process, and verifies every returned signature against the reference's known public key before use. Selection is by profile property, so swapping one for the other is configuration rather than code. SecretRedactor backstops every string that reaches a log or the last_error column, and the request DTOs reject anything shaped like a secret seed outright. Sequence numbers come from a leased pool of channel accounts claimed with SELECT ... FOR UPDATE SKIP LOCKED: N concurrent submissions get N distinct accounts and N distinct sequence numbers with no coordination beyond the database. A lease is only returned as AVAILABLE when the transaction actually reached a ledger; anything that never landed forces a resync against the chain, which is what stops one lost transaction from poisoning an account permanently. Submission runs as three claim-one-row workers. The signed envelope is durable before it is broadcast, never after, and the broadcast phase begins by asking the network about the hash it holds — so a restart resumes rather than double-submits. Failures are classified from the decoded TransactionResult and each gets the recovery it needs: tx_bad_seq resyncs and rebuilds, tx_insufficient_fee fee-bumps, tx_too_late rebuilds with fresh time bounds, and a failed simulation is terminal before anything is signed. Fee bumps double the fee until the ceiling, which turns a stall into a bounded state instead of unbounded spend. Adds network.lightsail:stellar-sdk, the maintained successor to the archived official Java SDK. This reverses decision 1 of ESCROW_ORCHESTRATION.md, which found no such SDK on Maven Central; every acceptance criterion here needs real XDR, and hand-rolling it was what that decision rightly refused to do. SorobanRpcClient still treats envelopes and results as opaque strings everywhere except getAccountSequence, where a ledger entry can only be addressed and read as XDR. Tests cover the concurrency criterion against real PostgreSQL (8 threads released by a barrier, 8 distinct sequence numbers), the full pipeline phase by phase, the KMS gateway contract over MockWebServer, the HTTP/problem+json contract and ADMIN gating, and the key-material guarantees. CI now runs ./mvnw -B verify with the existing Maven cache. Closes workman-labs#40
Overall recommendation
Critical, diff-anchored review (must be fixed before merge)
Other suggested improvements (actionable, diff-anchored)
Final words
|
…rics, runbook Review follow-ups on workman-labs#48. Security - Providers refuse to sign anything that is not a 32-byte transaction hash. An Ed25519 key that signs whatever it is handed is a signing oracle; the KMS provider checks before resolving the key, so an oversized payload never produces a request either. - KmsSigningProvider refuses to start against a plaintext or unauthenticated gateway (loopback excepted). In cleartext the bearer credential is readable and a response is modifiable — a substituted response is a substituted signature. - SigningProviderShapeTest pins the interface's method set and checks both implementations for public members that return key material or are merely named as though they might, for public instance fields, and for a missing toString() override. - Log-capture tests drive the whole custody lifecycle, including the failure paths, with a capturing appender on the root logger at TRACE, and assert the seed and API key appear nowhere. - Envelope/result columns are @JsonIgnore'd and both entities have hand-written toString()s rendering identifiers only. - 1,000 generated seeds against the DTO seed guard, plus 1,000 randomised legitimate aliases against the mirror-image risk of a guard too broad to use. Correctness - XDR columns are `text` rather than @lob. Hibernate maps @lob String onto a PostgreSQL oid; large objects are not removed when their row is deleted, so this table would leak one per submission, unbounded and unreclaimable by VACUUM — and reading one outside a transaction fails at runtime. - Retry-vs-terminal decisions move into FailureClassifier#recoveryFor, an exhaustive switch with no default arm, so a failure reason added later is a compile error rather than a silent inheritance of "retry it". - SigningProperties validates at startup. A fee ceiling below the base fee, or a bump multiplier of 1.0, now fails the boot rather than every transaction. Operability - stellar.signing.enabled pauses the workers without a deploy; submissions still queue and resume from their durable row state. - Micrometer counters for submissions, phase attempts, fee bumps, terminal states and lease events, at /actuator/prometheus behind authentication. Tag values are a closed set, asserted by test. - NO_CHANNEL_ACCOUNT drops to DEBUG (it is backpressure, not a fault); lease claims and releases log their status transition and sequence. Docs and tests - Mermaid sequence diagram of the pipeline marking the durability boundary, plus two tests pinning that the envelope is committed before anything is broadcast and that a crashed process asks before it resends. - Operator runbook: stuck leases, FEE_CEILING_REACHED, KMS outage, and a table of levers. - Lease recovery covered end to end: crash, sweep, resync, next allocation — and the negative, that a consumed release costs no round trip. 347 tests, 0 failures.
|
Thanks — this was a genuinely useful review. All of it is addressed in b5f4158. The suite is now 347 tests, 0 failures (was 285); Two of your items turned up real defects rather than just missing tests, so I'll lead with those. Two things this review actually caught1. The XDR columns were leaking PostgreSQL large objects. Your entity item made me look at how
2. Nothing constrained what a provider would sign. Your "KMS provider only accepts the 32-byte hash" item was framed as a test, but the check didn't exist — an Ed25519 key that signs whatever it is handed is a signing oracle. Both providers now enforce it, and the KMS one checks before resolving the key so an oversized payload never produces a request. It's an Critical items
Log-appender assertion that the seed is never logged — done, and broader than asked. The capture is on the root logger at
Durable commit before broadcast — confirm, comment, test — confirmed, and it holds structurally rather than by convention: phase 1 (
Operator runbook — new section covering exactly your three: (a) a lease that won't clear, (b)
On double-submits specifically: Conservative, documented, validated fee defaults — Controller responses / RFC 7807 — confirmed, already covered: Other suggested improvementsKMS signature verified locally — already there and now called out: Micrometer + Prometheus — added:
Tests assert the increments. Pool utilisation and cumulative fee spend want Lease DEBUG logging + SKIP LOCKED concurrency test — claims and releases now log the account, its status transition and its sequence, which is what you want when reconstructing a Index for polling queries — already present: Sequence diagram — added (mermaid), covering client → persist → sign → broadcast → poll → terminal, with the durability boundary highlighted and phase transaction scopes marked. Minor / stylisticLog levels — Deterministic mocking, no sleeps — already the case: the workers are driven by hand, timestamps are backdated rather than waited on, and the schedulers are pinned to 1-hour delays in surefire. The concurrency test's assertion is fully deterministic (distinct accounts, distinct numbers, each equal to that account's on-chain sequence + 1). Feature flag / rollback toggle — added as Security checklist
One thing worth flagging for your judgement: actuator is an app-wide dependency, not scoped to this feature. I've kept the surface minimal and authenticated, but if you'd rather metrics landed in their own PR I'm happy to pull them back out — say the word and I'll split it. |
meshackyaro
left a comment
meshackyaro
left a comment
There was a problem hiding this comment.
Excellent work on this signing service implementation
This is a textbook example of production-grade backend infrastructure. Every piece — security, durability, operability — is thoughtfully designed and rigorously tested.
What stands out:
-
Security by construction: The signing providers refuse arbitrary payloads; KMS startup checks enforce HTTPS and authentication; log-capture tests prove secrets never escape. You've eliminated whole categories of mistakes rather than just hoping they won't happen.
-
Durability principles: The three-phase pipeline with a hard durability boundary (envelope committed before broadcast) is the right answer for "what if we crash mid-submission." The tests prove it: a process that dies is asked about the hash before resending, not the other way around. That's the difference between idempotent recovery and double-submission.
-
Failure taxonomy:
RecoveryActionwith an exhaustive switch and no default arm is brilliant. A new failure reason added later is a compile error, not a silent "retry it" that could wreck a transaction already on the network. This is the kind of invariant that saves operators at 2 AM. -
Operational readiness: The pause lever, startup validation, metrics with bounded cardinality, and the operator runbook show you've thought through how this gets deployed and debugged. Micrometer counters on the right signals (fee bumps, lease reclaims, terminal failures by reason) mean operators see problems in near real-time, not after the fact.
-
Test discipline: 347 tests, including the hard ones — concurrency against real PostgreSQL, restart safety with durable row state, log capture at TRACE to prove secrets don't leak by any route, 1,000 generated seeds fuzzed against the validation guard. The test suite is a specification for the safety guarantees.
The follow-up commit taking feedback on security, metrics, and runbook documentation shows the maturity of your iteration. This is ready.
Great job. Approved.
…nced with development for workman-labs#48) into local CI fix # Conflicts: # backend-api/pom.xml
2 participants
Closes #40
Server-side signing and submission of Stellar/Soroban transactions. Callers hand over the operations to execute and nothing else — no source account, no sequence number, no fee, and no key — and the service sees the transaction through to a terminal state.
Design notes:
backend-api/docs/STELLAR_SIGNING.md.Acceptance criteria
SigningProviderabstraction, local + externalized-KMS impls selected by profilesigning/custody/—@ConditionalOnPropertyonstellar.signing.providerChannelAccountLeaseService,SELECT … FOR UPDATE SKIP LOCKEDTransactionSubmissionService.pollBroadcast()feeBump(),stellar.signing.fee.max-total-stroopstx_bad_seq,tx_insufficient_fee,tx_too_late, simulation failuresFailureClassifier+handleSendFailurestellar_transaction_submissions; envelope durable before broadcastsigning/model/,signing/repository/ChannelAccountLeaseIntegrationTestcache: maven); CI now runs./mvnw -B verifyGlobalExceptionHandler./mvnw clean verify→ 285 tests, 0 failures.The three properties worth reviewing closely
Keys.
SigningProviderexposesproviderId/supports/publicKey/signand nothing else — there is nosecretSeed()to accidentally call, andLocalSigningProviderTestasserts that method set by reflection so adding one later fails the build. Withprovider=kmsthe key never enters this process: only the 32-byte transaction hash crosses the wire, and every returned signature is verified against the reference's known public key, so a gateway signing with the wrong key fails immediately rather than as an opaquetxBAD_AUTHthat has already cost a sequence number. Accounts are registered by reference, never by key material — and since a Stellar seed is 56 characters of letters and digits, exactly the shape of a plausible alias, the DTOs carry an explicit pattern rejecting seed-shaped input rather than assuming it won't arrive.Sequence numbers. Stellar validates a sequence number as exactly source + 1, so handing out
nandn+1on one account parallelises nothing — the network takes one and rejects the other. Concurrency has to come from more accounts, which is what the pool is;SKIP LOCKEDmakes claiming "any free account" contention-free. The subtle half is release: a number allocated to a transaction that never landed is skipped, and every later transaction from that account would then fail. So confirmation and on-chain failure both release toAVAILABLE(both spend the number), while anything that never landed releases toNEEDS_RESYNCand the next lease re-reads the chain first.Termination. Every path is bounded: fee bumps stop at the ceiling (terminal
FEE_CEILING_REACHED), a transaction past its time bounds is rebuilt rather than polled forever, rebuilds and retries are bounded bymax-attemptsintoDEAD_LETTER, and a crashed holder's lease is swept — but conservatively, extending rather than reclaiming any lease whose submission is still in flight, since reusing its sequence number would race a transaction that may be in the mempool right now.Reversal worth flagging
This adds
network.lightsail:stellar-sdk:3.1.0, reversing decision 1 ofESCROW_ORCHESTRATION.md(#21), which found no Java Stellar SDK on Maven Central and therefore relayed pre-signed envelopes as opaque strings. That was right about the official SDK — SDF archivedjava-stellar-sdk— but this is its maintained successor. Setting a sequence number, fee-bumping, and tellingtxBAD_SEQfromtxINSUFFICIENT_FEEall require real XDR, and hand-rolling that encoding is precisely what decision 1 refused to do.SorobanRpcClientstill treats envelopes and results as opaque everywhere exceptgetAccountSequence, where a ledger entry can only be addressed and read as XDR. Rationale in full: New dependencies.Notes for reviewers
ddl-auto=updatewith entities as the source of truth, same as every existing table. Both new tables are new, so first deploy creates them and every deploy after is a no-op.GlobalExceptionHandlerand render asapplication/problem+json.SigningProviderExceptiondeliberately answers with a fixed detail string so nothing from the signing path can reach a client body./api/v1/escrow/orchestrationsendpoints are untouched. Migrating them onto this service would change a public contract and belongs in its own PR.