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.

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

richness for breach of protocol reasons #6925

Merged
merged 6 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ public void processMessage(final Capability cap, final Message message) {
.addArgument(code)
.addArgument(ethPeer::toString)
.log();
ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_RECEIVED_OTHER_MESSAGE_BEFORE_STATUS);
return;
}

Expand All @@ -324,9 +324,10 @@ public void processMessage(final Capability cap, final Message message) {

if (!ethPeer.validateReceivedMessage(ethMessage, getSupportedProtocol())) {
LOG.debug(
"Unsolicited message received (BREACH_OF_PROTOCOL), disconnecting from EthPeer: {}",
"Unsolicited message received {} (BREACH_OF_PROTOCOL), disconnecting from EthPeer: {}",
ethMessage.getData().getCode(),
ethPeer);
ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_UNSOLICITED_MESSAGE_RECEIVED);
return;
}

Expand Down Expand Up @@ -354,7 +355,8 @@ public void processMessage(final Capability cap, final Message message) {
.addArgument(e::toString)
.log();

ethPeer.disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL);
ethPeer.disconnect(
DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED);
}
maybeResponseData.ifPresent(
responseData -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ public void dispatchResponse(final EthMessage ethMessage) {
peer,
e);

peer.disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL);
peer.disconnect(
DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED);
}

if (count == 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,11 @@ public void processMessage(final Capability cap, final Message message) {
}
final EthMessage ethMessage = new EthMessage(ethPeer, messageData);
if (!ethPeer.validateReceivedMessage(ethMessage, getSupportedProtocol())) {
LOG.debug("Unsolicited message received from, disconnecting: {}", ethPeer);
ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
LOG.debug(
"Unsolicited message {} received from, disconnecting: {}",
ethMessage.getData().getCode(),
ethPeer);
ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_UNSOLICITED_MESSAGE_RECEIVED);
return;
}

Expand All @@ -123,7 +126,7 @@ public void processMessage(final Capability cap, final Message message) {
} catch (final RLPException e) {
LOG.debug(
"Received malformed message {} , disconnecting: {}", messageData.getData(), ethPeer, e);
ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED);
}
maybeResponseData.ifPresent(
responseData -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ protected Optional<List<BlockHeader>> processResponse(
LOG.debug(
"Sequential headers must form a chain through hashes (BREACH_OF_PROTOCOL), disconnecting peer: {}",
peer.getLoggableId());
peer.disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL);
peer.disconnect(
DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL_NON_SEQUENTIAL_HEADERS);
return Optional.empty();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ private void handleMessage(
peer.getLoggableId(),
e);
LOG.trace("Peer {} Malformed message data: {}", peer, message.getData());
peer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
peer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED);
promise.completeExceptionally(new PeerBreachedProtocolException());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ private void handleNewBlockFromNetwork(final EthMessage message) {
"Malformed NEW_BLOCK message received from peer (BREACH_OF_PROTOCOL), disconnecting: {}",
message.getPeer(),
e);
message.getPeer().disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
message.getPeer().disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED);
}
}

Expand Down Expand Up @@ -402,7 +402,7 @@ private void handleNewBlockHashesFromNetwork(final EthMessage message) {
"Malformed NEW_BLOCK_HASHES message received from peer (BREACH_OF_PROTOCOL), disconnecting: {}",
message.getPeer(),
e);
message.getPeer().disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
message.getPeer().disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ private CompletionStage<Void> handleFailedDownload(final Throwable error) {
LOG.warn(
"Invalid block detected (BREACH_OF_PROTOCOL). Disconnecting from sync target. {}",
ExceptionUtils.rootCause(error).getMessage());
syncState.disconnectSyncTarget(DisconnectReason.BREACH_OF_PROTOCOL);
syncState.disconnectSyncTarget(DisconnectReason.BREACH_OF_PROTOCOL_INVALID_BLOCK);
}

if (!cancelled.get() && syncTargetManager.shouldContinueDownloading()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,9 @@ CompletableFuture<List<BlockHeader>> processHeaders(
LOG.debug(
"Received invalid headers from peer (BREACH_OF_PROTOCOL), disconnecting from: {}",
headersResult.getPeer());
headersResult.getPeer().disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
headersResult
.getPeer()
.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_INVALID_HEADERS);
final InvalidBlockException exception;
if (invalidHeader == null) {
final String msg =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ private void processNewPooledTransactionHashesMessage(
peer,
ex);
LOG.trace("Message data: {}", transactionsMessage.getData());
peer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
peer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ private void processTransactionsMessage(
"Malformed transaction message received (BREACH_OF_PROTOCOL), disconnecting: {}",
peer,
ex);
peer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
peer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,6 @@ public void checkThatSequentialHeadersNotFormingAChainFails() {
assertThat(optionalBlockHeaders).isEmpty();
assertThat(peer.isDisconnected()).isTrue();
assertThat(((MockPeerConnection) peer.getConnection()).getDisconnectReason().get())
.isEqualTo(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL);
.isEqualTo(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL_NON_SEQUENTIAL_HEADERS);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,6 @@ public void checkThatSequentialHeadersNotFormingAChainFails() {
assertThat(optionalBlockHeaders).isEmpty();
assertThat(peer.isDisconnected()).isTrue();
assertThat(((MockPeerConnection) peer.getConnection()).getDisconnectReason().get())
.isEqualTo(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL);
.isEqualTo(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL_NON_SEQUENTIAL_HEADERS);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,8 @@ public void testInvalidBlockHandling(
}
selectTargetFuture.completeExceptionally(InvalidBlockException.create("Failed"));

verify(syncState, times(1)).disconnectSyncTarget(DisconnectReason.BREACH_OF_PROTOCOL);
verify(syncState, times(1))
.disconnectSyncTarget(DisconnectReason.BREACH_OF_PROTOCOL_INVALID_BLOCK);
}

private CompletableFuture<Void> expectPipelineStarted(final SyncTarget syncTarget) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public void disconnectsFromPeerOnBadFork() {
// We should have disconnected from our peer on the invalid chain
assertThat(peer.getEthPeer().isDisconnected()).isTrue();
assertThat(peer.getPeerConnection().getDisconnectReason())
.contains(DisconnectReason.BREACH_OF_PROTOCOL);
.contains(DisconnectReason.BREACH_OF_PROTOCOL_INVALID_BLOCK);
assertThat(syncState.syncTarget()).isEmpty();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,10 @@ private void setupHandlers() {
cap.getVersion(),
code,
message.getConnection().getPeerInfo().getNodeId());
message.getConnection().disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
message
.getConnection()
.disconnect(
DisconnectReason.BREACH_OF_PROTOCOL_INVALID_MESSAGE_CODE_FOR_PROTOCOL);
return;
}
inboundMessageCounter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ public void onDisconnect(
final PeerConnection connection,
final DisconnectReason reason,
final boolean initiatedByPeer) {
if (shouldBlock(reason, initiatedByPeer)) {
// we have a number of reasons that use the same code, but with different message strings
// so here we use the code of the reason param to ensure we get the no-message version
if (shouldBlock(DisconnectReason.forCode(reason.getValue().get(0)), initiatedByPeer)) {
if (maintainedPeers.contains(connection.getPeer())) {
LOG.debug(
"Skip adding maintained peer {} to peer denylist for reason {}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ protected void decode(final ChannelHandlerContext ctx, final ByteBuf in, final L
new OutboundMessage(
null,
DisconnectMessage.create(
DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL)))
DisconnectMessage.DisconnectReason
.BREACH_OF_PROTOCOL_MESSAGE_RECEIVED_BEFORE_HELLO_EXCHANGE)))
.addListener((f) -> ctx.close());
connectFuture.completeExceptionally(
new BreachOfProtocolException("Message received before HELLO's exchanged"));
Expand Down Expand Up @@ -260,7 +261,11 @@ public void exceptionCaught(final ChannelHandlerContext ctx, final Throwable thr
|| cause instanceof IllegalArgumentException) {
LOG.debug("Invalid incoming message (BREACH_OF_PROTOCOL)", throwable);
if (connectFuture.isDone() && !connectFuture.isCompletedExceptionally()) {
connectFuture.get().disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL);
connectFuture
.get()
.disconnect(
DisconnectMessage.DisconnectReason
.BREACH_OF_PROTOCOL_INVALID_MESSAGE_RECEIVED_CAUGHT_EXCEPTION);
return;
}
} else if (cause instanceof IOException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,21 @@ public enum DisconnectReason {
UNKNOWN(null),
REQUESTED((byte) 0x00),
TCP_SUBSYSTEM_ERROR((byte) 0x01),

BREACH_OF_PROTOCOL((byte) 0x02),
BREACH_OF_PROTOCOL_RECEIVED_OTHER_MESSAGE_BEFORE_STATUS(
(byte) 0x02, "Message other than status received first"),
BREACH_OF_PROTOCOL_UNSOLICITED_MESSAGE_RECEIVED((byte) 0x02, "Unsolicited message received"),
BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED((byte) 0x02, "Malformed message received"),
BREACH_OF_PROTOCOL_NON_SEQUENTIAL_HEADERS((byte) 0x02, "Non-sequential headers received"),
BREACH_OF_PROTOCOL_INVALID_BLOCK((byte) 0x02, "Invalid block detected"),
BREACH_OF_PROTOCOL_INVALID_HEADERS((byte) 0x02, "Invalid headers detected"),
BREACH_OF_PROTOCOL_INVALID_MESSAGE_CODE_FOR_PROTOCOL(
(byte) 0x02, "Invalid message code for specified protocol"),
BREACH_OF_PROTOCOL_MESSAGE_RECEIVED_BEFORE_HELLO_EXCHANGE(
(byte) 0x02, "A message was received before hello's exchanged"),
BREACH_OF_PROTOCOL_INVALID_MESSAGE_RECEIVED_CAUGHT_EXCEPTION(
(byte) 0x02, "An exception was caught decoding message"),
USELESS_PEER((byte) 0x03),
USELESS_PEER_USELESS_RESPONSES((byte) 0x03, "Useless responses: exceeded threshold"),
USELESS_PEER_TRAILING_PEER((byte) 0x03, "Trailing peer requirement"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ public void denylistPeerForBadBehavior() {
checkPermissions(denylist, peer.getPeer(), false);
}

@Test
public void denylistPeerForBadBehaviorWithDifferentMessage() {
final PeerConnection peer = generatePeerConnection();

checkPermissions(denylist, peer.getPeer(), true);
peerDenylistManager.onDisconnect(
peer, DisconnectReason.BREACH_OF_PROTOCOL_INVALID_MESSAGE_CODE_FOR_PROTOCOL, false);
checkPermissions(denylist, peer.getPeer(), false);
}

@Test
public void doesNotDenylistPeerForOurBadBehavior() {
final PeerConnection peer = generatePeerConnection();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ public void exceptionCaught_shouldDisconnectForBreachOfProtocolWhenFramingExcept

deFramer.exceptionCaught(ctx, new DecoderException(new FramingException("Test")));

verify(peerConnection).disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
verify(peerConnection)
.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_INVALID_MESSAGE_RECEIVED_CAUGHT_EXCEPTION);
}

@Test
Expand All @@ -148,7 +149,8 @@ public void exceptionCaught_shouldDisconnectForBreachOfProtocolWhenRlpExceptionT

deFramer.exceptionCaught(ctx, new DecoderException(new RLPException("Test")));

verify(peerConnection).disconnect(DisconnectReason.BREACH_OF_PROTOCOL);
verify(peerConnection)
.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_INVALID_MESSAGE_RECEIVED_CAUGHT_EXCEPTION);
}

@Test
Expand Down
Loading