Don't trust reported last seen times

[jvff]

Motivation

Zebra would previously trust the last_seen time reported on a gossiped peer. However, if that time is in the future, it could take precedence over all other peers in the reconnection queue. This could happen intentionally by a malicious peer or unintentionally due to clock skew. The reporting issue (#1871) has more information.

Solution

The proposed solution is to apply an offset to the reported last_seen times by a peer if any of them are in the future. To do this, the offset is calculated by subtracting the most advanced reported time (in the remote peer's clock) by the current time (in the local clock).

The code in this pull request has:

• Documentation Comments
• Unit Tests and Property Tests

Review

@teor2345 wrote the proposed solution in the original issue.

Related Issues

Closes #1871

Follow Up Work

[teor2345]

A few significant changes:

1. If any times are in the future, let's adjust all times, so we handle honest clock skewed peers correctly
2. Let's test peers that are exactly at the limit
3. Let's test using random limits, not just the current time when the test is running

I also have some general feedback about formatting and refactoring.

[teor2345]

Just a few comment tweaks, nothing major

[teor2345]

Some tests are failing because I added leap seconds to the Arbitrary methods in a recent PR: #2195

I've made some suggestions to fix that. (Zebra shouldn't crash if it's running during a leap second. But it's ok for the tests to maybe fail, because leap seconds are rare.)

[teor2345]

This looks good, the core code is really solid.

We need to do a few other fixes due to existing MetaAddr bugs. And I'd like to simplify the error handling in this PR.

My Leap Seconds PR

This PR failed due to #2195, which adds arbitrary leap seconds. (Sorry about that, I didn't expect it to make any difference.) So I made a fix in jvff#1

Existing MetaAddr Bugs

I also added a bunch of other properties we should test in jvff#1, but we need to merge #2203 to fix some MetaAddr serialization bugs first. Feel free to rebase and re-enable those tests.

Simplifying Error Handling in this PR

I'd like to re-design how we do error handling. We're handling errors in a few different places. So it would be easy to miss some, particularly as validation gets more complicated. I'd like to use idiomatic Rust error-handling, where each function returns a Result.

So the functions in this PR would look like:

struct MetaAddrValidationError;

fn MetaAddr::offset_last_seen_by(&self, offset: Duration) -> Result<MetaAddr, MetaAddrValidationError>

fn limit_last_seen_times(addrs: &Vec<MetaAddr>, last_seen_limit: DateTime<Utc>) -> Result<Vec<MetaAddr>, MetaAddrValidationError>

fn validate_addrs(addrs: impl IntoIterator<Item = MetaAddr>, last_seen_limit: DateTime<Utc>) -> Result<Vec<MetaAddr>, MetaAddrValidationError>

That way, we can propagate errors up to update_fanout using the try operator ?, and handle them in one place. (By not sending the addresses to the address book.)

I think we'll end up with simpler code that way. We can use checked functions for all our calculations, and use ? to return any errors.

(We might need to impl From<None> or From<NoneError> for MetaAddrValidationError, let me know how you go.)

Let me know what you think - we can do some pair programming on these changes if you'd like.

[teor2345]

This looks great.

I'll just commit some typo fixes, and then squash the PR.

[teor2345]

I love simple code

[teor2345]

Just a minor typo fix:

Suggested change

	// No overflow is possible, so apply offset to all addresses
	for addr in addrs {
	let old_last_seen = addr.get_last_seen().timestamp();
	let new_last_seen = old_last_seen - offset;
	addr.set_last_seen(new_last_seen.into());
	}
	} else {
	// An overflow will occur, so reject all gossiped peers
	// No underflow is possible, so apply offset to all addresses
	for addr in addrs {
	let old_last_seen = addr.get_last_seen().timestamp();
	let new_last_seen = old_last_seen - offset;
	addr.set_last_seen(new_last_seen.into());
	}
	} else {
	// An underflow will occur, so reject all gossiped peers

[teor2345]

Suggested change

	/// `last_seen` times to be before the limit causes an overflow.
	/// `last_seen` times to be before the limit causes an underflow.

[teor2345]

I think this code is about as simple as it gets.

If we used chrono::DateTime, we would need check for other errors. (Or justify that they can't possibly happen.)

Good call here!

[teor2345]

This is about as good as a proptest ever gets.

I'm glad we made the DateTime32 change.

[teor2345]

Here's an example of implementing min and max using iterators:

    let timestamps: Vec<_> = addrs
        .iter()
        .map(|addr| addr.get_last_seen().timestamp())
        .collect();
    let oldest_reported_seen_timestamp = timestamps.iter().min().cloned().unwrap_or(u32::MAX);
    let newest_reported_seen_timestamp = timestamps.iter().max().cloned().unwrap_or(u32::MIN);

In this case the code is about the same length, so let's stick with your version.

By the way, the fold/unwrap_or constants are never actually used. They participate in the calculations, but they never get applied to any data, because the array is empty. But I think they're better than using the Options in later code.

[teor2345]

Ha, caught by the auto-merge. I'll just commit those comment changes to main.