Skip to content

swaps: authorize credit account requests - #1103

Merged
bhandras merged 3 commits into
mainfrom
agent/credit-request-auth
Aug 11, 2026
Merged

swaps: authorize credit account requests#1103
bhandras merged 3 commits into
mainfrom
agent/credit-request-auth

Conversation

@bhandras

@bhandras bhandras commented Aug 5, 2026

Copy link
Copy Markdown
Member

What it does

  • defines a canonical, method-bound proof for account-scoped credit requests
  • signs short-lived request digests with the daemon identity key
  • passes the account key named by each request into the signer and rejects any key that does not match the wallet identity
  • attaches a fresh nonce and signature in both gRPC and REST swap transports
  • rejects missing local signing support before a protected request leaves the client

Protocol

The signature commits to the exact RPC method, deterministic request bytes, account public key, expiry, and a 32-byte random nonce. The authorization field is excluded from its own request digest. Proofs expire after one minute by default and signers reject expiries beyond five minutes.

The signer receives the account key extracted from the same canonical payload it signs. The daemon refuses to sign unless that key matches its identity key, preventing a caller from asking one wallet to authorize another wallet's account.

Compatibility

Ordinary Ark-to-Lightning quote and create requests remain unsigned when no credit account is supplied. Lightning-to-Ark route requests are account-scoped because the route may inspect or reserve receive credit, so receive callers must use a connection configured with an identity signer. The exported constructors document this distinction.

Testing

  • go test ./swaprpc ./sdk/swaps ./sdk/ark ./waved
  • go test -tags=swapruntime ./swapclientserver
  • make fmt-changed-check
  • make lint-changed-local workers=4
  • make commitmsg-lint range=origin/main..HEAD

The changed-file lint pass runs all 71 configured linters and reports zero issues.

@bhandras
bhandras marked this pull request as ready for review August 5, 2026 14:29
@bhandras
bhandras force-pushed the agent/credit-request-auth branch from de951f7 to 5c7994b Compare August 5, 2026 14:49
@bhandras

bhandras commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

@claude please review this PR.

Please focus on the protocol and client-side authorization boundary:

  • the canonical digest must bind the exact RPC method and all request fields except the authorization itself
  • account identity, expiry, and nonce must be included in the signed BIP-340 tagged digest
  • gRPC and REST paths must attach fresh proofs for every protected request
  • ordinary in-swap quote/create calls without a credit account must remain compatible
  • daemon signing permissions and error handling must fail closed

The downstream verifier persists nonces before handler execution, so please also flag any client behavior that could accidentally reuse a nonce or sign a different payload from the one transmitted.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 6m 43s —— View job


Review: credit account request authorization (PR #1103)

I traced the full client-side boundary: swaprpc/credit_account_auth.go (digest), sdk/swaps/credit_account_auth.go + grpc_conn.go (attach), waved/rpc_wallet.go + server.go (sign), and the call sites in sdk/swaps/{credits,in_swap}.go. Overall the design is sound and fails closed. Two protocol/compatibility items are worth resolving before merge, plus one minor robustness nit.

What holds up ✅

  • Method + all fields bound, auth excluded. CreditAccountRequestDigest prefixes the exact full method string + 0x00 separator, then a deterministic marshal of the request with AccountAuthorization cleared. TestCreditAccountRequestDigestCommitsToRequest proves the authorized digest equals the unsigned digest and that mutating any field changes it. Good.
  • Expiry + nonce in the tagged digest. CreditAccountAuthMessage = accountKey || requestDigest || uint64(expiry) || nonce, hashed under CreditAccountAuthTag (BIP-340 tagged, via signTaggedSchnorr). ✅
  • No payload-mismatch / no nonce reuse. In every grpc_conn.go path the sequence is authorizeCreditAccountRequest(ctx, req) → immediately g.client.X(ctx, req) with no field mutation in between, and the digest is computed from the same req before the auth is attached. Each call draws a fresh nonce from authRand and a fresh expiry, so an SDK/actor-level retry re-signs with a new nonce rather than replaying one. I found no gRPC-layer retry service config that would transparently resend the same nonce. ✅
  • Fail-closed signing. Nil signer → error; nil signature → error; validateCreditAccountAuthExpiry rejects past/>5min expiries on both the gRPC (SignCreditAccountAuthorization) and in-process (SignCreditAccountAuth) paths; requireWalletReady + nil identity-key guards. The daemon-internal signer is an in-process call sharing time.Now, so there is no client/daemon clock skew on the expiry check. ✅
  • Permission added. SignCreditAccountAuthorization granted under entitySwap:write in rpc_auth.go. ✅
  • Plain pay/quote compatibility. CreateInSwap/QuoteInSwap only authorize when len(accountPubKey) > 0, so ordinary in-swaps stay unsigned. ✅

Findings

1. RequestChannelID requires a signer unconditionally — receive path is not compatibility-guarded. [sdk/swaps/grpc_conn.go:102]
Unlike pay/quote (guarded by len(accountPubKey) > 0), RequestChannelID calls authorizeCreditAccountRequest on every receive, so it errors with credit account authorization signer is required when the conn has no signer. That means NewGRPCSwapServerConn(conn) (no variadic signer) and NewRESTSwapServerConn(addr) (never sets a signer) can no longer perform any receive, including plain non-credit ones. Production always wires a signer via newSwapServerClients, so this isn't hit there — but it contradicts the PR's stated "ordinary … requests remain unsigned" for the receive rail, and leaves two exported constructors silently broken. If unconditional signing is intended (server binds the identity for every credit-eligible receive), please either drop/guard the unauthenticated constructors or document that receive now mandates a signer. Fix this →

2. The signature always binds the daemon identity key, but the digest helper advertises a different per-request account key — an implicit, unchecked coupling. [swaprpc/credit_account_auth.go:160, waved/rpc_wallet.go:223]
SignCreditAccountAuth hardcodes r.server.clientKeyDesc.PubKey as the account key in the signed message. Meanwhile unsignedCreditAccountRequest returns the request's own account field as "the account identity committed by the signature" — account_pubkey for the credit calls, but client_vhtlc_pubkey for RequestChannelId. That returned key is then discarded by authorizeCreditAccountRequest (digest, _, err := ...), so nothing verifies that the request's declared account equals the signing identity.

This works today only because every call site sets account_pubkey == IdentityPubKey (credits.go:90, in_swap.go:576). But:

  • For RequestChannelId the digest helper claims the bound identity is the ephemeral vHTLC pubkey, which is not what gets signed (the identity key is). In production these keys differ; the REST test masks this by passing clientPriv.PubKey() as both the vHTLC pubkey and the signing key (rest_conn_test.go:78-88). A downstream verifier that reconstructs the RequestChannelId auth message from client_vhtlc_pubkey (the natural reading of this helper) would fail BIP-340 verification; it must instead source the identity key out-of-band (mTLS/mailbox transport identity), since the request carries no account_pubkey. Please confirm the server verifier does exactly that.
  • Recommend making the coupling explicit and fail-closed: pass the accountKey returned by CreditAccountRequestDigest into the signer and assert it equals the signing identity, and fix the RequestChannelId case (or the doc) so "account identity committed by the signature" is accurate. Fix this →

3. (minor) Shallow struct copy of a proto message. [swaprpc/credit_account_auth.go:165]
cloned := *typed copies a generated message by value (including the embedded protoimpl.MessageState, which is documented do-not-copy and can trip go vet copylocks). Prefer proto.Clone(typed) with a type assertion, matching how the tests already build modified messages. Fix this →

Server-side dependency to confirm

The downstream verifier must replicate the digest construction exactly: full method string + 0x00 separator + deterministic marshal + CreditAccountRequestTag, and for RequestChannelId it must verify against the daemon identity key (from transport), not client_vhtlc_pubkey. Since it persists nonces before handler execution, the client's fresh-nonce-per-call behavior is correct and won't cause self-inflicted replay rejections.


Note: I could not run go vet/gopls in this environment (command approval), so finding #3's copylocks claim is reasoned, not executed.
· branch agent/credit-request-auth

@bhandras
bhandras force-pushed the agent/credit-request-auth branch from 5c7994b to ca66bcd Compare August 5, 2026 15:05
Bind account-scoped credit requests to the wallet identity key.

Use a short-lived nonce and a deterministic request digest.
Expose the daemon identity signer without exporting key material.

Reject stale or overly broad authorization envelopes before signing.
Attach a fresh account proof before every credit-sensitive request.

Share the daemon signer across gRPC and REST swap transports.
@bhandras
bhandras force-pushed the agent/credit-request-auth branch from ca66bcd to 3eed78b Compare August 5, 2026 15:17
@bhandras
bhandras requested a review from sputn1ck August 5, 2026 15:19
@bhandras

bhandras commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

@claude please re-review the current head (3eed78b0).

The first review has been addressed as follows:

  • RequestChannelID remains intentionally authenticated for every receive. Receive route registration is account-scoped because it may inspect or reserve receive credit; allowing an unsigned request would preserve the account-freezing path this series closes. The gRPC/REST constructor and method docs now state which operations require a signer, and a regression test proves an unsigned receive fails before transport.
  • In the production receive flow, client_vhtlc_pubkey is the daemon identity key returned by IdentityPubKey; the separate payment-scoped receive-auth key is not passed in that field. The coupling is now explicit and fail-closed anyway: the canonical digest's account key is passed into CreditAccountAuthorizationSigner, and the daemon rejects signing unless it exactly matches its identity key. The daemon signing RPC carries and validates that account key, and tests cover a mismatch.
  • protobuf requests are cloned with proto.Clone; no generated message state is copied by value.
  • formatting, focused tests, the swapruntime transport test, all 71 changed-file linters, and commit-message lint pass locally.

Please focus the second pass on whether the explicit account-key signer contract closes the concern without weakening receive authorization, and flag any remaining merge blockers.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 2m 24s —— View job


Re-review: credit account request authorization (HEAD 3eed78b0)

I re-traced the full client→daemon boundary against the three findings from the first pass. All are resolved, and the receive-authorization concern is closed without weakening it. No merge blockers.

Prior findings — verified resolved ✅

#1 — RequestChannelID receive auth (intentional, guarded, tested). RequestChannelID still authorizes every receive (grpc_conn.go:106), which is the deliberate account-scoped behavior. The constructor docs now state the signer contract precisely: NewGRPCSwapServerConn — "A signer is required for receive and credit-account operations" (grpc_conn.go:34); NewRESTSwapServerConn — "receive and credit-account operations require the authenticated constructor" (grpc_conn.go:47). TestCreditAccountRPCRequiresSigner proves an unsigned ListCredits and unsigned RequestChannelID both fail with credit account authorization signer is required and never reach transport (require.Empty(client.listCreditReqs)). ✅

#2 — Explicit, fail-closed account-key signer contract (the focus of this pass). The coupling is now explicit end-to-end:

  • authorizeCreditAccountRequest threads the digest's own accountKey into the signer: g.creditAccountSigner(ctx, accountKey, digest, expiresAt, nonce) (credit_account_auth.go:48). It is no longer discarded.
  • The daemon signs only for itself: SignCreditAccountAuth computes identityKey and returns errCreditAccountIdentityMismatch unless bytes.Equal(accountKey, identityKey) (rpc_wallet.go:220-224), then builds the BIP-340 message from identityKey. The gRPC surface SignCreditAccountAuthorization carries account_pubkey, length-checks it, and maps the mismatch to codes.InvalidArgument.
  • I confirmed the production coupling holds: the real receive caller sets client_vhtlc_pubkey to the daemon identity key — clientKey, _ := s.client.daemon.IdentityPubKey(ctx) (out_swap.go:629) is passed straight into RequestChannelID (out_swap.go:680). So CreditAccountRequestDigest's accountKey (= client_vhtlc_pubkey for this method) equals the identity key, the equality gate passes, and the signature commits to the exact key the request transmits. If those keys ever diverge, signing fails closed (mismatch) rather than emitting a proof over a key the request doesn't carry. TestSignCreditAccountAuthorizationSignsDigest covers both the happy path and the foreign-key rejection. ✅

#3 — proto.Clone. unsignedCreditAccountRequest now uses proto.Clone(req) (credit_account_auth.go:199); no message state is copied by value. ✅

Payload / nonce integrity (re-confirmed on current head)

  • Same payload signed and sent. In every path the digest is derived from req (auth cleared via proto.Clone + SetCreditAccountAuthorization(nil)), then the proof is set on that same req with no field mutation before the RPC call. The signed digest and the transmitted request coincide. ✅
  • Fresh nonce per call, no reuse. Each call draws io.ReadFull(g.authRand, nonce) and a fresh authNow()+TTL expiry; a retry re-signs with new entropy. TestCreditAccountRPCUsesFreshAuthorization verifies two successive calls carry distinct nonces and independently-valid BIP-340 signatures — so the downstream persist-nonce-before-handler verifier won't see self-inflicted replays. ✅
  • Compatibility intact. CreateInSwap/QuoteInSwap still authorize only when len(accountPubKey) > 0, so ordinary in-swaps stay unsigned. ✅
  • Permission + REST parity. SignCreditAccountAuthorization granted under entitySwap:write (rpc_auth.go); REST test asserts the gateway body carries a populated account_authorization and uses NewAuthenticatedRESTSwapServerConn. ✅

Non-blocking nit

  • Doc drift: swaprpc/CLAUDE.md still states the package is "Fully generated; no hand-written Go files in this package," but credit_account_auth.go (+ its test) are now hand-written in package swaprpc. Worth a one-line update so the invariant stays accurate. Fix this →

Server-side dependency (unchanged, restated for the downstream verifier)

The verifier must reconstruct the digest identically — full method string + 0x00 + deterministic marshal (auth cleared) under CreditAccountRequestTag — and reconstruct the auth message under CreditAccountAuthTag from the request's own account field. For RequestChannelId that field is client_vhtlc_pubkey, which production binds to the daemon identity key; the verifier should BIP-340-verify against that same key. This is a server-repo obligation, not something this PR can enforce.

Verdict

The explicit account-key signer contract closes the first-pass concern and strengthens (not weakens) receive authorization: it now fails closed on any identity divergence, and the receive rail remains mandatorily signed with a regression test guarding it. I found no merge blockers — only the optional swaprpc/CLAUDE.md doc touch-up.

Note: I did not execute go test/make lint in this environment (command approval); verification above is by source reading. The PR states the focused tests, swapruntime transport test, all 71 changed-file linters, and commit-message lint pass locally.
· branch agent/credit-request-auth

@bhandras
bhandras merged commit cd74140 into main Aug 11, 2026
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant