From d040c6ab5c0ba7a087f257d797c283354c9fc3af Mon Sep 17 00:00:00 2001 From: akila94 Date: Fri, 5 Jan 2024 14:29:58 +0530 Subject: [PATCH] Fix code review comments --- .../CertificateVerificationManager.java | 73 ++++++++++++------- .../cache/CertCache.java | 21 +----- .../CertificatePathValidator.java | 1 + .../http/conn/ClientSSLSetupHandler.java | 2 +- .../http/conn/ServerSSLSetupHandler.java | 8 +- .../config/ServerConnFactoryBuilder.java | 38 ++++++++-- .../passthru/PassThroughHttpListener.java | 2 - .../PassThroughHttpMultiSSLListener.java | 4 + .../passthru/PassThroughHttpSender.java | 2 - 9 files changed, 85 insertions(+), 66 deletions(-) diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/CertificateVerificationManager.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/CertificateVerificationManager.java index ed70bc8f5a..100767e1ef 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/CertificateVerificationManager.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/CertificateVerificationManager.java @@ -51,7 +51,8 @@ public class CertificateVerificationManager { private int cacheSize = Constants.CACHE_DEFAULT_ALLOCATED_SIZE; private int cacheDelayMins = Constants.CACHE_DEFAULT_DELAY_MINS; - private boolean isFullCertChainValidationEnabled = false; + private boolean isFullCertChainValidationEnabled = true; + private boolean isCertExpiryValidationEnabled = false; private static final Log log = LogFactory.getLog(CertificateVerificationManager.class); public CertificateVerificationManager(Integer cacheAllocatedSize, Integer cacheDelayMins) { @@ -67,7 +68,8 @@ public CertificateVerificationManager(Integer cacheAllocatedSize, Integer cacheD } public CertificateVerificationManager(Integer cacheAllocatedSize, Integer cacheDelayMins, - boolean isFullCertChainValidationEnabled) { + boolean isFullCertChainValidationEnabled, + boolean isCertExpiryValidationEnabled) { if (cacheAllocatedSize != null && cacheAllocatedSize > Constants.CACHE_MIN_ALLOCATED_SIZE && cacheAllocatedSize < Constants.CACHE_MAX_ALLOCATED_SIZE) { @@ -78,6 +80,7 @@ public CertificateVerificationManager(Integer cacheAllocatedSize, Integer cacheD this.cacheDelayMins = cacheDelayMins; } this.isFullCertChainValidationEnabled = isFullCertChainValidationEnabled; + this.isCertExpiryValidationEnabled = isCertExpiryValidationEnabled; } /** @@ -93,7 +96,7 @@ public CertificateVerificationManager(Integer cacheAllocatedSize, Integer cacheD * @param peerCertificates javax.security.cert.X509Certificate[] array of peer certificate chain from peer/client. * @throws CertificateVerificationException */ - public void verifyRevocationStatus(javax.security.cert.X509Certificate[] peerCertificates) + public void verifyCertificateValidity(javax.security.cert.X509Certificate[] peerCertificates) throws CertificateVerificationException { X509Certificate[] convertedCertificates = convert(peerCertificates); @@ -123,7 +126,6 @@ public void verifyRevocationStatus(javax.security.cert.X509Certificate[] peerCer // Get cert cache and initialize it CertCache certCache = CertCache.getCache(); - certCache.init(cacheSize,cacheDelayMins); if (certCache.getCacheValue(peerCert.getSerialNumber().toString()) == null) { @@ -134,20 +136,22 @@ public void verifyRevocationStatus(javax.security.cert.X509Certificate[] peerCer } while (aliases.hasMoreElements()) { + + alias = aliases.nextElement(); try { - alias = aliases.nextElement(); - try { - issuerCert = (X509Certificate) trustStore.getCertificate(alias); - } catch (KeyStoreException e) { - throw new CertificateVerificationException("Unable to read the certificate from " + - "truststore with the alias: " + alias, e); - } + issuerCert = (X509Certificate) trustStore.getCertificate(alias); + } catch (KeyStoreException e) { + throw new CertificateVerificationException("Unable to read the certificate from " + + "truststore with the alias: " + alias, e); + } - if (issuerCert == null) { - throw new CertificateVerificationException("Issuer certificate not found in truststore"); - } + if (issuerCert == null) { + throw new CertificateVerificationException("Issuer certificate not found in truststore"); + } + try { peerCert.verify(issuerCert.getPublicKey()); + log.debug("Valid issuer certificate found in the client truststore. Caching.."); // Store the valid issuer cert in cache for future use @@ -160,7 +164,7 @@ public void verifyRevocationStatus(javax.security.cert.X509Certificate[] peerCer break; } catch (SignatureException | CertificateException | NoSuchAlgorithmException | InvalidKeyException | NoSuchProviderException e) { - // Unable to verify the signature. Check with the next certificate. + // Unable to verify the signature. Check with the next certificate in the next loop traversal. } } } else { @@ -188,16 +192,30 @@ public void verifyRevocationStatus(javax.security.cert.X509Certificate[] peerCer for (RevocationVerifier verifier : verifiers) { try { if (isFullCertChainValidationEnabled) { + + if (isCertExpiryValidationEnabled) { + log.debug("Validating certificate chain for expiry"); + if (isExpired(convertedCertificates)) { + throw new CertificateVerificationException("One of the provided certificates are expired"); + } + } + log.debug("Doing full certificate chain validation"); CertificatePathValidator pathValidator = new CertificatePathValidator(convertedCertificates, verifier); pathValidator.validatePath(); log.info("Path verification Successful. Took " + (System.currentTimeMillis() - start) + " ms."); } else { - if (log.isDebugEnabled()) { - log.debug("Validating client certificate with the issuer certificate retrieved from" + - "the trust store"); + + if (isCertExpiryValidationEnabled) { + log.debug("Validating the client certificate for expiry"); + if (isExpired(convertedCertificates)) { + throw new CertificateVerificationException("The provided certificate is expired"); + } } + + log.debug("Validating client certificate with the issuer certificate retrieved from" + + "the trust store"); verifier.checkRevocationStatus(peerCert, issuerCert); } return; @@ -241,23 +259,22 @@ private X509Certificate[] convert(javax.security.cert.X509Certificate[] certs) /** * Checks whether a provided certificate is expired or not at the time it is validated. * - * @param certificates array of certificates needs to be validated - * @throws CertificateVerificationException if one of the certs are expired, this exception will be thrown + * @param certificates certificates to be validated for expiry + * @return true if one of the certs are expired, false otherwise */ - public void isExpired(javax.security.cert.X509Certificate[] certificates) throws CertificateVerificationException { - - X509Certificate[] convertedCertificates = convert(certificates); + public boolean isExpired(X509Certificate[] certificates) { - for (X509Certificate cert : convertedCertificates) { + for (X509Certificate cert : certificates) { try { cert.checkValidity(); } catch (CertificateExpiredException e) { - String msg = "Peer certificate is expired"; - throw new CertificateVerificationException(msg); + log.error("Peer certificate is expired"); + return true; } catch (CertificateNotYetValidException e) { - String msg = "Peer certificate is not valid yet"; - throw new CertificateVerificationException(msg); + log.error("Peer certificate is not valid yet"); + return true; } } + return false; } } diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/cache/CertCache.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/cache/CertCache.java index 4dbd3d847a..9ada86e88d 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/cache/CertCache.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/cache/CertCache.java @@ -21,6 +21,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.synapse.commons.jmx.MBeanRegistrar; +import org.apache.synapse.transport.certificatevalidation.Constants; import java.security.cert.X509Certificate; import java.util.Iterator; @@ -49,30 +50,14 @@ public static CertCache getCache() { synchronized (CertCache.class) { if (cache == null) { cache = new CertCache(); + cacheManager = new CacheManager(cache, Constants.CACHE_DEFAULT_ALLOCATED_SIZE, + Constants.CACHE_DEFAULT_DELAY_MINS); } } } return cache; } - /** - * This initialize the Cache with a CacheManager. If this method is called, a cache manager will not be used. - * - * @param size max size of the cache - * @param delay defines how frequently the CacheManager will be started - */ - public void init(int size, int delay) { - if (cacheManager == null) { - synchronized (CertCache.class) { - if (cacheManager == null) { - cacheManager = new CacheManager(cache, size, delay); - CacheController mbean = new CacheController(cache,cacheManager); - MBeanRegistrar.getInstance().registerMBean(mbean, "CacheController", "CertCacheController"); - } - } - } - } - public synchronized X509Certificate getCacheValue(String serialNumber) { CertCacheValue cacheValue = hashMap.get(serialNumber); if (cacheValue != null) { diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/pathvalidation/CertificatePathValidator.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/pathvalidation/CertificatePathValidator.java index b041a7314d..b6335f88a4 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/pathvalidation/CertificatePathValidator.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/certificatevalidation/pathvalidation/CertificatePathValidator.java @@ -22,6 +22,7 @@ import org.apache.commons.logging.LogFactory; import org.apache.synapse.transport.certificatevalidation.*; +import java.lang.reflect.InvocationTargetException; import java.security.*; import java.security.cert.X509Certificate; import java.util.List; diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ClientSSLSetupHandler.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ClientSSLSetupHandler.java index 803be5cedd..e5c94d91f6 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ClientSSLSetupHandler.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ClientSSLSetupHandler.java @@ -184,7 +184,7 @@ public void verify(IOSession iosession, SSLSession sslsession) throws SSLExcepti if (verificationManager!=null) { try { - verificationManager.verifyRevocationStatus(sslsession.getPeerCertificateChain()); + verificationManager.verifyCertificateValidity(sslsession.getPeerCertificateChain()); } catch (CertificateVerificationException e) { throw new SSLException("Certificate Chain Validation failed for host : " + address, e); } diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ServerSSLSetupHandler.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ServerSSLSetupHandler.java index 3152d8f054..bb4e7c08d6 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ServerSSLSetupHandler.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ServerSSLSetupHandler.java @@ -31,10 +31,6 @@ import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.SocketAddress; -import java.security.cert.CertificateExpiredException; -import java.security.cert.CertificateNotYetValidException; -import java.util.Arrays; -import java.util.List; public class ServerSSLSetupHandler implements SSLSetupHandler { @@ -83,9 +79,7 @@ public void verify( if (verificationManager != null) { try { - X509Certificate[] peerCertChain = sslsession.getPeerCertificateChain(); - verificationManager.isExpired(peerCertChain); - verificationManager.verifyRevocationStatus(peerCertChain); + verificationManager.verifyCertificateValidity(sslsession.getPeerCertificateChain()); } catch (CertificateVerificationException e) { SocketAddress remoteAddress = iosession.getRemoteAddress(); String address; diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java index 2598138105..b9b4b0018e 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java @@ -324,16 +324,23 @@ public ServerConnFactoryBuilder parseSSL() throws AxisFault { // Checking whether the full certificate chain validation is enabled or not. boolean isFullCertChainValidationEnabled = true; - OMElement isFullCertChainValidationConfig = cvp.getParameterElement() + boolean isCertExpiryValidationEnabled = false; + OMElement fullCertChainValidationConfig = cvp.getParameterElement() .getFirstChildWithName(new QName("FullChainValidation")); + OMElement certExpiryValidationConfig = cvp.getParameterElement() + .getFirstChildWithName(new QName("ExpiryValidation")); - if (isFullCertChainValidationConfig != null - && StringUtils.equals("false", isFullCertChainValidationConfig.getText())) { + if (fullCertChainValidationConfig != null + && StringUtils.equals("false", fullCertChainValidationConfig.getText())) { isFullCertChainValidationEnabled = false; } + if (certExpiryValidationConfig != null && StringUtils.equals("true", certExpiryValidationConfig.getText())) { + isCertExpiryValidationEnabled = true; + } + certificateVerifier = new CertificateVerificationManager(cacheSize, cacheDelay, - isFullCertChainValidationEnabled); + isFullCertChainValidationEnabled, isCertExpiryValidationEnabled); } ssl = createSSLContext(keyStoreEl, trustStoreEl, clientAuthEl, httpsProtocolsEl, preferredCiphersEl, @@ -397,16 +404,31 @@ public ServerConnFactoryBuilder parseMultiProfileSSL() throws AxisFault { } boolean isFullCertChainValidationEnabled = true; - OMElement isFullCertChainValidationConfig = revocationVerifierConfig + boolean isCertExpiryValidationEnabled = false; + + OMElement fullCertChainValidationConfig = revocationVerifierConfig .getFirstChildWithName(new QName("FullChainValidation")); - if (isFullCertChainValidationConfig != null - && StringUtils.equals("false", isFullCertChainValidationConfig.getText())) { + OMElement certExpiryValidationConfig = revocationVerifierConfig + .getFirstChildWithName(new QName("ExpiryValidation")); + + if (fullCertChainValidationConfig != null + && StringUtils.equals("false", fullCertChainValidationConfig.getText())) { + isFullCertChainValidationEnabled = false; + } + + if (certExpiryValidationConfig != null + && StringUtils.equals("true", certExpiryValidationConfig.getText())) { + isCertExpiryValidationEnabled = true; + } + + if (fullCertChainValidationConfig != null + && StringUtils.equals("false", fullCertChainValidationConfig.getText())) { isFullCertChainValidationEnabled = false; } certificateVerifier = new CertificateVerificationManager(cacheSize, cacheDelay, - isFullCertChainValidationEnabled); + isFullCertChainValidationEnabled, isCertExpiryValidationEnabled); } SSLContextDetails ssl = createSSLContext(keyStoreEl, trustStoreEl, clientAuthEl, httpsProtocolsEl, diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpListener.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpListener.java index b69cb80abd..3e7f92c6d6 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpListener.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpListener.java @@ -679,8 +679,6 @@ public void reloadDynamicSSLConfig(TransportInDescription transportInDescription Parameter profilePathParam = transportInDescription.getParameter("dynamicSSLProfilesConfig"); if (oldParameter != null && profilePathParam != null) { - CertCache.resetCache(); - TrustStoreHolder.resetInstance(); transportInDescription.removeParameter(oldParameter); this.reloadSpecificEndPoints(transportInDescription); } diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpMultiSSLListener.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpMultiSSLListener.java index 208b364178..406ae5a53b 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpMultiSSLListener.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpMultiSSLListener.java @@ -5,10 +5,12 @@ import org.apache.axis2.description.ParameterInclude; import org.apache.axis2.description.TransportInDescription; import org.apache.http.HttpHost; +import org.apache.synapse.transport.certificatevalidation.cache.CertCache; import org.apache.synapse.transport.http.conn.Scheme; import org.apache.synapse.transport.nhttp.config.ServerConnFactoryBuilder; import org.apache.synapse.transport.dynamicconfigurations.ListenerProfileReloader; import org.apache.synapse.transport.dynamicconfigurations.SSLProfileLoader; +import org.apache.synapse.transport.nhttp.config.TrustStoreHolder; public class PassThroughHttpMultiSSLListener extends PassThroughHttpListener implements SSLProfileLoader{ @@ -39,6 +41,8 @@ protected ServerConnFactoryBuilder initConnFactoryBuilder(final TransportInDescr * @throws AxisFault */ public void reloadConfig(ParameterInclude transport) throws AxisFault { + CertCache.resetCache(); + TrustStoreHolder.resetInstance(); reloadDynamicSSLConfig((TransportInDescription) transport); } diff --git a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpSender.java b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpSender.java index 7379363af3..b977eb30a7 100644 --- a/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpSender.java +++ b/modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpSender.java @@ -707,8 +707,6 @@ private void handleException(String msg) throws AxisFault { public void reloadDynamicSSLConfig(TransportOutDescription transport) throws AxisFault { log.info("PassThroughHttpSender reloading SSL Config.."); try { - CertCache.resetCache(); - TrustStoreHolder.resetInstance(); ClientConnFactoryBuilder connFactoryBuilder = initConnFactoryBuilder(transport, this.configurationContext); connFactory = connFactoryBuilder.createConnFactory(targetConfiguration.getHttpParams());