Conversation
F2. Read the DRS specification first this time, and the endpoint shapes are its
own: hostname-based drs://host/id resolves to https on 443 under
/ga4gh/drs/v1/objects, and an object either carries an access_url directly or an
access_id to exchange at /access/{access_id}.
The design follows from one fact. A DRS URI names its own host and the URI comes
from the client, so following one unchecked makes this service a request
generator pointed wherever a workflow says, from inside a TRE, at whatever that
TRE can reach and the caller cannot. BIOCHEF_DRS_HOSTS is therefore an allowlist
with no default, and unset disables the source -- the same shape as
BIOCHEF_LOCAL_ROOT, whose comment says it confines itself because the client
chooses the path. Here the client chooses the host.
Compact identifiers are refused. Resolving drs://prefix:accession means asking a
third-party resolver which host to contact, which is taking an endpoint out of a
document and trusting it completely, and that is a mistake this service has
already made twice this week.
An object id becomes a path segment on the DRS server, so one containing a slash
or a leading dot is refused before anything is opened. A test asserts nothing is
opened at all for a host outside the allowlist, because an allowlist applied
after the request documents a policy already violated.
What arrives is checked against the sha-256 or md5 the object declares, and an
object declaring neither is refused rather than taken on trust. That is the gap
C1 exists to close and it does not stop being one because a different protocol
opened it. A server sending far more than its declared size is cut off, since
the workspace writer has no reason to know what was claimed.
An access_url may point at another host entirely, which is what a presigned S3
or GCS URL is, and that is why the allowlist covers the DRS server rather than
the bytes and why the checksum carries the weight.
The list was exact, deliberately, so that adding a provider is a decision somebody makes rather than something that happens quietly. Adding drs without updating it is exactly the case it was written for, and I pushed the previous commit with it red.
Audit of the DRS source, and both findings are patterns this project has already paid for once. urllib follows 3xx by default, so an allowlisted DRS server answering 302 sent this service wherever it liked, including hosts reachable only from inside. The allowlist checks the URI it was handed and the request went somewhere else, which makes the control decorative. The DRS API is now opened with redirects refused outright. The bytes are the other case and keep following them, because an access_url is expected to bounce to storage and the allowlist never covered that host, but only while it stays on https. A download that failed its checksum was left in the workspace under the name the workflow asked for, holding whatever the server sent. That is the gap C1 closed for tool bundles, reopened by a different protocol. Bytes are staged, verified, and only then given the name. The test for that was worthless at first and the mutation said so: removing the staging left it passing, because it asserted nothing was left behind and both designs clean up after a failure. The difference only exists while the bytes are in flight, so the reader now looks at the workspace on every chunk and the assertion is that the workflow's own file never appears until the checksum has been checked. That version fails against the unstaged one.
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.
Closes F2 (#21). Based on
datasource-interface(#69), the D1 it dependson, and nothing else — it adds a third provider behind that seam, which is what
that file’s own docstring says F1 and F2 should be.
The design follows from one fact
drs://host/idresolves tohttps://host/ga4gh/drs/v1/objects/id. The URInames its own host, and the URI comes from the client. Following one unchecked
makes this service a request generator pointed wherever a workflow says — from
inside a TRE, at whatever that TRE can reach and the caller cannot.
So
BIOCHEF_DRS_HOSTSis an allowlist with no default, and unset disablesthe source. Same shape as
BIOCHEF_LOCAL_ROOT, whose comment says it confinesitself because the client chooses the path. Here the client chooses the host.
Compact identifiers are refused. Resolving
drs://prefix:accessionmeansasking a third-party resolver which host to contact — taking an endpoint out of
a document and then trusting it completely, which is a mistake this service has
already made twice.
I read the specification before writing this time, which is why the endpoints,
the
access_idexchange and the checksum handling are the spec’s rather than myguess at them.
What the audit then found
Redirects walked around the allowlist.
urllibfollows 3xx by default, soan allowlisted DRS server answering 302 sent this service wherever it liked. The
allowlist checked the URI it was handed while the request ended up somewhere
else, which makes the control decorative. The DRS API now refuses redirects
outright; the bytes still follow them, because an
access_urlis expected tobounce to storage and the allowlist never covered that host — but only while it
stays on https.
A download failing its checksum was left under the workflow’s own name,
holding whatever the server sent. That is the C1 gap reopened by a different
protocol. Bytes are staged, verified, then renamed.
A test that was worth nothing until the mutation said so
The staging test asserted "nothing is left behind" — and passed with the
staging removed, because both designs clean up after a failure. The difference
only exists while the bytes are in flight.
It now watches the workspace on every chunk and asserts the workflow’s own file
never appears before the checksum has been checked. That version fails against
the unstaged one.
Other decisions
dot is refused before anything is opened — with a test asserting nothing is
opened at all for a host outside the allowlist, since an allowlist applied
after the request documents a policy already violated.
sha-256ormd5the object declares; one declaringneither is refused rather than taken on trust.
workspace writer has no reason to know what was claimed.
241 passed, 4 skipped. Nothing in the tests touches the network — thetransport is injected, so these are about what this service decides.
Note
I pushed one commit here with a pre-existing test red: the file that enumerates
the providers names them exactly, deliberately, and I added a third without
updating it. Fixed in the commit after, but it should not have gone out.