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

Adds support for the IPFS protocols: #8805

Closed
wants to merge 1 commit into from
Closed

Conversation

markg85
Copy link
Contributor

@markg85 markg85 commented May 5, 2022

This is definitely a WIP branch.
It adds support for the IPFS protocols (ipfs:// and ipns://).

This code seems to be working already so now i'm looking for feedback to tidy it up for the eventual merger in curl.

@markg85 markg85 force-pushed the master branch 4 times, most recently from dbf26ee to 48e0cf2 Compare May 5, 2022 22:15
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented May 6, 2022

Until we have docs and a few test cases for it, it is hard to judge if this works 100% correctly.

@markg85
Copy link
Contributor Author

markg85 commented May 6, 2022

Until we have docs and a few test cases for it, it is hard to judge if this works 100% correctly.

Sure thing!
But i do think i need a little guidance as to what you'd like to see documented and what you'd like to see tested.

For the testing, i suggest that the tests would only validate that - say - ipfs://QmbGtJg23skhvFmu9mJiePVByhfzu5rwo74MEkVDYAmF5T translates into the http gateway url that's detected. That could be localhost, could be a custom one, could be dweb.link. Say it's localhost that it should translate to: http://127.0.0.1:8080/ipfs/QmbGtJg23skhvFmu9mJiePVByhfzu5rwo74MEkVDYAmF5T

I don't think the tests should actually execute the web request!

Regarding the documentation, what exactly do you think should be documented and where would be the best place for that? I can for example document the logic in detecting the gateway.

@bagder
Copy link
Member

bagder commented May 8, 2022

I think a fair test would be to use an ipfs-using command line and make sure it gets sent off like a proper request based on what the gateway is set to.

Is the gateway supposed to be anything ? I mean it accepts all protocols now and right now the code doesn't even check that it is a URL.

@bagder
Copy link
Member

bagder commented Jul 10, 2022

Closed after two months with no feedback or action. Feel free to reopen when things move again.

@bagder bagder closed this Jul 10, 2022
@markg85
Copy link
Contributor Author

markg85 commented Jul 17, 2022

Hi @bagder, sorry for not being active! This is definitely on my list of things to work on further and complete this implementation. I'll update this pr soon. Definitely still this month!

@markg85
Copy link
Contributor Author

markg85 commented Jul 26, 2022

Could you re-open this PR? I apparently don't have the permission for it.
I pushed a new commit (https://github.com/markg85/curl) with lots of fixed based on your review.

src/tool_operate.c Outdated Show resolved Hide resolved
@markg85
Copy link
Contributor Author

markg85 commented Jul 27, 2022

Feel free to have a new fresh look at the latest patch.

I rewrote the url rewriting part completely to make use of curl API methods. There is now no custom parsing anymore besides the check to see if the parsed protocol is ipfs or ipns to even enter this code path to begin with.

What's still missing in the gateway detection part is a default fallback (when no gateway is found) to dweb.link. Just for reference, that site is maintained by ProtocolLabs (the creators of IPFS) and is allowed to be the fallback. Same is done on the ffmpeg side of things.

@bagder
Copy link
Member

bagder commented Jul 27, 2022

dweb.link. ... is allowed to be the fallback

The IPFS gateway gets to see all the IPFS traffic as there is no extra security layer on top of this. Using an IPFS gateway means you blindly trust that server and its operators and allow them to see and possibly even tamper with the data (and you can't tell). It's a really big deal. I don't think we should have any "random" site out there as a default gateway without very huge warning letters and flashing alarms going off.

@markg85
Copy link
Contributor Author

markg85 commented Jul 27, 2022

I don't think we should have any "random" site out there as a default gateway without very huge warning letters and flashing alarms going off.

The intent of having this is to give an "ipfs always works" idea. As it first will want to find a local gateway and then uses dweb.link as fallback. On the ffmpeg side this concern is sort of mitigated by just printing a chunk of text as warning notifying the user that dweb.link is used and that the user really should set up a local node instead. Is such a solution acceptable for curl too?

@bagder
Copy link
Member

bagder commented Jul 27, 2022

It makes me uncomfortable. The admins of that single gateway gets super powers and control over so many users' potential traffic when someone convinces ordinary users out there to run a curl command line using ipfs.

I understand the point of why someone might want to do this. I'm just not sure I agree that the method is a very good one for users ultimately. Using a gateway to access ipfs in the first place is a shaky thing, and you really should trust the gateway before you use it. I don't see how we can tell the world to trust dweb.link by default.

@jzakrzewski
Copy link
Contributor

A wild idea - fail if no local node can be detected but point users to "dweb.link" that they can enable if they trust them. It could still be seen as an "endorsement" from curl though and I don't know if it's desired.
What y'all think?

@markg85
Copy link
Contributor Author

markg85 commented Jul 28, 2022

I was thinking about something like that too @jzakrzewski
When no gateway is detected curl should then probably spit out some text telling the user what to do to make it work. In there could be an example with dweb.link. It could look as simple as:
# IPGS_GATEWAY=https://dweb.link curl ipfs://<cid>

What are your thoughts on that?

@lidel
Copy link
Contributor

lidel commented Jul 29, 2022

To be honest, hardcoding a specific gateway in a low level tool like curl makes me uncomfortable too.
Technical users lose the opportunity to learn more about the ease of switching between gateways, and that they can run their own.

@bagder thoughts on opt-in options below?
"no gateway set" error is a good opportunity to educate

(A) No endpoint endorsement at all

$ curl ipfs://bafkreicysg23kiwv34eg2d7qweipxwosdo2py4ldv42nbauguluen5v6am
Error: local gateway not found and/or IPFS_GATEWAY is not set
Learn how to run one: https://docs.ipfs.tech/install/command-line/

$ IPFS_GATEWAY=https://ipfs.io curl ipfs://bafkreicysg23kiwv34eg2d7qweipxwosdo2py4ldv42nbauguluen5v6am
hello

(B) Small endorsement of ipfs.io

If we want to show people "quick fix" option:

$ curl ipfs://bafkreicysg23kiwv34eg2d7qweipxwosdo2py4ldv42nbauguluen5v6am
Error: local gateway not found and/or IPFS_GATEWAY is not set. 
Try: IPFS_GATEWAY=https://ipfs.io
or run your own: https://docs.ipfs.tech/install/command-line/

@markg85
Copy link
Contributor Author

markg85 commented Aug 2, 2022

Any update here?
I'd like to finalize this patch and like to know what still needs to be done to get it in that shape.

The things i know i'm waiting on:

  • How/if a fallback is done. If not at all (option a from this post) then the current patch reflects this. I prefer to go for option b though.
  • If the redirect option as-is in the current patch is OK?

I probably do still have to make some test cases. Not exactly sure how that's supposed to work but i'll figure that out of the functionality is agreed upon.

Is there anything i'm missing?

@bagder
Copy link
Member

bagder commented Aug 2, 2022

* How/if a fallback is done. If not at all (option a [from this post](https://github.com/curl/curl/pull/8805#issuecomment-1199427911)) then the current patch reflects this. I prefer to go for option b though.

I would like any URLs mentioned for further documentation to go to curl.se rather than an externally controlled site/domain. That page may then refer to other sites etc, as then we can update accordingly in the future when we feel the need to.

* If the redirect option as-is in the current patch is OK?

I don't understand why you (as in the IPFS community) consider a simple HTTP 30x-redirect a "gateway" at all. That's just a bounce-off away to another host (that runs the real gateway), and doing that without users being fully aware of it feels borderline disingenuous.

So no, I don't think curl should follow such redirects by default. It is almost exactly the same situation as with other HTTP redirects.

I probably do still have to make some test cases. Not exactly sure how that's supposed to work but i'll figure that out of the functionality is agreed upon.

Is there anything i'm missing?

Docs. I think maybe docs/URL-SYNTAX.md could use an IPFS section that describes how it works.

bagder added a commit to curl/curl-www that referenced this pull request Sep 20, 2023
This makes docs/IPFS.md from the source tree into the docs/ipfs.html
page on the website.

Requires curl/curl#8805
docs/IPFS.md Show resolved Hide resolved
docs/IPFS.md Outdated Show resolved Hide resolved
docs/IPFS.md Outdated Show resolved Hide resolved
@markg85
Copy link
Contributor Author

markg85 commented Sep 22, 2023

Thank you for the feedback @lidel Will get that spell bot to play nice on it too ;)

- ipfs://<cid>
- ipns://<cid>

This allows you tu use ipfs in curl like:
curl ipfs://<cid>
and
curl ipns://<cid>

For more information consult the readme at:
https://curl.se/docs/ipfs.html
@bagder
Copy link
Member

bagder commented Sep 23, 2023

🎉 it is merged! Thanks for your hard work @markg85

bagder added a commit to curl/curl-www that referenced this pull request Sep 23, 2023
This makes docs/IPFS.md from the source tree into the docs/ipfs.html
page on the website.

Requires curl/curl#8805
bagder added a commit to curl/curl-www that referenced this pull request Sep 23, 2023
This makes docs/IPFS.md from the source tree into the docs/ipfs.html
page on the website.

Requires curl/curl#8805

Closes #298
@markg85
Copy link
Contributor Author

markg85 commented Sep 23, 2023

That's so awesome @bagder ! :D Thank you!
Can't wait for that that next curl release, that's gonna be a good one :)

ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
- ipfs://<cid>
- ipns://<cid>

This allows you tu use ipfs in curl like:
curl ipfs://<cid>
and
curl ipns://<cid>

For more information consult the readme at:
https://curl.se/docs/ipfs.html

Closes curl#8805
dfandrich added a commit that referenced this pull request Sep 25, 2023
Hard-coding the log directory name fails with parallel tests.

Follow-up to 65b563a

Ref: #8805
@bitplane
Copy link

bitplane commented Oct 3, 2023

Does this make use of an env variable for the gateway URL? I couldn't see it on the docs site. If it did then it'd be useable in all the tools and scripts that use curl under the hood by simply setting the variable beforehand, and as there's 2 million commits on GitHub that reference curl so that's IPFS in a lot of places. Is there a standard variable for the IPFS gateway?

@markg85
Copy link
Contributor Author

markg85 commented Oct 3, 2023

Does this make use of an env variable for the gateway URL? I couldn't see it on the docs site. If it did then it'd be useable in all the tools and scripts that use curl under the hood by simply setting the variable beforehand, and as there's 2 million commits on GitHub that reference curl so that's IPFS in a lot of places. Is there a standard variable for the IPFS gateway?

Yes!
See https://curl.se/docs/ipfs.html Anything containing IPFS_GATEWAY is what you're looking for. Set that environment variable to a gateway and you're good to go.

@bitplane
Copy link

bitplane commented Oct 3, 2023

Sorry! I was on my phone and find in page mustn't have been working. This is a great addition to curl, kudos! Next step wget, requests, http.client? :)

@markg85
Copy link
Contributor Author

markg85 commented Oct 3, 2023

@bitplane One of these: https://github.com/ipfs/integrations/issues
If you have others applications where you think IPFS support makes sense, add an issue there.

@marek22k
Copy link

marek22k commented Oct 4, 2023

It is said that the authenticity of the response can be verified by a hash. Does curl do this as well?

@markg85
Copy link
Contributor Author

markg85 commented Oct 4, 2023

It is said that the authenticity of the response can be verified by a hash. Does curl do this as well?

No.

Curl just gets the data from IPFS (through a gateway).
If you use a local gateway then that data is already "automatically verified" just by how a local gateway gets the data.

If you use a public gateway then you don't have this guarantee because you don't know what's running behind the gateway. Then you're in CAR land where you can verify the data is as you'd expect it to be, this explains it.

You should use a local gateway!

@marek22k
Copy link

marek22k commented Oct 4, 2023

Then why is there a "Verifiable responses" section? That sounds a lot like curl verifying that for more. Maybe you should write that clearly there?

@markg85
Copy link
Contributor Author

markg85 commented Oct 4, 2023

Then why is there a "Verifiable responses" section?

It literally says:

the user is able to fetch raw content-addressed data and perform hash verification themselves.

With that clickable link included to explain how to do just that. Care to elaborate what's not clear about that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration cmdline tool feature-window A merge of this requires an open feature window tests
Development

Successfully merging this pull request may close these issues.

None yet