[ANCHOR-1267]: Unauthenticated client_domain pre-auth thread exhaustion in SEP-10/SEP-45 takes the whole anchor SEP API offline from one host - #1990
Draft
amandagonsalves wants to merge 2 commits into
Draft
Conversation
* add client domain allow list for sep-45 requests * update client domain fetching with a bounded executor this prevents external calls from hanging indefinitely by enforcing a timeout
* add `client_allow_list` configuration for sep45 * update sep45 challenge generation to enforce client allow list * refactor client domain signing key fetching with a bounded executor * add tests for new sep45 rules and executor behavior
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
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.
Description
Sep10Service.createChallengeand bothSep45Servicecall sites (createArgsFromRequest,verifyArguments) fetch the wallet-suppliedclient_domain'sstellar.tomlsynchronously, on the servlet request thread, before authorization is checked. The fetch has no bulkhead:ClientDomainHelper.fetchSigningKeyFromClientDomaincan block for up to ~30s (a 15s OkHttp call timeout, doubled by the HTTPS→HTTP retry off mainnet), and nothing caps how many of these can run concurrently./authand/sep45/authare both unauthenticated by design (SEP-10 issues the JWT, so it can't require one), and the shipped default (sep10.client_attribution_required: false, and SEP-45 had no allow-list option at all) means any public hostname is accepted with zero preconditions. An unauthenticated attacker pointingclient_domainat a host that just never answers can park a servlet worker per request; ~20 req/s (3.2 KB/s) exhausts the default 200-thread Tomcat pool and takes down every SEP endpoint on that connector. The project's own Helm chart makes this worse:livenessProbe/readinessProbehit/healthon the same connector/pool, so starvation escalates to actual pod destruction and a crash-loop that outlives the attack.Correcting only the allow-list gap (giving SEP-45 the same opt-in
client_allow_listSEP-10 already has) does not close this: the DoS doesn't depend on the domain being disallowed. Any allow-listed domain — or any domain the attacker can influence the DNS/routing of — can still park a thread if it's slow to respond. The actual fix has to bound concurrency, not which domains are trusted. So this PR does both: extends the allow-list to SEP-45 (closes the confirmed SSRF-scope gap, report claim (ii)), and moves theclient_domainfetch off the servlet thread onto a small, bounded, reject-not-queue executor (closes the thread-exhaustion DoS, report claim (iii), the report's headline finding).Report claim (i) — that the existing SEP-10 allow-list guard "checks the wrong getter" — is not addressed here because it isn't a bug: the current guard matches the deliberate, already-shipped, already-tested design from the prior SEP-10 SSRF fix (
0979bd19, #3824178), which was itself reverted from a stricter draft specifically to avoid breaking operators who configureclients:for unrelated reasons.Changes
ClientDomainHelper.java: added a static, boundedThreadPoolExecutor(core=4, max=8, keepAlive=60s, SynchronousQueue,AbortPolicy) and a newfetchSigningKeyFromClientDomainBounded(clientDomain, allowHttpRetry)that submits the existing unbounded fetch to it and waits at most 2.5s viaFuture.get.RejectedExecutionException/TimeoutException(pool full or timed out) andExecutionException(unwrapped to the originalSepExceptionwhen that's the cause) both surface as a genericSepException("client_domain resolution unavailable")— no behavior change for the normal-latency case, since the timeout is well above any real anchor's expected TOML fetch time.Sep10Service.java:createChallengeand the package-privatefetchSigningKeyFromClientDomainwrapper (renamedfetchSigningKeyFromClientDomainBounded) now call the bounded method instead of the direct one.Sep45Service.java: both call sites switched to the bounded method the same way. AddedvalidateClientDomainAllowed(clientDomain), called before either fetch — mirrorsSep10Service.validateChallengeRequestClient's opt-in branch exactly: only enforced whensep45Config.getClientAllowList()is explicitly non-empty, so operators who never set the new field see no behavior change.Sep45Config.java(interface): addedgetClientAllowList()/getAllowedClientDomains().PropertySep45Config.java: addedclientAllowListfield and aClientServicedependency (new constructor param);getAllowedClientDomains()derives from theclients:section using the identical logicPropertySep10Configalready uses (falls back to all non-custodial clients' domains when the list is unset);validate()rejects any allow-list entry that doesn't name a configured client.SepBeans.java:sep45Config(...)bean now takes and passesClientService.anchor-config-default-values.yaml/anchor-config-schema-v1.yaml: documentedsep45.client_allow_list, mirroring the existingsep10.client_allow_listentry.ClientDomainHelperTest.kt: added tests proving (a) a normal, unsaturated call still fails with the ordinary fetch error, not the bulkhead one, and (b) once the bounded pool'smaximumPoolSizeis saturated by blocking tasks, a further call rejects in well under the 2.5s bound rather than hanging.Sep10ServiceTest.kt: updated the two references to the renamedfetchSigningKeyFromClientDomainBounded.Sep45ServiceTest.kt: added three tests mirroringSep10ServiceTest's allow-list coverage — rejects an out-of-listclient_domainwithout ever calling the fetch; allows anyclient_domainwhen no explicit list is set; allows an unlistedclient_domainwhenclients:exists only for unrelated configuration.Sep45ConfigTest.kt: updated thePropertySep45Configconstructor call for the newClientServiceparam; added allow-list derivation and validation tests mirroringSep10ConfigTest.Acceptance Criteria
client_domainpointed at a host that never responds fails in ~2.5s, not ~15–30s./healthand other SEP endpoints keep responding normally throughout.sep45.client_allow_listconfigured rejects aclient_domainoutside that list withSepNotAuthorizedException, before any outbound fetch is attempted.client_allow_listbehaves exactly as before this PR (no regression for existing deployments).Context
HackerOne #3903968
Testing
./gradlew :core:test --tests "org.stellar.anchor.sep10.Sep10ServiceTest" --tests "org.stellar.anchor.sep45.Sep45ServiceTest" --tests "org.stellar.anchor.util.ClientDomainHelperTest"./gradlew :platform:test --tests "org.stellar.anchor.platform.config.Sep45ConfigTest" --tests "org.stellar.anchor.platform.config.Sep10ConfigTest"./gradlew :core:test :platform:testDocumentation
N/A
Known limitations
N/A