feat!: renaming of TLS configuration parameters #1503

gabrik · 2024-10-03T10:31:21Z

Names in TLS configuration can be improved, currently we have:

server_certificate / server_certificate -> used when listening TLS/QUIC
client_certificate / client_private_key -> used when connecting TLS/QUIC
server_name_verification / client_auth -> used configure behaviour in connect/listen

The concept of server and client are never mentioned in zenoh docs/configuration/examples, so it can be a bit misleading.

I suggest to rename: server => listen and client => connect for certificate/key, server_name_verification => verify_name_on_connect and client_auth => enable_mtls

So this will make clear that if you want to listen TLS/QUIC you need a listen certificate, when you want to connect you need a connect certificate.

Summarizing this PR introduces the following changes:

server_certificate / server_private_key -> listen_certificate/listen_private_key
client_certificate / client_private_key -> connect_certificate/connect_private_key
server_name_verification -> verify_name_on_connect
client_auth -> enable_mtls

Sister PRs:

feat!: rename TLS configuration parameters atolab/zenoh-web#70

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

JEnoch · 2024-10-03T10:50:04Z

DEFAULT_CONFIG.json5

        // This could be dangerous because your CA can have signed a server cert for foo.com, that's later being used to host a server at baz.com. If you wan't your
        // ca to verify that the server at baz.com is actually baz.com, let this be true (default).
-        server_name_verification: null,
+        verify_name_on_connect: null,


The comments above indicate the value for thic config can be false or true (default).
But here it's null. Why ?

TBH, I do not know, let me set it as true

JEnoch · 2024-10-03T10:53:34Z

io/zenoh-links/zenoh-link-quic/src/lib.rs

-    pub const TLS_SERVER_NAME_VERIFICATION: &str = "server_name_verification";
-    pub const TLS_SERVER_NAME_VERIFICATION_DEFAULT: &str = "true";
+    pub const TLS_VERIFY_NAME_ON_CONNECT: &str = "verify_name_on_connect";
+    pub const TLS_VERIFY_NAME_ON_CONNECT_DEFAULT: &str = "true";


This TLS_VERIFY_NAME_ON_CONNECT_DEFAULT const seems never used, and doesn't exist in zenoh-link-tls

Yep I noticed it while doing the changes. I think in both QUIC and TLS we should use the default value when getting from the conf.

I'll fix it

gabrik · 2024-10-03T10:58:41Z

@RemiBarthe 👀

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

JEnoch · 2024-10-03T12:58:26Z

DEFAULT_CONFIG.json5

+        enable_mtls: false,
+        /// Path to the TLS connecting side private key
+        connect_private_key: null,
+        /// Path to the TLS client connecting side certificate


Remove client in comment.

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

oteffahi

Some error messages were not updated (I may also have missed some).
Other than that LGTM.

oteffahi · 2024-10-04T09:47:44Z

io/zenoh-links/zenoh-link-quic/src/utils.rs

            };
        }

-        match (c.client_private_key(), c.client_private_key_base64()) {
+        match (c.connect_private_key(), c.connect_private_key_base64()) {
            (Some(_), Some(_)) => {
                bail!("Only one between 'client_private_key' and 'client_private_key_base64' can be present!")


Update error message with new keys

oteffahi · 2024-10-04T09:47:50Z

io/zenoh-links/zenoh-link-quic/src/utils.rs

                    client_private_key.expose_secret(),
                ));
            }
            _ => {}
        }

-        match (c.client_certificate(), c.client_certificate_base64()) {
+        match (c.connect_certificate(), c.connect_certificate_base64()) {
            (Some(_), Some(_)) => {
                bail!("Only one between 'client_certificate' and 'client_certificate_base64' can be present!")


Update error message with new keys

oteffahi · 2024-10-04T09:48:12Z

io/zenoh-links/zenoh-link-quic/src/utils.rs

@@ -150,7 +151,7 @@ pub(crate) struct TlsServerConfig {

 impl TlsServerConfig {
    pub async fn new(config: &Config<'_>) -> ZResult<TlsServerConfig> {
-        let tls_server_client_auth: bool = match config.get(TLS_CLIENT_AUTH) {
+        let tls_server_client_auth: bool = match config.get(TLS_ENABLE_MTLS) {
            Some(s) => s
                .parse()
                .map_err(|_| zerror!("Unknown client auth argument: {}", s))?,


Update error message

oteffahi · 2024-10-04T09:48:21Z

io/zenoh-links/zenoh-link-quic/src/utils.rs

@@ -248,14 +249,14 @@ pub(crate) struct TlsClientConfig {

 impl TlsClientConfig {
    pub async fn new(config: &Config<'_>) -> ZResult<TlsClientConfig> {
-        let tls_client_server_auth: bool = match config.get(TLS_CLIENT_AUTH) {
+        let tls_client_server_auth: bool = match config.get(TLS_ENABLE_MTLS) {
            Some(s) => s
                .parse()
                .map_err(|_| zerror!("Unknown client auth argument: {}", s))?,


Update error message

oteffahi · 2024-10-04T09:48:33Z

io/zenoh-links/zenoh-link-tls/src/utils.rs

@@ -64,83 +64,84 @@ impl ConfigurationInspector<ZenohConfig> for TlsConfigurator {
            _ => {}
        }

-        match (c.server_private_key(), c.server_private_key_base64()) {
+        match (c.listen_private_key(), c.listen_private_key_base64()) {
            (Some(_), Some(_)) => {
                bail!("Only one between 'server_private_key' and 'server_private_key_base64' can be present!")


Update error message

oteffahi · 2024-10-04T09:48:57Z

io/zenoh-links/zenoh-link-tls/src/utils.rs

                    client_private_key.expose_secret(),
                ));
            }
            _ => {}
        }

-        match (c.client_certificate(), c.client_certificate_base64()) {
+        match (c.connect_certificate(), c.connect_certificate_base64()) {
            (Some(_), Some(_)) => {
                bail!("Only one between 'client_certificate' and 'client_certificate_base64' can be present!")


Update error message

oteffahi · 2024-10-04T09:49:05Z

io/zenoh-links/zenoh-link-tls/src/utils.rs

@@ -152,7 +153,7 @@ pub(crate) struct TlsServerConfig {

 impl TlsServerConfig {
    pub async fn new(config: &Config<'_>) -> ZResult<TlsServerConfig> {
-        let tls_server_client_auth: bool = match config.get(TLS_CLIENT_AUTH) {
+        let tls_server_client_auth: bool = match config.get(TLS_ENABLE_MTLS) {
            Some(s) => s
                .parse()
                .map_err(|_| zerror!("Unknown client auth argument: {}", s))?,


Update error message

oteffahi · 2024-10-04T09:49:11Z

io/zenoh-links/zenoh-link-tls/src/utils.rs

@@ -250,14 +251,14 @@ pub(crate) struct TlsClientConfig {

 impl TlsClientConfig {
    pub async fn new(config: &Config<'_>) -> ZResult<TlsClientConfig> {
-        let tls_client_server_auth: bool = match config.get(TLS_CLIENT_AUTH) {
+        let tls_client_server_auth: bool = match config.get(TLS_ENABLE_MTLS) {
            Some(s) => s
                .parse()
                .map_err(|_| zerror!("Unknown client auth argument: {}", s))?,


Update error message

oteffahi · 2024-10-04T09:50:37Z

io/zenoh-links/zenoh-link-quic/src/utils.rs

@@ -62,83 +62,84 @@ impl ConfigurationInspector<ZenohConfig> for TlsConfigurator {
            _ => {}
        }

-        match (c.server_private_key(), c.server_private_key_base64()) {
+        match (c.listen_private_key(), c.listen_private_key_base64()) {
            (Some(_), Some(_)) => {
                bail!("Only one between 'server_private_key' and 'server_private_key_base64' can be present!")


Update error message

oteffahi · 2024-10-04T09:50:42Z

io/zenoh-links/zenoh-link-quic/src/utils.rs

                    server_private_key.expose_secret(),
                ));
            }
            _ => {}
        }

-        match (c.server_certificate(), c.server_certificate_base64()) {
+        match (c.listen_certificate(), c.listen_certificate_base64()) {
            (Some(_), Some(_)) => {
                bail!("Only one between 'server_certificate' and 'server_certificate_base64' can be present!")


Update error message

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

oteffahi

LGTM

J-Loudet

LGTM

feat!: renaming of TLS configuration parameters

12a74cf

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

gabrik added the breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) label Oct 3, 2024

chore: fix typo

898b975

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

gabrik requested review from Charles-Schleich and OlivierHecart October 3, 2024 10:32

gabrik marked this pull request as ready for review October 3, 2024 10:32

JEnoch reviewed Oct 3, 2024

View reviewed changes

gabrik added 2 commits October 3, 2024 13:01

fix: setting proper default value in default config file

3d5da78

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

fix: using default values for verify_name_on_connect

2226121

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

gabrik force-pushed the feat/rename-tls-config branch from 22f2b3b to 2226121 Compare October 3, 2024 11:05

gabrik requested a review from JEnoch October 3, 2024 11:11

gabrik mentioned this pull request Oct 3, 2024

feat!: rename TLS configuration parameters atolab/zenoh-web#70

Merged

JEnoch requested changes Oct 3, 2024

View reviewed changes

chore: fix default config

22ee3f9

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

gabrik requested a review from JEnoch October 3, 2024 13:27

oteffahi suggested changes Oct 4, 2024

View reviewed changes

gabrik added 3 commits October 4, 2024 12:30

chore: updating error strings

bf10a91

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

chore: updating error strings

55f41b2

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

style: make cargo fmt happy

d82231c

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>

gabrik requested review from oteffahi and removed request for JEnoch, OlivierHecart and Charles-Schleich October 4, 2024 13:07

oteffahi approved these changes Oct 4, 2024

View reviewed changes

J-Loudet approved these changes Oct 4, 2024

View reviewed changes

gabrik merged commit 7e32e72 into main Oct 4, 2024
24 checks passed

gabrik deleted the feat/rename-tls-config branch October 4, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: renaming of TLS configuration parameters #1503

feat!: renaming of TLS configuration parameters #1503

gabrik commented Oct 3, 2024 •

edited

Loading

JEnoch Oct 3, 2024

gabrik Oct 3, 2024

JEnoch Oct 3, 2024 •

edited

Loading

gabrik Oct 3, 2024

gabrik commented Oct 3, 2024

JEnoch Oct 3, 2024

oteffahi left a comment

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi Oct 4, 2024

oteffahi left a comment

J-Loudet left a comment

feat!: renaming of TLS configuration parameters #1503

feat!: renaming of TLS configuration parameters #1503

Conversation

gabrik commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JEnoch Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrik commented Oct 3, 2024

Choose a reason for hiding this comment

oteffahi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oteffahi left a comment

Choose a reason for hiding this comment

J-Loudet left a comment

Choose a reason for hiding this comment

gabrik commented Oct 3, 2024 •

edited

Loading

JEnoch Oct 3, 2024 •

edited

Loading