Require Intent for GetVtxoChain/GetVirtualTxs#904
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds configurable indexer transaction exposure, intent-based PSBT proof validation, short-lived auth-token generation/validation, a SignMessage gRPC/schema and wallet signing API, nullable DB timestamp handling, tests/fixtures for auth/signing, and Go toolchain/CI/Dockerfile bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IndexerHandler
participant IndexerService
participant Validator as Intent/PSBT Validator
participant Signer
participant Database
Client->>IndexerHandler: GetVtxoChain(vtxoKey, intent)
IndexerHandler->>IndexerService: GetVtxoChain(vtxoKey, intent)
IndexerService->>IndexerService: Check txExposure
alt Withheld/Private
IndexerService->>Validator: ValidateIntentWithProof(intent)
Validator->>Database: Fetch vtxo data
Database-->>Validator: vtxo
Validator-->>IndexerService: validation result
alt Valid
IndexerService->>Signer: createAuthToken(outpoint)
Signer-->>IndexerService: authToken
IndexerService->>Database: Fetch vtxo chain
Database-->>IndexerService: chain
IndexerService-->>IndexerHandler: VtxoChainResp(chain, authToken)
else Invalid
IndexerService-->>IndexerHandler: error
end
else Public
IndexerService->>Database: Fetch vtxo chain
Database-->>IndexerService: chain
IndexerService-->>IndexerHandler: VtxoChainResp(chain)
end
IndexerHandler-->>Client: GetVtxoChainResponse
sequenceDiagram
participant Client
participant VirtualTxsHandler
participant IndexerService
participant TokenValidator as Auth Token Validator
participant Database
Client->>VirtualTxsHandler: GetVirtualTxs(authToken, txids)
VirtualTxsHandler->>IndexerService: GetVirtualTxs(authToken, txids)
IndexerService->>IndexerService: Check txExposure
alt Withheld
alt authToken present
IndexerService->>TokenValidator: validateAuthToken(authToken)
TokenValidator-->>IndexerService: valid
IndexerService->>Database: Fetch virtual txs (with signatures)
Database-->>IndexerService: txs
else no authToken
IndexerService->>Database: Fetch virtual txs
Database-->>IndexerService: txs (strip arkd sigs)
end
else Private
IndexerService->>TokenValidator: validateAuthToken(authToken)
TokenValidator-->>IndexerService: valid/invalid
alt valid
IndexerService->>Database: Fetch virtual txs (with signatures)
Database-->>IndexerService: txs
else invalid
IndexerService-->>VirtualTxsHandler: error (auth required)
end
else Public
IndexerService->>Database: Fetch virtual txs
Database-->>IndexerService: txs
end
IndexerService-->>VirtualTxsHandler: VirtualTxsResp
VirtualTxsHandler-->>Client: GetVirtualTxsResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@api-spec/protobuf/ark/v1/indexer.proto`:
- Around line 204-208: The GET request payload includes the Intent message
(fields intent.proof and intent.message) which can be large and exceed
URL/query-length limits; change the API so REST clients send Intent in the HTTP
body instead of query params — either (a) remove Intent from GetVtxoChainRequest
and create a separate POST RPC (e.g., GetVtxoChainWithIntent) that accepts
IndexerOutpoint + IndexerPageRequest + Intent in the request body, or (b) keep
the same RPC but update the google.api.http annotation to use POST and map the
Intent to the HTTP body (or a new wrapper message) so proof/message are sent in
the request body rather than as query parameters; apply this change around the
GetVtxoChainRequest and Intent message references.
In `@api-spec/protobuf/signer/v1/service.proto`:
- Around line 31-36: The SignMessage RPC binding currently exposes an
unauthenticated HTTP endpoint; either remove the HTTP gateway binding for the
SignMessage RPC (remove the option (meshapi.gateway.http) block on rpc
SignMessage(SignMessageRequest) returns (SignMessageResponse)) or add
authentication middleware so the endpoint requires macaroons like the main arkd
service; if adding auth, ensure the SignerService/SignMessage handler validates
macaroons on incoming requests (same macaroon check used by arkd) or restrict
the HTTP gateway to listen only on localhost so the endpoint is not reachable on
all interfaces.
In `@internal/core/application/indexer.go`:
- Around line 424-433: In the TxExposureWithheld branch, ValidateIntentWithProof
errors are being swallowed; change the logic in the case for TxExposureWithheld
so that after calling i.ValidateIntentWithProof(ctx, vtxoKey, intentForProof)
you distinguish validation failures from infrastructure/internal errors and only
suppress errors that indicate an invalid proof, while returning unexpected
errors to the caller; use the error type/value returned by
ValidateIntentWithProof (or wrap/inspect it) to detect "invalid proof" vs
DB/internal faults and only proceed to call i.createAuthToken when validation
succeeded, otherwise return the unexpected error instead of dropping it.
- Around line 497-509: The fallback incorrectly treats vtxoKey.Txid as raw tx
hex causing tx.Deserialize to always fail; replace the stubbed assignment with a
real fetch from the wallet (use s.wallet.GetTransaction(ctx,
vtxoOutpoint.Txid.String() or correct id) to obtain txhex and check its error),
then pass the returned raw hex into tx.Deserialize; ensure you handle and
propagate errors from s.wallet.GetTransaction (return an
errors.INVALID_PSBT_INPUT with InputMetadata on failure) and only store the
deserialized wire.MsgTx into boardingTxs[vtxoOutpoint.Txid] when deserialization
succeeds.
- Around line 483-484: The loop over outpoints can index past proof.Inputs
(psbtInput := proof.Inputs[idx+1]); add a bounds check before using idx+1 to
avoid a panic: inside the for loop that iterates outpoints, verify that idx+1 <
len(proof.Inputs) (or len(proof.Inputs) > idx+1) and handle the mismatch (return
an error, log and continue, or skip) instead of indexing blindly; update the
code around the outpoints loop and psbtInput usage to enforce this guard and
surface a clear error message if sizes are inconsistent.
- Around line 582-594: The TODO must be resolved: if the proof PSBT requires a
server counter-signature, call SignTransactionTapscript(ctx,
intentForProof.Proof) and use its returned signed PSBT when calling
intent.Verify (i.e., replace intentForProof.Proof with the signed result), and
propagate any error into the same INVALID_INTENT_PROOF path; additionally remove
or redact logging of the full intentForProof.Proof and intentForProof.Message
(do not log raw PSBT or full message), instead log only non-sensitive metadata
(e.g., proof ID or truncated hash) or omit these fields in the Tracef call to
avoid exposing input/outpoint/script details.
- Around line 414-448: The code in GetVtxoChain currently hardcodes
someExposureFromConfig := "public", bypassing auth branches; replace that
hardcoded value with the instance config i.txExposure (e.g., use exposure :=
i.txExposure), validate exposure != "" and then switch on TxExposure(exposure)
so the Withheld/Private branches (which call i.ValidateIntentWithProof and
i.createAuthToken) can execute as intended; ensure error messages remain the
same and no other logic changes in the switch.
- Around line 78-99: The code currently serializes the signer pubkey with
schnorr.SerializePubKey producing a 32-byte x-only key (in NewIndexerService)
which breaks ECDSA verification in validateAuthToken (which assumes full parity
via schnorr.ParsePubKey) and prevents matching PSBT PartialSig.PubKey (33-byte
compressed) in GetVirtualTxs; change NewIndexerService to store the full 33-byte
compressed pubkey (serialize the returned pubkey as the compressed form instead
of x-only) into indexerService.signerPubkey, update validateAuthToken to
parse/verify using btcec.ParsePubKey (or the equivalent compressed-key parser)
instead of schnorr.ParsePubKey, and ensure GetVirtualTxs compares
PartialSig.PubKey against the 33-byte compressed signerPubkey (so bytes.Equal
can match) rather than a 32-byte x-only key.
- Around line 753-786: validateAuthToken currently tries to parse a DER ECDSA
signature and double-hashes the msg, which is incompatible with
createAuthToken/SignMessage (which produce 64-byte schnorr signatures over the
raw message). Update validateAuthToken to parse the signature as a schnorr
signature (use the schnorr parse/verify API) instead of ecdsa.ParseDERSignature,
and verify against the raw msg (do not call chainhash.DoubleHashB on msg);
continue to parse the signer pubkey with schnorr.ParsePubKey(i.signerPubkey) and
call the schnorr verify method to return the boolean result.
In `@pkg/arkd-wallet/core/application/wallet/service.go`:
- Around line 731-739: SignMessage currently produces an ECDSA DER signature
(uses ecdsa.Sign) while the verifier in indexer.go expects Schnorr signatures
from a Schnorr-serialized public key (signerPubkey), so auth tokens will always
fail verification; fix by making signing and verification use the same scheme —
either change SignMessage to call schnorr.Sign(...) and return the Schnorr
signature format to match the existing verifier, or (alternatively) update
indexer.go to parse/derive the ECDSA public key and verify with
ecdsa.ParseDERSignature()/sig.Verify using that ECDSA pubkey; locate SignMessage
in service.go and the verification code in indexer.go (where signerPubkey is
parsed) and make both sides use the same signature algorithm (prefer using
schnorr.Sign if you want minimal changes to indexer.go).
🧹 Nitpick comments (4)
api-spec/openapi/swagger/ark/v1/indexer.openapi.json (1)
489-495: Verify:authTokenquery parameter lacks description.Other query parameters in this spec (e.g.,
spendableOnly,pendingOnly) include adescriptionfield. The newauthTokenparameter and theintent.proof/intent.messageparameters (Lines 578–591) omit descriptions. Adding brief descriptions would improve the developer experience for REST API consumers, especially given the new exposure modes (public/withheld/private) that affect how these fields are used.README.md (1)
111-111: Description could be more precise.The current description "Require intent for getting vtxo chain" undersells the feature. It also governs
GetVirtualTxsauth-token behavior and PSBT signature stripping inwithheldmode. A slightly more descriptive phrasing like "Controls indexer transaction data exposure level" would better convey the scope to operators.Minor nit — this won't block anything.
internal/config/config.go (2)
630-637:IndexerService()creates a new instance on every call — consider caching.Unlike
AppService()which caches inc.svc, this method creates a newIndexerService(including asigner.GetPubkeygRPC call) on every invocation. Currently it's only called once fromnewServer, but to stay consistent with other accessors and prevent accidental regressions:♻️ Proposed caching pattern
Add an
indexerSvcfield to theConfigstruct and cache the result:+ indexerSvc application.IndexerServiceThen update the accessor:
func (c *Config) IndexerService() (application.IndexerService, error) { + if c.indexerSvc != nil { + return c.indexerSvc, nil + } if c.signer == nil { if err := c.signerService(); err != nil { return nil, err } } - return application.NewIndexerService(c.repo, c.signer, c.IndexerTxExposure) + svc, err := application.NewIndexerService(c.repo, c.signer, c.IndexerTxExposure) + if err != nil { + return nil, err + } + c.indexerSvc = svc + return c.indexerSvc, nil }
567-571: Consider using theTxExposureconstants instead of hardcoded strings.The application layer defines
TxExposurePublic,TxExposureWithheld, andTxExposurePrivateconstants for theTxExposuretype. Using those here would keep validation in sync with the domain layer and prevent drift.♻️ Suggested change
- if c.IndexerTxExposure != "public" && c.IndexerTxExposure != "withheld" && - c.IndexerTxExposure != "private" { - return fmt.Errorf("indexer tx exposure must be either 'public', 'withheld', or 'private'") - } + if c.IndexerTxExposure != string(application.TxExposurePublic) && + c.IndexerTxExposure != string(application.TxExposureWithheld) && + c.IndexerTxExposure != string(application.TxExposurePrivate) { + return fmt.Errorf("indexer tx exposure must be either 'public', 'withheld', or 'private'") + }
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/config/config.go`:
- Around line 573-575: The long fmt.Errorf call in the check for
supportedIndexerTxExposures.supports(c.IndexerTxExposure) exceeds golines; fix
by wrapping the format arguments across lines so the string literal is shorter —
e.g. keep the format string in-line and move supportedIndexerTxExposures to the
next line (or split the message into two concatenated strings) so the call to
fmt.Errorf("indexer txn exposure type not supported, please select one of: %s",
supportedIndexerTxExposures) becomes formatted across lines and satisfies
golines while leaving the logic in supportedIndexerTxExposures.supports and
c.IndexerTxExposure unchanged.
In `@internal/core/application/indexer_test.go`:
- Around line 109-112: Tests construct indexerService with a 32-byte x-only key
via schnorr.SerializePubKey but production uses 33-byte compressed keys
(pubkey.SerializeCompressed()) and validateAuthToken will parse compressed keys
with btcec.ParsePubKey; update each test construction of indexerService (where
signerPubkey is set) to use mockSigner.publicKey.SerializeCompressed() (or the
equivalent compressed byte slice) instead of schnorr.SerializePubKey so the test
bytes match NewIndexerService/validateAuthToken behavior.
- Around line 70-76: Reformat the long stub signature for SignTransaction to
satisfy golines by breaking the parameter list across lines: locate the method
func (m *mockSignerService) SignTransaction and split its parameters (ctx
context.Context, partialTx string, extractRawTx bool) onto separate lines (or
otherwise wrap) while keeping the receiver, parameter names/types and the
(string, error) return intact; run gofmt/golines after editing to confirm CI
passes.
In `@internal/core/application/indexer.go`:
- Around line 636-646: The loop currently removes arkd signatures from
ptx.Inputs[j].PartialSigs, but Taproot virtual transactions store signatures in
TaprootScriptSpendSig; update the stripping logic in the same block to iterate
ptx.Inputs[j].TaprootScriptSpendSig (instead of PartialSigs), filter out entries
whose PubKey equals i.signerPubkey, and assign the filtered slice back to
ptx.Inputs[j].TaprootScriptSpendSig so arkd's taproot signatures are actually
removed in withheld-mode.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/core/application/indexer.go`:
- Around line 648-652: The code currently re-serializes the PSBT with
ptx.Serialize(&b) and assigns b.String() to virtualTxs[idx], which produces raw
binary but callers (NewFromRawBytes(..., true) and others) expect base64; update
the assignment to base64-encode the serialized bytes instead of storing raw
bytes so virtualTxs remains base64-encoded (use the bytes from the
strings.Builder or ptx.Serialize output and encode with
base64.StdEncoding.EncodeToString before assigning to virtualTxs[idx]).
- Around line 489-490: The current check conflates DB errors with "not found" by
using `if err != nil || len(vtxosResult) == 0` after calling
`i.repoManager.Vtxos().GetVtxos(ctx, []domain.Outpoint{vtxoOutpoint})`; change
this to first handle and return (or propagate/log) when `err != nil` so
infrastructure/database failures surface, and only when `err == nil` check
`len(vtxosResult) == 0` to decide to fall back to the on-chain wallet path;
update the surrounding logic that uses `vtxosResult` accordingly (references:
`GetVtxos`, `vtxosResult`, `err`, `vtxoOutpoint`).
🧹 Nitpick comments (2)
internal/config/config.go (1)
637-644: Missing nil-guard forc.wallet— onlyc.signeris defensively initialized.
IndexerService()checks and lazily initializesc.signerbut notc.wallet. If this method is ever called beforeValidate()finishes (or in a test/path that skips wallet init),c.walletwill benil, causing a nil-pointer dereference insideNewIndexerServiceor later inValidateIntentWithProofwheni.wallet.GetTransactionis called.Suggested fix
func (c *Config) IndexerService() (application.IndexerService, error) { + if c.wallet == nil { + if err := c.walletService(); err != nil { + return nil, err + } + } if c.signer == nil { if err := c.signerService(); err != nil { return nil, err } } return application.NewIndexerService(c.repo, c.signer, c.wallet, c.IndexerTxExposure) }internal/core/application/indexer.go (1)
759-792: Auth tokens have no expiry and no scope binding to the requested txids.
validateAuthTokenonly checks the signature — it does not verify that the outpoint embedded in the token relates to the txids being requested inGetVirtualTxs, nor does it enforce a time-to-live. A single valid token grants indefinite access to any virtual txs.If this is acceptable for the current threat model (the token just attests the caller passed through
GetVtxoChain), consider adding a comment documenting that decision. Otherwise, adding a timestamp to the signed payload and verifying token age would limit the blast radius of a leaked token.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/core/application/indexer.go`:
- Around line 515-525: Add a nil-check for psbtInput.WitnessUtxo immediately
after retrieving psbtInput to avoid panics when a proof PSBT omits WitnessUtxo;
if nil, return errors.INVALID_PSBT_INPUT (similar style to existing errors) with
a descriptive message like "missing WitnessUtxo for input" and attach the same
metadata (errors.InputMetadata{Txid: proofTxid, InputIndex: idx + 1}); this
prevents dereferences of psbtInput.WitnessUtxo when comparing PkScript
(prevout.PkScript vs psbtInput.WitnessUtxo.PkScript) and when comparing Value.
🧹 Nitpick comments (2)
internal/core/application/indexer.go (2)
601-610: Auth validation runs after DB fetch and pagination — wasted work when token is rejected.
GetTxsWithTxidsandpaginateexecute before the exposure switch. ForPrivatemode, invalid tokens cause an immediate rejection, making the preceding DB query unnecessary. Moving the auth check before the fetch would avoid the wasted I/O on unauthorized requests.
85-102:txExposureis not validated at construction time — invalid values surface only at request time.
NewIndexerServiceaccepts any string fortxExposure. An invalid value (e.g., a typo in config) won't be caught until a request hits thedefaultbranch inGetVtxoChainorGetVirtualTxs. Validating at startup would fail fast and prevent a misconfigured server from silently serving errors.Proposed fix
func NewIndexerService( repoManager ports.RepoManager, signer ports.SignerService, wallet ports.WalletService, txExposure string, ) (IndexerService, error) { + switch TxExposure(txExposure) { + case TxExposurePublic, TxExposureWithheld, TxExposurePrivate: + default: + return nil, fmt.Errorf("invalid tx exposure value: %q", txExposure) + } + pubkey, err := signer.GetPubkey(context.Background())
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/application/indexer.go (1)
612-696:⚠️ Potential issue | 🔴 Critical
TxExposurePubliccase inGetVirtualTxsreturns empty results — transactions are never fetched.The
Publicbranch (line 625–626) falls through without callingGetTxsWithTxidsor populatingtxs/virtualTxs. The variables are initialized as empty slices (lines 617–618), soGetVirtualTxsalways returns an empty response when exposure is"public".Both
Withheld(line 636) andPrivate(line 682) correctly callGetTxsWithTxidsand paginate. ThePubliccase is missing this.Proposed fix
switch TxExposure(i.txExposure) { case TxExposurePublic: // no need to validate auth + txs, err = i.repoManager.Rounds().GetTxsWithTxids(ctx, txids) + if err != nil { + return nil, err + } + virtualTxs, reps = paginate(txs, page, maxPageSizeVirtualTxs) case TxExposureWithheld:
🤖 Fix all issues with AI agents
In `@internal/core/application/indexer.go`:
- Around line 423-441: When TxExposure is TxExposurePrivate, don't silently
swallow all errors from ValidateIntentWithProof; distinguish between a
validation-failure (invalid intent/proof) and an infrastructure error
(DB/signer). Change the block around ValidateIntentWithProof in the indexer to:
call ValidateIntentWithProof(vtxoKey, intentForProof); if err == nil proceed to
createAuthToken; if err indicates a validation rejection (use the specific
sentinel/error type returned by ValidateIntentWithProof, e.g. ErrInvalidIntent
or a ValidationError) then continue without an auth token as before; otherwise
(for non-validation errors) return that error immediately. Update
ValidateIntentWithProof to export or wrap a sentinel/typed error if needed so
the caller can use errors.Is / type assertion to detect validation vs
infrastructure errors; keep references to TxExposurePrivate,
ValidateIntentWithProof, createAuthToken and GetVirtualTxs in your changes.
🧹 Nitpick comments (1)
internal/config/config.go (1)
637-649:c.repois not nil-checked before being passed toNewIndexerService.
IndexerService()lazily ensureswalletandsignerare initialized but passesc.repowithout checking. If called beforeValidate()(which initializesc.repoviac.repoManager()), this will passnilto the constructor. Other factory methods likeAppService()similarly assume prior initialization, so this may be by convention — but unlike wallet/signer, there's no guard here.Suggested guard
func (c *Config) IndexerService() (application.IndexerService, error) { + if c.repo == nil { + if err := c.repoManager(); err != nil { + return nil, err + } + } if c.wallet == nil {
# Conflicts: # .github/workflows/integration.yaml # .github/workflows/unit.yaml # api-spec/protobuf/gen/ark/v1/indexer.pb.go # api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.go # internal/test/e2e/delegate_utils_test.go
🔍 Arkana PR Review — Update (3 new commits since last review)Commits reviewed: Summary of ChangesThese three commits address several issues from the previous iteration:
Review Notes✅ Token Cache Improvements (Looks Good)
|
|
Review of new commits (
This is a necessary fix. Without it, the Good catch. LGTM on this commit. |
🔍 Arkana PR Review — #904Require Intent for GetVtxoChain/GetVirtualTxs — Stateless auth token system for VTXO chain/virtual tx access control SummaryThis PR introduces a configurable access control layer ( ArchitectureThe design is sound:
Security AnalysisStrengths:
Potential concerns:
Cross-Repo Impact
Test CoverageComprehensive unit tests covering: Nits
VerdictStrong security feature addressing a real griefing vector. The stateless token design is elegant and the three-tier exposure model gives operators flexibility. Point 4 (cache TTL race on re-add) is the main item to address before merge. |
🔍 Arkana PR Review — Incremental Review (commits
|
🔍 Arkana PR Review — New Commits (dc730a3, f7bf8d1)Two new commits from altafan since last review: makes intent message mandatory and adds the What changed (delta from 523a7ac → f7bf8d1)
Security Review✅ Auth token scheme looks solid overall:
✅ Intent validation:
Cross-repo Impact
Protocol CorrectnessThe three-tier exposure model (public/withheld/private) is well-designed:
The signature stripping for Overall: Solid anti-griefing improvement. The pointer receiver on |
🔍 Arkana Review — #904PR: Require Intent for GetVtxoChain/GetVirtualTxs OverviewThis PR adds intent-based access control to the indexer's The anti-griefing motivation is sound — preventing third parties from unilaterally exiting someone else's VTXOs is an important protection for the Ark user experience and smart contract safety. 🔴 Critical: Nil privkey panic for non-public modes
Recommendation: Either:
// In config.go IndexerService():
if privkey == nil && c.IndexerExposure != "public" {
return nil, fmt.Errorf("ARKD_INDEXER_SIGNING_PRIVKEY is required when INDEXER_EXPOSURE is %q", c.IndexerExposure)
}🟡 Medium Issues1. Token cache vs. token validity mismatch after restart 2. 3. Incomplete comment in // Helpers to
var valid boolThis looks like a leftover from a draft. 4. 5. No rate limiting on intent validation 🟢 Minor / Nits
✅ Positives
Cross-Repo ImpactAs noted in the PR description, this requires go-sdk changes for the new Review by Arkana 🔍 — [Agent: pr-reviewer] |
🔍 Arkana PR Review — #904PR: Require Intent for GetVtxoChain/GetVirtualTxs SummaryThis PR adds intent-based access control to the indexer service, preventing unauthorized parties from retrieving VTXO chain data and constructing broadcastable transactions. Three exposure modes are introduced:
Auth tokens are stateless schnorr-signed payloads ( Architecture Assessment ✅The design is sound:
Security Observations
Issues & Suggestions🔴 Potential auth bypass in 🟡 Token cache memory growth: 🟡 if len(outpoints) > 1 {
return nil, fmt.Errorf("only one outpoint expected in intent proof")
}This makes sense for the current use case but should be documented in the proto comments — callers might expect multi-outpoint intent proofs to work here since 🟡 🟢 Minor: README typo: There's a stray Cross-Repo Impact
Test CoverageComprehensive test suite:
VerdictStrong security improvement that addresses a real griefing vector. The three-tier exposure model is well-designed and the implementation is clean. The auth bypass edge case in withheld mode (🔴 above) should be clarified before merge. The other items are non-blocking improvements. Review by Arkana 🔍 • [Agent: pr-reviewer] |
| ) (*VirtualTxsResp, error) | ||
| GetVirtualTxsByIntent(ctx context.Context, intent Intent, page *Page) (*VirtualTxsResp, error) | ||
| GetBatchSweepTxs(ctx context.Context, batchOutpoint Outpoint) ([]string, error) | ||
| GetAsset(ctx context.Context, assetID string) ([]Asset, error) |
There was a problem hiding this comment.
Would it be worth adding a RevokeToken endpoint that deletes the cache entry keyed by the outpoint hash so the owner does not have to wait for the expiry if they know it leaked?
There was a problem hiding this comment.
we'll do this in a dedicated pr since it's an admin endpoint
There was a problem hiding this comment.
I was thinking it would be the intent issuer that would have the ability to revoke the token
There was a problem hiding this comment.
no revoking is something that only admin should be able to do
| return nil, err | ||
| } | ||
|
|
||
| token, err := i.createAuthToken(allOutpoints) |
There was a problem hiding this comment.
Should we check for a non-expired token cache entry before creating another one? An attacker with a valid proof could spam the endpoint and create millions of cache entries. This could DoS the indexer endpoints due to the mutex lock in the cache sweep task iterating through all the tokens.
If you check the cache for a non-expired entry for the same outpoints it is a form of rate limiting on cache growth.
Same for GetVirtualTxsByIntent
There was a problem hiding this comment.
on it. I'm gonna change createAuthToken so that if token already exists, the function uses the existing expiration. This way, we make the endpoint always return the same token if it already exists.
4 participants
Closes #843
The general flow is that a caller calls
GetVtxoChainwhich yields[]txidswhich they then pass a param to a call toGetVirtualTxs. The idea of this PR is to create an auth token insideGetVtxoChainif the user can prove ownership of the intent they passed in, then to use this auth token when callingGetVirtualTxs. The spirit here is that: "A public virtual mempool means griefing attacks by third party is possible. Imagine you received a vtxo and now someone can just start unilaterally exiting your vtxo and you end up back onchain. Really annoying and can cause a number of problems with smart contracts."This auth token is a message signed by the server's pubkey along with an outpoint, resulting in stateless auth system.
We need go-sdk changes:
A new intent param must be passed to
GetVtxoChainand the response will now include a new auth token (can be empty string as well). Depending on the new config for arkd for theINDEXER_TX_EXPOSUREfield (with the 3 options beingpublic,private, orwithheld) will determine the creation and enforcement of this auth token.We will also want tests against
GetVtxoChain, the newValidateIntentWithProofandGetVirtualTxsWhen
public: no auth token created, empty auth token in response ofGetVtxoChainand an empty auth token to be passed toGetVirtualTxs.When
private: the caller must passValidateIntentWithProofto get an auth token, and the auth token is required to callGetVirtualTxsWhen
withheld: the caller ofGetVtxoChainhas the option to pass OR failValidateIntentWithProofwhich will determine if they do or do not get an auth token. If they get a token they can use it when callingGetVrtualTxsand the behavior will function the same way as ifpublicwere set. However if they pass an invalid or empty auth token, we must remove from each PSBT the signature of arkd so the user cannot construct the full broadcastable txn.The newly added
ValidateIntentWithProofis largely the same flow as found here:arkd/internal/core/application/service.go
Lines 2048 to 2200 in 368bb9f
The
indexerServicenow that it needs to be able to sign a message for the auth token, and needs access to the server's pubkey (to locate inside the psbt for removal), we have given theindexerServiceaccess toports.SignerServiceandports.WalletService(needed for the SignMessage call).Requires changes in the go-sdk for
GetVtxoChain(which now requires an Intent be passed) and toGetVirtualTxs(which now requires an AuthToken to be passed). Thus the e2e tests without this sdk change will only work when the exposure type is set topublic.Note that theres a couple small changes in the sqlite. I re-ran
sqlc generateand it yielded couple changes to theAfterfield.Summary by CodeRabbit
New Features
Documentation
Tests