From 074343f8a875e7151b14670593cabfa5c01b0733 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Thu, 10 Nov 2022 14:06:02 +0800 Subject: [PATCH 1/7] [fix][client] Fix the Windows absolute path not recognized in auth param string ### Motivation When the auth param string contains a key-value pair whose value is a Windows absolute path like: ``` keyStorePath:C:\path\to\client.keystore.jks ``` The path will be discarded. See https://github.com/apache/pulsar/blob/06506761aff0eae4acbffa68c3431d2c96affe29/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java#L46-L49 ### Modifications When a key-value string can be split into more than 2 tokens by ':', treat the 1st token as the key, and the following part as the value. In addition, fix some tests cannot run on Windows because of the `URL#getPath` might return something like "/C:/path/to/file". Add a `ResourceUtils` class to convert it to the absolute path like "C:\path\to\file". --- .../auth/MockedPulsarServiceBaseTest.java | 14 ++++----- .../apache/pulsar/utils/ResourceUtils.java | 31 +++++++++++++++++++ .../client/impl/AuthenticationUtil.java | 7 +++-- 3 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 pulsar-broker/src/test/java/org/apache/pulsar/utils/ResourceUtils.java diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java index 8b3d8eb6cb7c2..187b804bd1d26 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java @@ -27,7 +27,6 @@ import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; import com.google.common.collect.Sets; -import com.google.common.io.Resources; import com.google.common.util.concurrent.MoreExecutors; import io.netty.channel.EventLoopGroup; import java.lang.reflect.Field; @@ -79,6 +78,7 @@ import org.apache.pulsar.metadata.api.extended.MetadataStoreExtended; import org.apache.pulsar.metadata.impl.ZKMetadataStore; import org.apache.pulsar.tests.TestRetrySupport; +import org.apache.pulsar.utils.ResourceUtils; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.MockZooKeeper; import org.apache.zookeeper.MockZooKeeperSession; @@ -93,20 +93,20 @@ */ public abstract class MockedPulsarServiceBaseTest extends TestRetrySupport { public final static String BROKER_KEYSTORE_FILE_PATH = - Resources.getResource("certificate-authority/jks/broker.keystore.jks").getPath(); + ResourceUtils.getPath("certificate-authority/jks/broker.keystore.jks"); public final static String BROKER_TRUSTSTORE_FILE_PATH = - Resources.getResource("certificate-authority/jks/broker.truststore.jks").getPath(); + ResourceUtils.getPath("certificate-authority/jks/broker.truststore.jks"); public final static String BROKER_TRUSTSTORE_NO_PASSWORD_FILE_PATH = - Resources.getResource("certificate-authority/jks/broker.truststore.nopassword.jks").getPath(); + ResourceUtils.getPath("certificate-authority/jks/broker.truststore.nopassword.jks"); public final static String BROKER_KEYSTORE_PW = "111111"; public final static String BROKER_TRUSTSTORE_PW = "111111"; public final static String CLIENT_KEYSTORE_FILE_PATH = - Resources.getResource("certificate-authority/jks/client.keystore.jks").getPath(); + ResourceUtils.getPath("certificate-authority/jks/client.keystore.jks"); public final static String CLIENT_TRUSTSTORE_FILE_PATH = - Resources.getResource("certificate-authority/jks/client.truststore.jks").getPath(); + ResourceUtils.getPath("certificate-authority/jks/client.truststore.jks"); public final static String CLIENT_TRUSTSTORE_NO_PASSWORD_FILE_PATH = - Resources.getResource("certificate-authority/jks/client.truststore.nopassword.jks").getPath(); + ResourceUtils.getPath("certificate-authority/jks/client.truststore.nopassword.jks"); public final static String CLIENT_KEYSTORE_PW = "111111"; public final static String CLIENT_TRUSTSTORE_PW = "111111"; diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/utils/ResourceUtils.java b/pulsar-broker/src/test/java/org/apache/pulsar/utils/ResourceUtils.java new file mode 100644 index 0000000000000..ea2a03aca7a7d --- /dev/null +++ b/pulsar-broker/src/test/java/org/apache/pulsar/utils/ResourceUtils.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.utils; + +import com.google.common.io.Resources; +import java.io.File; + +public class ResourceUtils { + + public static String getPath(String resourceName) { + // On Windows, URL#getPath might return a string that starts with a disk name, e.g. "/C:/" + // It's invalid to use this path to open a file, so we need to get the absolute path via File. + return new File(Resources.getResource(resourceName).getPath()).getAbsolutePath(); + } +} diff --git a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java index 45546bc6685a9..539fcf5722ae4 100644 --- a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java +++ b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java @@ -43,9 +43,10 @@ public static Map configureFromPulsar1AuthParamString(String aut if (isNotBlank(authParamsString)) { String[] params = authParamsString.split(","); for (String p : params) { - String[] kv = p.split(":"); - if (kv.length == 2) { - authParams.put(kv[0], kv[1]); + // The value could be a file path, which could contain a colon like "C:\\path\\to\\file" on Windows. + int index = p.indexOf(':'); + if (index >= 0 && (index + 1) < p.length()) { + authParams.put(p.substring(0, index), p.substring(index + 1)); } } } From 11d33314d8cadb43c536a84b26cb9e03d6dfcb86 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Thu, 10 Nov 2022 16:26:45 +0800 Subject: [PATCH 2/7] Add unit tests --- .../auth/MockedPulsarServiceBaseTest.java | 12 +++--- .../apache/pulsar/utils/ResourceUtils.java | 2 +- .../client/impl/AuthenticationUtil.java | 6 ++- .../client/impl/AuthenticationUtilTest.java | 37 +++++++++++++++++++ 4 files changed, 48 insertions(+), 9 deletions(-) create mode 100644 pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java index 187b804bd1d26..d035320b680b1 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java @@ -93,20 +93,20 @@ */ public abstract class MockedPulsarServiceBaseTest extends TestRetrySupport { public final static String BROKER_KEYSTORE_FILE_PATH = - ResourceUtils.getPath("certificate-authority/jks/broker.keystore.jks"); + ResourceUtils.getAbsolutePath("certificate-authority/jks/broker.keystore.jks"); public final static String BROKER_TRUSTSTORE_FILE_PATH = - ResourceUtils.getPath("certificate-authority/jks/broker.truststore.jks"); + ResourceUtils.getAbsolutePath("certificate-authority/jks/broker.truststore.jks"); public final static String BROKER_TRUSTSTORE_NO_PASSWORD_FILE_PATH = - ResourceUtils.getPath("certificate-authority/jks/broker.truststore.nopassword.jks"); + ResourceUtils.getAbsolutePath("certificate-authority/jks/broker.truststore.nopassword.jks"); public final static String BROKER_KEYSTORE_PW = "111111"; public final static String BROKER_TRUSTSTORE_PW = "111111"; public final static String CLIENT_KEYSTORE_FILE_PATH = - ResourceUtils.getPath("certificate-authority/jks/client.keystore.jks"); + ResourceUtils.getAbsolutePath("certificate-authority/jks/client.keystore.jks"); public final static String CLIENT_TRUSTSTORE_FILE_PATH = - ResourceUtils.getPath("certificate-authority/jks/client.truststore.jks"); + ResourceUtils.getAbsolutePath("certificate-authority/jks/client.truststore.jks"); public final static String CLIENT_TRUSTSTORE_NO_PASSWORD_FILE_PATH = - ResourceUtils.getPath("certificate-authority/jks/client.truststore.nopassword.jks"); + ResourceUtils.getAbsolutePath("certificate-authority/jks/client.truststore.nopassword.jks"); public final static String CLIENT_KEYSTORE_PW = "111111"; public final static String CLIENT_TRUSTSTORE_PW = "111111"; diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/utils/ResourceUtils.java b/pulsar-broker/src/test/java/org/apache/pulsar/utils/ResourceUtils.java index ea2a03aca7a7d..0e8f5c6d8c05e 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/utils/ResourceUtils.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/utils/ResourceUtils.java @@ -23,7 +23,7 @@ public class ResourceUtils { - public static String getPath(String resourceName) { + public static String getAbsolutePath(String resourceName) { // On Windows, URL#getPath might return a string that starts with a disk name, e.g. "/C:/" // It's invalid to use this path to open a file, so we need to get the absolute path via File. return new File(Resources.getResource(resourceName).getPath()).getAbsolutePath(); diff --git a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java index 539fcf5722ae4..159779c576ffb 100644 --- a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java +++ b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java @@ -45,8 +45,10 @@ public static Map configureFromPulsar1AuthParamString(String aut for (String p : params) { // The value could be a file path, which could contain a colon like "C:\\path\\to\\file" on Windows. int index = p.indexOf(':'); - if (index >= 0 && (index + 1) < p.length()) { - authParams.put(p.substring(0, index), p.substring(index + 1)); + String key = p.substring(0, index); + String value = p.substring(index + 1); + if (!key.isEmpty() && !value.isEmpty()) { + authParams.put(key, value); } } } diff --git a/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java b/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java new file mode 100644 index 0000000000000..c5cccba205409 --- /dev/null +++ b/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.client.impl; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import java.util.Map; +import org.testng.annotations.Test; + +public class AuthenticationUtilTest { + + @Test + public void testConfigureAuthParamString() { + Map params = AuthenticationUtil.configureFromPulsar1AuthParamString( + "key:value,path:C:\\path\\to\\file,null-key:,:null-value,:,key:value-2"); + assertEquals(params.size(), 2); + assertEquals(params.get("key"), "value-2"); + assertEquals(params.get("path"), "C:\\path\\to\\file"); + assertFalse(params.containsKey("null-key")); + } +} From c577abd53bd5109fb41a43d2cfc89a598b4a69cf Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Mon, 14 Nov 2022 17:20:30 +0800 Subject: [PATCH 3/7] Allow empty value --- .../org/apache/pulsar/client/impl/AuthenticationUtil.java | 5 ++--- .../apache/pulsar/client/impl/AuthenticationUtilTest.java | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java index 159779c576ffb..525ea4adc83e6 100644 --- a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java +++ b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java @@ -46,9 +46,8 @@ public static Map configureFromPulsar1AuthParamString(String aut // The value could be a file path, which could contain a colon like "C:\\path\\to\\file" on Windows. int index = p.indexOf(':'); String key = p.substring(0, index); - String value = p.substring(index + 1); - if (!key.isEmpty() && !value.isEmpty()) { - authParams.put(key, value); + if (!key.isEmpty()) { + authParams.put(key, p.substring(index + 1)); } } } diff --git a/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java b/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java index c5cccba205409..089a7fb6a9950 100644 --- a/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java +++ b/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java @@ -29,9 +29,9 @@ public class AuthenticationUtilTest { public void testConfigureAuthParamString() { Map params = AuthenticationUtil.configureFromPulsar1AuthParamString( "key:value,path:C:\\path\\to\\file,null-key:,:null-value,:,key:value-2"); - assertEquals(params.size(), 2); + assertEquals(params.size(), 3); assertEquals(params.get("key"), "value-2"); assertEquals(params.get("path"), "C:\\path\\to\\file"); - assertFalse(params.containsKey("null-key")); + assertEquals(params.get("null-key"), ""); } } From 78e763315db50a024664aa3a58facb86b08f2f9e Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Mon, 14 Nov 2022 21:49:49 +0800 Subject: [PATCH 4/7] Fix checkstyle --- .../org/apache/pulsar/client/impl/AuthenticationUtilTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java b/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java index 089a7fb6a9950..135b6d377994f 100644 --- a/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java +++ b/pulsar-client/src/test/java/org/apache/pulsar/client/impl/AuthenticationUtilTest.java @@ -19,7 +19,6 @@ package org.apache.pulsar.client.impl; import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertFalse; import java.util.Map; import org.testng.annotations.Test; From 1325e225b1c8556460ddc293ef30b194462bab38 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 15 Nov 2022 21:36:02 +0800 Subject: [PATCH 5/7] Fix the corner case --- .../java/org/apache/pulsar/client/impl/AuthenticationUtil.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java index 525ea4adc83e6..e2d9f0692433e 100644 --- a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java +++ b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/AuthenticationUtil.java @@ -45,6 +45,9 @@ public static Map configureFromPulsar1AuthParamString(String aut for (String p : params) { // The value could be a file path, which could contain a colon like "C:\\path\\to\\file" on Windows. int index = p.indexOf(':'); + if (index < 0) { + continue; + } String key = p.substring(0, index); if (!key.isEmpty()) { authParams.put(key, p.substring(index + 1)); From 38590f9d869435ecccc5e3dac93799edbdba476c Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 15 Nov 2022 23:35:58 +0800 Subject: [PATCH 6/7] Fix testAuthTlsWithJsonParam --- .../java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java b/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java index 7b81c2cf4496e..ef4b5927dbec3 100644 --- a/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java +++ b/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java @@ -2235,8 +2235,8 @@ public void testAuthTlsWithJsonParam() throws Exception { conf = conf = ((PulsarAdminImpl)tool.getPulsarAdminSupplier().get()) .getClientConfigData(); atuh = (AuthenticationTls) conf.getAuthentication(); - assertNull(atuh.getCertFilePath()); - assertNull(atuh.getKeyFilePath()); + assertEquals(atuh.getCertFilePath(), certFilePath); + assertEquals(atuh.getKeyFilePath(), keyFilePath); } @Test From 30a9c61627e5d7863878846bbd6640e42ddd7896 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Wed, 16 Nov 2022 19:49:33 +0800 Subject: [PATCH 7/7] Fix checkstyle --- .../java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java b/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java index ef4b5927dbec3..67630eadd2524 100644 --- a/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java +++ b/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java @@ -28,7 +28,6 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; import com.fasterxml.jackson.databind.ObjectMapper;