You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/07/09 03:44:35 UTC

[GitHub] [pulsar] nodece opened a new pull request, #14798: [improve][security] Add load multiple certificates support

nodece opened a new pull request, #14798:
URL: https://github.com/apache/pulsar/pull/14798

   Signed-off-by: Zixuan Liu <no...@gmail.com>
   
   Fixes #14733
   
   ### Motivation
   
   When a ca file includes multiple ca data, TrustManagerProxy only loads a ca data, we should add multiple ca data.
   
   ### Modifications
   
   - add load multiple certificates support
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   This change added tests and can be verified as follows:
   
   - Add TrustManagerProxyTest for this
   
   ### Documentation
   
   - [x] `no-need-doc` 
     
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] github-actions[bot] commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1140610553

   The pr had no activity for 30 days, mark with Stale label.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1104827221

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1091144117

   /pulsarbot run-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1091140424

   /pulsarbot run-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

michaeljmarshall commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1179471914

   @nodece - are you able to rebase this PR, and then the CI will work, and we can merge this. It should have been merged months ago.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1113087166

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1098636024

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall closed pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

michaeljmarshall closed pull request #14798: [improve][security] Add load multiple certificates support
URL: https://github.com/apache/pulsar/pull/14798


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1097494862

   @michaeljmarshall Good catch, this PR has been changed, could you review this PR again?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

michaeljmarshall commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1179471785

   Closing and reopening to trigger CI to run again.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1091143889

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1091137123

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1182946509

   @michaeljmarshall Thanks for your review, this PR has been merged into master branch.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece merged pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece merged PR #14798:
URL: https://github.com/apache/pulsar/pull/14798


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

michaeljmarshall commented on code in PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#discussion_r848710067


##########
pulsar-common/src/test/java/org/apache/pulsar/common/util/TrustManagerProxyTest.java:
##########
@@ -0,0 +1,48 @@
+/**
+ * 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.common.util;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import com.google.common.io.Resources;
+import java.security.cert.X509Certificate;
+import java.util.Arrays;
+import java.util.concurrent.Executors;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class TrustManagerProxyTest {
+    @DataProvider(name = "caDataProvider")
+    public static Object[][] caDataProvider() {
+        return new Object[][]{
+                {"ca/multiple-ca.pem", 2},
+                {"ca/single-ca.pem", 1}
+        };
+    }
+
+    @Test(dataProvider = "caDataProvider")
+    public void testLoadCA(String path, int count) {
+        String caPath = Resources.getResource(path).getPath();
+        TrustManagerProxy trustManagerProxy =
+                new TrustManagerProxy(caPath, 120, Executors.newSingleThreadScheduledExecutor());

Review Comment:
   Nit: I believe we need to cleanup this executor.



##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/TrustManagerProxy.java:
##########
@@ -74,11 +76,16 @@ private void updateTrustManager() throws CertificateException, KeyStoreException
             FileNotFoundException, IOException {
         CertificateFactory factory = CertificateFactory.getInstance("X.509");
         try (InputStream inputStream = new FileInputStream(certFile.getFileName())) {
-            X509Certificate certificate = (X509Certificate) factory.generateCertificate(inputStream);
-            String alias = certificate.getSubjectX500Principal().getName();
-            KeyStore keyStore = KeyStore.getInstance("JKS");
+            KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
             keyStore.load(null);
-            keyStore.setCertificateEntry(alias, certificate);
+
+            Collection<? extends Certificate> certificates = factory.generateCertificates(inputStream);

Review Comment:
   Did you notice the `SecurityUtility#loadCertificatesFromPemFile` method? It looks like that could replace some of this logic. Also, the logic in this method appears to duplicate some of the logic in the file you're modifying: https://github.com/apache/pulsar/blob/ec3821176612621e24dfbc4345525849a729fb06/pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java#L325-L331.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #14798: [improve][security] Add load multiple certificates support

Posted by GitBox <gi...@apache.org>.

nodece commented on PR #14798:
URL: https://github.com/apache/pulsar/pull/14798#issuecomment-1097751354

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org