Skip to content

Commit

Permalink
Fix code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@Akila94
Akila94 committed Jan 5, 2024
1 parent 495ef3e commit 90a9d11
Show file tree
Hide file tree
Showing 9 changed files with 85 additions and 66 deletions.
73 changes: 45 additions & 28 deletions ...va/org/apache/synapse/transport/certificatevalidation/CertificateVerificationManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -78,6 +80,7 @@ public CertificateVerificationManager(Integer cacheAllocatedSize, Integer cacheD
this.cacheDelayMins = cacheDelayMins;
}
this.isFullCertChainValidationEnabled = isFullCertChainValidationEnabled;
this.isCertExpiryValidationEnabled = isCertExpiryValidationEnabled;
}

/**
Expand All @@ -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);
Expand Down Expand Up @@ -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) {

Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
21 changes: 3 additions & 18 deletions ...ttp/src/main/java/org/apache/synapse/transport/certificatevalidation/cache/CertCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions ...ache/synapse/transport/certificatevalidation/pathvalidation/CertificatePathValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion ...ore/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ClientSSLSetupHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
8 changes: 1 addition & 7 deletions ...ore/nhttp/src/main/java/org/apache/synapse/transport/http/conn/ServerSSLSetupHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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;
Expand Down
38 changes: 30 additions & 8 deletions ...ttp/src/main/java/org/apache/synapse/transport/nhttp/config/ServerConnFactoryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 0 additions & 2 deletions ...re/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
4 changes: 4 additions & 0 deletions .../src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpMultiSSLListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{

Expand Down Expand Up @@ -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);
}

Expand Down
2 changes: 0 additions & 2 deletions ...core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpSender.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down

0 comments on commit 90a9d11

Please sign in to comment.