You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by kw...@apache.org on 2018/03/30 14:24:32 UTC

qpid-broker-j git commit: QPID-8064: [Broker-J] Ensure that when providing an alias for a FileKeyStore the entry corresponds to a private-key

Repository: qpid-broker-j
Updated Branches:
  refs/heads/master 818cd7d3e -> eef93a82a


QPID-8064: [Broker-J] Ensure that when providing an alias for a FileKeyStore the entry corresponds to a private-key


Project: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/commit/eef93a82
Tree: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/tree/eef93a82
Diff: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/diff/eef93a82

Branch: refs/heads/master
Commit: eef93a82accc372403ec948eaedeb82c0d39ec3e
Parents: 818cd7d
Author: Keith Wall <kw...@apache.org>
Authored: Fri Mar 30 15:24:03 2018 +0100
Committer: Keith Wall <kw...@apache.org>
Committed: Fri Mar 30 15:24:25 2018 +0100

----------------------------------------------------------------------
 .../qpid/server/security/FileKeyStoreImpl.java  | 55 ++++++++------------
 .../server/security/FileTrustStoreImpl.java     | 45 ++++++----------
 .../transport/network/security/ssl/SSLUtil.java | 11 ++++
 .../org/apache/qpid/server/util/StringUtil.java |  5 ++
 .../qpid/server/security/FileKeyStoreTest.java  |  4 +-
 .../server/security/FileTrustStoreTest.java     |  2 +-
 .../AclFileAccessControlProviderImpl.java       |  3 +-
 7 files changed, 59 insertions(+), 66 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/eef93a82/broker-core/src/main/java/org/apache/qpid/server/security/FileKeyStoreImpl.java
----------------------------------------------------------------------
diff --git a/broker-core/src/main/java/org/apache/qpid/server/security/FileKeyStoreImpl.java b/broker-core/src/main/java/org/apache/qpid/server/security/FileKeyStoreImpl.java
index 2b3f450..50e9560 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/security/FileKeyStoreImpl.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/security/FileKeyStoreImpl.java
@@ -55,7 +55,7 @@ import org.apache.qpid.server.model.StateTransition;
 import org.apache.qpid.server.transport.network.security.ssl.QpidBestFitX509KeyManager;
 import org.apache.qpid.server.transport.network.security.ssl.QpidServerX509KeyManager;
 import org.apache.qpid.server.transport.network.security.ssl.SSLUtil;
-import org.apache.qpid.server.util.ServerScopedRuntimeException;
+import org.apache.qpid.server.util.StringUtil;
 import org.apache.qpid.server.util.urlstreamhandler.data.Handler;
 
 @ManagedObject( category = false )
@@ -125,31 +125,14 @@ public class FileKeyStoreImpl extends AbstractKeyStore<FileKeyStoreImpl> impleme
 
     private void validateKeyStoreAttributes(FileKeyStore<?> fileKeyStore)
     {
-        java.security.KeyStore keyStore;
+        final String loggableStoreUrl = StringUtil.elideDataUrl(fileKeyStore.getStoreUrl());
         try
         {
             URL url = getUrlFromString(fileKeyStore.getStoreUrl());
             String password = fileKeyStore.getPassword();
             String keyStoreType = fileKeyStore.getKeyStoreType();
-            keyStore = SSLUtil.getInitializedKeyStore(url, password, keyStoreType);
-        }
-        catch (Exception e)
-        {
-            final String message;
-            if (e instanceof IOException && e.getCause() != null && e.getCause() instanceof UnrecoverableKeyException)
-            {
-                message = "Check key store password. Cannot instantiate key store from '" + fileKeyStore.getStoreUrl() + "'.";
-            }
-            else
-            {
-                message = "Cannot instantiate key store from '" + fileKeyStore.getStoreUrl() + "'.";
-            }
-
-            throw new IllegalConfigurationException(message, e);
-        }
+            java.security.KeyStore keyStore = SSLUtil.getInitializedKeyStore(url, password, keyStoreType);
 
-        try
-        {
             final String certAlias = fileKeyStore.getCertificateAlias();
             if (certAlias != null)
             {
@@ -158,32 +141,36 @@ public class FileKeyStoreImpl extends AbstractKeyStore<FileKeyStoreImpl> impleme
                 if (cert == null)
                 {
                     throw new IllegalConfigurationException(String.format(
-                            "Cannot find a certificate with alias '%s' in key store : %s",
+                            "Cannot find a certificate with alias '%s' in key store '%s'.",
                             certAlias,
-                            fileKeyStore.getStoreUrl()));
+                            loggableStoreUrl));
                 }
 
-                if (keyStore.isCertificateEntry(certAlias))
+                if (!keyStore.entryInstanceOf(certAlias, java.security.KeyStore.PrivateKeyEntry.class))
                 {
                     throw new IllegalConfigurationException(String.format(
-                            "Alias '%s' in key store : %s does not identify a key.",
+                            "Alias '%s' in key store '%s' does not identify a private key.",
                             certAlias,
-                            fileKeyStore.getStoreUrl()));
+                            loggableStoreUrl));
 
                 }
             }
-
-            if (!containsPrivateKey(keyStore))
+            else if (!containsPrivateKey(keyStore))
             {
-                throw new IllegalConfigurationException("Keystore must contain at least one private key.");
+                throw new IllegalConfigurationException(String.format(
+                        "Keystore '%s' must contain at least one private key.", loggableStoreUrl));
             }
         }
-        catch (KeyStoreException e)
+        catch (UnrecoverableKeyException e)
         {
-            // key store should be initialized above
-            throw new ServerScopedRuntimeException("Key store has not been initialized", e);
+            String message = String.format("Check key store password. Cannot instantiate key store from '%s'.", loggableStoreUrl);
+            throw new IllegalConfigurationException(message, e);
+        }
+        catch (IOException | GeneralSecurityException e)
+        {
+            final String message = String.format("Cannot instantiate key store from '%s'.", loggableStoreUrl);
+            throw new IllegalConfigurationException(message, e);
         }
-
 
         try
         {
@@ -191,8 +178,8 @@ public class FileKeyStoreImpl extends AbstractKeyStore<FileKeyStoreImpl> impleme
         }
         catch (NoSuchAlgorithmException e)
         {
-            throw new IllegalConfigurationException("Unknown keyManagerFactoryAlgorithm: "
-                    + fileKeyStore.getKeyManagerFactoryAlgorithm());
+            throw new IllegalConfigurationException(String.format("Unknown keyManagerFactoryAlgorithm: '%s'",
+                                                                  fileKeyStore.getKeyManagerFactoryAlgorithm()));
         }
 
         if(!fileKeyStore.isDurable())

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/eef93a82/broker-core/src/main/java/org/apache/qpid/server/security/FileTrustStoreImpl.java
----------------------------------------------------------------------
diff --git a/broker-core/src/main/java/org/apache/qpid/server/security/FileTrustStoreImpl.java b/broker-core/src/main/java/org/apache/qpid/server/security/FileTrustStoreImpl.java
index e562653..d6a4925 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/security/FileTrustStoreImpl.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/security/FileTrustStoreImpl.java
@@ -55,7 +55,7 @@ import org.apache.qpid.server.model.TrustStore;
 import org.apache.qpid.server.transport.network.security.ssl.QpidMultipleTrustManager;
 import org.apache.qpid.server.transport.network.security.ssl.QpidPeersOnlyTrustManager;
 import org.apache.qpid.server.transport.network.security.ssl.SSLUtil;
-import org.apache.qpid.server.util.ServerScopedRuntimeException;
+import org.apache.qpid.server.util.StringUtil;
 import org.apache.qpid.server.util.urlstreamhandler.data.Handler;
 
 public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> implements FileTrustStore<FileTrustStoreImpl>
@@ -149,30 +149,11 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
 
     private static void validateTrustStore(FileTrustStore trustStore)
     {
-        KeyStore keyStore;
+        final String loggableStoreUrl = StringUtil.elideDataUrl(trustStore.getStoreUrl());
         try
         {
-            keyStore = initializeKeyStore(trustStore);
-        }
-        catch (Exception e)
-        {
-            final String message;
-            if (e instanceof IOException && e.getCause() != null && e.getCause() instanceof UnrecoverableKeyException)
-            {
-                message = "Check trust store password. Cannot instantiate trust store from '"
-                          + trustStore.getStoreUrl()
-                          + "'.";
-            }
-            else
-            {
-                message = "Cannot instantiate trust store from '" + trustStore.getStoreUrl() + "'.";
-            }
+            KeyStore keyStore = initializeKeyStore(trustStore);
 
-            throw new IllegalConfigurationException(message, e);
-        }
-
-        try
-        {
             final Enumeration<String> aliasesEnum = keyStore.aliases();
             boolean certificateFound = false;
             while (aliasesEnum.hasMoreElements())
@@ -186,12 +167,19 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
             }
             if (!certificateFound)
             {
-                throw new IllegalConfigurationException("Trust store must contain at least one certificate.");
+                throw new IllegalConfigurationException(String.format(
+                        "Trust store '%s' must contain at least one certificate.", loggableStoreUrl));
             }
         }
-        catch (KeyStoreException e)
+        catch (UnrecoverableKeyException e)
         {
-            throw new ServerScopedRuntimeException("Trust store has not been initialized", e);
+            String message = String.format("Check trust store password. Cannot instantiate trust store from '%s'.", loggableStoreUrl);
+            throw new IllegalConfigurationException(message, e);
+        }
+        catch (IOException | GeneralSecurityException e)
+        {
+            final String message = String.format("Cannot instantiate trust store from '%s'.", loggableStoreUrl);
+            throw new IllegalConfigurationException(message, e);
         }
 
         try
@@ -200,7 +188,8 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
         }
         catch (NoSuchAlgorithmException e)
         {
-            throw new IllegalConfigurationException("Unknown trustManagerFactoryAlgorithm: " + trustStore.getTrustManagerFactoryAlgorithm());
+            throw new IllegalConfigurationException(String.format("Unknown trustManagerFactoryAlgorithm '%s'",
+                                                                  trustStore.getTrustManagerFactoryAlgorithm()));
         }
     }
 
@@ -247,7 +236,7 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
     }
 
     @Override
-    protected TrustManager[] getTrustManagersInternal() throws GeneralSecurityException
+    protected TrustManager[] getTrustManagersInternal()
     {
         TrustManager[] trustManagers = _trustManagers;
         if (trustManagers == null || trustManagers.length == 0)
@@ -258,7 +247,7 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
     }
 
     @Override
-    public Certificate[] getCertificates() throws GeneralSecurityException
+    public Certificate[] getCertificates()
     {
         Certificate[] certificates = _certificates;
         return certificates == null ? new Certificate[0] : Arrays.copyOf(certificates, certificates.length);

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/eef93a82/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/SSLUtil.java
----------------------------------------------------------------------
diff --git a/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/SSLUtil.java b/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/SSLUtil.java
index 9e6ab0a..520268c 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/SSLUtil.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/SSLUtil.java
@@ -427,6 +427,17 @@ public class SSLUtil
 
             ks.load(in, storeCharPassword);
         }
+        catch (IOException ioe)
+        {
+            if (ioe.getCause() instanceof GeneralSecurityException)
+            {
+                throw ((GeneralSecurityException) ioe.getCause());
+            }
+            else
+            {
+                throw ioe;
+            }
+        }
         return ks;
     }
 

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/eef93a82/broker-core/src/main/java/org/apache/qpid/server/util/StringUtil.java
----------------------------------------------------------------------
diff --git a/broker-core/src/main/java/org/apache/qpid/server/util/StringUtil.java b/broker-core/src/main/java/org/apache/qpid/server/util/StringUtil.java
index 2b986a3..b986370 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/util/StringUtil.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/util/StringUtil.java
@@ -37,6 +37,11 @@ public class StringUtil
 
     private Random _random = new Random();
 
+    public static String elideDataUrl(final String path)
+    {
+        return String.valueOf(path).toLowerCase().startsWith("data:") ? "data:..." : path;
+    }
+
     public String randomAlphaNumericString(int maxLength)
     {
         char[] result = new char[maxLength];

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/eef93a82/broker-core/src/test/java/org/apache/qpid/server/security/FileKeyStoreTest.java
----------------------------------------------------------------------
diff --git a/broker-core/src/test/java/org/apache/qpid/server/security/FileKeyStoreTest.java b/broker-core/src/test/java/org/apache/qpid/server/security/FileKeyStoreTest.java
index 535badb..441d000 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/security/FileKeyStoreTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/security/FileKeyStoreTest.java
@@ -154,7 +154,7 @@ public class FileKeyStoreTest extends QpidTestCase
         catch (IllegalConfigurationException ice)
         {
             String message = ice.getMessage();
-            assertTrue("Exception text not as unexpected:" + message, message.contains("does not identify a key"));
+            assertTrue("Exception text not as unexpected:" + message, message.contains("does not identify a private key"));
         }
     }
 
@@ -298,7 +298,7 @@ public class FileKeyStoreTest extends QpidTestCase
         catch (IllegalConfigurationException ice)
         {
             String message = ice.getMessage();
-            assertTrue("Exception text not as unexpected:" + message, message.contains("Keystore must contain at least one private key."));
+            assertTrue("Exception text not as unexpected:" + message, message.contains("must contain at least one private key"));
         }
     }
 

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/eef93a82/broker-core/src/test/java/org/apache/qpid/server/security/FileTrustStoreTest.java
----------------------------------------------------------------------
diff --git a/broker-core/src/test/java/org/apache/qpid/server/security/FileTrustStoreTest.java b/broker-core/src/test/java/org/apache/qpid/server/security/FileTrustStoreTest.java
index bab4f26..06be7a0 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/security/FileTrustStoreTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/security/FileTrustStoreTest.java
@@ -334,7 +334,7 @@ public class FileTrustStoreTest extends QpidTestCase
         catch (IllegalConfigurationException ice)
         {
             String message = ice.getMessage();
-            assertTrue("Exception text not as unexpected:" + message, message.contains("Trust store must contain at least one certificate."));
+            assertTrue("Exception text not as unexpected:" + message, message.contains("must contain at least one certificate"));
         }
     }
 

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/eef93a82/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AclFileAccessControlProviderImpl.java
----------------------------------------------------------------------
diff --git a/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AclFileAccessControlProviderImpl.java b/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AclFileAccessControlProviderImpl.java
index c8a90d7..0a80b81 100644
--- a/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AclFileAccessControlProviderImpl.java
+++ b/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AclFileAccessControlProviderImpl.java
@@ -40,6 +40,7 @@ import org.apache.qpid.server.model.StateTransition;
 import org.apache.qpid.server.security.access.Operation;
 import org.apache.qpid.server.security.access.config.AclFileParser;
 import org.apache.qpid.server.security.access.config.RuleBasedAccessControl;
+import org.apache.qpid.server.util.StringUtil;
 import org.apache.qpid.server.util.urlstreamhandler.data.Handler;
 
 public class AclFileAccessControlProviderImpl
@@ -86,7 +87,7 @@ public class AclFileAccessControlProviderImpl
             LOGGER.debug("Calling changeAttributes to try to force update");
             // force the change listener to fire, causing the parent broker to update its cache
             changeAttributes(Collections.<String,Object>emptyMap());
-            getEventLogger().message(AccessControlMessages.LOADED(String.valueOf(getPath()).startsWith("data:") ? "data:..." : getPath()));
+            getEventLogger().message(AccessControlMessages.LOADED(StringUtil.elideDataUrl(getPath())));
 
         }
         catch(RuntimeException e)


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org