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.

Sign up for GitHub

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

Jump to bottom

http: configurable allowed request methods for the API. #190

Merged
merged 1 commit into from
Apr 3, 2020
Merged

http: configurable allowed request methods for the API. #190

merged 1 commit into from
Apr 3, 2020

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Apr 2, 2020

Effectively enforce that the IPFS API is an RPC API that only handles POST requests.

@hsanjuan hsanjuan requested review from lidel and Stebalien April 2, 2020 19:39
@hsanjuan hsanjuan self-assigned this Apr 2, 2020
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 2, 2020

I need to fix tests... tomorrow...

Stebalien
Stebalien reviewed Apr 2, 2020
View reviewed changes
http/handler.go Outdated Show resolved Hide resolved
Stebalien
Stebalien requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I don't think we can do this here because we still want to support GET for read-only requests on the read-only API. Let's do this in go-ipfs itself.

Stebalien
Stebalien requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I don't think we can do this here because we still want to support GET for read-only requests on the read-only API. Let's do this in go-ipfs itself.

Stebalien
Stebalien requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I don't think we can do this here because we still want to support GET for read-only requests on the read-only API. Let's do this in go-ipfs itself.

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 2, 2020 •
edited
Loading

I don't think we can do this here because we still want to support GET for read-only requests on the read-only API. Let's do this in go-ipfs itself.

I am going to argue against this. This causes ambiguity between two APIs which are the same. Tools written to work on the gateway endpoint may not work on the API endpoint even though they are supposed to be using the same API.

More importantly, it does not fully fix our current problem. The fact that this is a Read-Only API does not mean that I wish to give anyone capabilities to, for example, pin ls. Apart from privacy, it is a DDoS vector. More worryingly, afaik you can use the Read-only API to cat, which efectivelly means making the node retrieve any content whatsoever and write it to disk.

Reconsider please?

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 3, 2020

More worryingly, afaik you can use the Read-only API to cat, which efectivelly means making the node retrieve any content whatsoever and write it to disk.

After sleeping, I realize that this runs on the gateway endpoint anyway so it should not matter as that is effectively cat'ing too. Still we have seen people complain that it is not obvious that a read-only API is running side-by-side to the gateway, and thus may lead to situations where people forget to secure these paths like they would secure /ipns /ipfs when putting behind nginx etc. The other issues still stand I think... at least in some setups.

@Stebalien
Copy link
Member

We really need to:

  1. Flesh out the gateway documentation.
  2. Have a flag to disable this.

@Stebalien
Copy link
Member

Discussed async: make make this an option.

@hsanjuan hsanjuan changed the title http: allow only POST requests to the API. http: configurable allowed request methods for the API. Apr 3, 2020
hsanjuan
hsanjuan commented Apr 3, 2020
View reviewed changes
http/config.go
Comment on lines +155 to +157
// For very small slices as these, this should be faster than
// a map lookup.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone did the numbers https://boltandnuts.wordpress.com/2017/11/20/go-slice-vs-maps/

Copy link
Member

Choose a reason for hiding this comment

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

Be careful. Go is constantly changing.

But this isn't performance critical anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Result on go 1.13 mapping string -> bool:

  • 1 item - array is faster.
  • 2 items - same time.
  • 3 items - map is faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 3, 2020

The fact that this is a Read-Only API does not mean that I wish to give anyone capabilities to, for example, pin ls.

For anyone following along, I misread: it's ls capabilities, not pin ls. The functionality offered by the read-only simply amounts to what the gateway offers already but with a more specialized tooling. Thus we can keep it.

@hsanjuan hsanjuan requested a review from Stebalien April 3, 2020 22:02
Stebalien
Stebalien approved these changes Apr 3, 2020
View reviewed changes
http/config.go
Comment on lines +155 to +157
// For very small slices as these, this should be faster than
// a map lookup.
Copy link
Member

Choose a reason for hiding this comment

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

Be careful. Go is constantly changing.

But this isn't performance critical anyways.

http/config.go Show resolved Hide resolved
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 3, 2020

Bah let me add a test

@hsanjuan
Http API: introduced HandledMethods option.
2fbebbe 
This Handler option is used it to allow/deny request by their method.

The API is an RPC API so it normally should only work with PUT, but we also
use a read-only, GET-based partial API that runs with the gateway.

This commit makes the actual allowed (or handled) methods configurable.

A test is added, and test facilities improved to be able to set/modify
this option.
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 3, 2020

I have squashed and improved the commit msg.

@hsanjuan hsanjuan requested a review from Stebalien April 3, 2020 23:00
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 3, 2020

@Stebalien technically you should review new changes to _test files. Merge when ready. I have a go-ipfs PR ready for when this can bubble, can send tomorrow.

@Stebalien Stebalien merged commit 90182ce into master Apr 3, 2020
@Stebalien Stebalien deleted the fix/api-post branch April 3, 2020 23:51
@lidel lidel mentioned this pull request Apr 4, 2020
Merged
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@lidel lidel Awaiting requested review from lidel

Assignees

@hsanjuan hsanjuan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hsanjuan @Stebalien