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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IPIP-431: Opt-in Extensible CAR Metadata on Trustless Gateway #431
base: main
Are you sure you want to change the base?
IPIP-431: Opt-in Extensible CAR Metadata on Trustless Gateway #431
Changes from all commits
2eb7b9e
3814b6a
cb1d8b3
1c0fbaa
a7e75d7
68715c4
ed86a0f
5056bde
9170c29
65ffcfc
9d1b61f
93c3c28
eacf51a
62fb207
72ed04c
e1fc296
152f4a6
b6069bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion points:
varint | CID
header?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willscott @rvagg thoughts? Value added in DAG-JSON prefixed with own CID is that it allows client to detect truncation beyond
0x00
byte.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe clients can already easily detect truncation of the metadata block.
{
and end with a matching}
.}
and the JSON parser will throw an error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion point:
In the current proposal, the top-level "data" object combines fields about "what was requested" (e.g. CAR & DAG params) with "what was returned" (e.g. CARv1 length in bytes).
I'd like to discuss an alternative: split
data
into two fieldsreq
andres
. The first will describe what the client requested, the second will describe what the server returned.Such division would allow us to shorten field names, e.g.
data.car_params.dup
can becomereq.dups
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitting into
req
andres
sgtm, improves clarityThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos
sig_key
with CID-encoded publiclibp2p-key
that can be used for signature verification.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is our use case:
To do so, we must not accept signatures from any identity, only the signature from the identity advertised by SP to IPNI.
I am arguing this is true for everybody else who wants to use the signature to verify that a metadata block submitted by an untrusted party was indeed produced by the expected Trustless Gateway.
Consider a simple attack vector: the attacker takes the metadata block produced by the origin gateway and replaces the signature with one created using the attacker's identity. Clients verifying the signature against the
sig_pubkey
field in the metadata will not notice the attack.Now I can see how including
sig_pubkey
can simplify troubleshooting:sig_pubkey
does not match the pubkey we expected, then we know the metadata block was signed by somebody elsesig_pubkey
matches but the signature does not, then we know the metadata block was modified from the original.Compare that with my proposal:
IMO, this improvement is not worth the cost of increasing metadata block size and, thus, egress traffic for Trustless Gateways.
Do you have any other use case for the signature in your mind?
IMO, the clients making retrieval requests don't need this signature for validating the metadata block, as they can rely on guarantees provided by the underlying transport - HTTPS.
Good point. We don't require all Gateways to sign the metadata block, SPARK needs the signature only from Storage Providers' servers handling retrieval (
booster-http
).Let's update the spec to explicitly mention the signature is an optional field.
As I wrote above, if you don't know the expected server identity, then the signature is not useful for you.
Having said that, I like the idea of adding more details about the identity/public key to the spec.
The proposed format CID-encoded public
libp2p-key
seems like a good candidate, although AFAICT, that's not the format advertised to IPNI. In IPNI, I see identities in the format that can be used in multiaddr's/p2p/{id}
part:Makes sense; I'll update the spec to require the metadata to be a DAG-JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos what happens when returned CAR is for:
Is the semantic meaning here to be "raw bytes of all files, ignoring UnixFS directory metadata", or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great questions! TBH, I don't know the answers. We don't need
data_bytes
for SPARK. I think this field was added based on the discussion in this proposal, but I could not find the specific comment requesting it.I am proposing to remove
data_field
from the spec. We can introduce it later if there is a clear need. We will better understand the desired semantics at that point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos What is the difference between
car_cid
and this field?Hardcoding Blake3 in field name and description makes no sense if you use Multihash. It could use functions other than blake3 in the future.
To reduce future confusion, could this be renamed to
car_checksum
? (and removecar_cid
since it is redundant?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is your description of
car_cid
, see #431 (comment):Regarding
b3checksum
:For SPARK, we specifically need the Blake3 hash of the CAR stream, and we need gateways to always return this hash. In particular, clients cannot ask the server to use Blake3 for the CAR checksum because the server could use this information to detect SPARK clients vs. other clients and provide different quality of service.
I agree it's confusing to have both
car_cid
andb3checksum
, but I don't see a better solution. Do you?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion point leading back to #431 (comment):
How do we represent the information about what content was requested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
We already have a multicodec for CAR. Couldn't you retrieve a CAR file from a gateway by CID e.g. https://w3s.link/ipfs/bagbaierabxhdw7wglmlehzgobjuoq3v3bdv64iagjdhu74ysjvdecrezxldq - you don't need to signal successful transmission if the content hashes to the same CID.
If you're using the graph API (
?format=car
orAccept: application/vnd.ipld.car
) you're being specific about what you want in the request and the client is verifying the blocks...and so they know if the transmission over HTTP completed successfully or not...right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a couple rough edges that motivated the desire to have additional signaling / metadata here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we get the "CAR CID" from?
It does not exist as a concept anywhere in the specs related to retrieval or routing.
AFAIK in the majority of real world use cases:
This works only if there is no HTTP midleware in-between client and server, which is never the case. There is always some HTTP middleware or CDN in production.
Once you are limited to HTTP semantics, you will cache truncated responses, and the client has to be smart enough to (1) detect that (2) be able to retry in a way that does not hit the same cache.
This is why it does not work in places like Rhea/Saturn, where HTTP responses are (last time i checked) cached blindly based only on HTTP semantics without understanding internal Block/DAG structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos Let's go extra mile here and elaborate what happens when CAR response with
0x00
-prefixed suffix is parsed by existing CAR software.My suggestion is to add some clear statement about expected interop, like "libraries and implementations SHOULD ignore the suffix after 0x00", otherwise we will create a bad UX/DX, where developer tries to debug things with existign tooling and the tooling errors.
I imagine we don't want things to fail due to
0x00
suffix, bare minimum being:ipfs dag import
should ignore suffixThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a great idea to think about compatibility with existing & future tooling and clearly describe our thinking. 👍🏻
The most important aspect is avoiding the "0x00 insertion attack" vector. You can find more details in the section Zero-length-block insertion attacks (including the Filecoin-specific logic). I am cross-posting the mitigation I proposed:
When developers use existing tooling, they will never receive a CAR file with the
0x00
suffix.There are two major ways how a CAR with a
0x00
suffix can emerge:Somebody makes an HTTP request to a Trustless Gateway, explicitly asks to receive CAR with
meta=eof+json
, saves the response body to a.car
file and forgets to extract the CAR payload from the container (remove the\x00{metadata}
trailer).Somebody uses a tool that is aware of
meta=eof+json
. The tool opts into this new feature when requesting content from a Trustless Gateway, but does not extract the CAR payload from the container in the response body before returning the content back to the user.I am arguing that (2) is a bug in the tooling, introduced by the change that modified Trustless Gateway requests to opt-into
meta=eof+json
, and therefore, the maintainers of that tool should fix that bug - make the tool adhere to spec.Regarding (1): do you think this will happen frequently enough to justify the effort required to change all libraries you mentioned to start ignoring the
0x00
byte?Maybe it's actually a good thing that the tooling reports an error because it tells the user they are using the new
meta=eof+json
feature incorrectly.As an alternative to silently stripping the
0x00
suffix, the tooling can detect the situation where0x00
is followed by a valid DAG-JSON object and report a more helpful error message to the user, advising them to either change the "accept" header in the request to the Trustless Gateway or else remove the0x00
suffix (unpack CARv1 from the container format).Thoughts?
go-car/cmd/car/inspect.go
seems to always treat0x00
as EOF, if I am reading the source code correctly:https://github.com/ipld/go-car/blob/5c5d432d582564f88fd2124f2fce4f2f3e47a654/cmd/car/inspect.go#L26
js-car
seems to always reject zero-length blocks:https://github.com/ipld/js-car/blob/562c39266edda8422e471b7f83eadc8b7362ea0c/src/decoder.js#L94-L97
I guess I can test how existing tooling handles zero-length blocks and document this behaviour in the IPIP, so that we better understand the current landscape.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging this TODO to show in the PR discussion.
Any suggestions for the artefacts I can link to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aschmahmann do we have anything on GH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging this TODO to show in the PR discussion.