diff --git a/plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java b/plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java index 90f620393ee..0a4df0adeb5 100644 --- a/plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java +++ b/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 { - primaryClientCertificate.checkValidity(); - } catch (final CertificateExpiredException | CertificateNotYetValidException e) { - final String errorMsg = String.format("Client certificate has expired with serial=%x, subject=%s from address=%s", - primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress); - LOG.error(errorMsg); - throw new CertificateException(errorMsg); } + try { + primaryClientCertificate.checkValidity(); + } catch (final CertificateExpiredException | CertificateNotYetValidException e) { + final String errorMsg = String.format("Client certificate has expired with serial=%x, subject=%s from address=%s", + primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress); + LOG.error(errorMsg); + if (!allowExpiredCertificate) { + 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)) { - activeCertMap.put(clientAddress, primaryClientCertificate); + if (authStrictness && !Strings.isNullOrEmpty(exceptionMsg)) { + 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}; } } diff --git a/plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCAProvider.java b/plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCAProvider.java index d7a998537bd..b0eebd41d76 100644 --- a/plugins/ca/root-ca/src/main/java/org/apache/cloudstack/ca/provider/RootCAProvider.java +++ b/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; } diff --git a/plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManagerTest.java b/plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManagerTest.java index 8161d75e0ef..87695b80a3b 100644 --- a/plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCACustomTrustManagerTest.java +++ b/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) diff --git a/plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCAProviderTest.java b/plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCAProviderTest.java index f9fd531c4c2..15514b91c78 100644 --- a/plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCAProviderTest.java +++ b/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"); } -} \ No newline at end of file +}