QPID-6354: Fix validation of certificates and private keys for non java keystores/truststores

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1656751 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 325 insertions, 12 deletions

diff --git a/qpid/java/broker-core/pom.xml b/qpid/java/broker-core/pom.xml
index 68301f07bd..516ac9a4c4 100644
--- a/qpid/java/broker-core/pom.xml
+++ b/qpid/java/broker-core/pom.xml
@@ -150,6 +150,9 @@
         <testResource>
             <directory>${basedir}/src/test/resources</directory>
         </testResource>
+        <testResource>
+            <directory>${basedir}/../test-profiles/test_resources/ssl</directory>
+        </testResource>
     </testResources>
 
     <plugins>
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaKeyStoreImpl.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaKeyStoreImpl.java
index 6231413dd7..f6298ab383 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaKeyStoreImpl.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaKeyStoreImpl.java
@@ -248,16 +248,16 @@ public class NonJavaKeyStoreImpl extends AbstractConfiguredObject<NonJavaKeyStor
     {
         try
         {
-            getUrlFromString(keyStore.getPrivateKeyUrl()).openStream();
-            getUrlFromString(keyStore.getCertificateUrl()).openStream();
+            readPrivateKey(getUrlFromString(keyStore.getPrivateKeyUrl()));
+            readCertificates(getUrlFromString(keyStore.getCertificateUrl()));
             if(keyStore.getIntermediateCertificateUrl() != null)
             {
-                getUrlFromString(keyStore.getIntermediateCertificateUrl()).openStream();
+                readCertificates(getUrlFromString(keyStore.getIntermediateCertificateUrl()));
             }
         }
-        catch (IOException e)
+        catch (IOException | GeneralSecurityException e )
         {
-            throw new IllegalArgumentException(e);
+            throw new IllegalConfigurationException("Cannot validate private key or certificate(s):" + e, e);
         }
     }
 
@@ -296,8 +296,7 @@ public class NonJavaKeyStoreImpl extends AbstractConfiguredObject<NonJavaKeyStor
         }
         catch (IOException | GeneralSecurityException e)
         {
-            LOGGER.error("Error attempting to create KeyStore from private key and certificates", e);
-            _keyManagers = new KeyManager[0];
+            throw new IllegalConfigurationException("Cannot load private key or certificate(s): " + e, e);
         }
     }
 
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java
index 48594a8320..993d689fb6 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java
@@ -261,11 +261,11 @@ public class NonJavaTrustStoreImpl
     {
         try
         {
-            getUrlFromString(keyStore.getCertificatesUrl()).openStream();
+            readCertificates(getUrlFromString(keyStore.getCertificatesUrl()));
         }
-        catch (IOException e)
+        catch (IOException | GeneralSecurityException e)
         {
-            throw new IllegalArgumentException(e);
+            throw new IllegalArgumentException("Cannot validate certificate(s):" + e, e);
         }
     }
 
@@ -297,8 +297,7 @@ public class NonJavaTrustStoreImpl
         }
         catch (IOException | GeneralSecurityException e)
         {
-            LOGGER.error("Error attempting to create KeyStore from private key and certificates", e);
-            _trustManagers = new TrustManager[0];
+            throw new IllegalConfigurationException("Cannot load certificate(s) :" + e, e);
         }
     }
 
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/NonJavaKeyStoreTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/NonJavaKeyStoreTest.java
new file mode 100644
index 0000000000..f8d4ee8d6c
--- /dev/null
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/NonJavaKeyStoreTest.java
@@ -0,0 +1,220 @@
+/*
+ * 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.qpid.server.security;
+
+
+import static org.apache.qpid.test.utils.TestSSLConstants.KEYSTORE_PASSWORD;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import javax.net.ssl.KeyManager;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.InputStream;
+import java.security.Key;
+import java.security.cert.Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.commons.codec.binary.Base64;
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
+import org.apache.qpid.server.configuration.updater.TaskExecutor;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObjectFactory;
+import org.apache.qpid.server.model.KeyStore;
+import org.apache.qpid.server.model.Model;
+import org.apache.qpid.test.utils.QpidTestCase;
+import org.apache.qpid.test.utils.TestFileUtils;
+
+public class NonJavaKeyStoreTest extends QpidTestCase
+{
+    private final Broker<?> _broker = mock(Broker.class);
+    private final TaskExecutor _taskExecutor = CurrentThreadTaskExecutor.newStartedInstance();
+    private final SecurityManager _securityManager = mock(SecurityManager.class);
+    private final Model _model = BrokerModel.getInstance();
+    private final ConfiguredObjectFactory _factory = _model.getObjectFactory();
+    private List<File> _testResources;
+
+    @Override
+    public void setUp() throws Exception
+    {
+        super.setUp();
+
+        when(_broker.getTaskExecutor()).thenReturn(_taskExecutor);
+        when(_broker.getModel()).thenReturn(_model);
+        when(_broker.getSecurityManager()).thenReturn(_securityManager);
+        _testResources = new ArrayList<>();
+    }
+
+    @Override
+    public void tearDown() throws Exception
+    {
+        try
+        {
+            super.tearDown();
+        }
+        finally
+        {
+            for (File resource: _testResources)
+            {
+                try
+                {
+                    resource.delete();
+                }
+                catch (Exception e)
+                {
+                   e.printStackTrace();
+                }
+            }
+        }
+    }
+
+    private File[] extractResourcesFromTestKeyStore(boolean pem) throws Exception
+    {
+        java.security.KeyStore ks = java.security.KeyStore.getInstance(java.security.KeyStore.getDefaultType());
+        try(InputStream is = getClass().getResourceAsStream("/java_broker_keystore.jks"))
+        {
+            ks.load(is, KEYSTORE_PASSWORD.toCharArray() );
+        }
+
+
+        File privateKeyFile = TestFileUtils.createTempFile(this, ".private-key.der");
+        try(FileOutputStream kos = new FileOutputStream(privateKeyFile))
+        {
+            Key pvt = ks.getKey("java-broker", KEYSTORE_PASSWORD.toCharArray());
+            if (pem)
+            {
+                kos.write("-----BEGIN PRIVATE KEY-----\n".getBytes());
+                kos.write(Base64.encodeBase64(pvt.getEncoded(), true));
+                kos.write("\n-----END PRIVATE KEY-----".getBytes());
+            }
+            else
+            {
+                kos.write(pvt.getEncoded());
+            }
+            kos.flush();
+        }
+
+        File certificateFile = TestFileUtils.createTempFile(this, ".certificate.der");
+
+        try(FileOutputStream cos = new FileOutputStream(certificateFile))
+        {
+            Certificate pub = ks.getCertificate("rootca");
+            if (pem)
+            {
+                cos.write("-----BEGIN CERTIFICATE-----\n".getBytes());
+                cos.write(Base64.encodeBase64(pub.getEncoded(), true));
+                cos.write("\n-----END CERTIFICATE-----".getBytes());
+            }
+            else
+            {
+                cos.write(pub.getEncoded());
+            }
+            cos.flush();
+        }
+
+        return new File[]{privateKeyFile,certificateFile};
+    }
+
+    public void testCreationOfTrustStoreFromValidPrivateKeyAndCertificateInDERFormat() throws Exception
+    {
+        runTestCreationOfTrustStoreFromValidPrivateKeyAndCertificateInDerFormat(false);
+    }
+
+    public void testCreationOfTrustStoreFromValidPrivateKeyAndCertificateInPEMFormat() throws Exception
+    {
+        runTestCreationOfTrustStoreFromValidPrivateKeyAndCertificateInDerFormat(true);
+    }
+
+    private void runTestCreationOfTrustStoreFromValidPrivateKeyAndCertificateInDerFormat(boolean isPEM)throws Exception
+    {
+        File[] resources = extractResourcesFromTestKeyStore(isPEM);
+        _testResources.addAll(Arrays.asList(resources));
+
+        Map<String,Object> attributes = new HashMap<>();
+        attributes.put(NonJavaKeyStore.NAME, "myTestTrustStore");
+        attributes.put("privateKeyUrl", resources[0].toURI().toURL().toExternalForm());
+        attributes.put("certificateUrl", resources[1].toURI().toURL().toExternalForm());
+        attributes.put(NonJavaKeyStore.TYPE, "NonJavaKeyStore");
+
+        NonJavaKeyStoreImpl fileTrustStore =
+                (NonJavaKeyStoreImpl) _factory.create(KeyStore.class, attributes,  _broker);
+
+        KeyManager[] keyManagers = fileTrustStore.getKeyManagers();
+        assertNotNull(keyManagers);
+        assertEquals("Unexpected number of key managers", 1, keyManagers.length);
+        assertNotNull("Key manager is null", keyManagers[0]);
+    }
+
+    public void testCreationOfTrustStoreFromValidPrivateKeyAndInvalidCertificate()throws Exception
+    {
+        File[] resources = extractResourcesFromTestKeyStore(true);
+        _testResources.addAll(Arrays.asList(resources));
+
+        File invalidCertificate = TestFileUtils.createTempFile(this, ".invalid.cert", "content");
+        _testResources.add(invalidCertificate);
+
+        Map<String,Object> attributes = new HashMap<>();
+        attributes.put(NonJavaKeyStore.NAME, "myTestTrustStore");
+        attributes.put("privateKeyUrl", resources[0].toURI().toURL().toExternalForm());
+        attributes.put("certificateUrl", invalidCertificate.toURI().toURL().toExternalForm());
+        attributes.put(NonJavaKeyStore.TYPE, "NonJavaKeyStore");
+
+        try
+        {
+            _factory.create(KeyStore.class, attributes, _broker);
+            fail("Created key store from invalid certificate");
+        }
+        catch(IllegalConfigurationException e)
+        {
+            // pass
+        }
+    }
+
+    public void testCreationOfTrustStoreFromInvalidPrivateKeyAndValidCertificate()throws Exception
+    {
+        File[] resources = extractResourcesFromTestKeyStore(true);
+        _testResources.addAll(Arrays.asList(resources));
+
+        File invalidPrivateKey = TestFileUtils.createTempFile(this, ".invalid.pk", "content");
+        _testResources.add(invalidPrivateKey);
+
+        Map<String,Object> attributes = new HashMap<>();
+        attributes.put(NonJavaKeyStore.NAME, "myTestTrustStore");
+        attributes.put("privateKeyUrl", invalidPrivateKey.toURI().toURL().toExternalForm());
+        attributes.put("certificateUrl", resources[1].toURI().toURL().toExternalForm());
+        attributes.put(NonJavaKeyStore.TYPE, "NonJavaKeyStore");
+
+        try
+        {
+            _factory.create(KeyStore.class, attributes, _broker);
+            fail("Created key store from invalid certificate");
+        }
+        catch(IllegalConfigurationException e)
+        {
+            // pass
+        }
+    }
+}
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/NonJavaTrustStoreTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/NonJavaTrustStoreTest.java
new file mode 100644
index 0000000000..798d9fcbe5
--- /dev/null
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/NonJavaTrustStoreTest.java
@@ -0,0 +1,92 @@
+/*
+ * 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.qpid.server.security;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import javax.net.ssl.TrustManager;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
+import org.apache.qpid.server.configuration.updater.TaskExecutor;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObjectFactory;
+import org.apache.qpid.server.model.Model;
+import org.apache.qpid.server.model.TrustStore;
+import org.apache.qpid.test.utils.QpidTestCase;
+
+
+public class NonJavaTrustStoreTest extends QpidTestCase
+{
+    private final Broker<?> _broker = mock(Broker.class);
+    private final TaskExecutor _taskExecutor = CurrentThreadTaskExecutor.newStartedInstance();
+    private final SecurityManager _securityManager = mock(SecurityManager.class);
+    private final Model _model = BrokerModel.getInstance();
+    private final ConfiguredObjectFactory _factory = _model.getObjectFactory();
+
+    @Override
+    public void setUp() throws Exception
+    {
+        super.setUp();
+
+        when(_broker.getTaskExecutor()).thenReturn(_taskExecutor);
+        when(_broker.getModel()).thenReturn(_model);
+        when(_broker.getSecurityManager()).thenReturn(_securityManager);
+    }
+
+    public void testCreationOfTrustStoreFromValidCertificate() throws Exception
+    {
+        Map<String,Object> attributes = new HashMap<>();
+        attributes.put(NonJavaTrustStore.NAME, "myTestTrustStore");
+        attributes.put("certificatesUrl", getClass().getResource("/java_broker.crt").toExternalForm());
+        attributes.put(NonJavaTrustStore.TYPE, "NonJavaTrustStore");
+
+        NonJavaTrustStoreImpl fileTrustStore =
+                (NonJavaTrustStoreImpl) _factory.create(TrustStore.class, attributes,  _broker);
+
+        TrustManager[] trustManagers = fileTrustStore.getTrustManagers();
+        assertNotNull(trustManagers);
+        assertEquals("Unexpected number of trust managers", 1, trustManagers.length);
+        assertNotNull("Trust manager unexpected null", trustManagers[0]);
+    }
+
+    public void testCreationOfTrustStoreFromNonCertificate() throws Exception
+    {
+        Map<String,Object> attributes = new HashMap<>();
+        attributes.put(NonJavaTrustStore.NAME, "myTestTrustStore");
+        attributes.put("certificatesUrl", getClass().getResource("/java_broker.req").toExternalForm());
+        attributes.put(NonJavaTrustStore.TYPE, "NonJavaTrustStore");
+
+        try
+        {
+            _factory.create(TrustStore.class, attributes, _broker);
+            fail("Trust store is created from certificate request file");
+        }
+        catch (IllegalConfigurationException e)
+        {
+            // pass
+        }
+    }
+
+}