You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by or...@apache.org on 2017/08/15 09:21:08 UTC
qpid-broker-j git commit: QPID-7869: [Java Broker] Address review
comments from Keith Wall
Repository: qpid-broker-j
Updated Branches:
refs/heads/master 10699183d -> 175829a6c
QPID-7869: [Java Broker] Address review comments from Keith Wall
* Remove getCertificatesInternal
* Change FileTrustStoreImpl to load certificates and trust managers in onOpen
* Change descriptions for context variables
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/175829a6
Tree: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/tree/175829a6
Diff: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/diff/175829a6
Branch: refs/heads/master
Commit: 175829a6c4762f90e02f34722f2429a22ae71e7e
Parents: 1069918
Author: Alex Rudyy <or...@apache.org>
Authored: Tue Aug 15 10:20:40 2017 +0100
Committer: Alex Rudyy <or...@apache.org>
Committed: Tue Aug 15 10:20:40 2017 +0100
----------------------------------------------------------------------
.../org/apache/qpid/server/model/KeyStore.java | 6 +-
.../apache/qpid/server/model/TrustStore.java | 6 +-
.../server/security/AbstractTrustStore.java | 5 +-
.../server/security/FileTrustStoreImpl.java | 207 ++++++++++---------
.../ManagedPeerCertificateTrustStoreImpl.java | 5 -
.../server/security/NonJavaTrustStoreImpl.java | 17 +-
.../security/SiteSpecificTrustStoreImpl.java | 16 +-
7 files changed, 124 insertions(+), 138 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/175829a6/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 92bb8f3..b48a248 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
@@ -29,14 +29,14 @@ public interface KeyStore<X extends KeyStore<X>> extends ConfiguredObject<X>
String CERTIFICATE_EXPIRY_WARN_PERIOD = "qpid.keystore.certificateExpiryWarnPeriod";
@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")
+ description = "The number of days before a certificate's expiry"
+ + " that certificate expiration warnings will be written to the log")
int DEFAULT_CERTIFICATE_EXPIRY_WARN_PERIOD = 30;
String CERTIFICATE_EXPIRY_CHECK_FREQUENCY = "qpid.keystore.certificateExpiryCheckFrequency";
@ManagedContextDefault(name = CERTIFICATE_EXPIRY_CHECK_FREQUENCY,
- description = "The interval of number of days to check certificate expiry")
+ description = "Period (in days) with which the Broker will repeat the certificate expiration warning")
int DEFAULT_CERTIFICATE_EXPIRY_CHECK_FREQUENCY = 1;
@DerivedAttribute
http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/175829a6/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 38325e0..768d26a 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
@@ -36,14 +36,14 @@ public interface TrustStore<X extends TrustStore<X>> extends ConfiguredObject<X>
String CERTIFICATE_EXPIRY_WARN_PERIOD = "qpid.truststore.certificateExpiryWarnPeriod";
@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")
+ description = "The number of days before a certificate's expiry"
+ + " that certificate expiration warnings will be written to the log")
int DEFAULT_CERTIFICATE_EXPIRY_WARN_PERIOD = 30;
String CERTIFICATE_EXPIRY_CHECK_FREQUENCY = "qpid.truststore.certificateExpiryCheckFrequency";
@ManagedContextDefault(name = CERTIFICATE_EXPIRY_CHECK_FREQUENCY,
- description = "The interval of number of days to check certificate expiry")
+ description = "Period (in days) with which the Broker will repeat the certificate expiration warning")
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/175829a6/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 b49502a..2530e55 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
@@ -213,7 +213,7 @@ public abstract class AbstractTrustStore<X extends AbstractTrustStore<X>>
try
{
- Certificate[] certificatesInternal = getCertificatesInternal();
+ Certificate[] certificatesInternal = getCertificates();
if (certificatesInternal.length > 0)
{
Arrays.stream(certificatesInternal)
@@ -296,7 +296,6 @@ 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()
@@ -357,7 +356,7 @@ public abstract class AbstractTrustStore<X extends AbstractTrustStore<X>>
{
try
{
- Certificate[] certificatesInternal = getCertificatesInternal();
+ Certificate[] certificatesInternal = getCertificates();
if (certificatesInternal.length > 0)
{
return Arrays.stream(certificatesInternal)
http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/175829a6/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 64cae80..ce6d037 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
@@ -26,6 +26,7 @@ import java.net.MalformedURLException;
import java.net.URL;
import java.security.GeneralSecurityException;
import java.security.KeyStore;
+import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
import java.security.UnrecoverableKeyException;
import java.security.cert.Certificate;
@@ -70,6 +71,9 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
@ManagedAttributeField
private String _password;
+ private volatile TrustManager[] _trustManagers;
+ private volatile Certificate[] _certificates;
+
static
{
Handler.register();
@@ -119,13 +123,39 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
validateTrustStore(updated);
}
+ @Override
+ protected void onOpen()
+ {
+ super.onOpen();
+ initialize();
+ }
+
+ @Override
+ protected void changeAttributes(final Map<String, Object> attributes)
+ {
+ super.changeAttributes(attributes);
+ if (attributes.containsKey(STORE_URL)
+ || attributes.containsKey(PASSWORD)
+ || attributes.containsKey(TRUST_STORE_TYPE)
+ || attributes.containsKey(TRUST_MANAGER_FACTORY_ALGORITHM)
+ || attributes.containsKey(PEERS_ONLY))
+ {
+ initialize();
+ }
+ }
+
+ private static KeyStore initializeKeyStore(final FileTrustStore trustStore)
+ throws GeneralSecurityException, IOException
+ {
+ URL trustStoreUrl = getUrlFromString(trustStore.getStoreUrl());
+ return SSLUtil.getInitializedKeyStore(trustStoreUrl, trustStore.getPassword(), trustStore.getTrustStoreType());
+ }
private static void validateTrustStore(FileTrustStore trustStore)
{
try
{
- URL trustStoreUrl = getUrlFromString(trustStore.getStoreUrl());
- SSLUtil.getInitializedKeyStore(trustStoreUrl, trustStore.getPassword(), trustStore.getTrustStoreType());
+ initializeKeyStore(trustStore);
}
catch (Exception e)
{
@@ -197,104 +227,13 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
@Override
protected TrustManager[] getTrustManagersInternal() throws GeneralSecurityException
{
- String trustStorePassword = getPassword();
- String trustStoreType = _trustStoreType;
- String trustManagerFactoryAlgorithm = _trustManagerFactoryAlgorithm;
-
- try
- {
- URL trustStoreUrl = getUrlFromString(_storeUrl);
-
- KeyStore ts = SSLUtil.getInitializedKeyStore(trustStoreUrl, trustStorePassword, trustStoreType);
- final TrustManagerFactory tmf = TrustManagerFactory
- .getInstance(trustManagerFactoryAlgorithm);
- tmf.init(ts);
-
- TrustManager[] delegateManagers = tmf.getTrustManagers();
- if (delegateManagers.length == 0)
- {
- throw new IllegalStateException("Truststore " + this + " defines no trust managers");
- }
- else if (delegateManagers.length == 1)
- {
- if (_peersOnly && delegateManagers[0] instanceof X509TrustManager)
- {
- return new TrustManager[] {new QpidPeersOnlyTrustManager(ts,
- ((X509TrustManager) delegateManagers[0]))};
- }
- else
- {
- return delegateManagers;
- }
- }
- else
- {
- final Collection<TrustManager> trustManagersCol = new ArrayList<>();
- final QpidMultipleTrustManager mulTrustManager = new QpidMultipleTrustManager();
- for (TrustManager tm : delegateManagers)
- {
- if (tm instanceof X509TrustManager)
- {
- if (_peersOnly)
- {
- mulTrustManager.addTrustManager(new QpidPeersOnlyTrustManager(ts, (X509TrustManager) tm));
- }
- else
- {
- mulTrustManager.addTrustManager((X509TrustManager) tm);
- }
- }
- else
- {
- trustManagersCol.add(tm);
- }
- }
- if (! mulTrustManager.isEmpty())
- {
- trustManagersCol.add(mulTrustManager);
- }
- return trustManagersCol.toArray(new TrustManager[trustManagersCol.size()]);
- }
- }
- catch (IOException e)
- {
- throw new GeneralSecurityException(e);
- }
+ return _trustManagers;
}
@Override
public Certificate[] getCertificates() throws GeneralSecurityException
{
- String trustStorePassword = getPassword();
- String trustStoreType = _trustStoreType;
-
- try
- {
- URL trustStoreUrl = getUrlFromString(_storeUrl);
-
- KeyStore ts = SSLUtil.getInitializedKeyStore(trustStoreUrl, trustStorePassword, trustStoreType);
-
- final Collection<Certificate> certificates = new ArrayList<>();
-
- Enumeration<String> aliases = ts.aliases();
- while (aliases.hasMoreElements())
- {
- certificates.add(ts.getCertificate(aliases.nextElement()));
- }
-
- return certificates.toArray(new Certificate[certificates.size()]);
-
- }
- catch (IOException e)
- {
- throw new GeneralSecurityException(e);
- }
- }
-
- @Override
- protected Certificate[] getCertificatesInternal() throws GeneralSecurityException
- {
- return getCertificates();
+ return _certificates;
}
private static URL getUrlFromString(String urlString) throws MalformedURLException
@@ -325,4 +264,82 @@ public class FileTrustStoreImpl extends AbstractTrustStore<FileTrustStoreImpl> i
_path = null;
}
}
+
+ private void initialize()
+ {
+ try
+ {
+ KeyStore ts = initializeKeyStore(this);
+ _trustManagers = createTrustManagers(ts);
+ _certificates = createCertificates(ts);
+ }
+ catch (Exception e)
+ {
+ throw new IllegalConfigurationException(String.format("Cannot instantiate trust store '%s'", getName()), e);
+ }
+ }
+
+ private TrustManager[] createTrustManagers(final KeyStore ts) throws NoSuchAlgorithmException, KeyStoreException
+ {
+ final TrustManagerFactory tmf = TrustManagerFactory.getInstance(_trustManagerFactoryAlgorithm);
+ tmf.init(ts);
+
+ TrustManager[] delegateManagers = tmf.getTrustManagers();
+ if (delegateManagers.length == 0)
+ {
+ throw new IllegalStateException("Truststore " + this + " defines no trust managers");
+ }
+ else if (delegateManagers.length == 1)
+ {
+ if (_peersOnly && delegateManagers[0] instanceof X509TrustManager)
+ {
+ return new TrustManager[] {new QpidPeersOnlyTrustManager(ts, ((X509TrustManager) delegateManagers[0]))};
+ }
+ else
+ {
+ return delegateManagers;
+ }
+ }
+ else
+ {
+ final Collection<TrustManager> trustManagersCol = new ArrayList<>();
+ final QpidMultipleTrustManager mulTrustManager = new QpidMultipleTrustManager();
+ for (TrustManager tm : delegateManagers)
+ {
+ if (tm instanceof X509TrustManager)
+ {
+ if (_peersOnly)
+ {
+ mulTrustManager.addTrustManager(new QpidPeersOnlyTrustManager(ts, (X509TrustManager) tm));
+ }
+ else
+ {
+ mulTrustManager.addTrustManager((X509TrustManager) tm);
+ }
+ }
+ else
+ {
+ trustManagersCol.add(tm);
+ }
+ }
+ if (! mulTrustManager.isEmpty())
+ {
+ trustManagersCol.add(mulTrustManager);
+ }
+ return trustManagersCol.toArray(new TrustManager[trustManagersCol.size()]);
+ }
+ }
+
+ private Certificate[] createCertificates(final KeyStore ts) throws KeyStoreException
+ {
+ final Collection<Certificate> certificates = new ArrayList<>();
+
+ Enumeration<String> aliases = ts.aliases();
+ while (aliases.hasMoreElements())
+ {
+ certificates.add(ts.getCertificate(aliases.nextElement()));
+ }
+
+ return certificates.toArray(new Certificate[certificates.size()]);
+ }
}
http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/175829a6/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 1f04b85..ba78084 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
@@ -211,9 +211,4 @@ public class ManagedPeerCertificateTrustStoreImpl
}
}
- @Override
- protected Certificate[] getCertificatesInternal() throws GeneralSecurityException
- {
- return getCertificates();
- }
}
http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/175829a6/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 ddcb291..152e4d3 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
@@ -98,14 +98,8 @@ public class NonJavaTrustStoreImpl
@Override
public Certificate[] getCertificates() throws GeneralSecurityException
{
- try
- {
- return SSLUtil.readCertificates(getUrlFromString(getCertificatesUrl()));
- }
- catch (IOException e)
- {
- throw new GeneralSecurityException(e);
- }
+ X509Certificate[] certificates = _certificates;
+ return certificates == null ? new X509Certificate[0] : certificates;
}
@Override
@@ -137,13 +131,6 @@ public class NonJavaTrustStoreImpl
validateTrustStoreAttributes(changedStore);
}
- @Override
- protected Certificate[] getCertificatesInternal() throws GeneralSecurityException
- {
- X509Certificate[] certificates = _certificates;
- return certificates == null ? new X509Certificate[0] : certificates;
- }
-
private void validateTrustStoreAttributes(NonJavaTrustStore<?> keyStore)
{
try
http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/175829a6/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 9e3ea69..38439e5 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
@@ -158,7 +158,8 @@ public class SiteSpecificTrustStoreImpl
@Override
public Certificate[] getCertificates() throws GeneralSecurityException
{
- return new Certificate[]{_x509Certificate};
+ X509Certificate x509Certificate = _x509Certificate;
+ return x509Certificate == null ? new Certificate[0] : new Certificate[]{x509Certificate};
}
@StateTransition(currentState = {State.ACTIVE, State.ERRORED}, desiredState = State.DELETED)
@@ -339,19 +340,6 @@ public class SiteSpecificTrustStoreImpl
doSync(modelFuture);
}
- @Override
- protected Certificate[] getCertificatesInternal() throws GeneralSecurityException
- {
- if (_x509Certificate == null)
- {
- return new Certificate[0];
- }
- else
- {
- return new Certificate[]{_x509Certificate};
- }
- }
-
private static class AlwaysTrustManager implements X509TrustManager
{
@Override
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org