Respond to a PING even if the source doesn't match the ENR #184

ackintosh · 2023-05-19T08:14:20Z

Description

Our discv5 implementation drops a request if the source IP address doesn't match the ENR. For PING request, due to this behaviour, the source node can't notice that its external IP address has been changed.

Here is a simulation that reproduce that issue.
ackintosh/discv5-testground#98

This PR makes discv5 to respond PING even if the source IP address doesn't match.

Notes & open questions

one-time sessions

I introduced one-time session at handler to encrypt PONG response without storing a (normal) session. One-time session here is a session for a specific request such that a PING request mentioned above. As the name implies, one-time session is removed once it's used.

New config parameters

Added config parameters for one-time session introduced by this PR.

one_time_session_timeout
one_time_session_cache_capacity

These are similar to the parameters for normal sessions, but the default values are set to 30 seconds, and 100 respectively.

Simulation test for this PR

Also I have created a Testground test plan that simulates IP changes.
ackintosh/discv5-testground#98

The result is fine. Here is a sequence diagram that illustrates how this change behaves on IP changes.

sequenceDiagram
    participant Node1
    participant Node2 ... Node N

    rect rgb(10, 10, 10)
    Note left of Node1: They communicate with each other<br> to establish a session.
    Note over Node1,Node2 ... Node N: Session established
    end

    rect rgb(10, 10, 10)
    Note left of Node1: Node1 changes its IP address<br> but doesn't update its ENR.
    Note over Node1: *** Change its IP address ***
    end

    Node1 ->> Node2 ... Node N: PING
    Note over Node2 ... Node N: No session with the new IP exists
    Node2 ... Node N ->> Node1: WHOAREYOU
    Note over Node1: Establish new session
    Node1 ->> Node2 ... Node N: Auth message<br> (The ENR record has the old IP)
    Note over Node2 ... Node N: `verify_enr()` results in false<br> as the ENR and src IP don't match.
    Note over Node2 ... Node N: Establish one-time session with Node1's new IP
    
    rect rgb(0, 255, 0)
    Note over Node2 ... Node N: Respond the PING using the one-time session
    end

    Node2 ... Node N ->> Node1: PONG

    rect rgb(0, 255, 0)
    Note over Node1: Update local ENR to the new IP via the IpVote process.
    end

    Node1 ->> Node2 ... Node N: PING
    Note over Node2 ... Node N: No (normal) session with the new IP exists
    Node2 ... Node N ->> Node1: WHOAREYOU
    Note over Node1: Establish new session
    Node1 ->> Node2 ... Node N: Auth message<br> (The ENR record has the new IP)

    Note over Node2 ... Node N: `verify_enr()` results in true here
    rect rgb(0, 255, 0)
    Note over Node2 ... Node N: Establish new (normal) session
    end

    Node2 ... Node N ->> Node1: PONG

Change checklist

Self-review
Documentation updates if relevant
Tests if relevant

divagant-martian

Couple suggestions about the logic and some about the code. Ty for the great work as always

src/handler/mod.rs

divagant-martian · 2023-05-29T03:00:13Z

src/handler/mod.rs

@@ -813,6 +828,37 @@ impl Handler {
                        // established. If so process them.
                        self.send_next_request::<P>(node_address).await;
                    } else {
+                        // Respond to PING request even if the ENR or NodeAddress don't match


Here if we store just one one-time-session per peer, we would want to check first for one such session being already established before doing the decode

we would want to check first for one such session being already established

You mean we would want to move the second statement of below (else if ... self.remove_one_time_session() ...) to the top of the if statement?

discv5/src/handler/mod.rs

Lines 522 to 535 in fce0fce

// Check for an established session

let packet = if let Some(session) = self.sessions.get_mut(&node_address) {

session.encrypt_message::<P>(self.node_id, &response.encode())

} else if let Some(mut session) = self.remove_one_time_session(&node_address, &response.id)

{

session.encrypt_message::<P>(self.node_id, &response.encode())

} else {

// Either the session is being established or has expired. We simply drop the

// response in this case.

return warn!(

"Session is not established. Dropping response {} for node: {}",

response, node_address.node_id

);

};

yep, this is what I mean

My slight concern about doing that is, the second statement (if ... self.remove_one_time_session() ...) is false in most cases. Establishing a one-time session is under limited conditions, so the result of the second statement should be false in most send_response() calls.

Do you have a good reason for moving the second statement to the first?

My reasoning is that is not impossible to have a one-time session created for a peer, then a normal session created, and then hit this line. A bit of a race condition. Moving the check up would prevent this. It's a matter of correctness but I understand your pov as a matter of performance. What are your thoughts on this?

I see your point. Your suggestion is right in terms of correctness.

a one-time session created for a peer, then a normal session created, and then hit this line

Yeah it looks a bit of a race condition but I think that if a normal session created, it's fine to use the normal session even if a one-time session (for the request) exists since the normal session is valid as well. The one-time session will be evicted over time without being used.

@AgeManning do you have any opinion here?

Not really.
Its a single hash lookup that we are worried about. I think if we have a session, we shouldn't establish a one-time session. But there is a race condition where a session gets created after. So I think its a relatively rare case that we have both sessions. In this case, we probably want to prioritise the normal session over the one-time session.

Seeing as we are probably running this a bit, I agree with @ackintosh in that it's nicer to avoid the hash lookup (i.e check and remove the one-time session) to handle this edge case. It's also no real loss because the one-time sessions only last 30 seconds (assuming they get pruned).

But imo we are debating over a single hash lookup vs correctness, so i'm really on the fence.

Maybe just leave it as @ackintosh has it, because we don't have to do another commit?
I don't feel strongly at all, if either one of you do, happy to go that way.

also of no strong opinion so merged 🤷 seems we are all happy so

so that we don't send a request with a session technically still available

…not-match

ackintosh · 2023-06-15T22:20:53Z

@AgeManning @divagant-martian This PR is ready for review now.

divagant-martian

minor things. Otherwise letting age finish the review

divagant-martian · 2023-06-19T02:00:17Z

src/config.rs

+    /// The one-time session timeout. Default: 30 seconds.
+    pub one_time_session_timeout: Duration,
+
+    /// The maximum number of established one-time sessions to maintain. Default: 100.
+    pub one_time_session_cache_capacity: usize,
+


Since these config values are user-facing, we should probably explain here/somewhere what one time sessions are and what they do

Yeah. I'm thinking of moving some more things to be constants in the code. I don't think there are many people changing the configs here. These potentially could be constants, what do you think?

yep good idea

divagant-martian · 2023-06-19T02:14:37Z

src/handler/mod.rs

+                            self.one_time_sessions
+                                .insert(node_address.clone(), (request.id.clone(), session));
+                            if let Err(e) = self
+                                .service_send
+                                .send(HandlerOut::Request(node_address, Box::new(request)))
+                                .await
+                            {
+                                warn!("Failed to report request to application {}", e)
+                            }


this will self clean, but if failed to report the request, there is no point in storing the one-time-session

Thanks for pointing it out!

AgeManning

This looks good to me. It should make us a little more friendly to people adjusting their IPs.

AgeManning · 2023-06-19T04:50:30Z

src/config.rs

+    /// The one-time session timeout. Default: 30 seconds.
+    pub one_time_session_timeout: Duration,
+
+    /// The maximum number of established one-time sessions to maintain. Default: 100.
+    pub one_time_session_cache_capacity: usize,
+


Yeah. I'm thinking of moving some more things to be constants in the code. I don't think there are many people changing the configs here. These potentially could be constants, what do you think?

src/handler/mod.rs

Co-authored-by: Age Manning <Age@AgeManning.com>

ackintosh · 2023-06-20T22:57:47Z

Thank you guys for the review! I have made changes based on the feedback.

@AgeManning Please let us know if you have any thoughts on #184 (comment).

ackintosh added 3 commits May 19, 2023 15:26

Extract ping request from message

f133400

Respond PING request using one-time session

0d56ee1

cargo fmt

7841d1d

This comment was marked as resolved.

Sign in to view

ackintosh mentioned this pull request May 28, 2023

The handler instructs the service on how to update connection direction ackintosh/discv5#1

Closed

3 tasks

This comment was marked as resolved.

Sign in to view

divagant-martian reviewed May 29, 2023

View reviewed changes

divagant-martian requested a review from AgeManning May 29, 2023 03:23

ackintosh added 7 commits May 31, 2023 07:17

Fix typo

e49dc65

Avoid macros for readability

394f93e

Move the session failure up

ed69f2c

so that we don't send a request with a session technically still available

Store one session per node

e6d95bc

Tweak comment

fce0fce

Fix clippy warnings

4960465

Don't update the time when access one-time sessions

aacf2ca

ackintosh mentioned this pull request Jun 4, 2023

Fix unexpected ConnectionDirection update #188

Merged

3 tasks

ackintosh added 4 commits June 10, 2023 16:17

Add parameters for one-time session

d88590d

Merge branch 'master' into pong-even-if-src-not-match

3a70c74

Merge remote-tracking branch 'upstream/master' into pong-even-if-src-…

47b32c4

…not-match

Merge remote-tracking branch 'upstream/master' into pong-even-if-src-…

ad68d9b

…not-match

AgeManning mentioned this pull request Jun 14, 2023

Handle IP changes gracefully #194

Closed

ackintosh mentioned this pull request Jun 14, 2023

ip-change ackintosh/discv5-testground#98

Merged

ackintosh changed the title ~~[WIP] Respond PING even if the source doesn't match the ENR~~ Respond PING even if the source doesn't match the ENR Jun 14, 2023

Add test for remove_one_time_session

d056db2

ackintosh marked this pull request as ready for review June 15, 2023 22:15

ackintosh added the ready-for-review label Jun 16, 2023

divagant-martian reviewed Jun 19, 2023

View reviewed changes

AgeManning approved these changes Jun 19, 2023

View reviewed changes

Update src/handler/mod.rs

af4f804

Co-authored-by: Age Manning <Age@AgeManning.com>

ackintosh added 3 commits June 21, 2023 07:05

Move the one-time session related configs to be constants

dcba087

Remove the one-time-session if failed to report the request

d6a43ce

Fix tests

11de4b2

ackintosh changed the title ~~Respond PING even if the source doesn't match the ENR~~ Respond to a PING even if the source doesn't match the ENR Jun 20, 2023

divagant-martian merged commit 94e09e1 into sigp:master Jun 21, 2023

ackintosh deleted the pong-even-if-src-not-match branch June 22, 2023 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respond to a PING even if the source doesn't match the ENR #184

Respond to a PING even if the source doesn't match the ENR #184

ackintosh commented May 19, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

divagant-martian left a comment

divagant-martian May 29, 2023

ackintosh Jun 3, 2023

divagant-martian Jun 3, 2023

ackintosh Jun 3, 2023

divagant-martian Jun 5, 2023

ackintosh Jun 10, 2023

divagant-martian Jun 19, 2023

AgeManning Jun 21, 2023

divagant-martian Jun 21, 2023

ackintosh commented Jun 15, 2023

divagant-martian left a comment

divagant-martian Jun 19, 2023

AgeManning Jun 19, 2023

divagant-martian Jun 19, 2023

divagant-martian Jun 19, 2023

ackintosh Jun 20, 2023

AgeManning left a comment

AgeManning Jun 19, 2023

ackintosh commented Jun 20, 2023

	// Check for an established session
	let packet = if let Some(session) = self.sessions.get_mut(&node_address) {
	session.encrypt_message::<P>(self.node_id, &response.encode())
	} else if let Some(mut session) = self.remove_one_time_session(&node_address, &response.id)
	{
	session.encrypt_message::<P>(self.node_id, &response.encode())
	} else {
	// Either the session is being established or has expired. We simply drop the
	// response in this case.
	return warn!(
	"Session is not established. Dropping response {} for node: {}",
	response, node_address.node_id
	);
	};

Respond to a PING even if the source doesn't match the ENR #184

Respond to a PING even if the source doesn't match the ENR #184

Conversation

ackintosh commented May 19, 2023 • edited Loading

Description

Notes & open questions

one-time sessions

New config parameters

Simulation test for this PR

Change checklist

This comment was marked as resolved.

This comment was marked as resolved.

divagant-martian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackintosh commented Jun 15, 2023

divagant-martian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgeManning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackintosh commented Jun 20, 2023

ackintosh commented May 19, 2023 •

edited

Loading