Skip to content

gnrc_ipv6_nib: add support for timestamp option in RAs, use for timesync#18121

Open
benpicco wants to merge 9 commits intoRIOT-OS:masterfrom
benpicco:ndp_opt_timestamp_t
Open

gnrc_ipv6_nib: add support for timestamp option in RAs, use for timesync#18121
benpicco wants to merge 9 commits intoRIOT-OS:masterfrom
benpicco:ndp_opt_timestamp_t

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented May 19, 2022 •
edited
Loading

Contribution description

There is an option to include a timestamp with the router advertisement.
This gives us a simple way to propagate a common time base in the network.

Testing procedure

Enable the gnrc_ipv6_nib_timestamp module, router advertisements now include the router's RTT timestamp:

2022-05-19 17:11:51,247 - INFO # nib: Handle packet (icmpv6->type = 134)
2022-05-19 17:11:51,251 - INFO # nib: Received valid router advertisement:
2022-05-19 17:11:51,255 - INFO #      - Source address: fe80::a463:683b:2c:3c74
2022-05-19 17:11:51,258 - INFO #      - Destination address: ff02::1
2022-05-19 17:11:51,260 - INFO #      - Cur Hop Limit: 0
2022-05-19 17:11:51,262 - INFO #      - Flags: --
2022-05-19 17:11:51,264 - INFO #      - Router Lifetime: 1800s
2022-05-19 17:11:51,267 - INFO #      - Reachable Time: 0ms
2022-05-19 17:11:51,269 - INFO #      - Retrans Timer: 0ms
2022-05-19 17:11:51,272 - INFO # nib: received Timestamp option:
2022-05-19 17:11:51,274 - INFO #      - Seconds: 1652980302
2022-05-19 17:11:51,276 - INFO #      - Milliseconds: 890
2022-05-19 17:11:51,279 - INFO #      - remote: 2051990535 ticks
2022-05-19 17:11:51,282 - INFO #      - local: 2051990540 ticks
2022-05-19 17:11:51,284 - INFO #      - diff: 1 ticks
2022-05-19 17:11:51,286 - INFO #      - new diff: 2 ticks
2022-05-19 17:11:53,828 - INFO # nib: Handle packet (icmpv6->type = 134)
2022-05-19 17:11:53,832 - INFO # nib: Received valid router advertisement:
2022-05-19 17:11:53,836 - INFO #      - Source address: fe80::a463:683b:2c:3c74
2022-05-19 17:11:53,839 - INFO #      - Destination address: ff02::1
2022-05-19 17:11:53,841 - INFO #      - Cur Hop Limit: 0
2022-05-19 17:11:53,842 - INFO #      - Flags: --
2022-05-19 17:11:53,845 - INFO #      - Router Lifetime: 1800s
2022-05-19 17:11:53,847 - INFO #      - Reachable Time: 0ms
2022-05-19 17:11:53,850 - INFO #      - Retrans Timer: 0ms
2022-05-19 17:11:53,853 - INFO # nib: received Timestamp option:
2022-05-19 17:11:53,855 - INFO #      - Seconds: 1652980304
2022-05-19 17:11:53,857 - INFO #      - Milliseconds: 181
2022-05-19 17:11:53,860 - INFO #      - remote: 2052075108 ticks
2022-05-19 17:11:53,863 - INFO #      - local: 2052075106 ticks
2022-05-19 17:11:53,864 - INFO #      - diff: 2 ticks
2022-05-19 17:11:53,868 - INFO #      - new diff: 0 ticks (2 ticks too fast)

2022-05-19 17:11:55,701 - INFO # > rtc gettime
2022-05-19 17:11:55,702 - INFO # 
2022-05-19 17:11:55,703 - INFO # 2022-05-19 17:11:45

Issues/PRs references

includes #18120

@github-actions github-actions bot added Area: build system Area: Build system Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System labels May 19, 2022
@benpicco benpicco added the State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. label May 19, 2022
@benpicco benpicco requested a review from fabian18 May 19, 2022 13:48
@benpicco benpicco force-pushed the ndp_opt_timestamp_t branch 3 times, most recently from 08fc45d to d93e9f8 Compare May 19, 2022 15:11
@benpicco benpicco mentioned this pull request May 19, 2022
Open
@benpicco benpicco force-pushed the ndp_opt_timestamp_t branch from d93e9f8 to 67f000e Compare May 30, 2022 19:51
miri64
miri64 reviewed May 31, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tending to NACK this. While, yes this option exists, time synchronization is not its intended use case, worse even: the usage for time synchronization directly contradicts the intended use case (detection of replay attacks).

sys/net/gnrc/network_layer/ipv6/nib/nib.c Outdated
uint64_t local = rtt64_get_counter();
now += diff;

rtt64_set_counter(now);
Copy link
Copy Markdown
Member

@miri64 miri64 May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not what the timestamp option is for. It is intended to detect replay attacks in unsolicited RA (see https://www.rfc-editor.org/rfc/rfc3971.html#section-5.3.1). Using it to synchronize the local time somewhat contradicts the standardized usage.

@benpicco benpicco force-pushed the ndp_opt_timestamp_t branch from 67f000e to 749e177 Compare May 31, 2022 08:41
@github-actions github-actions bot added the Area: pkg Area: External package ports label May 31, 2022
@benpicco benpicco force-pushed the ndp_opt_timestamp_t branch from 749e177 to 1373c19 Compare May 31, 2022 08:50
miri64
miri64 requested changes May 31, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss #18121 (review) first.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented May 31, 2022

The idea is to get multiple boards in a sensor network synced up with reasonable accuracy. Adding the timestamp to the RA basically gets me that 'for free' (avoiding round-trips with NTP / separate SNTP thread).

I didn't expect to get this PR merged as is, but couldn't RAs as a source of timestamps make sense in an isolated network?

@miri64
Copy link
Copy Markdown
Member

miri64 commented May 31, 2022

The idea is to get multiple boards in a sensor network synced up with reasonable accuracy. Adding the timestamp to the RA basically gets me that 'for free' (avoiding round-trips with NTP / separate SNTP thread).

Not if the timestamp has a completely different use case... The timestamp option, as far as I understand is there to detect repeat attacks with Secure Neighbor Discovery (SEND). If you set your clock from these timestamps (i.e. use these timestamps as your truth), any repeat attacks cannot be detected, in contrast, you use an attackers clock as your truth.

I didn't expect to get this PR merged as is, but couldn't RAs as a source of timestamps make sense in an isolated network?

  1. At the moment this is not an optional module, so as soon as you add timestamp option support, this is how the network will behave.
  2. There needs to be at least a massive boilerplate, that this is not how the option is intended to be used according to RFC 3971, because anyone who would finally implement RFC 3971 (which would be possible even for 6Lo-ND with RFC 8505 as far as I understand), would otherwise face a huge WTF.

@fabian18
Copy link
Copy Markdown
Contributor

fabian18 commented May 31, 2022

anyone who would finally implement RFC 3971 (which would be possible even for 6Lo-ND with RFC 8505 as far as I understand), would otherwise face a huge WTF.

That´s my task btw. I agree with miri. I did not plan to implement this option because of the lack of time sync right now but I would be in favor not to misuse the option.

@benpicco benpicco force-pushed the ndp_opt_timestamp_t branch from 1373c19 to af60a3f Compare May 31, 2022 10:47
@miri64
Copy link
Copy Markdown
Member

miri64 commented May 31, 2022

@benpicco what's with @maribu et al.'s work on timesync? Could that help for your scenario?

@benpicco benpicco force-pushed the ndp_opt_timestamp_t branch from 53c1f54 to f97e982 Compare June 17, 2022 09:35
@maribu
Copy link
Copy Markdown
Member

maribu commented Jun 17, 2022

@benpicco what's with @maribu et al.'s work on timesync? Could that help for your scenario?

Apart from the concerns that this could later result in pain when the timestamp option is used to detect replay attack, I personally would favor an application layer protocol. That would work with any network device using any L2 technology.

Note that since the UDP SOCK API can expose hardware timestamps via the auxiliary data, we can even get accuracy in the order of < µs. I am doing just that for PTP synchronization via UDP multicast. I hope I get this upstream sometime soon :)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 21, 2022

Apart from the concerns that this could later result in pain when the timestamp option is used to detect replay attack, I personally would favor an application layer protocol.

Then it should go into the application layer and not misusing options in the network layer's control protocol ;-). But thanks for the clarification.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 21, 2022

The idea was to get the time sync 'for free' without any additional network traffic.
But I still have to test how well this works in practice.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 21, 2022

The idea was to get the time sync 'for free' without any additional network traffic.

If you are complicating the SEND implementation, I am not sure, you can say it is "for free".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 requested changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@cgundogan cgundogan Awaiting requested review from cgundogan

@fabian18 fabian18 Awaiting requested review from fabian18

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: drivers Area: Device drivers Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benpicco @miri64 @fabian18 @maribu