Skip to content
Closed
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 @@ -128,8 +128,10 @@ private void deleteSession(final SessionKey sessionKey) {
if (removedSession != null) {
// Mark inactive to prevent registering any new nonces
removedSession.markInactive();
// And then clean up the last recorded nonce, if any
removedSession.getLastOutboundNonce().ifPresent(lastNonceToSession::remove);
// And then clean up every nonce that was indexed for this session. The session may have
// recorded multiple recent outbound nonces (bounded by MAX_RECENT_OUTBOUND_NONCES on the
// session), and each one points back to this session in lastNonceToSession.
removedSession.getRecentOutboundNonces().forEach(lastNonceToSession::remove);
}
}

Expand All @@ -145,9 +147,14 @@ public Optional<NodeSession> getNodeSessionByLastOutboundNonce(final Bytes12 non
return Optional.ofNullable(lastNonceToSession.get(nonce));
}

public void onSessionLastNonceUpdate(
final NodeSession session, final Optional<Bytes12> previousNonce, final Bytes12 newNonce) {
previousNonce.ifPresent(lastNonceToSession::remove);
/**
* Called when the session records a new outbound nonce. {@code evictedNonce} is the
* oldest-tracked nonce that was dropped to make room (if any), and must be removed from the
* lookup index so it no longer points back at this session. {@code newNonce} is then indexed.
*/
public void onSessionRecentNonceUpdate(
final NodeSession session, final Optional<Bytes12> evictedNonce, final Bytes12 newNonce) {
evictedNonce.ifPresent(lastNonceToSession::remove);
lastNonceToSession.put(newNonce, session);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,22 @@ protected void handlePacket(Envelope envelope) {
Bytes12 msgNonce = unknownPacket.getHeader().getStaticHeader().getNonce();

// If we have already issued a WHOAREYOU for this exact ordinary packet nonce, resend that
// same challenge — the initiator may have already signed against it. For any other nonce,
// fall through and issue a fresh WHOAREYOU; previous in-flight challenges remain queued so a
// delayed handshake signed against one of them can still be validated.
// same challenge — the initiator may have already signed against it.
if (session.hasPendingWhoAreYouForNonce(msgNonce)) {
session.resendOutgoingWhoAreYouFor(msgNonce);
return;
}

// Otherwise, if the session is still mid-handshake (WHOAREYOU_SENT), resend the earliest
// pending challenge rather than issue a brand-new one. Per devp2p discv5 conformance
// (geth v5test TestHandshakeResend): an unauthorized ordinary packet arriving while a
// handshake is in flight must receive the SAME WHOAREYOU as the first one, even if the
// peer's retry carries a fresh per-packet nonce.
if (session.getState() == SessionState.WHOAREYOU_SENT
&& session.resendFirstPendingWhoAreYou()) {
return;
}

try {
Bytes16 idNonce = Bytes16.random(Functions.getRandom());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,11 @@ protected void handlePacket(final Envelope envelope) {
final NodeRecord nodeRecord = session.getNodeRecord().orElseThrow();

Bytes12 whoAreYouNonce = whoAreYouPacket.getHeader().getStaticHeader().getNonce();
boolean nonceMatches =
session.getLastOutboundNonce().map(whoAreYouNonce::equals).orElse(false);
// Accept the WHOAREYOU if its nonce names ANY of our recently-sent outbound nonces, not
// only the latest. The peer (per devp2p discv5 conformance) is allowed to resend the
// original WHOAREYOU when an unauthorized retry arrives, in which case the nonce points
// back to an earlier outbound of ours rather than the most recent one.
boolean nonceMatches = session.isRecentOutboundNonce(whoAreYouNonce);
if (!nonceMatches) {
LOG.trace(
"Verification not passed for message [{}] from node {} in status {}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Random;
Expand Down Expand Up @@ -63,6 +66,15 @@ public class NodeSession {
*/
@VisibleForTesting static final int MAX_PENDING_WHOAREYOU = 5;

/**
* Maximum number of recent outbound packet nonces remembered per session. A WHOAREYOU received
* from the peer is accepted only when its nonce matches one of these (i.e. references an ordinary
* message we recently sent and have not yet had answered). Bounded for the same reason as {@link
* #MAX_PENDING_WHOAREYOU} — and sized the same so that, in the worst case, the peer's cached
* challenges and our recent outbound nonces correspond one-for-one.
*/
@VisibleForTesting static final int MAX_RECENT_OUTBOUND_NONCES = MAX_PENDING_WHOAREYOU;

private final Bytes32 homeNodeId;
private final LocalNodeRecordStore localNodeRecordStore;
private final NodeSessionManager nodeSessionManager;
Expand Down Expand Up @@ -94,7 +106,15 @@ protected boolean removeEldestEntry(final Map.Entry<Bytes12, PendingWhoAreYou> e
}
});

private Optional<Bytes12> lastOutboundNonce = Optional.empty();
// Recent outbound packet nonces, in insertion order so the oldest can be evicted when the
// bound is reached. Used by the WHOAREYOU handler on the initiator side: an incoming WHOAREYOU
// is accepted iff its nonce names one of these — i.e. references an ordinary message we
// recently sent and have not yet had answered. Strict equality against only the *latest*
// outbound nonce would reject a peer that resends an earlier WHOAREYOU after we have sent
// another ordinary packet in between (e.g. a retry), which is exactly what the devp2p discv5
// conformance test TestHandshakeResend exercises.
private final LinkedHashSet<Bytes12> recentOutboundNonces = new LinkedHashSet<>();

private boolean active = true;
private final Function<Random, Bytes12> nonceGenerator;

Expand Down Expand Up @@ -210,6 +230,30 @@ public void resendOutgoingWhoAreYouFor(final Bytes12 nonce) {
sendOutgoing(pending.maskingIV(), pending.packet());
}

/**
* Resends the earliest pending WHOAREYOU (byte-for-byte, same maskingIV) without requiring a
* matching msgNonce. Used when an unauthorized ordinary message arrives from a peer that is still
* mid-handshake (session state {@code WHOAREYOU_SENT}) but is carrying a fresh per-packet nonce,
* i.e. a retry rather than a literal retransmit. Returns {@code true} if a pending challenge was
* found and resent.
*/
public boolean resendFirstPendingWhoAreYou() {
final PendingWhoAreYou pending;
synchronized (pendingWhoAreYou) {
final Iterator<PendingWhoAreYou> it = pendingWhoAreYou.values().iterator();
if (!it.hasNext()) {
return false;
}
pending = it.next();
}
LOG.trace(
() ->
String.format(
"Resending earliest pending WhoAreYou %s in session %s", pending.packet(), this));
sendOutgoing(pending.maskingIV(), pending.packet());
return true;
}

/**
* Returns the challenge data ({@code maskingIV || header bytes}) for every pending WHOAREYOU. The
* handshake handler attempts to match the incoming handshake against each candidate because the
Expand Down Expand Up @@ -319,19 +363,60 @@ public synchronized void cancelAllRequests(final String message) {
/** Generates random nonce */
public synchronized Bytes12 generateNonce() {
final Bytes12 newNonce = nonceGenerator.apply(rnd);
final Optional<Bytes12> oldNonce = lastOutboundNonce;
lastOutboundNonce = Optional.of(newNonce);
Optional<Bytes12> evictedNonce = Optional.empty();
// Add to the recent-nonce set, evicting the oldest entry if we exceed the bound. Insertion
// order is preserved by LinkedHashSet, so the iterator's first element is the oldest.
if (recentOutboundNonces.size() >= MAX_RECENT_OUTBOUND_NONCES) {
final Iterator<Bytes12> it = recentOutboundNonces.iterator();
if (it.hasNext()) {
evictedNonce = Optional.of(it.next());
it.remove();
}
}
recentOutboundNonces.add(newNonce);
if (active) {
// Update while synchronized to ensure only one update in flight at a time. Otherwise the
// previous nonce may not have been recorded before we try to remove it leading to a memory
// leak. Also only records the session if it's active to avoid re-adding a removed session
nodeSessionManager.onSessionLastNonceUpdate(this, oldNonce, newNonce);
// evicted nonce may not have been removed from the index before we try to add the new one,
// leading to a memory leak. Also only records the session if it's active to avoid re-adding
// a removed session.
nodeSessionManager.onSessionRecentNonceUpdate(this, evictedNonce, newNonce);
}
return newNonce;
}

/**
* Returns the most recently generated outbound nonce. Retained for callers that only need the
* latest; new code that needs to check whether a nonce was ever recently sent should use {@link
* #isRecentOutboundNonce(Bytes12)} instead.
*/
public synchronized Optional<Bytes12> getLastOutboundNonce() {
return lastOutboundNonce;
if (recentOutboundNonces.isEmpty()) {
return Optional.empty();
}
Bytes12 last = null;
for (final Bytes12 nonce : recentOutboundNonces) {
last = nonce;
}
return Optional.ofNullable(last);
}

/**
* Returns {@code true} if {@code nonce} is one of the recently generated outbound nonces still
* tracked on this session (bounded by {@link #MAX_RECENT_OUTBOUND_NONCES}). Used by the WHOAREYOU
* handler to accept challenges that name any unanswered recent outbound, not only the latest one
* — see the class-level comment on {@code recentOutboundNonces}.
*/
public synchronized boolean isRecentOutboundNonce(final Bytes12 nonce) {
return recentOutboundNonces.contains(nonce);
}

/**
* Returns an immutable snapshot of all recent outbound nonces, oldest first. Intended for use by
* {@code NodeSessionManager} when the session is being torn down so that every indexed nonce can
* be cleaned up.
*/
public synchronized Collection<Bytes12> getRecentOutboundNonces() {
return List.copyOf(recentOutboundNonces);
}

public synchronized void markInactive() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,22 +61,44 @@ void shouldNotChangeStateWhenResendingForKnownNonce() {
}

@Test
void shouldSendNewWhoAreYouWhenIncomingNonceIsUnknown() {
void shouldResendFirstPendingChallengeWhenSessionInWhoAreYouSentAndNonceUnknown() {
final NodeSession session = mock(NodeSession.class);
when(session.getState()).thenReturn(SessionState.WHOAREYOU_SENT);
when(session.getNodeRecord()).thenReturn(Optional.empty());

final OrdinaryMessagePacket packet = createOrdinaryPacket();
final Bytes12 incomingNonce = packet.getHeader().getStaticHeader().getNonce();
// No pending WhoAreYou for this nonce — an earlier challenge was for a different nonce.
// No cached WhoAreYou for this exact nonce — the peer retried with a fresh per-packet nonce.
when(session.hasPendingWhoAreYouForNonce(incomingNonce)).thenReturn(false);
when(session.resendFirstPendingWhoAreYou()).thenReturn(true);

handler.handle(envelopeWith(session, packet));

// Fallback fires: resend the earliest cached WHOAREYOU, do NOT generate a new one.
verify(session).resendFirstPendingWhoAreYou();
verify(session, never()).resendOutgoingWhoAreYouFor(any());
verify(session, never()).sendOutgoingWhoAreYou(any());
verify(session, never()).setState(any());
}

@Test
void shouldGenerateFreshWhoAreYouWhenSessionInWhoAreYouSentButNoPendingChallenge() {
// Defensive path: state says WHOAREYOU_SENT but the pending cache is empty (e.g.
// mid-shutdown). Behaviour must still be safe — emit a fresh challenge matching the
// incoming nonce rather than dropping the packet.
final NodeSession session = mock(NodeSession.class);
when(session.getState()).thenReturn(SessionState.WHOAREYOU_SENT);
when(session.getNodeRecord()).thenReturn(Optional.empty());
final OrdinaryMessagePacket packet = createOrdinaryPacket();
final Bytes12 incomingNonce = packet.getHeader().getStaticHeader().getNonce();
when(session.hasPendingWhoAreYouForNonce(incomingNonce)).thenReturn(false);
when(session.resendFirstPendingWhoAreYou()).thenReturn(false);

handler.handle(envelopeWith(session, packet));

final ArgumentCaptor<WhoAreYouPacket> captor = ArgumentCaptor.forClass(WhoAreYouPacket.class);
verify(session).sendOutgoingWhoAreYou(captor.capture());
assertThat(captor.getValue().getHeader().getStaticHeader().getNonce()).isEqualTo(incomingNonce);
verify(session).setState(SessionState.WHOAREYOU_SENT);
}

@Test
Expand All @@ -91,6 +113,7 @@ void shouldSendNewWhoAreYouWithIncomingNonceWhenInInitialState() {
final ArgumentCaptor<WhoAreYouPacket> captor = ArgumentCaptor.forClass(WhoAreYouPacket.class);
verify(session).sendOutgoingWhoAreYou(captor.capture());
verify(session, never()).resendOutgoingWhoAreYouFor(any());
verify(session, never()).resendFirstPendingWhoAreYou();
verify(session).setState(SessionState.WHOAREYOU_SENT);

// The WHOAREYOU nonce must echo the incoming packet's nonce so the initiator can
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,58 @@ void resendOutgoingWhoAreYouFor_shouldPreserveOriginalChallenge() {
assertThat(singleChallenge()).isEqualTo(challengeAfterSend);
}

@Test
void resendFirstPendingWhoAreYou_shouldResendEarliestPendingPacket() {
final Bytes12 firstNonce = Bytes12.wrap(Bytes.random(12));
final Bytes12 secondNonce = Bytes12.wrap(Bytes.random(12));
session.sendOutgoingWhoAreYou(createWhoAreYouPacket(firstNonce));
session.sendOutgoingWhoAreYou(createWhoAreYouPacket(secondNonce));

final ArgumentCaptor<NetworkParcelV5> sendsBefore =
ArgumentCaptor.forClass(NetworkParcelV5.class);
verify(outgoingPipeline, times(2)).accept(sendsBefore.capture());
final Bytes firstSentBytes = sendsBefore.getAllValues().get(0).getPacket().getBytes();

final boolean resent = session.resendFirstPendingWhoAreYou();
assertThat(resent).isTrue();

final ArgumentCaptor<NetworkParcelV5> sendsAfter =
ArgumentCaptor.forClass(NetworkParcelV5.class);
verify(outgoingPipeline, times(3)).accept(sendsAfter.capture());
// Third send must be byte-for-byte identical to the first WHOAREYOU emission, not the
// second — earliest pending is preserved so an initiator signing against challenge #1
// continues to validate.
assertThat(sendsAfter.getAllValues().get(2).getPacket().getBytes()).isEqualTo(firstSentBytes);
}

@Test
void resendFirstPendingWhoAreYou_shouldReturnFalseWhenNoPendingPacket() {
final boolean resent = session.resendFirstPendingWhoAreYou();

assertThat(resent).isFalse();
verify(outgoingPipeline, never()).accept(any());
}

@Test
void resendFirstPendingWhoAreYou_shouldReturnFalseAfterHandshakeStateReset() {
final Request<?> request = createRequestMock();
final RequestInfo requestInfo = session.createNextRequest(request);

final ArgumentCaptor<Runnable> timeoutHandlerCaptor = ArgumentCaptor.forClass(Runnable.class);
verify(expirationScheduler).put(eq(requestInfo.getRequestId()), timeoutHandlerCaptor.capture());

session.sendOutgoingWhoAreYou(createWhoAreYouPacket(Bytes12.wrap(Bytes.random(12))));
session.setState(SessionState.WHOAREYOU_SENT);
// Simulate request timeout — clears pendingWhoAreYou and resets state to INITIAL.
timeoutHandlerCaptor.getValue().run();
assertThat(session.getState()).isEqualTo(SessionState.INITIAL);

final boolean resent = session.resendFirstPendingWhoAreYou();

assertThat(resent).isFalse();
verify(outgoingPipeline, times(1)).accept(any()); // only the original send
}

@Test
void sendOutgoingWhoAreYou_shouldStoreMultiplePendingChallengesWithoutOverwriting() {
final Bytes12 nonce1 = Bytes12.wrap(Bytes.random(12));
Expand Down
Loading