feat: HTTP retrieval by block CID by LexLuthr · Pull Request #946 · filecoin-project/boost

LexLuthr · 2022-11-14T12:29:52Z

No description provided.

willscott · 2022-11-14T12:34:00Z

Where is the logic here that would prevent serving blocks that should not be retrievable by provider policy?

willscott · 2022-11-14T12:37:11Z

cmd/booster-http/server.go

 }

+func (s *HttpServer) blockBasePath() string {
+	return s.path + "/block/"


lets have a spec for this interface?

In particular, there are a set of semantics that have become fairly precise around how the IPFS gateways serve a block over HTTP.

We should, unless we have very strong reason otherwise, attempt to replicate those semantics.

Otherwise, we end up having to maintain support for this alternative endpoint with slightly different semantics. that we haven't fully documented anywhere.

I agree we should replicate the gateway semantics - could you point us to any existing docs about gateway semantics that we can take a look at?

https://github.com/ipfs/specs/blob/main/http-gateways/TRUSTLESS_GATEWAY.md

So it seems like what's missing is:

Set Content-Type header to application/vnd.ipld.raw

Set Content-Disposition header to attachment; filename="<cid>.car"

Is there anything else I'm missing?

we should consider path semantics of /ipfs/<cid> rather than /block/<cid> as well

Sorry I'm not familiar enough with the spec to know what you mean by path semantics - can you point us to a specific section?

We probably need "X-Content-Type-Options" set as "nosniff" as well

data is requested by CID, only supported path is /ipfs/{cid}

just that the implemented handler is at the path "/block/" and the gateway spec is expecting it to be at "/ipfs/"

Oh I see - sure that seems like an easy change to make

All requested changes done. @willscott and @dirkmc Can you review again?

dirkmc · 2022-11-14T13:23:41Z

We need to implement provider policy for booster-http, but that's true of all endpoints, so I think it's outside the scope of this PR

willscott · 2022-11-14T14:59:02Z

cmd/booster-http/server.go

+	filename := r.URL.Query().Get("filename")
+	if filename == "" {
+		filename = blockCidStr
+	}
+
+	utf8Name := url.PathEscape(filename)
+	asciiName := url.PathEscape(regexp.MustCompile("[[:^ascii:]]").ReplaceAllLiteralString(filename, "_"))
+	w.Header().Set("Content-Disposition", fmt.Sprintf("%s; filename=\"%s\"; filename*=UTF-8''%s", "attachment", asciiName, utf8Name))


how strongly do we feel we need this additional logic?

this diverges from the headers the gateway spec details, and means you can make a url that downloads as an attacker-controlled filename / includes data not in the block itself.

Are talking about the filename logic in case user did not specify one? Or filename within the header itself?

I see what you mean. Let me remove the filename input altogether to avoid this. Filename would be same as block Cid to mitigate this.

dirkmc · 2022-11-14T15:19:01Z

cmd/booster-http/server.go

+	utf8Name := url.PathEscape(filename)
+	asciiName := url.PathEscape(regexp.MustCompile("[[:^ascii:]]").ReplaceAllLiteralString(filename, "_"))
+	w.Header().Set("Content-Disposition", fmt.Sprintf("%s; filename=\"%s\"; filename*=UTF-8''%s", "attachment", asciiName, utf8Name))
+	w.Header().Set("Content-Type", "application/vnd.ipld.raw")


It looks like the Content-Type gets overridden in the parameter passed to serveContent()

LaurenSpiegel · 2022-11-14T23:49:02Z

Could you please do a related PR to the docs so we can see how this appears from a user perspective along with the piece cid endpoints and query params?

LexLuthr · 2022-11-15T08:30:16Z

Could you please do a related PR to the docs so we can see how this appears from a user perspective along with the piece cid endpoints and query params?

I will update the docs once this PR is merged and a release has the new code. Targeted release for this feature will be v1.5.1-rc1

dirkmc · 2022-11-15T08:38:37Z

cmd/booster-http/server.go

+	utf8Name := url.PathEscape(blockCidStr)
+	asciiName := url.PathEscape(regexp.MustCompile("[[:^ascii:]]").ReplaceAllLiteralString(blockCidStr, "_"))


Instead of doing all the escaping, can you just use blockCid.String()?

I might be wrong but I think this due to how Go treats a string as slice of bytes(runes). It would still try to use the special characters so forcing them to escape would be a safer choice.
BTW, I picked this directly from Kubo.
https://github.com/ipfs/kubo/blob/1d5e46ac97dbd9862818b191517a4dc04186a26e/core/corehttp/gateway_handler.go#L688-L692

cmd/booster-http/http_test.go

dirkmc · 2022-11-16T08:07:33Z

@LexLuthr some other PRs got merged into nonsense/piece-store. Could you please resolve the conflicts and merge when ready.

willscott reviewed Nov 14, 2022

View reviewed changes

willscott mentioned this pull request Nov 14, 2022

booster-bitswap block filter #731

Closed

dirkmc force-pushed the feat/piece-store-http branch 3 times, most recently from 7b31cb5 to 3b9cf50 Compare November 14, 2022 14:12

LexLuthr requested a review from willscott November 14, 2022 14:40

LexLuthr force-pushed the feat/http-block-retrieval branch from 230da21 to 387b25c Compare November 14, 2022 14:58

willscott reviewed Nov 14, 2022

View reviewed changes

dirkmc reviewed Nov 14, 2022

View reviewed changes

dirkmc reviewed Nov 15, 2022

View reviewed changes

willscott mentioned this pull request Nov 15, 2022

API replacement for retrieval filter #954

Closed

Base automatically changed from feat/piece-store-http to nonsense/piece-store November 16, 2022 08:05

dirkmc approved these changes Nov 16, 2022

View reviewed changes

dirkmc and others added 9 commits November 16, 2022 16:13

refactor: merge piece directory into booster-http

5a349af

http get by block cid

41cd524

refactor: merge piece directory into booster-http

f2d9ca9

change path semantics to same as kubo

f8894c5

resolve conflicts

360b98a

mitigate attacker-controlled filename

9da1624

fix Content-Type

fd98b7c

check headers in test

c4813b2

remove escape

3e2126b

LexLuthr force-pushed the feat/http-block-retrieval branch from 6d1bf3a to 3e2126b Compare November 16, 2022 10:43

LexLuthr merged commit f5e1948 into nonsense/piece-store Nov 16, 2022

LexLuthr deleted the feat/http-block-retrieval branch November 16, 2022 10:48

jacobheun mentioned this pull request Nov 17, 2022

Consolidating booster-http endpoints #960

Closed

1 task

		utf8Name := url.PathEscape(blockCidStr)
		asciiName := url.PathEscape(regexp.MustCompile("[[:^ascii:]]").ReplaceAllLiteralString(blockCidStr, "_"))

Conversation

LexLuthr commented Nov 14, 2022

Uh oh!

willscott commented Nov 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirkmc commented Nov 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LexLuthr Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurenSpiegel commented Nov 14, 2022

Uh oh!

LexLuthr commented Nov 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dirkmc commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LexLuthr Nov 14, 2022 •

edited

Loading