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

Make peer test more strict #38

Closed
lidel opened this issue Feb 20, 2021 · 0 comments · Fixed by #40
Closed

Make peer test more strict #38

lidel opened this issue Feb 20, 2021 · 0 comments · Fixed by #40
Assignees
@lidel
Labels
kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP

Comments

@lidel
Copy link
Member

lidel commented Feb 20, 2021 •
edited
Loading

Lack of /p2p/{libp2p-key} in /dnsaddr makes peerMultiaddr test flaky:

  • We return true for /dnsaddr without /p2p/{peerid} but it does not work outside of ipfs swarm connect and is not really secure/reliable:
    • No way to tell if DNS TXT record with key exist at all (could be invalid addr)
    • No way to protect against MITM - someone could swap key in DNS response
  • This is especially troublesome in context of origins and delegates in https://ipfs.github.io/pinning-services-api-spec/ – caused issues because Pinata returned /dnsaddr without suffix, and go-ipfs returned Error: invalid p2p multiaddr
  • While handy for swarm connect, I believe its way safer for everyone if we harden the check and require /p2p suffix with key at all times.
@lidel lidel added kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP labels Feb 20, 2021
@lidel lidel self-assigned this Feb 20, 2021
lidel added a commit that referenced this issue Mar 3, 2021
@lidel
fix(peerMultiaddr): require /p2p/{key}
32b6b0d 
Closes #38

BREAKING CHANGE: /dnsaddr without explicit /p2p/{key} is no longer a
valid peer multiaddr. See
#38 for rationale why.
@lidel lidel mentioned this issue Mar 3, 2021
Merged
@lidel lidel closed this as completed in #40 Mar 3, 2021
lidel added a commit that referenced this issue Mar 3, 2021
@lidel
fix(peerMultiaddr): require /p2p/{key} (#40)
25a2436 
Closes #38

BREAKING CHANGE: /dnsaddr without explicit /p2p/{key} is no longer a
valid peer multiaddr. See #38 for rationale why.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lidel lidel

Labels
kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(peerMultiaddr): require /p2p/{key} ipfs-shipyard/is-ipfs
1 participant
@lidel