fix(auth): enforce client certificates and configured CRL checks

src/main/java/io/mapsmessaging/security/ssl/CrlTrustManager.java

@@ -19,60 +19,114 @@
  */
 
 package io.mapsmessaging.security.ssl;
+
 import java.net.Socket;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
-import javax.net.ssl.*;
+import java.util.Objects;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509ExtendedTrustManager;
+import javax.net.ssl.X509TrustManager;
 
 public class CrlTrustManager extends X509ExtendedTrustManager {
+
+  private final X509TrustManager trustManager;
   private final CertificateRevocationManager revocationManager;
 
   public CrlTrustManager(CertificateRevocationManager revocationManager) {
-    this.revocationManager = revocationManager;
+    this(createDefaultTrustManager(), revocationManager);
+  }
+
+  public CrlTrustManager(X509TrustManager trustManager, CertificateRevocationManager revocationManager) {
+    this.trustManager = Objects.requireNonNull(trustManager);
+    this.revocationManager = Objects.requireNonNull(revocationManager);
   }
 
   @Override
   public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket) throws CertificateException {
+    if (trustManager instanceof X509ExtendedTrustManager extendedTrustManager) {
+      extendedTrustManager.checkClientTrusted(chain, authType, socket);
+    } else {
+      trustManager.checkClientTrusted(chain, authType);
+    }
     checkRevocation(chain);
   }
 
   @Override
   public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket) throws CertificateException {
+    if (trustManager instanceof X509ExtendedTrustManager extendedTrustManager) {
+      extendedTrustManager.checkServerTrusted(chain, authType, socket);
+    } else {
+      trustManager.checkServerTrusted(chain, authType);
+    }
     checkRevocation(chain);
   }
 
   @Override
   public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine) throws CertificateException {
+    if (trustManager instanceof X509ExtendedTrustManager extendedTrustManager) {
+      extendedTrustManager.checkClientTrusted(chain, authType, engine);
+    } else {
+      trustManager.checkClientTrusted(chain, authType);
+    }
     checkRevocation(chain);
   }
 
   @Override
   public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine) throws CertificateException {
-    checkRevocation(chain);
-  }
-
-  // Implement other required methods delegating to defaultTrustManager
-
-  private void checkRevocation(X509Certificate[] chain) throws CertificateException {
-    for (X509Certificate certificate : chain) {
-      if (revocationManager.isCertificateRevoked(certificate)) {
-        throw new CertificateException("Certificate is revoked");
-      }
+    if (trustManager instanceof X509ExtendedTrustManager extendedTrustManager) {
+      extendedTrustManager.checkServerTrusted(chain, authType, engine);
+    } else {
+      trustManager.checkServerTrusted(chain, authType);
     }
+    checkRevocation(chain);
   }
 
   @Override
   public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException {
+    trustManager.checkClientTrusted(chain, authType);
     checkRevocation(chain);
   }
 
   @Override
   public void checkServerTrusted(X509Certificate[] chain, String authType) throws CertificateException {
+    trustManager.checkServerTrusted(chain, authType);
     checkRevocation(chain);
   }
 
   @Override
   public X509Certificate[] getAcceptedIssuers() {
-    return new X509Certificate[0];
+    return trustManager.getAcceptedIssuers();
+  }
+
+  private static X509TrustManager createDefaultTrustManager() {
+    try {
+      TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+      trustManagerFactory.init((KeyStore) null);
+      for (TrustManager manager : trustManagerFactory.getTrustManagers()) {
+        if (manager instanceof X509TrustManager trustManager) {
+          return trustManager;
+        }
+      }
+      throw new IllegalStateException("No default X509 trust manager is available");
+    } catch (GeneralSecurityException e) {
+      throw new IllegalStateException("Unable to initialise the default X509 trust manager", e);
+    }
+  }
+
+  private void checkRevocation(X509Certificate[] chain) throws CertificateException {
+    try {
+      for (X509Certificate certificate : chain) {
+        if (revocationManager.isCertificateRevoked(certificate)) {
+          throw new CertificateException("Certificate is revoked");
+        }
+      }
+    } catch (RuntimeException e) {
+      throw new CertificateException("Unable to validate certificate revocation status", e);
+    }
   }
 }

src/main/java/io/mapsmessaging/security/ssl/SslHelper.java

@@ -31,9 +31,6 @@
 import java.net.URL;
 import java.security.*;
 import java.security.cert.CertificateException;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
 import javax.net.ssl.*;
 
 /**
@@ -130,12 +127,21 @@ public static SSLContext createContext(String context, ConfigurationProperties c
       // Now check to see if there is a CRL configured, if so then construct the cert revocation during cert validation
       TrustManager[] trustManagers = trustManagerFactory.getTrustManagers();
       String crlUrlPath = config.getProperty("crlUrl");
-      if(crlUrlPath != null && !crlUrlPath.isEmpty()){
-        List<TrustManager> trustManagerList = new ArrayList<>(Arrays.asList(trustManagers));
+      if (crlUrlPath != null && !crlUrlPath.isEmpty()) {
         URL crlUrl = URI.create(crlUrlPath).toURL();
-        CertificateRevocationManager certificateRevocationManager = new CertificateRevocationManager(crlUrl, config.getLongProperty("crlInterval", 60*60*24)); // Default daily
-        trustManagerList.add(new CrlTrustManager(certificateRevocationManager));
-        trustManagers = trustManagerList.toArray(trustManagers);
+        long crlInterval = config.getLongProperty("crlInterval", 24L * 60L * 60L * 1000L);
+        CertificateRevocationManager certificateRevocationManager = new CertificateRevocationManager(crlUrl, crlInterval);
+        boolean crlManagerInstalled = false;
+        for (int index = 0; index < trustManagers.length; index++) {
+          if (trustManagers[index] instanceof X509TrustManager trustManager) {
+            trustManagers[index] = new CrlTrustManager(trustManager, certificateRevocationManager);
+            crlManagerInstalled = true;
+            break;
+          }
+        }
+        if (!crlManagerInstalled) {
+          throw new KeyManagementException("CRL configured but no X509 trust manager is available");
+        }
       }
 
       sslContext.init(keyManagers, trustManagers, new SecureRandom());
@@ -153,10 +159,15 @@ public static SSLContext createContext(String context, ConfigurationProperties c
     return sslContext;
   }
 
-  public static SSLEngine createSSLEngine(SSLContext sslContext, ConfigurationProperties tls){
+  public static SSLEngine createSSLEngine(SSLContext sslContext, ConfigurationProperties tls) {
     SSLEngine sslEngine = sslContext.createSSLEngine();
-    sslEngine.setNeedClientAuth(tls.getBooleanProperty("clientCertificateRequired", false));
-    sslEngine.setWantClientAuth(tls.getBooleanProperty("clientCertificateWanted", false));
+    boolean clientCertificateRequired = tls.getBooleanProperty("clientCertificateRequired", false);
+    boolean clientCertificateWanted = tls.getBooleanProperty("clientCertificateWanted", false);
+    if (clientCertificateRequired) {
+      sslEngine.setNeedClientAuth(true);
+    } else {
+      sslEngine.setWantClientAuth(clientCertificateWanted);
+    }
     return sslEngine;
   }
 

src/test/java/io/mapsmessaging/security/ssl/CrlTest.java

@@ -21,24 +21,80 @@
 package io.mapsmessaging.security.ssl;
 
 import io.mapsmessaging.security.certificates.CertificateUtils;
-import java.io.IOException;
 import java.net.URL;
 import java.security.cert.Certificate;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
-import org.bouncycastle.operator.OperatorCreationException;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.net.ssl.X509TrustManager;
 import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 class CrlTest {
 
-  @Disabled // Need a new crl list to test from
   @Test
-  void simpleCrlTest() throws IOException, CertificateException, OperatorCreationException {
-    CertificateRevocationManager certificateRevocationManager = new CertificateRevocationManager(new URL("http://crls.pki.goog/gts1c3/zdATt0Ex_Fk.crl"), 10L*24L*60L*60L*1000L);
-    Assertions.assertNotNull(certificateRevocationManager);
-    Certificate cert = CertificateUtils.generateSelfSignedCertificateSecret("fred").getCertificate();
-    Assertions.assertFalse(certificateRevocationManager.isCertificateRevoked((X509Certificate)cert));
+  void configured_crl_check_runs_after_default_trust_validation() throws Exception {
+    X509Certificate certificate = createCertificate();
+    AtomicBoolean trustValidationCalled = new AtomicBoolean();
+    CrlTrustManager trustManager = new CrlTrustManager(
+        new RecordingTrustManager(certificate, trustValidationCalled),
+        revocationManager(false));
+
+    trustManager.checkServerTrusted(new X509Certificate[]{certificate}, "RSA");
+
+    Assertions.assertTrue(trustValidationCalled.get());
+    Assertions.assertArrayEquals(new X509Certificate[]{certificate}, trustManager.getAcceptedIssuers());
+  }
+
+  @Test
+  void revoked_certificate_is_rejected() throws Exception {
+    X509Certificate certificate = createCertificate();
+    CrlTrustManager trustManager = new CrlTrustManager(
+        new RecordingTrustManager(certificate, new AtomicBoolean()),
+        revocationManager(true));
+
+    Assertions.assertThrows(
+        CertificateException.class,
+        () -> trustManager.checkServerTrusted(new X509Certificate[]{certificate}, "RSA"));
+  }
+
+  private X509Certificate createCertificate() throws Exception {
+    Certificate certificate = CertificateUtils.generateSelfSignedCertificateSecret("fred").getCertificate();
+    return (X509Certificate) certificate;
+  }
+
+  private CertificateRevocationManager revocationManager(boolean revoked) throws Exception {
+    return new CertificateRevocationManager(new URL("file:/unused.crl"), 1000L) {
+      @Override
+      public boolean isCertificateRevoked(X509Certificate certificate) {
+        return revoked;
+      }
+    };
+  }
+
+  private static final class RecordingTrustManager implements X509TrustManager {
+
+    private final X509Certificate[] acceptedIssuers;
+    private final AtomicBoolean validationCalled;
+
+    private RecordingTrustManager(X509Certificate acceptedIssuer, AtomicBoolean validationCalled) {
+      this.acceptedIssuers = new X509Certificate[]{acceptedIssuer};
+      this.validationCalled = validationCalled;
+    }
+
+    @Override
+    public void checkClientTrusted(X509Certificate[] chain, String authType) {
+      validationCalled.set(true);
+    }
+
+    @Override
+    public void checkServerTrusted(X509Certificate[] chain, String authType) {
+      validationCalled.set(true);
+    }
+
+    @Override
+    public X509Certificate[] getAcceptedIssuers() {
+      return acceptedIssuers;
+    }
   }
 }

src/test/java/io/mapsmessaging/security/ssl/SslHelperTest.java

@@ -54,4 +54,35 @@ void simpleHelperTest() throws Exception {
     SSLEngine sslEngine = SslHelper.createSSLEngine(sslContext, new ConfigurationProperties() );
     Assertions.assertNotNull(sslEngine);
   }
+
+  @Test
+  void required_client_certificate_takes_precedence_over_wanted() throws Exception {
+    SSLEngine sslEngine = createEngine(true, true);
+
+    Assertions.assertTrue(sslEngine.getNeedClientAuth());
+    Assertions.assertFalse(sslEngine.getWantClientAuth());
+  }
+
+  @Test
+  void wanted_client_certificate_is_preserved() throws Exception {
+    SSLEngine sslEngine = createEngine(false, true);
+
+    Assertions.assertFalse(sslEngine.getNeedClientAuth());
+    Assertions.assertTrue(sslEngine.getWantClientAuth());
+  }
+
+  @Test
+  void client_certificate_authentication_can_be_disabled() throws Exception {
+    SSLEngine sslEngine = createEngine(false, false);
+
+    Assertions.assertFalse(sslEngine.getNeedClientAuth());
+    Assertions.assertFalse(sslEngine.getWantClientAuth());
+  }
+
+  private SSLEngine createEngine(boolean required, boolean wanted) throws Exception {
+    ConfigurationProperties properties = new ConfigurationProperties();
+    properties.put("clientCertificateRequired", required);
+    properties.put("clientCertificateWanted", wanted);
+    return SslHelper.createSSLEngine(SSLContext.getDefault(), properties);
+  }
 }