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

Resolution commands rework #5635

Closed
Stebalien opened this issue Oct 23, 2018 · 4 comments · Fixed by #6087
Closed

Resolution commands rework #5635

Stebalien opened this issue Oct 23, 2018 · 4 comments · Fixed by #6087
Assignees

Comments

@Stebalien
Copy link
Member

  1. ipfs name resolve and ipfs dns should be recursive by default, the current behavior is a surprising foot-gun. We should consider just changing this as I'm sure it'll fix significantly more bugs than it causes.
  2. ipfs dns should resolve up to the first non-dnslink path by default. It shouldn't return an error when it hits a non-dnslink path, it should just return that path.

I'd also like to return a partial result when we hit the recursion limit (along with the error) but that's less pressing.


Can anyone think of anything this would break?

@alanshaw, how will this affect JS? Is this doable?

@whyrusleeping any idea why we aren't recursive by default?

@lidel
Copy link
Member

lidel commented Oct 23, 2018

Can anyone think of anything this would break?

👍 for recursive by default, but given the fact we expose both commands on our public gateways, we should also implement recursion limit enabled by default (or even as a hard limit), otherwise people can easily DOS us by requesting lookup for circular IPNS links.

cc #4293 and #4181 where Lars suggested following limits from http tools:

Agreed -- probably with a sensible recursion limit (10 or so), similar to what curl/wget would do for redirects.

For the record, wget follows up to 20 redirects by default:

--max-redirect=number Specifies the maximum number of redirections to follow for a resource. The default is 20, which is usually far more than necessary. However, on those occasions where you want to allow more (or fewer), this is the option to use.
https://www.gnu.org/software/wget/manual/html_node/HTTP-Options.html

Curl limit is 50:

--max-redirs <num> Set maximum number of redirection-followings allowed. If -L/--location is used, this option can be used to prevent curl from following redirections "in absurdum". By default, the limit is set to 50 redirections. Set this option to -1 to make it limitless.
https://linux.die.net/man/1/curl

@alanshaw
Copy link
Member

Same for ipfs.resolve?

@alanshaw
Copy link
Member

I've opened an issue on the interface repo so we can track this: https://github.com/ipfs/interface-ipfs-core/issues/377

@Stebalien
Copy link
Member Author

I agree, a recursion limit is a must. @whyrusleeping didn't object and can't remember why we didn't make this recursive by default either.

Note: Users can still recursively resolve using ipfs cat, etc. so this shouldn't change much from a security standpoint.

Stebalien added a commit that referenced this issue Mar 16, 2019
This is what users expect.

fixes #5635
fixes #5585
fixes #4181
fixes #4293
fixes #6086

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@ghost ghost assigned Stebalien Mar 16, 2019
@ghost ghost added the status/in-progress In progress label Mar 16, 2019
@ghost ghost removed the status/in-progress In progress label Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants