Configuration and network policy: bound the header block and the destination, and let a stage set both - #17
Merged
Conversation
§10.1 of the design policy asks for a bound on "the response header block and the total response size, not only the body the caller asked for". The body had one from the first commit -- the caller's buffer -- and the header block had none: `OnHeader` appended every line to a table whose size the server chose. The transport now counts header bytes as they arrive, summed across interim `1xx` responses so that a stream of small `100 Continue`s is not a way around it, and abandons the exchange at 64 KiB. The count is taken before a line is copied, so the bound holds for the allocation it exists to prevent. A response abandoned there is refused whole by the exchange layer, before its status is read: the status line is the one part that arrived intact, and an open that took `Content-Length` and `Accept-Ranges` from the prefix that fit would be acting on a response nobody finished receiving. `InvalidResponse`, naming the bound, never retried. The corpus gained the row that proves it, `OversizedHeaders`: a correct response padded after the fields that matter with a megabyte of kilobyte-sized fields. Kilobyte lines, because a row made of one huge line would be refused by libcurl's own per-line ceiling and would say nothing about this repository's bound. The self-test asserts the size from the bytes on the wire and asserts everything else against the Normal row, so a client cannot pass by refusing a response that was also malformed. It also found that the bound was not optional. With it removed, libcurl 8.7.1 opens the asset behind that megabyte without complaint; and with libcurl's `CURLE_TOO_LARGE` reclassified as something else, the row still passes, so the refusal is this code's and not the library's. The second change here is a case owed since v0.2.0. The scheme allowlist holds at every redirect hop as a consequence -- a `Location` goes through the same parser as an identifier, and that parser accepts two schemes -- and a consequence is the kind of property nothing notices losing. Seven targets are now refused and asserted never requested, and the two scheme-less forms that stay inside the allowlist are asserted still followed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
§10.2 of the design policy: an identifier can arrive from a layer nobody here authored, and a resolver that connects wherever it is told is a request- forgery primitive. Reach is a declared policy rather than whatever the host's network allows, and until now there was nothing to declare it with. `DestinationPolicy` is a set of four address classes -- public, private, loopback, link-local -- classified by numeric value and never by name. An IPv6 address that carries an IPv4 one, mapped or compatible or behind the NAT64 prefix, is the class of the address it carries, because that is where the connection ends up; `[::ffff:169.254.169.254]` is link-local. It is judged twice, and neither judgement is redundant. At connect time, in `CURLOPT_OPENSOCKETFUNCTION`, against each address libcurl is about to connect to after the name was resolved and before a socket exists: that is what makes it hold for `localhost`, for a name whose answer changed between lookups, and for `127.1`. A name is refused only when every address it resolved to is, so a refused IPv6 attempt followed by a permitted IPv4 one that did not answer is still reported as the network failure it was. And before any request, against a literal in the URL at every redirect hop: behind a proxy the connection is the proxy's, and this is the only check that sees the destination at all. The connect-time half was checked by removing it. The literal cases all still pass, and `localhost` walks straight past a policy that refuses loopback; `usdAssetHttp_destination_policy`'s name case is the one test that fails, and it is there for exactly that reason. The default is `public,private,loopback`. Loopback and private networks stay reachable because local fixture servers and intranet hosts are what `http` is registered for, and the corpus runs under the default unchanged; link-local is refused because nothing legitimate serves USD from it and the credential endpoint of a cloud instance is what reliably lives there. The resolver reads the set from `USD_HTTP_RESOLVER_DESTINATIONS`, strictly: an unknown class name refuses the whole value and keeps the default, because a policy that dropped the word it did not recognize is not the policy that was written. A refusal is `AccessDenied`, naming the class, with no request sent and no retry -- the code a `403` gets, on DIAGNOSTICS.md §1's test, because a caller does the same thing about both. libcurl is also now told `http,https` and nothing else. The parser already enforces the allowlist; this makes a parser that ever widened widen into a refusal. And the bundle README stops claiming that nothing cached outlives the process, which has not been true since v0.4.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The environment was a bootstrap with nothing behind it. A host that opens two
stages against two servers under two policies cannot be served by a
process-global, and CONFIGURATION.md has said since v0.2.0 that the
`ArResolverContext` a stage is opened with is where that belongs. This is it.
A context is made from a string, through OpenUSD's own entry point:
ArGetResolver().CreateContextFromString(
"https", "USD_HTTP_RESOLVER_DESTINATIONS=public; ...")
with the environment's own names, so the surface stays one vocabulary and a
value in a context is refused or adjusted for exactly the reasons the same
value in the environment would be. No header of this repository reaches a host
that way, which is ADR-0001's rule for consumers extended to the hosts that
configure them. Eight variables may be set per stage -- the deadlines,
retries, redirects, the destination policy, and the two coalescing limits.
The block size, the budgets, and the persistent directory may not: every stage
shares the store they configure, and the store's stripes are sized for one
block size, so a stage asking for larger blocks would fetch each one and
watch it evicted on arrival. The environment is snapshot at construction and
a context resolves against the snapshot, so a host mutating its environment
mid-session does not reconfigure a stage it opened an hour ago.
Two things had to be decided that no document had said, and both were found by
writing the case first and then removing the fix.
Every identifier this resolver owns is context-dependent. Not because it
resolves to a different path under two contexts -- it resolves to itself --
but because whether it resolves at all can, and OpenUSD's layer registry acts
on the answer: for a path that is not context-dependent, `SdfLayer::FindOrOpen`
finds an already-loaded layer by identifier whatever `Resolve` has just said.
Answer `false` and a stage whose context refuses loopback opens a loopback
layer, as long as some other stage opened it first.
And the table of opens `Resolve` retains is keyed by the transport options as
well as the identifier. It was correct keyed by identifier while one process
had one configuration; a reader keeps the options it was opened with, and a
reader retained under a permissive policy was one `OpenAsset` away from a stage
whose context refuses its destination. Keyed by identifier alone, the refusing
context got the reader.
A context also reads back from Python, as its canonical string. Without a
to-Python conversion `ctx.Get()` raised, and `Usd.Stage.__repr__` printed
`pathResolverContext=<invalid repr>` for any stage opened with one. The
conversion is registered once Python is running, so a C++ host never pays for
it.
The configuration warnings now tell an adjusted value from a refused one. A
block size rounded down used to end "using the default", which was false.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A review of this branch found the same shape of defect four times over: a guarantee that held in the case written for it and not in the one beside it. Each is fixed here, and each fix has a case that fails without it. Through a proxy, the destination policy saw neither the connection -- that is the proxy's -- nor the host, because the pre-flight read canonical literals only and libcurl normalizes `2852039166`, `0xa9fea9fe`, and `%31%36%39.254.169.254` to 169.254.169.254 before the proxy reads the request. The transport now takes the host from libcurl's own URL parser before each request and judges that. The new case uses the fixture server as the proxy: with the check removed, four of five spellings reach it. Refusing link-local did not refuse the metadata endpoints that are not link-local. AWS's IPv6 endpoint is unique-local, Alibaba's is in the shared address space, Azure's WireServer is public. They are now a class of their own, `metadata`, classified by value before the ranges and refused by default; permitting `link-local` does not permit it. Inside an `ArResolverScopedCache`, OpenUSD caches `Resolve` by path alone for a resolver that does not implement scoped caches, which walked past a per-stage policy the way the layer registry had. The bundle now declares `implementsScopedCaches` and keeps the scope's cache itself, keyed by identifier and configuration. Asset info had the same gap by another route, and now answers from memory only for a caller whose policy covers one the identity was reached under. And implementing contexts means OpenUSD constructs this resolver in every process that opens any stage -- local ones included, and sometimes twice. A constructor that configured the persistent tier created its directory for hosts that never named a remote URL. Construction now does nothing; configuration happens at first use. A child process whose only stage is local now leaves no directory behind, and with configuration back in the constructor it did. Smaller, in the same pass: an abandoned header block is no longer retried behind a `503`; a destination list is trimmed of line breaks rather than refused onto the wider default; context values are kept as the parser read them, so `060000` and `60000` are one context; a context's adjustments are judged over the environment it will be layered on; the Python conversion takes the GIL and no other lock; the context type moved out of OpenUSD's namespace, where `ArResolverContext` would have matched it by name against any other plugin's; admitted sockets are close-on-exec; and the two getenv-reading entry points nothing called are gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The start of
v0.7.0, ahead ofv0.6.0for the reasonv0.5.0was: this iscode, and the consumer integration is waiting on a gigabyte fixture and a host.
Three of phase 7's rows — the bounds §10 of the design policy names and the tree
did not enforce, and the per-stage configuration surface that lets a host state
them for one stage rather than for the whole process.
What it does
A bound on the response header block (
660925b). 64 KiB per exchange,summed across interim
1xxresponses and counted in the transport before a lineis stored. With the caller's buffer already bounding the body, that is the whole
of §10.1's "bound the response header block and the total response size". A
response abandoned at the bound is refused whole, as
InvalidResponse, whateverits status — its status line arrived intact, and an open that took
Content-LengthandAccept-Rangesfrom the prefix that fit would be acting on aresponse nobody finished receiving. The corpus gains a nineteenth row,
OversizedHeaders, and the redirect-hop scheme allowlist gets the explicit caseimplementation-status.md said it was owed.
A destination policy (
856ff4e,e6e8768),USD_HTTP_RESOLVER_DESTINATIONS:a set of address classes —
public,private,loopback,link-local,metadata— a connection may reach. The default ispublic,private,loopback:loopback and intranet hosts are what
httpis registered for, and link-localand the well-known instance-metadata endpoints are refused — the latter a class
of its own, by value, because AWS's IPv6 endpoint is unique-local and Alibaba's
is in the shared address space. It is judged three times — at connect time, in
CURLOPT_OPENSOCKETFUNCTION, against the address a name actually resolved to;before each request, against the host as libcurl's own URL parser will send it,
which is what holds through a proxy; and at every redirect hop against a
canonical literal. IPv4-mapped, -compatible, and NAT64 addresses are the class
of the IPv4 address they carry. A refusal is
AccessDeniednaming the class,unsent and unretried. libcurl is also told
http,httpsand nothing else.Per-stage configuration through
ArResolverContext(9bf6459,e6e8768):The environment's own names and parser, so a value in a context is refused or
adjusted for exactly the reasons it would be in the environment, and kept as
the parser read it so that equivalent spellings are one context. The
environment is snapshot at the resolver's first use and a context resolves
against it: context, then environment, then default. No header of this
repository reaches a host that configures it this way.
What the review found (
e6e8768)A
/code-reviewpass over the first three commits found the same shape ofdefect four times: a guarantee that held in the case written for it and not the
one beside it. Each is closed, and each fix has a case that fails without it.
connect is the proxy's, and libcurl normalizes
2852039166to169.254.169.254before the proxy reads it. The host is now judged aslibcurl will send it;
usdAssetHttp_destination_policyuses the fixtureserver as the proxy, and with the check removed four of five spellings reach
it.
those became the
metadataclass.Resolveby path, walking past a per-stagepolicy the way the layer registry had. The bundle declares
implementsScopedCachesand keys the scope's cache by configuration. Assetinfo had the same gap and now answers from memory only for a caller whose
policy could have reached the asset.
any stage. The constructor had configured the persistent tier, creating its
directory for local-only hosts. Construction now does nothing and
configuration happens at first use; a local-only child process is asserted to
leave no directory behind.
Smaller, in the same pass: no retry of an abandoned header block behind a
503; line breaks trimmed from a destination list instead of falling back tothe wider default; context adjustments judged over the environment; the Python
conversion under the GIL and no other lock; the context type out of OpenUSD's
namespace; close-on-exec sockets; dead getenv entry points removed.
Things worth reviewing
The header bound was not optional. With it removed, libcurl 8.7.1 opens an
asset behind a megabyte of ordinary header fields; the library's ceilings are on
a single line, and a block of kilobyte lines never reaches them. With libcurl's
own
CURLE_TOO_LARGEreclassified as something else, the row still passes, sothe refusal is this code's.
Only the connect-time check stops a name. Removing it leaves every literal
case passing and lets
localhostthrough a policy that refuses loopback.usdAssetHttp_destination_policy's name case is the one test that fails, whichis what it is there for.
Every identifier this resolver owns is now context-dependent. Not because it
resolves to a different path under two contexts, but because whether it resolves
at all can — and OpenUSD's layer registry acts on the answer. For a path that is
not context-dependent,
SdfLayer::FindOrOpenfinds an already-loaded layer byidentifier whatever
Resolvehas just said, so a stage whose context refuses adestination would be handed the layer by any stage that opened the URL first.
Answering
falsefails exactly thestillRefusedcase inhttpResolver_stage.The only other reader of that answer in OpenUSD is
usdUtilspackaging, whichsends URI paths down its remap path either way.
The retained opens are keyed by transport options as well as identifier.
Correct keyed by identifier while one process had one configuration; a reader
keeps the options it was opened with, and keyed by identifier alone the refusing
context in
TestRetainedOpenIsNotHandedAcrossContextsgot a reader a permissiveresolve had left behind.
The metadata list is a list. Eight addresses, by value, documented in
CONFIGURATION.md §2.1. It is the one piece of provider-specific knowledge in
the policy, and the alternative — calling link-local "the metadata endpoint" —
was true of one address out of eight.
Four variables are not settable per stage. The block size, the two budgets,
and the persistent directory stay the environment's, although the roadmap's
v0.7.0 scope lists block size and budgets among what a context resolves. Every
stage shares the store they configure, and the store's stripes are sized for one
block size — eight blocks to a stripe, 2 MiB at the defaults — so a stage asking
for larger blocks would fetch each one and watch it evicted on arrival. A context
naming one is told so when it is created. This is a judgement call and the place
to disagree with it.
A Python conversion in the bundle. Without one,
ctx.Get()raisedTypeError: No to_python (by-value) converter found, andUsd.Stage.__repr__printed
pathResolverContext=<invalid repr>. The context converts to itscanonical string, registered once Python is running. It uses the vendored
pxr_boost::pythonthattfalready links, underPXR_PYTHON_SUPPORT_ENABLED.Tests
usdAssetHttp_destination— address classification as a table: every classboundary from both sides, the metadata endpoints and their neighbours, the
mapped/compatible/NAT64 spellings, and the non-canonical forms left to the
transport.
usdAssetHttp_destination_policy— the client-side and connect-time halvesover a real socket, through
localhost,127.1, and a proxy.usdAssetHttp_protocol— the abandoned header block at open, at a redirect,mid-read, and behind a
503; seven refused redirect schemes; the policy'spre-flight at every hop and its presence on every request the transport sees.
usdAssetFixture_corpus,usdAssetHttp_corpus_projection—OversizedHeadersfrom the wire, and projected.
httpResolver_configuration— the variable, the context string grammar,canonical values, line breaks, judgement over the environment, precedence,
and the fingerprint.
httpResolver_stage— a context configuring one stage, including through alayer another stage loaded, a retained reader, a scoped cache, and asset
info; a context bounding the transport; and a local-only child process left
untouched.
Verified locally on macOS arm64 (Apple clang 17, libcurl 8.7.1, OpenUSD 26.08):
core 31/31, ASan + UBSan 31/31, TSan 31/31, plugin 36/36, and the plugin's
configuration code under ASan + UBSan by hand. The first three commits passed
all twelve CI lanes, Windows included;
e6e8768addsSetHandleInformationonWindows and
curl_urlon every platform, which CI is the first to build.Not in this PR
needs an ADR on how a host supplies a provider before it needs code; formation
composition; and the
/Breprolink flag for reproducible Windows binaries.VERSION,openstrata.toml, the plugin manifest, and theCMake fallback are untouched, and the changelog section is
Unreleased.🤖 Generated with Claude Code