identify: specify max message size

[lidel]

This PR proposes specifying max size of Identify message to unify behavior across implementations.

Limit of 4096 bytes is based on current value in rust-libp2p, but we can pick something else.
(I was not able to quickly find any limit in go-libp2p)

[marten-seemann]

Suggested change

	Implementations must reject `Identify` messages bigger than 4096 bytes.
	Implementations SHOULD NOT send `Identify` messages bigger than 4096 bytes.

[mxinden]

Proposal by @marten-seemann sounds good to me.

To add some rational.

• There might be valid use-cases where > 4096 bytes is a good idea. Thus using SHOULD. This would allow individual implementations to offer a flag to disable the check.
• Pushing the requirement to the sender instead of the receiver would follow the robustness princicple, i.e. be conservative of what one sends, but liberal in what one accepts. Though in the end, I don't think it matters much.

@lidel in addition of the above proposal, what do you think of mentioning the main motivation, resilience against DOS attacks. Something along the lines: "This limit allows receivers to drop messages exceeding the limit, thus better defend against DOS attacks."

[lidel]

sgtm, will apply

Suggested change

	Implementations must reject `Identify` messages bigger than 4096 bytes.
	Implementations SHOULD NOT send `Identify` messages bigger than 4096 bytes.
	This limit allows receivers to drop messages exceeding the limit, thus better defend against DOS attacks.

[mxinden]

Looks good to me from the rust-libp2p side. Thanks @lidel for the patch!

@Menduist what do you think for nim-libp2p?

@Nashatyrev what do you think for jvm-libp2p?

[mxinden]

Proposal by @marten-seemann sounds good to me.

To add some rational.

• There might be valid use-cases where > 4096 bytes is a good idea. Thus using SHOULD. This would allow individual implementations to offer a flag to disable the check.
• Pushing the requirement to the sender instead of the receiver would follow the robustness princicple, i.e. be conservative of what one sends, but liberal in what one accepts. Though in the end, I don't think it matters much.

@lidel in addition of the above proposal, what do you think of mentioning the main motivation, resilience against DOS attacks. Something along the lines: "This limit allows receivers to drop messages exceeding the limit, thus better defend against DOS attacks."

[Menduist]

LGTM, notes that this implies a limit on the SPR too

[mxinden]

Unless there are any objections, I will move forward here next week.

@lidel could you please bump the specification revision and accept @marten-seemann's suggestion above?

[lidel]

@mxinden done in e4cb42c :)

[Nashatyrev]

Just for the reference here is Jvm-libp2p commit addressing the issue: libp2p/jvm-libp2p@5e23467
We set the Identify message size limit to 1Mb there.

May be 4Kb is too restrictive for general use? There could be applications with a lot of supported protocols which wouldn't fit to Identify.protocols field

From my perspective it's important to bound the size with any reasonable value. So may be 1Mb would be as safe as 4Kb.

[Nashatyrev]

E.g. Ethereum has just 6 RPC methods, but each of them

• is identified by ~64 symbol protocol name
• has several versions (distinct protocol for each version)
• during hardforks the total number of protocols might increase x2 (as the protocol name incorporates fork identifier)

So even now the protocols field could be as large as 2Kb.
After adding 2-3 new method versions it could exceed 4Kb

I see that 4Kb is kind of 'recommendation' but just a bit worried that this limit could be hardcoded to some Libp2p implementation and could just stop working at some point.
(upd): like it's currently done in Rust :)

[Winterhuman]

Maybe the maximum could be 16KiB to mimic the IPNS record max size? If IPNS is safe using 16KiB then it seems it should be a safe value in general

[mxinden]

I see that 4Kb is kind of 'recommendation' but just a bit worried that this limit could be hardcoded to some Libp2p implementation and could just stop working at some point.

I doubt we will find a value that will fit all use-cases. Thus agreed that this is a recommendation and should be configurable by implementations.

Thanks for sharing details for the Ethereum network.

@Nashatyrev unless you feel strongly about bumping this value, I suggest we proceed here.

[Nashatyrev]

@Nashatyrev unless you feel strongly about bumping this value, I suggest we proceed here.

I'm totally fine with this 👍

[marten-seemann]

I disagree that setting a small message size is the way to go. This has already led us to implement a chunked encoding (https://github.com/libp2p/go-libp2p/pull/958/files). I would have hoped that the days of chunked encodings were behind us...

Regarding DoS defense, there's little advantage to splitting up a large message into multiple smaller ones. Yes, you get incremental verifiability, but this hardly counts as an advantage when we're dealing with chunks of a few kB.

Really, if we actually think that the maximum of data we can accept without risking exposing ourselves to a DoS attack is that close to sizes that are actually useful for the protocol, there's something wrong with the protocol (I do think that Identify is a bloated protocol, but that's a different story).
We need to acknowledge: Shipping lists of supported protocols and lists of multiaddrs takes up space. A DoS protection limit should be set such that well-behaved implementations don't have to worry about running into this limit. DoS limits exist to deter malicious nodes, not to make the life of well-behaved nodes miserable.

Looking at memory consumption of typical a libp2p connection, a size limit of 4 kB seems extreme stingy. We allow stream muxers to consume up to 15 MB in both go-libp2p and rust-libp2p. A memory limit of 128 - 256 kB per Identify wouldn't be crazy, if the number of concurrent Identify streams is limited.