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.DeserializedResponses config flag #9789

Merged
merged 4 commits into from
May 29, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Apr 3, 2023

Pair PR of ipfs/boxo#252.

I simply update Kubo to use the new configuration structure and I keep all the defaults of Kubo, that is, everything is a trusted gateway unless DeserializedResponses is set to false in the configuration. Also added a simple test to check if DeserializedResponses is honoured. Further tests can be found in Boxo.

Steps:

@hacdias hacdias requested a review from aschmahmann April 3, 2023 11:58
@hacdias hacdias self-assigned this Apr 3, 2023
@hacdias hacdias marked this pull request as ready for review April 3, 2023 11:58
@hacdias hacdias requested a review from lidel as a code owner April 3, 2023 11:58
@hacdias hacdias force-pushed the boxo/issue/225 branch 4 times, most recently from 775bace to 3de34fd Compare April 5, 2023 12:09
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.

Could tweak the name/meaning in Kubo, and add OnlyTrustless to the config?
It is a pretty important feature: people who want to run public gateway but don't want to be responsible for phising etc could easily remove the risk by enabling this.

This way, we don't need to deal with migrations / changing default behavior in Kubo (OnlyTrustless being false by default), but allow people to opt-in if they want.

@hacdias hacdias force-pushed the boxo/issue/225 branch 2 times, most recently from e3f6bf5 to 2613244 Compare April 6, 2023 12:32
@hacdias hacdias changed the title refactor: use updated gateway with trusted mode feat(gw): trustless mode via OnlyTrustless Apr 6, 2023
@hacdias hacdias requested a review from lidel April 6, 2023 12:32
@hacdias hacdias requested a review from a team as a code owner April 11, 2023 08:30
core/corehttp/gateway.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Also tests please ?

@hacdias
Copy link
Member Author

hacdias commented Apr 24, 2023

@Jorropo I added a very simple test. Don't forget that most of this is tested in Boxo.

@Jorropo
Copy link
Contributor

Jorropo commented Apr 30, 2023

Don't forget that most of this is tested in Boxo.

We still want to check that the 2 lines of code plumbing the config to the boxo field are correct. 🙂

@hacdias hacdias changed the title feat(gw): trustless mode via OnlyTrustless feat(gw): configurable trusted mode May 2, 2023
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM ty

@hacdias hacdias force-pushed the boxo/issue/225 branch 2 times, most recently from 3b51534 to d461b58 Compare May 10, 2023 10:54
@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.

TrustedModeDeserializedResponses

See ipfs/boxo#252 (review) for details

@hacdias hacdias requested a review from lidel May 16, 2023 12:20
@hacdias hacdias changed the title feat(gw): configurable trusted mode feat(gateway): configurable deserialised responses May 16, 2023
@lidel lidel force-pushed the boxo/issue/225 branch 2 times, most recently from bd3ada1 to a446062 Compare May 23, 2023 22:49
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.

I've switched this PR to the latest version from ipfs/boxo#252 (review).

Good overall, but we need better UX and regression tests for all config variants.

I know this is extra work, would not ask normally for this level of paranoia, but misconfiguration of this flag could expose potential Trustless Gateway operators to risks that come with hosting deserialized data – details below.

test/cli/gateway_test.go Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
@lidel lidel changed the title feat(gateway): configurable deserialised responses feat: Gateway.DeserializedResponses config flag May 29, 2023
Add CAR and Accept header variants as regression tests, just in case things
will break during yet another refactor (;
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.

Thanks!

Switched to boxo main branch, added changelog and some tests, merging.

@lidel lidel merged commit c10b804 into master May 29, 2023
16 checks passed
@lidel lidel deleted the boxo/issue/225 branch May 29, 2023 22:59
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.

None yet

3 participants