Skip to content
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

Closed
wants to merge 9 commits into from

Conversation

juliangruber
Copy link

@juliangruber juliangruber commented Jul 25, 2023

This PR is expected to stay open for as long as the Request Attestation spec is still in development.

Copy link

@bajtos bajtos left a 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 Show resolved Hide resolved
httpipfs.go Outdated Show resolved Hide resolved
httpipfs.go Outdated Show resolved Hide resolved
@juliangruber juliangruber requested a review from bajtos July 26, 2023 07:47
httpipfs.go Outdated
Comment on lines 149 to 157
res.Header().Set(
"X-Attestation",
fmt.Sprintf(
"\"%s.%s\"",
base64.StdEncoding.EncodeToString(b),
base64.StdEncoding.EncodeToString(sigSigned),
),
)

Copy link

@bajtos bajtos Aug 1, 2023

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.

Copy link

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.

@@ -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"),
Copy link
Member

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?

@rvagg
Copy link
Member

rvagg commented Aug 4, 2023

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:

frisbii/httpipfs.go

Lines 186 to 189 in a2011b6

// closeWithUnterminatedChunk attempts to take control of the the http conn and terminate the stream early
//
// (copied from github.com/filecoin-project/lassie/pkg/server/http/ipfs.go)
func closeWithUnterminatedChunk(res http.ResponseWriter) error {

See also in Saturn:

https://github.com/filecoin-saturn/L1-node/blob/56387b9615b44197b814fe0f9dfd5e461f135482/container/shim/src/fetchers/lassie.js#L225-L229

You get a nice error condition on the client side, curl will report an error condition, and apparently in nginx too, mostly importantly you don't get a "this is OK" cache state.

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.

@bajtos
Copy link

bajtos commented Aug 7, 2023

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.

@juliangruber
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants