Skip to content

Commit b029acf

Browse files
Slair1Slair1
committed
allow cert renewal even if auth strictness is false
includes updated tests and logging
1 parent d4635e3 commit b029acf

4 files changed

Lines changed: 74 additions & 25 deletions

File tree

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

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -79,32 +79,35 @@ public void checkClientTrusted(final X509Certificate[] certificates, final Strin
7979
if (LOG.isDebugEnabled()) {
8080
printCertificateChain(certificates, s);
8181
}
82-
if (!authStrictness) {
83-
return;
84-
}
85-
if (certificates == null || certificates.length < 1 || certificates[0] == null) {
82+
83+
final X509Certificate primaryClientCertificate = (certificates != null && certificates.length > 0 && certificates[0] != null) ? certificates[0] : null;
84+
String exceptionMsg = "";
85+
86+
if (authStrictness && primaryClientCertificate == null) {
8687
throw new CertificateException("In strict auth mode, certificate(s) are expected from client:" + clientAddress);
88+
} else if (primaryClientCertificate == null) {
89+
return;
8790
}
88-
final X509Certificate primaryClientCertificate = certificates[0];
8991

9092
// Revocation check
9193
final BigInteger serialNumber = primaryClientCertificate.getSerialNumber();
9294
if (serialNumber == null || crlDao.findBySerial(serialNumber) != null) {
9395
final String errorMsg = String.format("Client is using revoked certificate of serial=%x, subject=%s from address=%s",
9496
primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
9597
LOG.error(errorMsg);
96-
throw new CertificateException(errorMsg);
98+
exceptionMsg = (Strings.isNullOrEmpty(exceptionMsg)) ? errorMsg : (exceptionMsg + ". " + errorMsg);
9799
}
98100

99101
// Validity check
100-
if (!allowExpiredCertificate) {
101-
try {
102-
primaryClientCertificate.checkValidity();
103-
} catch (final CertificateExpiredException | CertificateNotYetValidException e) {
104-
final String errorMsg = String.format("Client certificate has expired with serial=%x, subject=%s from address=%s",
105-
primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
106-
LOG.error(errorMsg);
107-
throw new CertificateException(errorMsg); }
102+
try {
103+
primaryClientCertificate.checkValidity();
104+
} catch (final CertificateExpiredException | CertificateNotYetValidException e) {
105+
final String errorMsg = String.format("Client certificate has expired with serial=%x, subject=%s from address=%s",
106+
primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
107+
LOG.error(errorMsg);
108+
if (!allowExpiredCertificate) {
109+
throw new CertificateException(errorMsg);
110+
}
108111
}
109112

110113
// Ownership check
@@ -122,13 +125,21 @@ public void checkClientTrusted(final X509Certificate[] certificates, final Strin
122125
if (!certMatchesOwnership) {
123126
final String errorMsg = "Certificate ownership verification failed for client: " + clientAddress;
124127
LOG.error(errorMsg);
125-
throw new CertificateException(errorMsg);
128+
exceptionMsg = (Strings.isNullOrEmpty(exceptionMsg)) ? errorMsg : (exceptionMsg + ". " + errorMsg);
126129
}
127-
if (activeCertMap != null && !Strings.isNullOrEmpty(clientAddress)) {
128-
activeCertMap.put(clientAddress, primaryClientCertificate);
130+
if (authStrictness && !Strings.isNullOrEmpty(exceptionMsg)) {
131+
throw new CertificateException(exceptionMsg);
129132
}
130133
if (LOG.isDebugEnabled()) {
131-
LOG.debug("Client/agent connection from ip=" + clientAddress + " has been validated and trusted.");
134+
if (authStrictness) {
135+
LOG.debug("Client/agent connection from ip=" + clientAddress + " has been validated and trusted.");
136+
} else {
137+
LOG.debug("Client/agent connection from ip=" + clientAddress + " accepted without certificate validation.");
138+
}
139+
}
140+
141+
if (primaryClientCertificate != null && activeCertMap != null && !Strings.isNullOrEmpty(clientAddress)) {
142+
activeCertMap.put(clientAddress, primaryClientCertificate);
132143
}
133144
}
134145

@@ -138,9 +149,6 @@ public void checkServerTrusted(X509Certificate[] x509Certificates, String s) thr
138149

139150
@Override
140151
public X509Certificate[] getAcceptedIssuers() {
141-
if (!authStrictness) {
142-
return null;
143-
}
144152
return new X509Certificate[]{caCertificate};
145153
}
146154
}

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,9 +267,16 @@ public SSLEngine createSSLEngine(final SSLContext sslContext, final String remot
267267
final boolean allowExpiredCertificate = rootCAAllowExpiredCert.value();
268268

269269
TrustManager[] tms = new TrustManager[]{new RootCACustomTrustManager(remoteAddress, authStrictness, allowExpiredCertificate, certMap, caCertificate, crlDao)};
270+
270271
sslContext.init(kmf.getKeyManagers(), tms, new SecureRandom());
271272
final SSLEngine sslEngine = sslContext.createSSLEngine();
272-
sslEngine.setNeedClientAuth(authStrictness);
273+
// If authStrictness require SSL and validate client cert, otherwise prefer SSL but don't validate client cert
274+
if (authStrictness) {
275+
sslEngine.setNeedClientAuth(true); // Require SSL and client cert validation
276+
} else {
277+
sslEngine.setWantClientAuth(true); // Prefer SSL but don't validate client cert
278+
}
279+
273280
return sslEngine;
274281
}
275282

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

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,43 @@ public void setUp() throws Exception {
6262
}
6363

6464
@Test
65-
public void testAuthNotStrict() throws Exception {
65+
public void testAuthNotStrictWithInvalidCert() throws Exception {
6666
final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, true, certMap, caCertificate, crlDao);
6767
trustManager.checkClientTrusted(null, null);
68-
Assert.assertNull(trustManager.getAcceptedIssuers());
68+
}
69+
70+
@Test
71+
public void testAuthNotStrictWithRevokedCert() throws Exception {
72+
Mockito.when(crlDao.findBySerial(Mockito.any(BigInteger.class))).thenReturn(new CrlVO());
73+
final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, true, certMap, caCertificate, crlDao);
74+
trustManager.checkClientTrusted(new X509Certificate[]{caCertificate}, "RSA");
75+
Assert.assertTrue(certMap.containsKey(clientIp));
76+
Assert.assertEquals(certMap.get(clientIp), caCertificate);
77+
}
78+
79+
@Test
80+
public void testAuthNotStrictWithInvalidCertOwnership() throws Exception {
81+
Mockito.when(crlDao.findBySerial(Mockito.any(BigInteger.class))).thenReturn(null);
82+
final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, true, certMap, caCertificate, crlDao);
83+
trustManager.checkClientTrusted(new X509Certificate[]{caCertificate}, "RSA");
84+
Assert.assertTrue(certMap.containsKey(clientIp));
85+
Assert.assertEquals(certMap.get(clientIp), caCertificate);
86+
}
87+
88+
@Test(expected = CertificateException.class)
89+
public void testAuthNotStrictWithDenyExpiredCertAndOwnership() throws Exception {
90+
Mockito.when(crlDao.findBySerial(Mockito.any(BigInteger.class))).thenReturn(null);
91+
final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, false, certMap, caCertificate, crlDao);
92+
trustManager.checkClientTrusted(new X509Certificate[]{expiredClientCertificate}, "RSA");
93+
}
94+
95+
@Test
96+
public void testAuthNotStrictWithAllowExpiredCertAndOwnership() throws Exception {
97+
Mockito.when(crlDao.findBySerial(Mockito.any(BigInteger.class))).thenReturn(null);
98+
final RootCACustomTrustManager trustManager = new RootCACustomTrustManager(clientIp, false, true, certMap, caCertificate, crlDao);
99+
trustManager.checkClientTrusted(new X509Certificate[]{expiredClientCertificate}, "RSA");
100+
Assert.assertTrue(certMap.containsKey(clientIp));
101+
Assert.assertEquals(certMap.get(clientIp), expiredClientCertificate);
69102
}
70103

71104
@Test(expected = CertificateException.class)

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ public void testCreateSSLEngineWithoutAuthStrictness() throws Exception {
133133
provider.rootCAAuthStrictness = Mockito.mock(ConfigKey.class);
134134
Mockito.when(provider.rootCAAuthStrictness.value()).thenReturn(Boolean.FALSE);
135135
final SSLEngine e = provider.createSSLEngine(SSLUtils.getSSLContext(), "/1.2.3.4:5678", null);
136+
Assert.assertTrue(e.getWantClientAuth());
136137
Assert.assertFalse(e.getNeedClientAuth());
137138
}
138139

@@ -149,4 +150,4 @@ public void testGetProviderName() throws Exception {
149150
Assert.assertEquals(provider.getProviderName(), "root");
150151
}
151152

152-
}
153+
}

0 commit comments

Comments
 (0)