From fb0cb1ce71a14b8109c828a8d9fcfc15b84e6a09 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Wed, 1 Mar 2023 15:10:46 -0600 Subject: [PATCH 1/5] [feat] Add hostname verification config to ClusterData --- .../pulsar/broker/service/BrokerService.java | 16 ++++++++++++---- .../pulsar/common/policies/data/ClusterData.java | 4 ++++ .../org/apache/pulsar/admin/cli/CmdClusters.java | 7 +++++++ .../apache/pulsar/admin/cli/PulsarAdminTool.java | 4 ++-- .../common/policies/data/ClusterDataImpl.java | 13 +++++++++++++ .../policies/data/ClusterDataImplTest.java | 1 + .../org/apache/pulsar/functions/LocalRunner.java | 4 ++-- 7 files changed, 41 insertions(+), 8 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java index 15d48e59849b6..b6bd12ce7aba9 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java @@ -1295,6 +1295,10 @@ public PulsarClient getReplicationClient(String cluster, Optional c } String serviceUrlTls = isNotBlank(data.getBrokerServiceUrlTls()) ? data.getBrokerServiceUrlTls() : data.getServiceUrlTls(); + // In the event that the zookeeper data has not been updated, allow local broker config to + // override the cluster config. + final boolean hostnameVerificationEnabled = data.isTlsHostnameVerificationEnabled() + || pulsar.getConfiguration().isTlsHostnameVerificationEnabled(); if (data.isBrokerClientTlsEnabled()) { configTlsSettings(clientBuilder, serviceUrlTls, data.isBrokerClientTlsEnabledWithKeyStore(), @@ -1308,7 +1312,7 @@ public PulsarClient getReplicationClient(String cluster, Optional c data.getBrokerClientTrustCertsFilePath(), data.getBrokerClientKeyFilePath(), data.getBrokerClientCertificateFilePath(), - pulsar.getConfiguration().isTlsHostnameVerificationEnabled() + hostnameVerificationEnabled ); } else if (pulsar.getConfiguration().isBrokerClientTlsEnabled()) { configTlsSettings(clientBuilder, serviceUrlTls, @@ -1323,7 +1327,7 @@ public PulsarClient getReplicationClient(String cluster, Optional c pulsar.getConfiguration().getBrokerClientTrustCertsFilePath(), pulsar.getConfiguration().getBrokerClientKeyFilePath(), pulsar.getConfiguration().getBrokerClientCertificateFilePath(), - pulsar.getConfiguration().isTlsHostnameVerificationEnabled() + hostnameVerificationEnabled ); } else { clientBuilder.serviceUrl( @@ -1432,6 +1436,10 @@ public PulsarAdmin getClusterPulsarAdmin(String cluster, Optional c } String adminApiUrl = isTlsEnabled ? data.getServiceUrlTls() : data.getServiceUrl(); builder.serviceHttpUrl(adminApiUrl); + // In the event that the zookeeper data has not been updated, allow local broker config to + // override the cluster config. + final boolean hostnameVerificationEnabled = data.isTlsHostnameVerificationEnabled() + || pulsar.getConfiguration().isTlsHostnameVerificationEnabled(); if (data.isBrokerClientTlsEnabled()) { configAdminTlsSettings(builder, data.isBrokerClientTlsEnabledWithKeyStore(), @@ -1445,7 +1453,7 @@ public PulsarAdmin getClusterPulsarAdmin(String cluster, Optional c data.getBrokerClientTrustCertsFilePath(), data.getBrokerClientKeyFilePath(), data.getBrokerClientCertificateFilePath(), - pulsar.getConfiguration().isTlsHostnameVerificationEnabled() + hostnameVerificationEnabled ); } else if (conf.isBrokerClientTlsEnabled()) { configAdminTlsSettings(builder, @@ -1460,7 +1468,7 @@ public PulsarAdmin getClusterPulsarAdmin(String cluster, Optional c conf.getBrokerClientTrustCertsFilePath(), conf.getBrokerClientKeyFilePath(), conf.getBrokerClientCertificateFilePath(), - pulsar.getConfiguration().isTlsHostnameVerificationEnabled() + hostnameVerificationEnabled ); } diff --git a/pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/ClusterData.java b/pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/ClusterData.java index 212a1575f9934..c298940c6fa02 100644 --- a/pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/ClusterData.java +++ b/pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/ClusterData.java @@ -48,6 +48,8 @@ public interface ClusterData { boolean isTlsAllowInsecureConnection(); + boolean isTlsHostnameVerificationEnabled(); + boolean isBrokerClientTlsEnabledWithKeyStore(); String getBrokerClientTlsTrustStoreType(); @@ -97,6 +99,8 @@ interface Builder { Builder tlsAllowInsecureConnection(boolean enabled); + Builder tlsHostnameVerificationEnabled(boolean enabled); + Builder brokerClientTlsEnabledWithKeyStore(boolean enabled); Builder brokerClientTlsTrustStoreType(String trustStoreType); diff --git a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdClusters.java b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdClusters.java index 173595c9b19a4..96e228e67d211 100644 --- a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdClusters.java +++ b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdClusters.java @@ -320,6 +320,10 @@ abstract class ClusterDetailsCommand extends BaseCommand { @Parameter(names = "--tls-allow-insecure", description = "Allow insecure tls connection", required = false) protected Boolean tlsAllowInsecureConnection; + @Parameter(names = "--hostname-verification-enabled", description = "Enable hostname verification", + required = false) + protected Boolean tlsHostnameVerificationEnabled; + @Parameter(names = "--tls-enable-keystore", description = "Whether use KeyStore type to authenticate", required = false) protected Boolean brokerClientTlsEnabledWithKeyStore; @@ -411,6 +415,9 @@ void processArguments() throws Exception { if (tlsAllowInsecureConnection != null) { builder.tlsAllowInsecureConnection(tlsAllowInsecureConnection); } + if (tlsHostnameVerificationEnabled != null) { + builder.tlsHostnameVerificationEnabled(tlsHostnameVerificationEnabled); + } if (brokerClientTlsEnabledWithKeyStore != null) { builder.brokerClientTlsEnabledWithKeyStore(brokerClientTlsEnabledWithKeyStore); } diff --git a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java index 7a7a4c4b44449..93e1414b63cca 100644 --- a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java +++ b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java @@ -82,8 +82,8 @@ public static class RootParams { @Parameter(names = { "--tls-trust-cert-path" }, description = "Allow TLS trust cert file path") String tlsTrustCertsFilePath; - @Parameter(names = { "--tls-enable-hostname-verification" }, - description = "Enable TLS common name verification") + @Parameter(names = { "--tls-enable-hostname-verification", "--hostname-verification-enabled" }, + description = "Enable TLS hostname verification") Boolean tlsEnableHostnameVerification; @Parameter(names = {"--tls-provider"}, description = "Set up TLS provider. " diff --git a/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java b/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java index 2ca75245a8c22..625f85583dc2e 100644 --- a/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java +++ b/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java @@ -109,6 +109,11 @@ public final class ClusterDataImpl implements ClusterData, Cloneable { + " authority." ) private boolean tlsAllowInsecureConnection; + @ApiModelProperty( + name = "tlsHostnameVerificationEnabled", + value = "Enable TLS hostname verification" + ) + private boolean tlsHostnameVerificationEnabled = false; @ApiModelProperty( name = "brokerClientTlsEnabledWithKeyStore", value = "Whether internal client use KeyStore type to authenticate with other Pulsar brokers" @@ -203,6 +208,7 @@ public ClusterDataImplBuilder clone() { .peerClusterNames(peerClusterNames) .brokerClientTlsEnabled(brokerClientTlsEnabled) .tlsAllowInsecureConnection(tlsAllowInsecureConnection) + .tlsHostnameVerificationEnabled(tlsHostnameVerificationEnabled) .brokerClientTlsEnabledWithKeyStore(brokerClientTlsEnabledWithKeyStore) .brokerClientTlsTrustStoreType(brokerClientTlsTrustStoreType) .brokerClientTlsTrustStore(brokerClientTlsTrustStore) @@ -231,6 +237,7 @@ public static class ClusterDataImplBuilder implements ClusterData.Builder { private LinkedHashSet peerClusterNames; private boolean brokerClientTlsEnabled = false; private boolean tlsAllowInsecureConnection = false; + private boolean tlsHostnameVerificationEnabled = false; private boolean brokerClientTlsEnabledWithKeyStore = false; private String brokerClientTlsTrustStoreType = "JKS"; private String brokerClientTlsTrustStore; @@ -303,6 +310,11 @@ public ClusterDataImplBuilder tlsAllowInsecureConnection(boolean tlsAllowInsecur return this; } + public ClusterDataImplBuilder tlsHostnameVerificationEnabled(boolean tlsHostnameVerificationEnabled) { + this.tlsHostnameVerificationEnabled = tlsHostnameVerificationEnabled; + return this; + } + public ClusterDataImplBuilder brokerClientTlsEnabledWithKeyStore(boolean brokerClientTlsEnabledWithKeyStore) { this.brokerClientTlsEnabledWithKeyStore = brokerClientTlsEnabledWithKeyStore; return this; @@ -387,6 +399,7 @@ public ClusterDataImpl build() { peerClusterNames, brokerClientTlsEnabled, tlsAllowInsecureConnection, + tlsHostnameVerificationEnabled, brokerClientTlsEnabledWithKeyStore, brokerClientTlsTrustStoreType, brokerClientTlsTrustStore, diff --git a/pulsar-common/src/test/java/org/apache/pulsar/common/policies/data/ClusterDataImplTest.java b/pulsar-common/src/test/java/org/apache/pulsar/common/policies/data/ClusterDataImplTest.java index ca4cba2cf9749..1c2f2dd65999b 100644 --- a/pulsar-common/src/test/java/org/apache/pulsar/common/policies/data/ClusterDataImplTest.java +++ b/pulsar-common/src/test/java/org/apache/pulsar/common/policies/data/ClusterDataImplTest.java @@ -42,6 +42,7 @@ public void verifyClone() { .peerClusterNames(new LinkedHashSet<>()) .brokerClientTlsEnabled(true) .tlsAllowInsecureConnection(false) + .tlsHostnameVerificationEnabled(true) .brokerClientTlsEnabledWithKeyStore(true) .brokerClientTlsTrustStoreType("JKS") .brokerClientTlsTrustStore("/my/trust/store") diff --git a/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java b/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java index ed9b0af3b43d8..b0615f458ad5f 100644 --- a/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java +++ b/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java @@ -179,8 +179,8 @@ public RuntimeEnv convert(String value) { @Parameter(names = "--tlsAllowInsecureConnection", description = "Allow insecure tls connection\n", hidden = true, arity = 1) protected boolean tlsAllowInsecureConnection; - @Parameter(names = "--tlsHostNameVerificationEnabled", description = "Enable hostname verification", hidden = true - , arity = 1) + @Parameter(names = {"--tlsHostNameVerificationEnabled", "--tlsHostnameVerificationEnabled"}, + description = "Enable hostname verification", hidden = true, arity = 1) protected boolean tlsHostNameVerificationEnabled; @Parameter(names = "--tlsTrustCertFilePath", description = "tls trust cert file path", hidden = true) protected String tlsTrustCertFilePath; From 2cff8e198d01e9ff3ae506757043b1031dc06833 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Tue, 9 May 2023 22:18:28 -0500 Subject: [PATCH 2/5] Update default to true for PIP --- .../apache/pulsar/common/policies/data/ClusterDataImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java b/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java index 625f85583dc2e..b822f31f52a17 100644 --- a/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java +++ b/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java @@ -113,7 +113,7 @@ public final class ClusterDataImpl implements ClusterData, Cloneable { name = "tlsHostnameVerificationEnabled", value = "Enable TLS hostname verification" ) - private boolean tlsHostnameVerificationEnabled = false; + private boolean tlsHostnameVerificationEnabled = true; @ApiModelProperty( name = "brokerClientTlsEnabledWithKeyStore", value = "Whether internal client use KeyStore type to authenticate with other Pulsar brokers" @@ -237,7 +237,7 @@ public static class ClusterDataImplBuilder implements ClusterData.Builder { private LinkedHashSet peerClusterNames; private boolean brokerClientTlsEnabled = false; private boolean tlsAllowInsecureConnection = false; - private boolean tlsHostnameVerificationEnabled = false; + private boolean tlsHostnameVerificationEnabled = true; private boolean brokerClientTlsEnabledWithKeyStore = false; private String brokerClientTlsTrustStoreType = "JKS"; private String brokerClientTlsTrustStore; From 5a7cde1db19de188936cd63d4de92142d5a106e7 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Tue, 9 May 2023 22:20:57 -0500 Subject: [PATCH 3/5] Simplify geo-replication client configuration --- .../org/apache/pulsar/broker/service/BrokerService.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java index b6bd12ce7aba9..b2190e830f09a 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java @@ -1295,10 +1295,6 @@ public PulsarClient getReplicationClient(String cluster, Optional c } String serviceUrlTls = isNotBlank(data.getBrokerServiceUrlTls()) ? data.getBrokerServiceUrlTls() : data.getServiceUrlTls(); - // In the event that the zookeeper data has not been updated, allow local broker config to - // override the cluster config. - final boolean hostnameVerificationEnabled = data.isTlsHostnameVerificationEnabled() - || pulsar.getConfiguration().isTlsHostnameVerificationEnabled(); if (data.isBrokerClientTlsEnabled()) { configTlsSettings(clientBuilder, serviceUrlTls, data.isBrokerClientTlsEnabledWithKeyStore(), @@ -1312,7 +1308,7 @@ public PulsarClient getReplicationClient(String cluster, Optional c data.getBrokerClientTrustCertsFilePath(), data.getBrokerClientKeyFilePath(), data.getBrokerClientCertificateFilePath(), - hostnameVerificationEnabled + data.isTlsHostnameVerificationEnabled() ); } else if (pulsar.getConfiguration().isBrokerClientTlsEnabled()) { configTlsSettings(clientBuilder, serviceUrlTls, @@ -1327,7 +1323,7 @@ public PulsarClient getReplicationClient(String cluster, Optional c pulsar.getConfiguration().getBrokerClientTrustCertsFilePath(), pulsar.getConfiguration().getBrokerClientKeyFilePath(), pulsar.getConfiguration().getBrokerClientCertificateFilePath(), - hostnameVerificationEnabled + pulsar.getConfiguration().isTlsHostnameVerificationEnabled() ); } else { clientBuilder.serviceUrl( From a9af5418336091bbcd5de29556d8c117e9fe6a61 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Tue, 9 May 2023 22:23:23 -0500 Subject: [PATCH 4/5] Simplify geo-replication client configuration again --- .../org/apache/pulsar/broker/service/BrokerService.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java index b2190e830f09a..ca76a86659141 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java @@ -1432,10 +1432,6 @@ public PulsarAdmin getClusterPulsarAdmin(String cluster, Optional c } String adminApiUrl = isTlsEnabled ? data.getServiceUrlTls() : data.getServiceUrl(); builder.serviceHttpUrl(adminApiUrl); - // In the event that the zookeeper data has not been updated, allow local broker config to - // override the cluster config. - final boolean hostnameVerificationEnabled = data.isTlsHostnameVerificationEnabled() - || pulsar.getConfiguration().isTlsHostnameVerificationEnabled(); if (data.isBrokerClientTlsEnabled()) { configAdminTlsSettings(builder, data.isBrokerClientTlsEnabledWithKeyStore(), @@ -1449,7 +1445,7 @@ public PulsarAdmin getClusterPulsarAdmin(String cluster, Optional c data.getBrokerClientTrustCertsFilePath(), data.getBrokerClientKeyFilePath(), data.getBrokerClientCertificateFilePath(), - hostnameVerificationEnabled + data.isTlsHostnameVerificationEnabled() ); } else if (conf.isBrokerClientTlsEnabled()) { configAdminTlsSettings(builder, @@ -1464,7 +1460,7 @@ public PulsarAdmin getClusterPulsarAdmin(String cluster, Optional c conf.getBrokerClientTrustCertsFilePath(), conf.getBrokerClientKeyFilePath(), conf.getBrokerClientCertificateFilePath(), - hostnameVerificationEnabled + conf.isTlsHostnameVerificationEnabled() ); } From 66018289213c96f995a9b0d29a5cf23c536873a3 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Tue, 9 May 2023 22:27:28 -0500 Subject: [PATCH 5/5] Defer changing argument names to another PR --- .../java/org/apache/pulsar/admin/cli/PulsarAdminTool.java | 4 ++-- .../main/java/org/apache/pulsar/functions/LocalRunner.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java index 93e1414b63cca..7a7a4c4b44449 100644 --- a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java +++ b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java @@ -82,8 +82,8 @@ public static class RootParams { @Parameter(names = { "--tls-trust-cert-path" }, description = "Allow TLS trust cert file path") String tlsTrustCertsFilePath; - @Parameter(names = { "--tls-enable-hostname-verification", "--hostname-verification-enabled" }, - description = "Enable TLS hostname verification") + @Parameter(names = { "--tls-enable-hostname-verification" }, + description = "Enable TLS common name verification") Boolean tlsEnableHostnameVerification; @Parameter(names = {"--tls-provider"}, description = "Set up TLS provider. " diff --git a/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java b/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java index b0615f458ad5f..ed9b0af3b43d8 100644 --- a/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java +++ b/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java @@ -179,8 +179,8 @@ public RuntimeEnv convert(String value) { @Parameter(names = "--tlsAllowInsecureConnection", description = "Allow insecure tls connection\n", hidden = true, arity = 1) protected boolean tlsAllowInsecureConnection; - @Parameter(names = {"--tlsHostNameVerificationEnabled", "--tlsHostnameVerificationEnabled"}, - description = "Enable hostname verification", hidden = true, arity = 1) + @Parameter(names = "--tlsHostNameVerificationEnabled", description = "Enable hostname verification", hidden = true + , arity = 1) protected boolean tlsHostNameVerificationEnabled; @Parameter(names = "--tlsTrustCertFilePath", description = "tls trust cert file path", hidden = true) protected String tlsTrustCertFilePath;