-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add prototype Meridian header #10
Conversation
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.
Nice!
FWIW, here is our design document to explain the context for this proposal:
https://www.notion.so/pl-strflt/Content-retrieval-attestation-a6df8198d5a248ceac4c004ee971759c
httpipfs.go
Outdated
res.Header().Set( | ||
"X-Attestation", | ||
fmt.Sprintf( | ||
"\"%s.%s\"", | ||
base64.StdEncoding.EncodeToString(b), | ||
base64.StdEncoding.EncodeToString(sigSigned), | ||
), | ||
) | ||
|
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.
We need to send the attestation in response trailer headers, after the retrieved content was transmitted.
I implemented this change in 5cf3fb9 334077a.
@juliangruber PTAL, could you also deploy my change to fly.io please?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
It turns now Fetch API does not allow clients to read HTTP trailers :(
Quoting from denoland/deno#10214 (comment)
Trailers are not supported in the
fetch
spec anymore, and likely will not be re-introduced unless there is significant demand: whatwg/fetch#981. I don't think we will be supporting them either if the web does not. Removal happened in whatwg/fetch#772.
Let's not deploy my change yet.
5cf3fb9
to
68ded2c
Compare
Signed-off-by: Miroslav Bajtoš <[email protected]>
@@ -119,6 +131,27 @@ func (hi *HttpIpfs) ServeHTTP(res http.ResponseWriter, req *http.Request) { | |||
|
|||
selNode := unixfsnode.UnixFSPathSelectorBuilder(path.String(), dagScope.TerminalSelectorSpec(), false) | |||
|
|||
sig := RequestSignature{ | |||
RequestId: req.Header.Get("X-Request-Id"), |
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.
probably need to gate this on whether this header is included; maybe if there's no request id then you don't get an attestation?
So far we've avoided trailers for any kind of signalling, mainly due to lack of decent support but also I think there's concerns through the various pipelines. There's probably some discussions in ipfs/specs#402 somewhere, but it's come up particularly around the desire to include error conditions; but also as a way to get proper timing data instead of the Server-Timings header which can only include data up to the point of sending actual data. What we've been doing instead for error states is this hack: Lines 186 to 189 in a2011b6
See also in Saturn: You get a nice error condition on the client side, No objection to attempting to use trailers, I'm just wondering how they're going to interact with the whole pipeline that we end up serving from here on upward. |
Thank you for reviewing this PR, @rvagg. We are considering a different approach now: append an extra block (a tombstone) to the CAR file. The proposal is outlined here: https://www.notion.so/pl-strflt/SPARK-Content-retrieval-attestation-a6df8198d5a248ceac4c004ee971759c?pvs=4#dcf10f1e5c1c44038cb39d10575763f0 Feel free to ignore this pull request for the time being until we figure out what exactly we want to implement. |
I'm going to close this PR for now, as it's not actionable. Let's reopen or create a new one with an updated strategy |
This PR is expected to stay open for as long as the Request Attestation spec is still in development.