Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close TLS and QUIC links on certificate expiration #1564

Merged
merged 23 commits into from
Nov 19, 2024

Conversation

oteffahi
Copy link
Contributor

@oteffahi oteffahi commented Oct 25, 2024

Quic and TLS links currently do not close when remote certificate expires. This PR makes instances monitor the expiration of the remote certificate chain, closing the link at the moment of expiration.

This is configurable via the transport.link.tls.close_link_on_expiration key in the config file. It is set by default to false to maintain the old behavior as default.

Copy link

PR missing one of the required labels: {'bug', 'dependencies', 'documentation', 'breaking-change', 'new feature', 'enhancement', 'internal'}

@oteffahi oteffahi added bug Something isn't working enhancement Existing things could work better and removed bug Something isn't working labels Oct 25, 2024
@oteffahi oteffahi force-pushed the links/close-on-cert-expiration branch from 716f7ae to 65d3b41 Compare October 31, 2024 14:15
@oteffahi
Copy link
Contributor Author

oteffahi commented Nov 8, 2024

Commit efb6cb931fa34ab9ab2df5d4912390684c539432 changes the approach to one expiration task per link instance, as the previous implementation had a design flaw where Weak references to closed links wouldn't be cleaned up if said links were closed by the transport (i.e not closed by expiration task), leading to potential memory exhaustion attacks.

Expiration tasks are relatively low in cost as they sleep for most of their lifecycle.

@oteffahi oteffahi marked this pull request as ready for review November 12, 2024 15:51
@oteffahi oteffahi requested review from fuzzypixelz, Mallets, wyfo and OlivierHecart and removed request for Mallets November 12, 2024 15:51
@oteffahi oteffahi self-assigned this Nov 14, 2024
@oteffahi oteffahi marked this pull request as draft November 15, 2024 12:14
@oteffahi oteffahi marked this pull request as ready for review November 15, 2024 13:25
io/zenoh-link-commons/src/tls.rs Outdated Show resolved Hide resolved
io/zenoh-link-commons/src/tls.rs Outdated Show resolved Hide resolved
io/zenoh-links/zenoh-link-quic/src/unicast.rs Show resolved Hide resolved
io/zenoh-links/zenoh-link-quic/src/unicast.rs Outdated Show resolved Hide resolved
io/zenoh-links/zenoh-link-quic/src/unicast.rs Outdated Show resolved Hide resolved
io/zenoh-link-commons/src/tls.rs Outdated Show resolved Hide resolved
@oteffahi oteffahi requested a review from wyfo November 15, 2024 15:15
@oteffahi
Copy link
Contributor Author

oteffahi commented Nov 19, 2024

In cb0d6c4 I reworked the implementation to dodge the performance issues of block_in_place for expiration_task cleanup, and also addresses another issue that arises from the expiration task and transport potentially calling link.close().await concurrently.

@J-Loudet J-Loudet merged commit 924394c into eclipse-zenoh:main Nov 19, 2024
13 checks passed
@oteffahi oteffahi changed the title Close TLS and Quic links on certificate expiration Close TLS and QUIC links on certificate expiration Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Existing things could work better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants