Security: Avoid reconnecting to peers that are likely unreachable

[jvff]

Motivation

Zebra would keep trying to connect to Failed peers, even if they are likely unreachable and will always fail. This is a Denial-of-Service risk and places extra load on the network.

This closes #1865.

Solution

Filter the list of peers to reconnect to so that it doesn't contain peers that are likely unreachable.

A peer is considered to be probably unreachable if it was last seen (or was reported last seen) more than three days ago and if the previous connection attempt to it failed.

Review

I had discussed the implementation solution with @teor2345.

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

Peers that are likely to be unreachable should be removed from the AddressBook. This might be part of #1873.

[teor2345]

This looks good, I just want to double-check an edge case.

I also have some name suggestions that might make the code easier to read.

(Sorry, I merged the latest main branch into this PR, must have got confused with another one.)

[teor2345]

Do we need to test this code in an integration test?
What happens if Zebra has no recent peers?

[jvff]

Do we need to test this code in an integration test? What happens if Zebra has no recent peers?

What do you mean by an integration test? Something like the acceptance tests we have?

[teor2345]

Some optional comment tweaks.

[teor2345]

Do we need to test this code in an integration test? What happens if Zebra has no recent peers?

What do you mean by an integration test? Something like the acceptance tests we have?

I was thinking of an integration test for all of zebra_network::init. I don't think we need to test zebrad, because it doesn't modify the address book.

Here's a similar test:

zebra/zebra-network/src/peer_set/initialize/tests/vectors.rs

Lines 1217 to 1229 in 62bfa15

	/// Open a local listener on `listen_addr` for `network`.
	/// Asserts that the local listener address works as expected.
	async fn local_listener_port_with(listen_addr: SocketAddr, network: Network) {
	let config = Config {
	listen_addr,
	network,
	// Stop Zebra making outbound connections
	initial_mainnet_peers: HashSet::new(),
	initial_testnet_peers: HashSet::new(),
	..Config::default()
	};

But you'd want to stuff the active address book with outdated peers.

Here's one way to generate fake peers:

zebra/zebra-network/src/peer_set/initialize/tests/vectors.rs

Lines 1186 to 1194 in 62bfa15

	// Create a list of dummy IPs, and initialize a config using them as the
	// initial peers. The amount of these peers will overflow
	// `PEERSET_INITIAL_TARGET_SIZE`.
	let mut peers = HashSet::new();
	for address_number in 0..PEER_COUNT {
	peers.insert(
	SocketAddr::new(Ipv4Addr::new(127, 1, 1, address_number as _).into(), 1).to_string(),
	);
	}

[teor2345]

I was thinking of an integration test for all of zebra_network::init. I don't think we need to test zebrad, because it doesn't modify the address book.

Thinking about this a bit more...

The tricky part of this test is working out what the candidate set is doing, or if the network stack has hung. So maybe we'll need to test the crawler task separately?

There are tests for the crawler task in the same module.

[jvff]

The tricky part of this test is working out what the candidate set is doing, or if the network stack has hung. So maybe we'll need to test the crawler task separately?

This seems more complicated than anticipated. Maybe we should do this in a separate PR? I opened #3048 to do it later.

[teor2345]

This seems more complicated than anticipated. Maybe we should do this in a separate PR? I opened #3048 to do it later.

Fair enough. I don't think these code changes are particularly risky by themselves, and we do have decent test coverage of the crawler from other tests in that same module.

I added two specific scenarios that I am concerned about to ticket #3048.

I'll try to be clearer about specific test scenarios next time. Even if we decide to do them in a later ticket, we need to be specific about what we're testing. Otherwise we won't know how to estimate or schedule the ticket.

[teor2345]

Let's document what happens when peers have no last seen time.

[teor2345]

I tried a manual merge, let's see if it works.