Repository: qpid-broker-j
Updated Branches:
  refs/heads/master 84256b2c7 -> 47c64a075


QPID-7869: Improve trust store implementations

* fix NPE whilst checking expiry in SiteSpecificTrustore
* add documentation to context variables
* Move duplicate code for getCertificateDetails and checkCerificateExpiry into 
AbstractTtrustStore
* Remove unsused field _ts in QpidPeersOnlyTrustManager


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/47c64a07
Tree: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/tree/47c64a07
Diff: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/diff/47c64a07

Branch: refs/heads/master
Commit: 47c64a07543c9a9144bf39a65fce3fe8dc4bae97
Parents: 84256b2
Author: Alex Rudyy <oru...@apache.org>
Authored: Thu Aug 10 13:02:19 2017 +0100
Committer: Alex Rudyy <oru...@apache.org>
Committed: Thu Aug 10 13:03:17 2017 +0100

----------------------------------------------------------------------
 .../org/apache/qpid/server/model/KeyStore.java  |  7 ++-
 .../apache/qpid/server/model/TrustStore.java    |  7 ++-
 .../server/security/AbstractTrustStore.java     | 61 ++++++++++++++++++--
 .../server/security/FileTrustStoreImpl.java     | 41 +------------
 .../ManagedPeerCertificateTrustStoreImpl.java   | 30 +---------
 .../server/security/NonJavaTrustStoreImpl.java  | 38 +++---------
 .../security/SiteSpecificTrustStoreImpl.java    | 22 +++----
 .../security/ssl/QpidPeersOnlyTrustManager.java |  4 +-
 8 files changed, 85 insertions(+), 125 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/47c64a07/broker-core/src/main/java/org/apache/qpid/server/model/KeyStore.java
----------------------------------------------------------------------
diff --git 
a/broker-core/src/main/java/org/apache/qpid/server/model/KeyStore.java 
b/broker-core/src/main/java/org/apache/qpid/server/model/KeyStore.java
index efbe6d5..92bb8f3 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/model/KeyStore.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/model/KeyStore.java
@@ -28,12 +28,15 @@ public interface KeyStore<X extends KeyStore<X>> extends 
ConfiguredObject<X>
 {
     String CERTIFICATE_EXPIRY_WARN_PERIOD = 
"qpid.keystore.certificateExpiryWarnPeriod";
 
-    @ManagedContextDefault(name = CERTIFICATE_EXPIRY_WARN_PERIOD)
+    @ManagedContextDefault(name = CERTIFICATE_EXPIRY_WARN_PERIOD,
+            description = "The number of days before the certificate expiry 
date"
+                          + " to issue the operational log warning about the 
certificate expiry")
     int DEFAULT_CERTIFICATE_EXPIRY_WARN_PERIOD = 30;
 
     String CERTIFICATE_EXPIRY_CHECK_FREQUENCY = 
"qpid.keystore.certificateExpiryCheckFrequency";
 
-    @ManagedContextDefault(name = CERTIFICATE_EXPIRY_CHECK_FREQUENCY)
+    @ManagedContextDefault(name = CERTIFICATE_EXPIRY_CHECK_FREQUENCY,
+            description = "The interval of number of days to check certificate 
expiry")
     int DEFAULT_CERTIFICATE_EXPIRY_CHECK_FREQUENCY = 1;
 
     @DerivedAttribute

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/47c64a07/broker-core/src/main/java/org/apache/qpid/server/model/TrustStore.java
----------------------------------------------------------------------
diff --git 
a/broker-core/src/main/java/org/apache/qpid/server/model/TrustStore.java 
b/broker-core/src/main/java/org/apache/qpid/server/model/TrustStore.java
index 5ef3c74..38325e0 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/model/TrustStore.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/model/TrustStore.java
@@ -35,12 +35,15 @@ public interface TrustStore<X extends TrustStore<X>> 
extends ConfiguredObject<X>
 
     String CERTIFICATE_EXPIRY_WARN_PERIOD = 
"qpid.truststore.certificateExpiryWarnPeriod";
 
-    @ManagedContextDefault(name = CERTIFICATE_EXPIRY_WARN_PERIOD)
+    @ManagedContextDefault(name = CERTIFICATE_EXPIRY_WARN_PERIOD,
+            description = "The number of days before the certificate expiry 
date"
+                          + " to issue the operational log warning about the 
certificate expiry")
     int DEFAULT_CERTIFICATE_EXPIRY_WARN_PERIOD = 30;
 
     String CERTIFICATE_EXPIRY_CHECK_FREQUENCY = 
"qpid.truststore.certificateExpiryCheckFrequency";
 
-    @ManagedContextDefault(name = CERTIFICATE_EXPIRY_CHECK_FREQUENCY)
+    @ManagedContextDefault(name = CERTIFICATE_EXPIRY_CHECK_FREQUENCY,
+            description = "The interval of number of days to check certificate 
expiry")
     int DEFAULT_CERTIFICATE_EXPIRY_CHECK_FREQUENCY = 1;
     @ManagedContextDefault(name = 
"qpid.truststore.trustAnchorValidityEnforced")
     boolean DEFAULT_TRUST_ANCHOR_VALIDITY_ENFORCED = false;

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/47c64a07/broker-core/src/main/java/org/apache/qpid/server/security/AbstractTrustStore.java
----------------------------------------------------------------------
diff --git 
a/broker-core/src/main/java/org/apache/qpid/server/security/AbstractTrustStore.java
 
b/broker-core/src/main/java/org/apache/qpid/server/security/AbstractTrustStore.java
index 3c6dca1..b49502a 100644
--- 
a/broker-core/src/main/java/org/apache/qpid/server/security/AbstractTrustStore.java
+++ 
b/broker-core/src/main/java/org/apache/qpid/server/security/AbstractTrustStore.java
@@ -29,7 +29,9 @@ import java.security.cert.CertificateNotYetValidException;
 import java.security.cert.TrustAnchor;
 import java.security.cert.X509Certificate;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
@@ -37,6 +39,7 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
 
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.X509TrustManager;
@@ -47,6 +50,7 @@ import com.google.common.util.concurrent.ListenableFuture;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.logging.EventLogger;
 import org.apache.qpid.server.logging.messages.TrustStoreMessages;
 import org.apache.qpid.server.model.AbstractConfigurationChangeListener;
@@ -84,7 +88,7 @@ public abstract class AbstractTrustStore<X extends 
AbstractTrustStore<X>>
 
     private ScheduledFuture<?> _checkExpiryTaskFuture;
 
-    public AbstractTrustStore(Map<String, Object> attributes, Broker<?> broker)
+    AbstractTrustStore(Map<String, Object> attributes, Broker<?> broker)
     {
         super(broker, attributes);
 
@@ -120,7 +124,7 @@ public abstract class AbstractTrustStore<X extends 
AbstractTrustStore<X>>
         
_broker.getEventLogger().message(TrustStoreMessages.OPERATION(operation));
     }
 
-    protected void initializeExpiryChecking()
+    void initializeExpiryChecking()
     {
         int checkFrequency = getCertificateExpiryCheckFrequency();
         if(getBroker().getState() == State.ACTIVE)
@@ -148,13 +152,13 @@ public abstract class AbstractTrustStore<X extends 
AbstractTrustStore<X>>
         }
     }
 
-    protected final ListenableFuture<Void> deleteIfNotInUse()
+    final ListenableFuture<Void> deleteIfNotInUse()
     {
         // verify that it is not in use
         String storeName = getName();
 
         Collection<Port<?>> ports = new ArrayList<>(_broker.getPorts());
-        for (Port port : ports)
+        for (Port<?> port : ports)
         {
             Collection<TrustStore> trustStores = port.getTrustStores();
             if(trustStores != null)
@@ -199,9 +203,32 @@ public abstract class AbstractTrustStore<X extends 
AbstractTrustStore<X>>
         return Futures.immediateFuture(null);
     }
 
-    protected abstract void checkCertificateExpiry();
+    private void checkCertificateExpiry()
+    {
+        int expiryWarning = getCertificateExpiryWarnPeriod();
+        if(expiryWarning > 0)
+        {
+            long currentTime = System.currentTimeMillis();
+            Date expiryTestDate = new Date(currentTime + (ONE_DAY * (long) 
expiryWarning));
+
+            try
+            {
+                Certificate[] certificatesInternal = getCertificatesInternal();
+                if (certificatesInternal.length > 0)
+                {
+                    Arrays.stream(certificatesInternal)
+                          .filter(cert -> cert instanceof X509Certificate)
+                          .forEach(x509cert -> 
checkCertificateExpiry(currentTime, expiryTestDate, (X509Certificate) 
x509cert));
+                }
+            }
+            catch (GeneralSecurityException e)
+            {
+                LOGGER.debug("Unexpected exception whilst checking certificate 
expiry", e);
+            }
+        }
+    }
 
-    protected void checkCertificateExpiry(final long currentTime,
+    private void checkCertificateExpiry(final long currentTime,
                                           final Date expiryTestDate,
                                           final X509Certificate cert)
     {
@@ -269,6 +296,7 @@ public abstract class AbstractTrustStore<X extends 
AbstractTrustStore<X>>
     }
 
     protected abstract TrustManager[] getTrustManagersInternal() throws 
GeneralSecurityException;
+    protected abstract Certificate[] getCertificatesInternal() throws 
GeneralSecurityException;
 
     @Override
     public final int getCertificateExpiryWarnPeriod()
@@ -324,6 +352,27 @@ public abstract class AbstractTrustStore<X extends 
AbstractTrustStore<X>>
         return _excludedVirtualHostNodeMessageSources;
     }
 
+    @Override
+    public List<CertificateDetails> getCertificateDetails()
+    {
+        try
+        {
+            Certificate[] certificatesInternal = getCertificatesInternal();
+            if (certificatesInternal.length > 0)
+            {
+                return Arrays.stream(certificatesInternal)
+                             .filter(cert -> cert instanceof X509Certificate)
+                             .map(x509cert -> new 
CertificateDetailsImpl((X509Certificate) x509cert))
+                             .collect(Collectors.toList());
+            }
+            return Collections.emptyList();
+        }
+        catch (GeneralSecurityException e)
+        {
+            throw new IllegalConfigurationException("Failed to extract 
certificate details", e);
+        }
+    }
+
     private boolean isSelfSigned(X509Certificate cert) throws 
GeneralSecurityException
     {
         try

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/47c64a07/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 efb88b1..64cae80 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
@@ -29,16 +29,11 @@ import java.security.KeyStore;
 import java.security.NoSuchAlgorithmException;
 import java.security.UnrecoverableKeyException;
 import java.security.cert.Certificate;
-import java.security.cert.X509Certificate;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
-import java.util.Date;
 import java.util.Enumeration;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.stream.Collectors;
 
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
@@ -297,41 +292,9 @@ public class FileTrustStoreImpl extends 
AbstractTrustStore<FileTrustStoreImpl> i
     }
 
     @Override
-    public List<CertificateDetails> getCertificateDetails()
+    protected Certificate[] getCertificatesInternal() throws 
GeneralSecurityException
     {
-        try
-        {
-            return Arrays.stream(getCertificates())
-                         .filter(cert -> cert instanceof X509Certificate)
-                         .map(x509cert -> new 
CertificateDetailsImpl((X509Certificate) x509cert))
-                         .collect(Collectors.toList());
-        }
-        catch (GeneralSecurityException e)
-        {
-            throw new IllegalConfigurationException("Failed to extract 
certificate details", e);
-        }
-    }
-
-    @Override
-    protected void checkCertificateExpiry()
-    {
-        int expiryWarning = getCertificateExpiryWarnPeriod();
-        if(expiryWarning > 0)
-        {
-            long currentTime = System.currentTimeMillis();
-            Date expiryTestDate = new Date(currentTime + (ONE_DAY * (long) 
expiryWarning));
-
-            try
-            {
-                Arrays.stream(getCertificates())
-                      .filter(cert -> cert instanceof X509Certificate)
-                      .forEach(x509cert -> checkCertificateExpiry(currentTime, 
expiryTestDate,
-                                                                  
(X509Certificate) x509cert));
-            }
-            catch (GeneralSecurityException e)
-            {
-            }
-        }
+        return getCertificates();
     }
 
     private static URL getUrlFromString(String urlString) throws 
MalformedURLException

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/47c64a07/broker-core/src/main/java/org/apache/qpid/server/security/ManagedPeerCertificateTrustStoreImpl.java
----------------------------------------------------------------------
diff --git 
a/broker-core/src/main/java/org/apache/qpid/server/security/ManagedPeerCertificateTrustStoreImpl.java
 
b/broker-core/src/main/java/org/apache/qpid/server/security/ManagedPeerCertificateTrustStoreImpl.java
index 18c36da..1f04b85 100644
--- 
a/broker-core/src/main/java/org/apache/qpid/server/security/ManagedPeerCertificateTrustStoreImpl.java
+++ 
b/broker-core/src/main/java/org/apache/qpid/server/security/ManagedPeerCertificateTrustStoreImpl.java
@@ -28,7 +28,6 @@ import java.security.cert.X509Certificate;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -176,21 +175,6 @@ public class ManagedPeerCertificateTrustStoreImpl
     }
 
     @Override
-    public List<CertificateDetails> getCertificateDetails()
-    {
-        List<CertificateDetails> details = new ArrayList<>();
-        for(Certificate cert : _storedCertificates)
-        {
-            if(cert instanceof X509Certificate)
-            {
-                details.add(new CertificateDetailsImpl((X509Certificate)cert));
-            }
-        }
-        return details;
-    }
-
-
-    @Override
     public void removeCertificates(final List<CertificateDetails> certs)
     {
         final Map<String, Set<BigInteger>> certsToRemove = new HashMap<>();
@@ -228,18 +212,8 @@ public class ManagedPeerCertificateTrustStoreImpl
     }
 
     @Override
-    protected void checkCertificateExpiry()
+    protected Certificate[] getCertificatesInternal() throws 
GeneralSecurityException
     {
-        int expiryWarning = getCertificateExpiryWarnPeriod();
-        if(expiryWarning > 0)
-        {
-            long currentTime = System.currentTimeMillis();
-            Date expiryTestDate = new Date(currentTime + (ONE_DAY * (long) 
expiryWarning));
-
-            _storedCertificates.stream()
-                               .filter(cert -> cert instanceof X509Certificate)
-                               .forEach(x509cert -> 
checkCertificateExpiry(currentTime, expiryTestDate,
-                                                                           
(X509Certificate) x509cert));
-        }
+        return getCertificates();
     }
 }

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/47c64a07/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java
----------------------------------------------------------------------
diff --git 
a/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java
 
b/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java
index 6a8f834..ddcb291 100644
--- 
a/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java
+++ 
b/broker-core/src/main/java/org/apache/qpid/server/security/NonJavaTrustStoreImpl.java
@@ -61,18 +61,16 @@ public class NonJavaTrustStoreImpl
 {
     private static final Logger LOGGER = 
LoggerFactory.getLogger(NonJavaTrustStoreImpl.class);
 
-    @ManagedAttributeField( afterSet = "updateTrustManagers" )
-    private String _certificatesUrl;
-
-    private volatile TrustManager[] _trustManagers = new TrustManager[0];
-
-
-
     static
     {
         Handler.register();
     }
 
+    @ManagedAttributeField( afterSet = "updateTrustManagers" )
+    private String _certificatesUrl;
+
+    private volatile TrustManager[] _trustManagers = new TrustManager[0];
+
     private X509Certificate[] _certificates;
 
     @ManagedObjectFactoryConstructor
@@ -87,17 +85,6 @@ public class NonJavaTrustStoreImpl
         return _certificatesUrl;
     }
 
-
-    @Override
-    public List<CertificateDetails> getCertificateDetails()
-    {
-        return (_certificates == null)
-                ? Collections.emptyList()
-                : Arrays.stream(_certificates)
-                        .map(CertificateDetailsImpl::new)
-                        .collect(Collectors.toList());
-    }
-
     @Override
     protected TrustManager[] getTrustManagersInternal() throws 
GeneralSecurityException
     {
@@ -151,19 +138,10 @@ public class NonJavaTrustStoreImpl
     }
 
     @Override
-    protected void checkCertificateExpiry()
+    protected Certificate[] getCertificatesInternal() throws 
GeneralSecurityException
     {
-        int expiryWarning = getCertificateExpiryWarnPeriod();
-        if(expiryWarning > 0)
-        {
-            long currentTime = System.currentTimeMillis();
-            Date expiryTestDate = new Date(currentTime + (ONE_DAY * (long) 
expiryWarning));
-
-            Arrays.stream(_certificates)
-                  .filter(cert -> cert instanceof X509Certificate)
-                  .forEach(x509cert -> checkCertificateExpiry(currentTime, 
expiryTestDate,
-                                                              x509cert));
-        }
+        X509Certificate[] certificates = _certificates;
+        return certificates == null ? new X509Certificate[0] : certificates;
     }
 
     private void validateTrustStoreAttributes(NonJavaTrustStore<?> keyStore)

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/47c64a07/broker-core/src/main/java/org/apache/qpid/server/security/SiteSpecificTrustStoreImpl.java
----------------------------------------------------------------------
diff --git 
a/broker-core/src/main/java/org/apache/qpid/server/security/SiteSpecificTrustStoreImpl.java
 
b/broker-core/src/main/java/org/apache/qpid/server/security/SiteSpecificTrustStoreImpl.java
index bb081ed..9e3ea69 100644
--- 
a/broker-core/src/main/java/org/apache/qpid/server/security/SiteSpecificTrustStoreImpl.java
+++ 
b/broker-core/src/main/java/org/apache/qpid/server/security/SiteSpecificTrustStoreImpl.java
@@ -31,9 +31,6 @@ import java.security.cert.CertificateEncodingException;
 import java.security.cert.CertificateException;
 import java.security.cert.CertificateFactory;
 import java.security.cert.X509Certificate;
-import java.util.Collections;
-import java.util.Date;
-import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Callable;
 import java.util.concurrent.Executors;
@@ -324,11 +321,6 @@ public class SiteSpecificTrustStoreImpl
     }
 
     @Override
-    public List<CertificateDetails> getCertificateDetails()
-    {
-        return Collections.singletonList(new 
CertificateDetailsImpl(_x509Certificate));
-    }
-    @Override
     public void refreshCertificate()
     {
         logOperation("refreshCertificate");
@@ -348,15 +340,15 @@ public class SiteSpecificTrustStoreImpl
     }
 
     @Override
-    protected void checkCertificateExpiry()
+    protected Certificate[] getCertificatesInternal() throws 
GeneralSecurityException
     {
-        int expiryWarning = getCertificateExpiryWarnPeriod();
-        if(expiryWarning > 0)
+        if (_x509Certificate == null)
         {
-            long currentTime = System.currentTimeMillis();
-            Date expiryTestDate = new Date(currentTime + (ONE_DAY * (long) 
expiryWarning));
-
-            checkCertificateExpiry(currentTime, expiryTestDate, 
_x509Certificate);
+            return new Certificate[0];
+        }
+        else
+        {
+            return new Certificate[]{_x509Certificate};
         }
     }
 

http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/47c64a07/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/QpidPeersOnlyTrustManager.java
----------------------------------------------------------------------
diff --git 
a/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/QpidPeersOnlyTrustManager.java
 
b/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/QpidPeersOnlyTrustManager.java
index b3f9610..19cdd87 100644
--- 
a/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/QpidPeersOnlyTrustManager.java
+++ 
b/broker-core/src/main/java/org/apache/qpid/server/transport/network/security/ssl/QpidPeersOnlyTrustManager.java
@@ -39,15 +39,13 @@ import javax.net.ssl.X509TrustManager;
 public class QpidPeersOnlyTrustManager implements X509TrustManager
 {
 
-    private final KeyStore _ts;
     private final X509TrustManager _delegate;
     private final List<Certificate> _trustedCerts = new ArrayList<>();
 
     public QpidPeersOnlyTrustManager(KeyStore ts, X509TrustManager 
trustManager) throws KeyStoreException
     {
-        _ts = ts;
         _delegate = trustManager;
-        Enumeration<String> aliases = this._ts.aliases();
+        Enumeration<String> aliases = ts.aliases();
         while (aliases.hasMoreElements())
         {
             _trustedCerts.add(ts.getCertificate(aliases.nextElement()));


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

Reply via email to