Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

ipfs resolve fails when dnslink points to another dnslink #3692

Closed
olizilla opened this issue May 17, 2021 · 13 comments · Fixed by #3707
Closed

ipfs resolve fails when dnslink points to another dnslink #3692

olizilla opened this issue May 17, 2021 · 13 comments · Fixed by #3707
Assignees
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/in-progress In progress

Comments

@olizilla
Copy link
Member

ipfs resolve isn't happy about dnslinks that point to other domains.

$ dig +short txt _dnslink.ipfs.io
"dnslink=/ipns/website.ipfs.io"

$ iim use js
✔ selected js-ipfs version 0.55.1

$ ipfs resolve /ipns/ipfs.io
invalid character '.' in 'website.ipfs.io'
@olizilla olizilla added the need/triage Needs initial labeling and prioritization label May 17, 2021
@olizilla
Copy link
Member Author

Here ipfs resolve short-cuts where it is given an name and tries to return the response from ipfs dns rather than fully resolving

// lets check if we have a domain ex. /ipns/ipfs.io and resolve with dns
if (isDomain(hash)) {
yield appendRemainder(await dns(hash, options), remainder)
return
}

olizilla added a commit to olizilla/js-ipfs-fetch that referenced this issue May 17, 2021
- make POST return a base32 encoded cidv1.
- default to using cidv1 everywhere.
- update to use the new ipfs-core module.

base36 is the right thing to use to respect the max authority length for ipns:// scheme urls.
The assumption is folks should use base32 for CIDs in ipfs:// scheme urls, and it's the default output for cidv1.

The new ipfs-core modules has the same api as the `ipfs` module but doesn't include the cli and http-server code with it.

At time of writing the tests pass apart from the last one, which hangs. I think it's due to an unrelated bug ipfs/js-ipfs#3692

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@lidel lidel added P0 Critical: Tackled by core team ASAP kind/bug A bug in existing code (including security flaws) effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful and removed need/triage Needs initial labeling and prioritization labels May 17, 2021
@lidel
Copy link
Member

lidel commented May 17, 2021

Ouch, ipfs resolve /ipns/ipfs.io failing is pretty bad, that is an example for core feature referred from docs/forums/SO etc :( – for this reason I'm marking this as P0.
@olizilla will you have time to PR fix? If not we should add this ahead of the tribute que for @hugomrdias this week.

@olizilla olizilla self-assigned this May 18, 2021
@olizilla
Copy link
Member Author

I can PR a fix for this

@olizilla
Copy link
Member Author

Weird, we have a core interface test for exactly this address

// Test resolve turns /ipns/domain.com into /ipfs/QmHash
it('should resolve an IPNS DNS link', async function () {
this.retries(3)
const resolved = await ipfs.resolve('/ipns/ipfs.io')
expect(isIpfs.ipfsPath(resolved)).to.be.true()
})

@olizilla
Copy link
Member Author

olizilla commented May 18, 2021

something more mysterious is happening here... the test passes, and the dns command does indeed handle recursive look ups by default

❯ npx aegir test -f test/interface-core.js -t node 
Test Node.js


  interface-ipfs-core tests
    .resolve
resolved /ipfs/bafybeidcwrcmqbzm557vyomf2evvo4zez2uc5qfbajvbhilm62vvvfmmui
      ✓ should resolve an IPNS DNS link (94ms)

async function recursiveResolveDnslink (domain, depth) {
if (depth === 0) {
throw errcode(new Error('recursion limit exceeded'), 'ERR_DNSLINK_RECURSION_LIMIT')
}
let dnslinkRecord
try {
dnslinkRecord = await resolveDnslink(domain)
} catch (err) {
// If the code is not ENOTFOUND or ERR_DNSLINK_NOT_FOUND or ENODATA then throw the error
if (err.code !== 'ENOTFOUND' && err.code !== 'ERR_DNSLINK_NOT_FOUND' && err.code !== 'ENODATA') {
throw err
}
if (domain.startsWith('_dnslink.')) {
// The supplied domain contains a _dnslink component
// Check the non-_dnslink domain
dnslinkRecord = await resolveDnslink(domain.replace('_dnslink.', ''))
} else {
// Check the _dnslink subdomain
const _dnslinkDomain = `_dnslink.${domain}`
// If this throws then we propagate the error
dnslinkRecord = await resolveDnslink(_dnslinkDomain)
}
}
const result = dnslinkRecord.replace('dnslink=', '')
const domainOrCID = result.split('/')[2]
const isIPFSCID = isIPFS.cid(domainOrCID)
if (isIPFSCID || !depth) {
return result
}
return recursiveResolveDnslink(domainOrCID, depth - 1)

@olizilla
Copy link
Member Author

the ipfs resolve /ipns/ipfs.io command fails, yet the ipfs dns ipfs.io command succeeds...

~/Code/ipfs/js-ipfs/packages/ipfs on master •
❯ /Users/oli/.iim/dists/js-ipfs@0.55.1/ipfs resolve /ipns/ipfs.io                                                         16:32:30
invalid character '.' in 'website.ipfs.io'

~/Code/ipfs/js-ipfs/packages/ipfs on master •
❯ /Users/oli/.iim/dists/js-ipfs@0.55.1/ipfs dns ipfs.io                                                                   16:32:37
/ipfs/bafybeidcwrcmqbzm557vyomf2evvo4zez2uc5qfbajvbhilm62vvvfmmui

@olizilla
Copy link
Member Author

the ipfs-cli does nothing but hand off to the ipfs instance which i assume is a ipfs-core instance

async handler ({ ctx: { print, ipfs }, name, recursive, cidBase, timeout }) {
const res = await ipfs.resolve(name, { recursive, cidBase, timeout })
print(stripControlCharacters(res))
}

@olizilla
Copy link
Member Author

ohhhh, the cli command is not recursive by default!

command: 'resolve <name>',
description: 'Resolve the value of names to IPFS',
builder: {
recursive: {
alias: 'r',
type: 'boolean',
default: false
},

it probably should be.

@olizilla
Copy link
Member Author

The error message where recursive is false but a dnslink points to another dnslink needs improving here

@lidel
Copy link
Member

lidel commented May 22, 2021

Hm.. you are right. ipfs resolve in go-ipfs was flipped to recursive by default some time ago (~2019?), js-ipfs should do the same. Ref. ipfs/kubo#6087

@lidel lidel added this to Weekly Candidates in Maintenance Priorities - JS May 24, 2021
@lidel
Copy link
Member

lidel commented May 24, 2021

@olizilla is this still on your plate, or should we unassign so this can be picked up by JS tribute as part of https://github.com/orgs/ipfs/projects/13#card-61290478 ?

This is ready to be picked up by a JS triage tribute.

@lidel
Copy link
Member

lidel commented Jun 7, 2021

@yusefnapora (note from JS triage) you may want to pick up this first, before anything else (P0)

@BigLep BigLep moved this from Weekly Candidates to In Review in Maintenance Priorities - JS Jun 14, 2021
@lidel lidel added the status/in-progress In progress label Jun 14, 2021
@lidel lidel moved this from In Review to In Progress in Maintenance Priorities - JS Jun 25, 2021
@lidel
Copy link
Member

lidel commented Jul 2, 2021

Note from triage:

  • need CI/tests fix
  • can be picked up by tribute or wait until @yusefnapora is back in rotation

@lidel lidel removed the P0 Critical: Tackled by core team ASAP label Jul 2, 2021
@lidel lidel added the P2 Medium: Good to have, but can wait until someone steps up label Jul 2, 2021
@BigLep BigLep moved this from In Progress to Parked/Blocked in Maintenance Priorities - JS Jul 2, 2021
@lidel lidel moved this from Parked/Blocked to In Review in Maintenance Priorities - JS Jul 16, 2021
@lidel lidel moved this from In Review to In Progress in Maintenance Priorities - JS Jul 16, 2021
Maintenance Priorities - JS automation moved this from In Progress to Done Jul 19, 2021
achingbrain added a commit that referenced this issue Jul 19, 2021
This changes the default behaviour of the `jsipfs resolve` cli command to be recursive by default. Closes #3692.

```shell
# in packages/ipfs
node src/cli.js resolve /ipns/ipfs.io
/ipfs/bafybeiagozluzfopjadeigrjlsmktseozde2xc5prvighob7452imnk76a
```

BREAKING CHANGE: resolve is now recursive by default

Co-authored-by: Alex Potsides <alex@achingbrain.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/in-progress In progress
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants