dtls_time: migrate to ztimer_msec

[fjmolinas]

RIOT is currently migrating from its xtimer to ztimer high level timer abstraction.

The new API allows better sleep management. xtimer was based in microseconds and in this library-port, it was using 64bits. This PR is changing it to use ztimer_msec, so milliseconds based. It's also using 32bits timestamps instead of 64bits used by xtimer.

Since contiki also uses 32bits my assumption is that a 32bit millisecond timer should be enough as a backend for the dtls_time api, is there any reason why this assumption would be wrong?

I have provided test results for this in the PR making this change in RIOT-OS/RIOT, (see RIOT-OS/RIOT#17564). In that PR this change is provided as a patch, but if this PR is accepted I can simply change the commit hash.

[boaks]

Hi Francisco,

for contributions to Eclipse you need a ECA, please read CONTRIBUTING for more.

About 32 bit milliseconds: AFAIK that should work, but requires special care for potential overflow after about 49 days.

dtls.c/dtls_check_retransmit

while (node && node->t <= now)

If I remember well, that should be:

while (node && (now - node->t) > 0)

In the case of an overflow node->t may be something as 0xfffffff8 and now 0x4.

[fjmolinas]

Hi @boaks thanks for your answer, since you seem to point out at least one occurrence where 64bit (or at least no overflow) are assumed, could there be more? I can also leave the values in 64bits, the ztimer_msec has a 64bit version I can use here as well.

[boaks]

I can also leave the values in 64bits, the ztimer_msec has a 64bit version I can use here as well.

FMPOV, a 32 bit retransmission timer should not be too bad. That's only my opinion.

could there be more?

Sure, e.g. netq_insert_node. It was more a remark, that it should be clear, if a 49 day overflow should be considered or not.

[kfessel]

i just searched trough tinydtls

all dtls_ticks( ) uses are in dtls.c
l1692: is used to produce a timestamp in the future when the timeout is reached

l2076: produce 4 bytes of of "random" for the dtls handshake (server) (tries to get seconds -> we are loosing 10Bit)
l2683: same but client
these seem to be done for debugging and should be removed for other cases (4 bytes of unix time are not that random) there is nothing about doing this in the dtls rfc

l4248: inits prng (not relevant to riot it is ignored, initilised by riot)

l4395: calculate another timestamp (timeout) see l1692:

l4458: walks the packet resend queue until now is reached uses the timstamps from (l1692 and l4395) needs to be made 32 bit safe

netqc: netq_insert_node:
does a insertion sort based on the ->t value from l1692: and l4395:

[boaks]

I think, checking for the usage of netq_t.t may also be considered.

[boaks]

(Even, if clock_time_t netq_t.t more or less shows, that the 32 bit overflow may already be an issue.)

[kfessel]

dtls_time.h: l59 has typedef uint32_t clock_time_t -> 32 bit is the default -> these are issues that should be fixed
especially debug data where randomness should be should be removed for default build (l2076 and l2683)

[obgm]

@kfessel Thanks for looking up the uses of the timer functions in the code. It would have been easier to read if you had used the current develop head or generated persistent links.

Regarding the use of 32 bit unix time stamp in ClientHello and ServerHello: DTLS 1.2 is defined as a sort of "update" to TLS 1.2. The code in dtls_send_client_hello() and dtls_send_server_hello() reflects the definition from Section 7.4.1.2 of RFC 5246. Indeed, the opinion on the usefulness of these 32 bits have changed since then (e.g., Section 12 of RFC 7925), and RFC 8446, of course, has a real 32 bit random value. I will provide a fix to remove that part. See commit 68b2521

[kfessel]

Sorry i went a bit quick with the write down. And used current riot checkout

[boaks]

@kfessel
@fjmolinas

just in the case you're interested: for DTLS 1.2 there is a upcoming new feature, RFC9146, which overcomes issues caused by address changes. If you're interested to test that with RIOT, that would be great. The complex part is the server-side, and currently implemented by me in Eclipse/Californium. A simple, client-only implementation is available here in branch feature/connection_id and my PR #108. I will try to update my PR, and maybe afterwards, Olaf may reset the branch to the update PR.

[kfessel]

i created an issue about that 32 Bit overflow concerns: #125

@boaks the feature/connection_id branch seems to carry the same issue

[boaks]

seems to carry the same issue

sure. it was not about testing a fix for this, it was about interest in DTLS CID / RFC9146.
(I misused this PR in order to get your attention ;-) )

[HendrikVE]

@fjmolinas I believe you still need to accept the legal agreement and force push so your changes can be accepted ;)

[boaks]

You still need to register and sign a ECA, please read CONTRIBUTING for more.

Please use the e-mail of your commits for that.

[fjmolinas]

@fjmolinas I believe you still need to accept the legal agreement and force push so your changes can be accepted ;)

With the potential 32bit issues mentioned above, I'm not sure this makes sense now.

[boaks]

With the potential 32bit issues mentioned above, I'm not sure this makes sense now.

Yes.
It would start making sense consider issue #125 and in combination with PR #131.

[fjmolinas]

With the potential 32bit issues mentioned above, I'm not sure this makes sense now.

Yes. It would start making sense consider issue #125 and in combination with PR #131.

So should we wait for that PR then? (fixed the ECA side)

[boaks]

Thanks for signing the ECA!

[kfessel]

with #131 merged the 32bit issue described here by @boaks and in #125 is solved, therefor this would be nice to move forward for better RIOT-OS support.

[obgm]

Yes, step 1 would be resolving the conflict.

[kfessel]

o i didn't see the conflict lets alert @fjmolinas so he is aware of it

[fjmolinas]

Rebased to main!

[boaks]

Not really wrong, but I would prefer to have that listed for the code-cleanup, issue #143

[fjmolinas]

Hmm I think its because I rebased maybe against the wrong branch, I guess I should have done it against develop....

[fjmolinas]

Fixed I wrongly rebased to main which had pulled in unrelated changes.

[boaks]

your right, main has 2022 and develop has 2021.

[fjmolinas]

Since this PR had been opened against develop I kept it that way, or should I re-open against main?

[boaks]

FMPOV, that's a detail of issue #140.
For now it's a "raw idea", to have a "main" and a "develop", mainly to relax the time constraints for team members. Though this project works different with git as I'm used to it, I'm not sure, if my way will the right one. (My way would be, PR against "main", cherrypick in advance to "develop", if indicated by time constraints.)
Anyway, though this PR is dated before #140, I don't think, it's required to recreate a PR against main.

[fjmolinas]

Makes sense, so we can move forward with this one as is.

[boaks]

LGTM

(I merge it to develop, we merge it to main with obgm LGTM).

[boaks]

LGTM

(I merge it to develop, we merge it to main with obgm LGTM).

[fjmolinas]

Thanks for the patience and the review!

[obgm]

Looks good, can be merged into master main

[boaks]

Merged to main (cherrypick).
Done.