Penalize peers that send an invalid rpc request #6986
Conversation
| ConnectionEvent::ListenUpgradeError(e) => { | ||
| if matches!(e.error, RPCError::InvalidData(_)) { | ||
| // Peer is not complying with the protocol. Notify the application and disconnect. | ||
| let inbound_substream_id = self.current_inbound_substream_id; | ||
| self.current_inbound_substream_id.0 += 1; | ||
|
|
||
| self.events_out.push(HandlerEvent::Err(HandlerErr::Inbound { | ||
| id: inbound_substream_id, | ||
| proto: Protocol::DataColumnsByRange, // FIXME: replace this hardcoded protocol | ||
| error: e.error, | ||
| })); | ||
| self.shutdown(None); | ||
| } | ||
| } |
There was a problem hiding this comment.
// FIXME: replace this hardcoded protocol
The RPCError::InvalidData error is emitted by the inbound codec here. The problem I'm facing is that the handler cannot determine which protocol the invalid data belongs to. I think changing the type of InboundUpgrade::Error from RPCError to (Protocol, RPCError) might solves this. What do you think?
|
This pull request has merge conflicts. Could you please resolve them @ackintosh? 🙏 |
# Conflicts: # beacon_node/lighthouse_network/tests/rpc_tests.rs
|
Some required checks have failed. Could you please take a look @ackintosh? 🙏 |
| ConnectionEvent::ListenUpgradeError(e) => { | ||
| if matches!(e.error.1, RPCError::InvalidData(_)) { |
There was a problem hiding this comment.
you don't need the nested matches Akihito, you can do:
| ConnectionEvent::ListenUpgradeError(e) => { | |
| if matches!(e.error.1, RPCError::InvalidData(_)) { | |
| ConnectionEvent::ListenUpgradeError(ListenUpgradeError { | |
| error: (_, RPCError::IoError(_)), | |
| .. | |
| }) => { |
| @@ -652,7 +652,7 @@ where | |||
| E: EthSpec, | |||
| { | |||
| type Output = InboundOutput<TSocket, E>; | |||
| type Error = RPCError; | |||
| type Error = (Protocol, RPCError); | |||
There was a problem hiding this comment.
why use a tuple instead of putting the Protocol inside RPCError?
There was a problem hiding this comment.
I saw a struct that has Protocol and RPCError as separate fields like this, and I thought it was a reasonable way to represent what the error is and where it happened separately:
lighthouse/beacon_node/lighthouse_network/src/rpc/handler.rs
Lines 938 to 946 in c56c63e
Issue Addressed
Since #6847, invalid
BlocksByRange/BlobsByRangerequests, which do not comply with the spec, are handled in the Handler. Any peer that sends an invalid request is penalized and disconnected.However, other kinds of invalid rpc request, which result in decoding errors, are just dropped. No penalty is applied and the connection with the peer remains.
Proposed Changes
I have added handling for the
ListenUpgradeErrorevent to notify the application of anRPCError:InvalidDataerror and disconnect to the peer that sent the invalid rpc request.I also added tests for handling invalid rpc requests.