Skip to content

Commit 077caa6

Browse files
lidellidel
authored
fix(bitswap): ignore identity CIDs instead of killing connection (#1117)
* fix(bitswap): ignore identity CIDs instead of killing connection third-party implementations may not be aware that identity CIDs should not appear in Bitswap wantlists, and their users should not be punished with dropped connections for naive CID handling. unify behavior with oversized CIDs to reduce connection churn: silently ignore and continue processing the rest of the message. - engine.go: replace error return with debug log + continue - engine_test.go: flip assertions to verify no disconnect, check identity CID is silently dropped from wantlist * docs: add changelog entry for bitswap identity CID fix * test(bitswap): improve identity CID test coverage - restructure into subtests covering want-block, want-have, cancel, and all-identity-CIDs edge case - verify the specific CID that remains, not just the count - verify identity cancel does not corrupt existing wantlist entries - use t.Context() instead of context.Background() * test(bitswap): improve oversized CID test coverage - restructure into subtests covering want-block, want-have, cancel, and all-oversized edge case - verify the specific CID that remains, not just the count - verify oversized cancel does not corrupt existing wantlist entries
1 parent 942b497 commit 077caa6

File tree

3 files changed

+230
-52
lines changed

3 files changed

+230
-52
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ The following emojis are used to highlight certain changes:
3030

3131
### Fixed
3232

33+
- `bitswap/server`: incoming identity CIDs in wantlist messages are now silently ignored instead of killing the connection to the remote peer. Some IPFS implementations naively send identity CIDs, and disconnecting them for it caused unnecessary churn. [#1117](https://github.com/ipfs/boxo/pull/1117)
34+
3335
### Security
3436

3537

‎bitswap/server/internal/decision/engine.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ package decision
44
import (
55
"cmp"
66
"context"
7-
"errors"
87
"fmt"
98
"slices"
109
"sync"
@@ -889,7 +888,12 @@ func (e *Engine) splitWantsCancelsDenials(p peer.ID, m bsmsg.BitSwapMessage) ([]
889888
continue
890889
}
891890
if c.Prefix().MhType == mh.IDENTITY {
892-
return nil, nil, nil, errors.New("peer canceled an identity CID")
891+
// Ignore identity CIDs rather than killing the connection.
892+
// Some implementations naively send these without malicious
893+
// intent, and peers don't update fast enough to justify
894+
// disconnecting them.
895+
log.Debugw("ignoring identity CID in wantlist", "local", e.self, "from", p, "cid", c)
896+
continue
893897
}
894898

895899
if et.Cancel {

‎bitswap/server/internal/decision/engine_test.go‎

Lines changed: 222 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1644,74 +1644,246 @@ func TestWantlistGrowsToLimit(t *testing.T) {
16441644
}
16451645
}
16461646

1647+
// TestIgnoresCidsAboveLimit verifies that CIDs exceeding MaxCidSize are
1648+
// silently dropped from wantlist messages without killing the connection.
16471649
func TestIgnoresCidsAboveLimit(t *testing.T) {
16481650
const cidLimit = 64
1649-
warsaw := newTestEngine("warsaw", WithMaxCidSize(cidLimit))
1650-
defer warsaw.Engine.Close()
1651-
riga := newTestEngine("riga")
1652-
defer riga.Engine.Close()
16531651

1654-
// Send in two messages to test reslicing.
1655-
m := message.New(true)
1652+
// mkOversizedCid builds a CIDv1 whose total byte length exceeds cidLimit.
1653+
mkOversizedCid := func() cid.Cid {
1654+
var hash mh.Multihash
1655+
hash = binary.AppendUvarint(hash, mh.BLAKE3)
1656+
hash = binary.AppendUvarint(hash, cidLimit)
1657+
startOfDigest := len(hash)
1658+
hash = append(hash, make(mh.Multihash, cidLimit)...)
1659+
rand.Read(hash[startOfDigest:])
1660+
return cid.NewCidV1(cid.Raw, hash)
1661+
}
1662+
1663+
t.Run("want-block filters oversized and keeps legitimate CID", func(t *testing.T) {
1664+
warsaw := newTestEngine("warsaw", WithMaxCidSize(cidLimit))
1665+
defer warsaw.Engine.Close()
1666+
riga := newTestEngine("riga")
1667+
defer riga.Engine.Close()
1668+
1669+
legitimateCid := blocks.NewBlock([]byte("legitimate")).Cid()
1670+
m := message.New(true)
1671+
m.AddEntry(legitimateCid, 0, pb.Message_Wantlist_Block, true)
1672+
m.AddEntry(mkOversizedCid(), 0, pb.Message_Wantlist_Block, true)
1673+
1674+
if warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m) {
1675+
t.Fatal("connection should not be killed for oversized CIDs")
1676+
}
16561677

1657-
m.AddEntry(blocks.NewBlock([]byte("Hæ")).Cid(), 0, pb.Message_Wantlist_Block, true)
1678+
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1679+
if len(wl) != 1 {
1680+
t.Fatalf("expected 1 entry in wantlist, got %d", len(wl))
1681+
}
1682+
if wl[0].Cid != legitimateCid {
1683+
t.Fatalf("expected legitimate CID %s in wantlist, got %s", legitimateCid, wl[0].Cid)
1684+
}
1685+
})
16581686

1659-
var hash mh.Multihash
1660-
hash = binary.AppendUvarint(hash, mh.BLAKE3)
1661-
hash = binary.AppendUvarint(hash, cidLimit)
1662-
startOfDigest := len(hash)
1663-
hash = append(hash, make(mh.Multihash, cidLimit)...)
1664-
rand.Read(hash[startOfDigest:])
1665-
m.AddEntry(cid.NewCidV1(cid.Raw, hash), 0, pb.Message_Wantlist_Block, true)
1687+
// want-have entries take a separate code path (haveKs) in MessageReceived.
1688+
t.Run("want-have filters oversized and keeps legitimate CID", func(t *testing.T) {
1689+
warsaw := newTestEngine("warsaw", WithMaxCidSize(cidLimit))
1690+
defer warsaw.Engine.Close()
1691+
riga := newTestEngine("riga")
1692+
defer riga.Engine.Close()
16661693

1667-
warsaw.Engine.MessageReceived(context.Background(), riga.Peer, m)
1694+
legitimateCid := blocks.NewBlock([]byte("legitimate-have")).Cid()
1695+
m := message.New(true)
1696+
m.AddEntry(legitimateCid, 0, pb.Message_Wantlist_Have, true)
1697+
m.AddEntry(mkOversizedCid(), 0, pb.Message_Wantlist_Have, true)
16681698

1669-
if warsaw.Peer == riga.Peer {
1670-
t.Fatal("Sanity Check: Peers have same Key!")
1671-
}
1699+
if warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m) {
1700+
t.Fatal("connection should not be killed for oversized CIDs")
1701+
}
16721702

1673-
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1674-
if len(wl) != 1 {
1675-
t.Fatalf("expected 1 entry in wantlist, got %d", len(wl))
1676-
}
1703+
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1704+
if len(wl) != 1 {
1705+
t.Fatalf("expected 1 entry in wantlist, got %d", len(wl))
1706+
}
1707+
if wl[0].Cid != legitimateCid {
1708+
t.Fatalf("expected legitimate CID %s in wantlist, got %s", legitimateCid, wl[0].Cid)
1709+
}
1710+
})
1711+
1712+
t.Run("oversized cancel does not affect existing wantlist", func(t *testing.T) {
1713+
warsaw := newTestEngine("warsaw", WithMaxCidSize(cidLimit))
1714+
defer warsaw.Engine.Close()
1715+
riga := newTestEngine("riga")
1716+
defer riga.Engine.Close()
1717+
1718+
firstCid := blocks.NewBlock([]byte("first")).Cid()
1719+
m := message.New(true)
1720+
m.AddEntry(firstCid, 0, pb.Message_Wantlist_Block, true)
1721+
warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m)
1722+
1723+
// Send a second message: a new legitimate want + cancel of an oversized CID.
1724+
secondCid := blocks.NewBlock([]byte("second")).Cid()
1725+
m.Reset(false)
1726+
m.AddEntry(secondCid, 0, pb.Message_Wantlist_Block, true)
1727+
m.Cancel(mkOversizedCid())
1728+
1729+
if warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m) {
1730+
t.Fatal("connection should not be killed for oversized CID in cancel")
1731+
}
1732+
1733+
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1734+
if len(wl) != 2 {
1735+
t.Fatalf("expected 2 entries in wantlist, got %d", len(wl))
1736+
}
1737+
if !findCid(firstCid, wl) {
1738+
t.Fatal("first legitimate want was removed by oversized cancel")
1739+
}
1740+
if !findCid(secondCid, wl) {
1741+
t.Fatal("second legitimate want was not added")
1742+
}
1743+
})
1744+
1745+
// When every entry is oversized, splitWantsCancelsDenials returns
1746+
// all-empty slices. Verify the engine handles this without crashing.
1747+
t.Run("message with only oversized CIDs", func(t *testing.T) {
1748+
warsaw := newTestEngine("warsaw", WithMaxCidSize(cidLimit))
1749+
defer warsaw.Engine.Close()
1750+
riga := newTestEngine("riga")
1751+
defer riga.Engine.Close()
1752+
1753+
m := message.New(true)
1754+
m.AddEntry(mkOversizedCid(), 0, pb.Message_Wantlist_Block, true)
1755+
m.AddEntry(mkOversizedCid(), 0, pb.Message_Wantlist_Have, true)
1756+
1757+
if warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m) {
1758+
t.Fatal("connection should not be killed for oversized-only message")
1759+
}
1760+
1761+
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1762+
if len(wl) != 0 {
1763+
t.Fatalf("expected empty wantlist, got %d entries", len(wl))
1764+
}
1765+
})
16771766
}
16781767

1679-
func TestKillConnectionForInlineCid(t *testing.T) {
1680-
warsaw := newTestEngine("warsaw")
1681-
defer warsaw.Engine.Close()
1682-
riga := newTestEngine("riga")
1683-
defer riga.Engine.Close()
1768+
// TestIgnoresIdentityCid verifies that identity CIDs (multihash type IDENTITY)
1769+
// are silently dropped from wantlist messages instead of killing the peer
1770+
// connection. Some IPFS implementations naively send these.
1771+
func TestIgnoresIdentityCid(t *testing.T) {
1772+
// mkIdentityCid builds a CIDv1 with a random IDENTITY multihash.
1773+
mkIdentityCid := func() cid.Cid {
1774+
var hash mh.Multihash
1775+
hash = binary.AppendUvarint(hash, mh.IDENTITY)
1776+
const digestSize = 32
1777+
hash = binary.AppendUvarint(hash, digestSize)
1778+
startOfDigest := len(hash)
1779+
hash = append(hash, make(mh.Multihash, digestSize)...)
1780+
rand.Read(hash[startOfDigest:])
1781+
return cid.NewCidV1(cid.Raw, hash)
1782+
}
1783+
1784+
t.Run("want-block filters identity and keeps legitimate CID", func(t *testing.T) {
1785+
warsaw := newTestEngine("warsaw")
1786+
defer warsaw.Engine.Close()
1787+
riga := newTestEngine("riga")
1788+
defer riga.Engine.Close()
1789+
1790+
legitimateCid := blocks.NewBlock([]byte("legitimate")).Cid()
1791+
m := message.New(true)
1792+
m.AddEntry(legitimateCid, 0, pb.Message_Wantlist_Block, true)
1793+
m.AddEntry(mkIdentityCid(), 0, pb.Message_Wantlist_Block, true)
1794+
1795+
if warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m) {
1796+
t.Fatal("connection should not be killed for identity CIDs")
1797+
}
16841798

1685-
if warsaw.Peer == riga.Peer {
1686-
t.Fatal("Sanity Check: Peers have same Key!")
1687-
}
1799+
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1800+
if len(wl) != 1 {
1801+
t.Fatalf("expected 1 entry in wantlist, got %d", len(wl))
1802+
}
1803+
if wl[0].Cid != legitimateCid {
1804+
t.Fatalf("expected legitimate CID %s in wantlist, got %s", legitimateCid, wl[0].Cid)
1805+
}
1806+
})
16881807

1689-
// Send in two messages to test reslicing.
1690-
m := message.New(true)
1808+
// want-have entries take a separate code path (haveKs) in MessageReceived.
1809+
t.Run("want-have filters identity and keeps legitimate CID", func(t *testing.T) {
1810+
warsaw := newTestEngine("warsaw")
1811+
defer warsaw.Engine.Close()
1812+
riga := newTestEngine("riga")
1813+
defer riga.Engine.Close()
16911814

1692-
m.AddEntry(blocks.NewBlock([]byte("Hæ")).Cid(), 0, pb.Message_Wantlist_Block, true)
1815+
legitimateCid := blocks.NewBlock([]byte("legitimate-have")).Cid()
1816+
m := message.New(true)
1817+
m.AddEntry(legitimateCid, 0, pb.Message_Wantlist_Have, true)
1818+
m.AddEntry(mkIdentityCid(), 0, pb.Message_Wantlist_Have, true)
16931819

1694-
var hash mh.Multihash
1695-
hash = binary.AppendUvarint(hash, mh.IDENTITY)
1696-
const digestSize = 32
1697-
hash = binary.AppendUvarint(hash, digestSize)
1698-
startOfDigest := len(hash)
1699-
hash = append(hash, make(mh.Multihash, digestSize)...)
1700-
rand.Read(hash[startOfDigest:])
1701-
m.AddEntry(cid.NewCidV1(cid.Raw, hash), 0, pb.Message_Wantlist_Block, true)
1820+
if warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m) {
1821+
t.Fatal("connection should not be killed for identity CIDs")
1822+
}
17021823

1703-
if !warsaw.Engine.MessageReceived(context.Background(), riga.Peer, m) {
1704-
t.Fatal("connection was not killed when receiving inline in cancel")
1705-
}
1824+
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1825+
if len(wl) != 1 {
1826+
t.Fatalf("expected 1 entry in wantlist, got %d", len(wl))
1827+
}
1828+
if wl[0].Cid != legitimateCid {
1829+
t.Fatalf("expected legitimate CID %s in wantlist, got %s", legitimateCid, wl[0].Cid)
1830+
}
1831+
})
17061832

1707-
m.Reset(true)
1833+
t.Run("identity cancel does not affect existing wantlist", func(t *testing.T) {
1834+
warsaw := newTestEngine("warsaw")
1835+
defer warsaw.Engine.Close()
1836+
riga := newTestEngine("riga")
1837+
defer riga.Engine.Close()
1838+
1839+
firstCid := blocks.NewBlock([]byte("first")).Cid()
1840+
m := message.New(true)
1841+
m.AddEntry(firstCid, 0, pb.Message_Wantlist_Block, true)
1842+
warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m)
1843+
1844+
// Send a second message: a new legitimate want + cancel of an identity CID.
1845+
secondCid := blocks.NewBlock([]byte("second")).Cid()
1846+
m.Reset(false)
1847+
m.AddEntry(secondCid, 0, pb.Message_Wantlist_Block, true)
1848+
m.Cancel(mkIdentityCid())
1849+
1850+
if warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m) {
1851+
t.Fatal("connection should not be killed for identity CID in cancel")
1852+
}
17081853

1709-
m.AddEntry(blocks.NewBlock([]byte("Hæ")).Cid(), 0, pb.Message_Wantlist_Block, true)
1710-
m.Cancel(cid.NewCidV1(cid.Raw, hash))
1854+
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1855+
if len(wl) != 2 {
1856+
t.Fatalf("expected 2 entries in wantlist, got %d", len(wl))
1857+
}
1858+
if !findCid(firstCid, wl) {
1859+
t.Fatal("first legitimate want was removed by identity cancel")
1860+
}
1861+
if !findCid(secondCid, wl) {
1862+
t.Fatal("second legitimate want was not added")
1863+
}
1864+
})
17111865

1712-
if !warsaw.Engine.MessageReceived(context.Background(), riga.Peer, m) {
1713-
t.Fatal("connection was not killed when receiving inline in cancel")
1714-
}
1866+
// When every entry is an identity CID, splitWantsCancelsDenials returns
1867+
// all-empty slices. Verify the engine handles this without crashing.
1868+
t.Run("message with only identity CIDs", func(t *testing.T) {
1869+
warsaw := newTestEngine("warsaw")
1870+
defer warsaw.Engine.Close()
1871+
riga := newTestEngine("riga")
1872+
defer riga.Engine.Close()
1873+
1874+
m := message.New(true)
1875+
m.AddEntry(mkIdentityCid(), 0, pb.Message_Wantlist_Block, true)
1876+
m.AddEntry(mkIdentityCid(), 0, pb.Message_Wantlist_Have, true)
1877+
1878+
if warsaw.Engine.MessageReceived(t.Context(), riga.Peer, m) {
1879+
t.Fatal("connection should not be killed for identity-only message")
1880+
}
1881+
1882+
wl := warsaw.Engine.WantlistForPeer(riga.Peer)
1883+
if len(wl) != 0 {
1884+
t.Fatalf("expected empty wantlist, got %d entries", len(wl))
1885+
}
1886+
})
17151887
}
17161888

17171889
func TestWantlistBlocked(t *testing.T) {

0 commit comments

Comments
 (0)