tls: use monotonic timer for monitoring DTLS peer inactivity #450
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed. ℹ️ To verify your latest change (45e83b9), label this PR with |
Danielius1922
changed the base branch from
master
to
adam/feature/434-fix-time-adjustment
Danielius1922
force-pushed
the
adam/feature/434-fix-time-adjustment-tls-peer
branch
5 times, most recently
from
4577be9 to
2d295c6
Compare
Danielius1922
force-pushed
the
adam/feature/434-fix-time-adjustment-tls-peer
branch
22 times, most recently
from
4f90c22 to
029bd1d
Compare
Danielius1922
force-pushed
the
adam/feature/434-fix-time-adjustment-tls-peer
branch
4 times, most recently
from
900f6a5 to
651a5f2
Compare
Danielius1922
force-pushed
the
adam/feature/434-fix-time-adjustment-tls-peer
branch
from
651a5f2 to
c8006b3
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
Danielius1922
force-pushed
the
adam/feature/434-fix-time-adjustment-tls-peer
branch
from
c8006b3 to
9c9a386
Compare
jkralik
reviewed
May 24, 2023
Comment on lines
+573
to
574
| if ((peer->endpoint.flags & TCP) == 0) { | ||
| mbedtls_ssl_close_notify(&peer->ssl_ctx); |
There was a problem hiding this comment.
From this code it seems to be more general - not only for DTLS (udp tls), but also for TLS (TCP) connection
Comment on lines
2026
to
+2029
| mbedtls_ssl_set_timer_cb(&peer->ssl_ctx, &peer->timer, ssl_set_timer, | ||
| ssl_get_timer); | ||
| oc_ri_add_timed_event_callback_seconds( | ||
| peer, oc_tls_inactive, (oc_clock_time_t)OC_DTLS_INACTIVITY_TIMEOUT); | ||
| oc_ri_add_timed_event_callback_ticks(peer, oc_dtls_inactive, | ||
| DTLS_INACTIVITY_TIMEOUT_TICKS); |
There was a problem hiding this comment.
now I see the limitation to DTLS
Comment on lines
2026
to
+2029
| mbedtls_ssl_set_timer_cb(&peer->ssl_ctx, &peer->timer, ssl_set_timer, | ||
| ssl_get_timer); | ||
| oc_ri_add_timed_event_callback_seconds( | ||
| peer, oc_tls_inactive, (oc_clock_time_t)OC_DTLS_INACTIVITY_TIMEOUT); | ||
| oc_ri_add_timed_event_callback_ticks(peer, oc_dtls_inactive, | ||
| DTLS_INACTIVITY_TIMEOUT_TICKS); |
There was a problem hiding this comment.
Maybe this timer could be used to close also TCP-TLS connection when we implement the closing idle sessions for TCP
not in this PR
There was a problem hiding this comment.
I'm checking the header for mbedtls_ssl_set_timer_cb and it should work for TLS as well. It is mandatory for DTLS, but not for TLS so that's probably the reason why it is like this.
Danielius1922
force-pushed
the
adam/feature/434-fix-time-adjustment-tls-peer
branch
3 times, most recently
from
c701254 to
565abe0
Compare
The previous implementation of monitoring DTLS peers' activity relied on the absolute system time to track the last activity timestamp. However, this approach had the unintended consequence of being affected by changes in the system time. For instance, modifying the system time by an interval equivalent to the DTLS inactivity timeout would trigger a false inactivity timeout and result in the closure of the DTLS session. To address this issue, the monitoring mechanism has been updated to utilize a monotonic timer. This timer is independent of system time changes and provides a more accurate measure of the duration of inactivity. By using a monotonic timer, the DTLS session will only be closed if no activity occurs within the specified timeout period, regardless of any changes in the system time. This refactor improves the reliability and accuracy of DTLS peer monitoring, ensuring that the DTLS sessions are closed based on actual inactivity rather than being influenced by system time adjustments.
Danielius1922
force-pushed
the
adam/feature/434-fix-time-adjustment-tls-peer
branch
from
565abe0 to
469ea8f
Compare
|
SonarCloud Quality Gate failed. |
jkralik
added
OCF Conformance Testing
jkralik
approved these changes
May 25, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.