Update Dart fetchOhttpKeys to support https#1500
Update Dart fetchOhttpKeys to support https#1500spacebear21 wants to merge 5 commits intopayjoin:masterfrom
Conversation
Coverage Report for CI Build 25404741159Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 85.088%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Concept ACK. This approach makes way more sense than shipping another TLS stack for one function by binding reqwest/bitreq etc. Dart bindings can own this as just one weird trick in dart, it's really not that much code surface, and we're not inventing anything new here. This is well studied protocol work that's really well understood. The one thing I'd might flag is whether/how this fingerprints requests vs any other https-in-https CONNECT client used for payjoin. |
Hmm good point... I had Claude compare each implementation and this looks like an intractable problem. Each implementation uses different request headers which would be trivial to fix, but the bigger issue is that each TLS implementation also produces a distinct fingerprint. Both the relay and directory see this fingerprint. Each library ships a different cipher suite list and ordering, TLS extensions, supported groups, etc. |
|
I think the trade-off between shipping a complete TLS stack with each integration for this one function and losing this fingerprint just for the bootstrap mechanism is one where I lean on dealing with the fingerprint. This is all the more reason to make sure caching is done properly in the reference implementation and perhaps bootstrapping properly is even documented in the spec. |
spacebear21
force-pushed
the
dart-fetch-ohttp-keys-https
branch
from
6903cff to
2769c67
Compare
Bespoke implementation of tls-in-tls proxy because it's apparently not supported by any dart native library
spacebear21
force-pushed
the
dart-fetch-ohttp-keys-https
branch
from
2769c67 to
4d6f209
Compare
DanGould
left a comment
DanGould
left a comment
There was a problem hiding this comment.
Used 3 adversarial runs to go back and forth trying to break and fix this to come up with these suggested changes included in this commit
- 10s per-phase timeout. Today
_openConnectTunneland_sendGetawaitdone.futureunbounded, so a slow/hostile relay or directory hangs the wallet forever. Wrapped each phase with.timeout(timeout, onTimeout: () => throw HttpException(...)); surfaced as aDuration timeoutparameter (default 10s). Worst case is 4× timeout but that's tolerable; we can tighten to a shared deadline later if needed. Rust already got this fromreqwest utf8.decode→latin1.decode+ try/catch in the CONNECT listener (and the_sendGetheader parse). Status lines are ASCII per RFC 7230 §3.1.2 but obs-text in headers is legal-but-not-UTF8; today a non-UTF8 byte in any header throws synchronously inside the read handler, escapes theStream.listen(onError:)plumbing, and parks the Completer forever (so even with the timeout above, it'd be the timeout cause rather than a clean error). latin1 never throws on byte input. This is dart-specific- HTTP framing in
_sendGet. Today it reads-until-closedand slices atheaderEnd + 4, which (a) hangs on any directory that doesn't honorConnection: close(kept-alive responses), and (b) silently corrupts anyTransfer-Encoding: chunkedresponse — nginx + Hyper backends and Cloudflare both default to chunked for sub-buffer-size bodies, so this is a real risk against any CDN-fronted directory (says robot). New_parseResponseparses the header block, branches onContent-Lengthor single-chunkTransfer-Encoding: chunked, rejects CL+TE coexistence and multi-chunk/extensions/trailers, caps body at 1 KiB (Key Configurations are <100 bytes per RFC 9540 §3.1, an order of magnitude under any reverse-proxy buffer size, so single-chunk is the de facto behavior). Existing test fixture continues to work.
What it complained about that I deliberately did NOT change in this patch (may land separately):
- Cert pin semantics (
onBadCertificatebyte-equality vs Rust'sadd_root_certificatechain-validating). Production callers pass nothing → uses default system trust → unaffected. We'll lock the cross-binding contract in a separate spec/code PR if this asymmetry is too much of a hassle to bear - Typed
OhttpKeysFetchExceptionwithkind/phaseenums. BBM today doescatch (e) { continue }; the typed surface could be used for future smarter retry. not blocking. - Smuggling-defense header parsing (duplicate
Content-Length, etc.). The minimum parser is good enough for non-hostile directories; a hostile directory can already serve bad keys directly, so the smuggling vector doesn't unlock anything new. - BIP 77 §OHTTP Bootstrapping spec patches (caching MUST/SHOULD, 401/410 eviction, HTTPS-relay-is-not-privacy classification, etc.). Orthogonal — separate spec PR. There's definitely some guidance here that can inform better behavior for clients, and I've got them drafted.
Applied diff from changes in payjoin#1500 (review) Co-authored-by: Dan Gould <d@ngould.dev>
|
applied the diff from DanGould@da58c5f as a separate commit onto this PR |
The CONNECT and GET read loops accumulated bytes without an upper bound, so a malicious relay or directory could stream arbitrary amounts of data within the per-phase timeout and exhaust client memory before the 1 KiB body cap could fire. Add a 16 KiB cap on both buffers, well above any plausible legitimate response. The CONNECT path also discarded any bytes that arrived after the CRLFCRLF that ends the response headers. Those bytes were already consumed from the OS receive buffer, so neither the subsequent RawSecureSocket.secure handshake nor the re-attached subscription in _sendGet ever saw them. With an HTTPS directory the TLS handshake would hang on data that had already been delivered; with an HTTP directory the bytes were a relay-controlled prefix to the response stream. Carry the leftover bytes out of _openConnectTunnel. For the HTTPS path, refuse to proceed if any bytes are present (the destination server cannot speak before the client sends ClientHello). For the HTTP path, prepend them to the response buffer so the parser sees the full byte stream.
Uri.parse is permissive: a URL with an unrecognized scheme (file://, gopher://, etc.) silently fell through with relayIsHttps == false, turning into a plain TCP connect that failed downstream with a confusing error. Likewise nothing rejected an empty host or a host containing characters that Uri.parse happens to permit, leaving header-injection a single Uri behavior change away. Add a single-purpose authority validator that the function calls for both the relay and the resolved keys URL: scheme must be http or https, host must match an allowlist (ASCII letters/digits, dot, hyphen, underscore, colon for IPv6 literals), port must be in 1..65535. Failures throw ArgumentError so misuse surfaces synchronously at the call site rather than as an opaque socket error. Tighten the status-line regex to require a space or end-of-line after the three-digit status code. The previous pattern parsed the prefix of "HTTP/1.1 2009" as 200, accepting a malformed response from a hostile relay or directory.
|
Claude caught some more low-hanging fruit for handling adversarial payloads that are fixed in the more recent commits and tested for documentation purposes since this is ~unreviewable by a human. I also added a disclaimer to the top of the file explaining why this manual implementation is required currently. |
|
Which model? Opus 4.7? With this llm-driven behavior, I think we've gotta make sure we're using at least 2 different models, so I put sonnet at it for one more round which found this:
|
| final sizeLine = latin1.decode(raw.sublist(0, crlf1)); | ||
| if (sizeLine.contains(';')) { | ||
| throw HttpException('Chunk extensions not supported'); | ||
| } |
There was a problem hiding this comment.
RFC 9112 §7.1.1 permits chunk-extensions; Caddy emits them by default. Strip rather than reject so we don't fail closed on RFC-conformant proxies. 1 KiB cap still bounds risk.
| final sizeLine = latin1.decode(raw.sublist(0, crlf1)); | |
| if (sizeLine.contains(';')) { | |
| throw HttpException('Chunk extensions not supported'); | |
| } | |
| final hex = sizeLine.contains(';') ? sizeLine.substring(0, sizeLine.indexOf(';')) : sizeLine; | |
| final n = int.tryParse(hex.trim(), radix: 16); |
|
@chavic is it possible for you to pop in here and run a different model against what's going on here? Ideally an adversarial back and forth? |
Yeah, lemme see what's happening |
|
@DanGould @spacebear21, I opened an alternative approach in #1532. I went back through the Dart SDK issues and tested the current Dart behavior. That lets Dart's This avoids the bespoke CONNECT and HTTP response parser from #1500 while still supporting HTTPS OHTTP relays. I also added focused tests for HTTP relay, HTTPS relay, chunk extensions, and invalid schemes. |
|
Superseded by #1532 |
4 participants
Bespoke implementation of tls-in-tls proxy because it's apparently not supported by any dart native library
Authored by Claude Opus 4.7 . I've skimmed the code for basic sanity, and confirmed this actually works in BBM tests, but I have not thoroughly reviewed the logic or audited line-by-line as this is too low-level for me to do properly in a realistic time frame.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.