allow cert renewal even if auth strictness is false (#4852)

includes updated tests and logging

plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java

@@ -79,13 +79,16 @@ public final class RootCACustomTrustManager implements X509TrustManager {
         if (LOG.isDebugEnabled()) {
             printCertificateChain(certificates, s);
         }
-        if (!authStrictness) {
+
+        final X509Certificate primaryClientCertificate = (certificates != null && certificates.length > 0 && certificates[0] != null) ? certificates[0] : null;
+        String exceptionMsg = "";
+
+        if (authStrictness && primaryClientCertificate == null) {
+            throw new CertificateException("In strict auth mode, certificate(s) are expected from client:" + clientAddress);
+        } else if (primaryClientCertificate == null) {
+            LOG.info("No certificate was received from client, but continuing since strict auth mode is disabled");
             return;
         }
-        if (certificates == null || certificates.length < 1 || certificates[0] == null) {
-            throw new CertificateException("In strict auth mode, certificate(s) are expected from client:" + clientAddress);
-        }
-        final X509Certificate primaryClientCertificate = certificates[0];
 
         // Revocation check
         final BigInteger serialNumber = primaryClientCertificate.getSerialNumber();
@@ -93,18 +96,19 @@ public final class RootCACustomTrustManager implements X509TrustManager {
             final String errorMsg = String.format("Client is using revoked certificate of serial=%x, subject=%s from address=%s",
                     primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
             LOG.error(errorMsg);
-            throw new CertificateException(errorMsg);
+            exceptionMsg = (Strings.isNullOrEmpty(exceptionMsg)) ? errorMsg : (exceptionMsg + ". " + errorMsg);
         }
 
         // Validity check
-        if (!allowExpiredCertificate) {
+        try {
-            try {
+            primaryClientCertificate.checkValidity();
-                primaryClientCertificate.checkValidity();
+        } catch (final CertificateExpiredException | CertificateNotYetValidException e) {
-            } catch (final CertificateExpiredException | CertificateNotYetValidException e) {
+            final String errorMsg = String.format("Client certificate has expired with serial=%x, subject=%s from address=%s",
-                final String errorMsg = String.format("Client certificate has expired with serial=%x, subject=%s from address=%s",
+                    primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
-                        primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
+            LOG.error(errorMsg);
-                LOG.error(errorMsg);
+            if (!allowExpiredCertificate) {
-                throw new CertificateException(errorMsg);                }
+                throw new CertificateException(errorMsg);
+            }
         }
 
         // Ownership check
@@ -122,13 +126,21 @@ public final class RootCACustomTrustManager implements X509TrustManager {
         if (!certMatchesOwnership) {
             final String errorMsg = "Certificate ownership verification failed for client: " + clientAddress;
             LOG.error(errorMsg);
-            throw new CertificateException(errorMsg);
+            exceptionMsg = (Strings.isNullOrEmpty(exceptionMsg)) ? errorMsg : (exceptionMsg + ". " + errorMsg);
         }
-        if (activeCertMap != null && !Strings.isNullOrEmpty(clientAddress)) {
+        if (authStrictness && !Strings.isNullOrEmpty(exceptionMsg)) {
-            activeCertMap.put(clientAddress, primaryClientCertificate);
+            throw new CertificateException(exceptionMsg);
         }
         if (LOG.isDebugEnabled()) {
-            LOG.debug("Client/agent connection from ip=" + clientAddress + " has been validated and trusted.");
+            if (authStrictness) {
+                LOG.debug("Client/agent connection from ip=" + clientAddress + " has been validated and trusted.");
+            } else {
+                LOG.debug("Client/agent connection from ip=" + clientAddress + " accepted without certificate validation.");
+            }
+        }
+
+        if (primaryClientCertificate != null && activeCertMap != null && !Strings.isNullOrEmpty(clientAddress)) {
+            activeCertMap.put(clientAddress, primaryClientCertificate);
         }
     }
 
@@ -138,9 +150,6 @@ public final class RootCACustomTrustManager implements X509TrustManager {
 
     @Override
     public X509Certificate[] getAcceptedIssuers() {
-        if (!authStrictness) {
-            return null;
-        }
         return new X509Certificate[]{caCertificate};
     }
 }

plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCAProvider.java

@@ -267,9 +267,16 @@ public final class RootCAProvider extends AdapterBase implements CAProvider, Con
         final boolean allowExpiredCertificate = rootCAAllowExpiredCert.value();
 
         TrustManager[] tms = new TrustManager[]{new RootCACustomTrustManager(remoteAddress, authStrictness, allowExpiredCertificate, certMap, caCertificate, crlDao)};
+
         sslContext.init(kmf.getKeyManagers(), tms, new SecureRandom());
         final SSLEngine sslEngine = sslContext.createSSLEngine();
-        sslEngine.setNeedClientAuth(authStrictness);
+        // If authStrictness require SSL and validate client cert, otherwise prefer SSL but don't validate client cert
+        if (authStrictness) {
+            sslEngine.setNeedClientAuth(true);  // Require SSL and client cert validation
+        } else {
+            sslEngine.setWantClientAuth(true);  // Prefer SSL but don't validate client cert
+        }
+
         return sslEngine;
     }
 

plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManagerTest.java

@@ -62,10 +62,43 @@ public class RootCACustomTrustManagerTest {
     }
 
     @Test
-    public void testAuthNotStrict() throws Exception {
+    public void testAuthNotStrictWithInvalidCert() throws Exception {
         final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, true, certMap, caCertificate, crlDao);
         trustManager.checkClientTrusted(null, null);
-        Assert.assertNull(trustManager.getAcceptedIssuers());
+    }
+
+    @Test
+    public void testAuthNotStrictWithRevokedCert() throws Exception {
+        Mockito.when(crlDao.findBySerial(Mockito.any(BigInteger.class))).thenReturn(new CrlVO());
+        final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, true, certMap, caCertificate, crlDao);
+        trustManager.checkClientTrusted(new X509Certificate[]{caCertificate}, "RSA");
+        Assert.assertTrue(certMap.containsKey(clientIp));
+        Assert.assertEquals(certMap.get(clientIp), caCertificate);
+    }
+
+    @Test
+    public void testAuthNotStrictWithInvalidCertOwnership() throws Exception {
+        Mockito.when(crlDao.findBySerial(Mockito.any(BigInteger.class))).thenReturn(null);
+        final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, true, certMap, caCertificate, crlDao);
+        trustManager.checkClientTrusted(new X509Certificate[]{caCertificate}, "RSA");
+        Assert.assertTrue(certMap.containsKey(clientIp));
+        Assert.assertEquals(certMap.get(clientIp), caCertificate);
+    }
+
+    @Test(expected = CertificateException.class)
+    public void testAuthNotStrictWithDenyExpiredCertAndOwnership() throws Exception {
+        Mockito.when(crlDao.findBySerial(Mockito.any(BigInteger.class))).thenReturn(null);
+        final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, false, certMap, caCertificate, crlDao);
+        trustManager.checkClientTrusted(new X509Certificate[]{expiredClientCertificate}, "RSA");
+    }
+
+    @Test
+    public void testAuthNotStrictWithAllowExpiredCertAndOwnership() throws Exception {
+        Mockito.when(crlDao.findBySerial(Mockito.any(BigInteger.class))).thenReturn(null);
+        final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, true, certMap, caCertificate, crlDao);
+        trustManager.checkClientTrusted(new X509Certificate[]{expiredClientCertificate}, "RSA");
+        Assert.assertTrue(certMap.containsKey(clientIp));
+        Assert.assertEquals(certMap.get(clientIp), expiredClientCertificate);
     }
 
     @Test(expected = CertificateException.class)

plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCAProviderTest.java

@@ -133,6 +133,7 @@ public class RootCAProviderTest {
         provider.rootCAAuthStrictness = Mockito.mock(ConfigKey.class);
         Mockito.when(provider.rootCAAuthStrictness.value()).thenReturn(Boolean.FALSE);
         final SSLEngine e = provider.createSSLEngine(SSLUtils.getSSLContext(), "/1.2.3.4:5678", null);
+        Assert.assertTrue(e.getWantClientAuth());
         Assert.assertFalse(e.getNeedClientAuth());
     }
 
@@ -149,4 +150,4 @@ public class RootCAProviderTest {
         Assert.assertEquals(provider.getProviderName(), "root");
     }
 
-}
+}