identify protocol could close connection if remote peer sends invalid public key

[jameshiew]

Description

Remote peer shouldn't ever be providing a public key that doesn't match their peer ID, in this case could close the connection

I haven't tested but it looks like this is what js-libp2p does, for example (https://github.com/libp2p/js-libp2p/blob/d19974d93a1015acfca95c2155dbcffc5fd6a6c0/packages/protocol-identify/src/identify.ts#L106-L108)

Motivation

See discussion at * #5707 (comment)

Current Implementation

identify protocol connection to remote peer stays open

Are you planning to do it yourself in a pull request ?

Maybe

[drHuangMHT]

The specs doesn't seem to have any information about what to do when there is a pubkey mismatch. An event of it can be emitted though.

[dariusc93]

The specs doesn't seem to have any information about what to do when there is a pubkey mismatch. An event of it can be emitted though.

Thats a good point. Maybe we can add an event for some type of mismatch and let the node itself handle the disconnection (or whatever it wish to do with the peer in question) while we take the question to spec on what to do on a public key mismatch?

[jxs]

I'd suggest we bubble up this discussion with a PR to the specs, so that the decision is transversal to all the implementations.

[dariusc93]

For now, at least until it is determined in the specs, I think we should propagate the error to the behaviour and emit it to swarm with the connection id and let the developer decide on what to do with the connection. We could even go as far preventing the connection handler from accepting any additional messages on such error since I cannot imagine in any use-case where it would be acceptable to accept any additional identify messages with a mis-match public key.

Thoughts?

CC @jxs @drHuangMHT

[jxs]

For now, at least until it is determined in the specs, I think we should propagate the error to the behaviour and emit it to swarm with the connection id and let the developer decide on what to do with the connection. We could even go as far preventing the connection handler from accepting any additional messages on such error since I cannot imagine in any use-case where it would be acceptable to accept any additional identify messages with a mis-match public key.

that on the other hand makes it so that by default nothing happens, as it is delegated to the user. Without thinking much I'd rather close the connection, is there a reason maintaining the connection after makes sense?

[elenaf9]

Seems like there is positive feedback on closing the connection in this scenario libp2p/specs#651 (comment), so I think we could follow up there with a PR that adds it to the specs, and then implement it here. @jameshiew would you be interested in doing that?

[jameshiew]

Hey @elenaf9 , I won't be able to work on this unfortunately, if someone else picks it up I'm happy to be involved in PRs though!

[dariusc93]

that on the other hand makes it so that by default nothing happens, as it is delegated to the user. Without thinking much I'd rather close the connection, is there a reason maintaining the connection after makes sense?

Dont believe so. Its moreso following the same pattern that the ping protocol does by sending the error to swarm and allowing the user to handle it with its own connection management. I guess in this case, it would make more sense to close the connection itself than maintaining it.