From 0172e88d7560f451e98d6f1539487529c89cfc4c Mon Sep 17 00:00:00 2001 From: Sean Lair Date: Wed, 24 Mar 2021 14:11:31 -0500 Subject: [PATCH] allow cert renewal even if auth strictness is false includes updated tests and logging even w/o strictness --- .../ca/provider/RootCACustomTrustManager.java | 50 +++++++++++-------- .../ca/provider/RootCAProvider.java | 9 +++- .../RootCACustomTrustManagerTest.java | 38 +++++++++++++- .../ca/provider/RootCAProviderTest.java | 15 ++++-- 4 files changed, 83 insertions(+), 29 deletions(-) diff --git a/plugins/ca/root-ca/src/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java b/plugins/ca/root-ca/src/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java index 90f620393ee2..8a87478a8c50 100644 --- a/plugins/ca/root-ca/src/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java +++ b/plugins/ca/root-ca/src/org/apache/cloudstack/ca/provider/RootCACustomTrustManager.java @@ -79,13 +79,15 @@ public void checkClientTrusted(final X509Certificate[] certificates, final Strin if (LOG.isDebugEnabled()) { printCertificateChain(certificates, s); } - if (!authStrictness) { - return; - } - if (certificates == null || certificates.length < 1 || certificates[0] == null) { + + 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) { + return; } - final X509Certificate primaryClientCertificate = certificates[0]; // Revocation check final BigInteger serialNumber = primaryClientCertificate.getSerialNumber(); @@ -93,18 +95,19 @@ public void checkClientTrusted(final X509Certificate[] certificates, final Strin 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 +125,21 @@ public void checkClientTrusted(final X509Certificate[] certificates, final Strin 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 +149,6 @@ public void checkServerTrusted(X509Certificate[] x509Certificates, String s) thr @Override public X509Certificate[] getAcceptedIssuers() { - if (!authStrictness) { - return null; - } return new X509Certificate[]{caCertificate}; } } diff --git a/plugins/ca/root-ca/src/org/apache/cloudstack/ca/provider/RootCAProvider.java b/plugins/ca/root-ca/src/org/apache/cloudstack/ca/provider/RootCAProvider.java index d7a998537bd7..b0eebd41d761 100644 --- a/plugins/ca/root-ca/src/org/apache/cloudstack/ca/provider/RootCAProvider.java +++ b/plugins/ca/root-ca/src/org/apache/cloudstack/ca/provider/RootCAProvider.java @@ -267,9 +267,16 @@ public SSLEngine createSSLEngine(final SSLContext sslContext, final String remot 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/test/org/apache/cloudstack/ca/provider/RootCACustomTrustManagerTest.java b/plugins/ca/root-ca/test/org/apache/cloudstack/ca/provider/RootCACustomTrustManagerTest.java index 8161d75e0ef1..02daad4e2861 100644 --- a/plugins/ca/root-ca/test/org/apache/cloudstack/ca/provider/RootCACustomTrustManagerTest.java +++ b/plugins/ca/root-ca/test/org/apache/cloudstack/ca/provider/RootCACustomTrustManagerTest.java @@ -62,10 +62,44 @@ public void setUp() throws Exception { } @Test - public void testAuthNotStrict() throws Exception { + public void testAuthNotStrictWithInvalidCert() 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(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/test/org/apache/cloudstack/ca/provider/RootCAProviderTest.java b/plugins/ca/root-ca/test/org/apache/cloudstack/ca/provider/RootCAProviderTest.java index d5d64289da28..6a180c847299 100644 --- a/plugins/ca/root-ca/test/org/apache/cloudstack/ca/provider/RootCAProviderTest.java +++ b/plugins/ca/root-ca/test/org/apache/cloudstack/ca/provider/RootCAProviderTest.java @@ -41,7 +41,9 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; + import org.mockito.runners.MockitoJUnitRunner; +import org.mockito.Mockito; @RunWith(MockitoJUnitRunner.class) public class RootCAProviderTest { @@ -133,17 +135,20 @@ public void testRevokeCertificate() throws Exception { @Test public void testCreateSSLEngineWithoutAuthStrictness() throws Exception { - overrideDefaultConfigValue(RootCAProvider.rootCAAuthStrictness, "_defaultValue", "false"); + 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.assertFalse(e.getUseClientMode()); + + Assert.assertTrue(e.getWantClientAuth()); Assert.assertFalse(e.getNeedClientAuth()); } @Test public void testCreateSSLEngineWithAuthStrictness() throws Exception { - overrideDefaultConfigValue(RootCAProvider.rootCAAuthStrictness, "_defaultValue", "true"); + provider.rootCAAuthStrictness = Mockito.mock(ConfigKey.class); + Mockito.when(provider.rootCAAuthStrictness.value()).thenReturn(Boolean.TRUE); final SSLEngine e = provider.createSSLEngine(SSLUtils.getSSLContext(), "/1.2.3.4:5678", null); - Assert.assertFalse(e.getUseClientMode()); + Assert.assertTrue(e.getNeedClientAuth()); } @@ -152,4 +157,4 @@ public void testGetProviderName() throws Exception { Assert.assertEquals(provider.getProviderName(), "root"); } -} \ No newline at end of file +}