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

feat(gateway)!: deserialised responses turned off by default #252

Merged
merged 1 commit into from
May 29, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Apr 3, 2023

Closes #225. Adds support for trustless and trusted gateway modes.

  • Default is Trustless Mode as per gateway: add trustless-only mode #225. In addition,
    • Can be globally overwritten by setting Config.TrustedMode to true
    • Can be overwritten per known gateway by setting gateway.TrustedMode to true/false
    • localhost, 127.0.0.1 and ::1 are implicit trusted mode.
  • Cleans up the following:
    • Moves all gateway configuration to Config, including NoDNSLink and PublicGateways. This is motivated by the fact that we now need to access this information from the main handler in order to determine if it's a trustless or trusted gateway.
    • Reworked the context hostname keys. See code comments for explanation.
  • Examples are set in trusted mode because that's what our tests expect.
    • Perhaps in a separate PR we could rework the tests for the proxy version to instead use the trustless gateway as it is a perfect example.
  • In trustless mode, requests for trusted resources return 501 406.
  • Includes many tests, except for /ipns/{cid}?format=ipns-key because we still don't have a good way of doing that.

Sharness will fail here because structures and defaults have changed. Kubo PR: ipfs/kubo#9789

@lidel @aschmahmann I may have missed some very specific case. Please take a look at let me know what you think.

@hacdias hacdias self-assigned this Apr 3, 2023
@hacdias hacdias requested a review from lidel as a code owner April 3, 2023 09:08
gateway/gateway_test.go Outdated Show resolved Hide resolved
@hacdias hacdias marked this pull request as draft April 3, 2023 09:10
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #252 (e10c5fb) into main (8ec71db) will increase coverage by 1.02%.
The diff coverage is 86.27%.

❗ Current head e10c5fb differs from pull request most recent head 4629dda. Consider uploading reports for the commit 4629dda to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #252      +/-   ##
==========================================
+ Coverage   48.26%   49.28%   +1.02%     
==========================================
  Files         279      279              
  Lines       33538    33585      +47     
==========================================
+ Hits        16187    16554     +367     
+ Misses      15672    15298     -374     
- Partials     1679     1733      +54     
Impacted Files Coverage Δ
gateway/gateway.go 88.52% <ø> (ø)
gateway/handler_unixfs_dir.go 61.49% <25.00%> (-1.41%) ⬇️
gateway/hostname.go 65.44% <69.23%> (+3.92%) ⬆️
gateway/handler.go 68.03% <84.44%> (+10.46%) ⬆️
examples/gateway/common/handler.go 95.12% <100.00%> (+1.00%) ⬆️
gateway/handler_unixfs__redirects.go 36.55% <100.00%> (ø)

... and 27 files with indirect coverage changes

@hacdias hacdias force-pushed the issue/225 branch 2 times, most recently from 5c14166 to 2264ac2 Compare April 3, 2023 11:39
@hacdias hacdias changed the title feat(gateway): trustless mode feat(gateway)!: new trustless mode, and by default Apr 3, 2023
@hacdias hacdias requested a review from aschmahmann April 3, 2023 11:58
@hacdias hacdias marked this pull request as ready for review April 3, 2023 11:58
@hacdias hacdias requested a review from a team as a code owner April 3, 2023 11:58
gateway/handler.go Outdated Show resolved Hide resolved
gateway/handler.go Outdated Show resolved Hide resolved
gateway/handler.go Outdated Show resolved Hide resolved
gateway/handler.go Outdated Show resolved Hide resolved
gateway/hostname.go Outdated Show resolved Hide resolved
gateway/gateway.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the issue/225 branch 2 times, most recently from 81fc2f9 to 4d3bd38 Compare April 6, 2023 12:25
@hacdias hacdias requested a review from lidel April 6, 2023 12:25
@hacdias hacdias force-pushed the issue/225 branch 2 times, most recently from 165ddf6 to 2a8720b Compare April 12, 2023 07:39
@hacdias hacdias force-pushed the issue/225 branch 2 times, most recently from c26b26a to 0f7d1af Compare April 25, 2023 09:00
@hacdias hacdias requested a review from Jorropo May 2, 2023 12:37
@hacdias hacdias force-pushed the issue/225 branch 2 times, most recently from 68dafd2 to 98cf731 Compare May 10, 2023 10:51
@lidel lidel self-assigned this May 11, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took this for a spin with fresh eyes and functionally looks ok, but the UX felt off.

My bad, I know I suggested going with "TrustedMode" 🙈 The problem is deeper than just having something that work fine as boolean.

I feel we need to make another pass at the name (i know.. but its important to get this one right) -- @hacdias lmk thoughts on the below direction:

Problem

We've created a single flag, and this in turn builds a false dichotomy around "trust".

People familiar with content-addressing understand what "trustless" mean and interpret "trusted" deserialized responses as a "step back", paradoxically, something "less useful, because it requires trust".

However everyone outside our bubble will see "trust" and "no trust" and always pick "trust".

Proposed change

If we replace "trusted" with "deserialized" it would convey a slightly different meaning:

  • Deserialized Mode: (disabled by default, user opt-in) This configuration mode indicates that the gateways supports returning data in a deserialized format, allowing for direct data manipulation and interpretation without the need for further processing, at the cost of the client being unable to do the verification (unless Trustless Mode is enabled as well).

    • We want this to be disabled by default, implementers having to explicitly opt-in in their configs.
  • Trustless Mode: (implicitly enabled) This mode indicates if gateway supports returning content-addressed data in its original state, without any deserialization or interpretation, allowing for external verification and processing.

    • We want this to be ALWAYS enabled. The ability to fetch a raw block should not be negotiable, this is the core function of HTTP Gateway

Rationale

My main point is to avoid the semantic trap around "trust", and convey functional meaning instead.

By using "deserialized [responses]" instead of "trusted [mode]," the focus shifts to the nature of the responses being returned by the gateway, highlighting whether they are in a processed (deserialized) or raw (unprocessed) form. It is what matters to the end user, and the developer that integrates boxo/gateway in their own app -- we can't expect people to understand "trusted" vs "trustless".

By having two separate terms, we avoid false dichotomy: "deserialized" and "trustless" are siblings, not the opposites (and we always support trustless, while deserialized is opt-in).

TLDR

👉 I propose we rename TrustedMode to DeserializedResponses.
We still can keep it as bool (disabled by default, require opt-in) but the configuration flag becomes self-explanatory and removes ambiguity/confusion around the semantic meaning of "trust".

ipns/name.go Outdated Show resolved Hide resolved
@hacdias hacdias requested a review from lidel May 16, 2023 12:16
@hacdias hacdias changed the title feat(gateway)!: new trustless mode, and by default feat(gateway)!: deserialised responses turned off by default May 16, 2023
lidel
lidel previously requested changes May 23, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hacdias I've pushed some tweaks in godocs and tiny HTTP fixes/renames for readability (details in the commits).

The DeserializedResponse handling inside of boxo/gateway looks ok, but we should move ipns.Name` to separate PR (details below).

ipns/name.go Outdated Show resolved Hide resolved
@hacdias hacdias dismissed stale reviews from aschmahmann and lidel May 29, 2023 08:04

addressed

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @hacdias ❤️

Hopefully, this will make it easier for people to leverage https://specs.ipfs.tech/http-gateways/trustless-gateway/ and expose deserialized responses only when they really need to.

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

Successfully merging this pull request may close these issues.

gateway: add trustless-only mode
3 participants