Skip to content

Commit 76d5ce3

Browse files
Slair1Slair1
authored
allow cert renewal even if auth strictness is false (#4852)
includes updated tests and logging
1 parent a1a3aff commit 76d5ce3

File tree

4 files changed

+75
-25
lines changed

4 files changed

+75
-25
lines changed

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

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -79,32 +79,36 @@ 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+
LOG.info("No certificate was received from client, but continuing since strict auth mode is disabled");
90+
return;
8791
}
88-
final X509Certificate primaryClientCertificate = certificates[0];
8992

9093
// Revocation check
9194
final BigInteger serialNumber = primaryClientCertificate.getSerialNumber();
9295
if (serialNumber == null || crlDao.findBySerial(serialNumber) != null) {
9396
final String errorMsg = String.format("Client is using revoked certificate of serial=%x, subject=%s from address=%s",
9497
primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
9598
LOG.error(errorMsg);
96-
throw new CertificateException(errorMsg);
99+
exceptionMsg = (Strings.isNullOrEmpty(exceptionMsg)) ? errorMsg : (exceptionMsg + ". " + errorMsg);
97100
}
98101

99102
// 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); }
103+
try {
104+
primaryClientCertificate.checkValidity();
105+
} catch (final CertificateExpiredException | CertificateNotYetValidException e) {
106+
final String errorMsg = String.format("Client certificate has expired with serial=%x, subject=%s from address=%s",
107+
primaryClientCertificate.getSerialNumber(), primaryClientCertificate.getSubjectDN(), clientAddress);
108+
LOG.error(errorMsg);
109+
if (!allowExpiredCertificate) {
110+
throw new CertificateException(errorMsg);
111+
}
108112
}
109113

110114
// Ownership check
@@ -122,13 +126,21 @@ public void checkClientTrusted(final X509Certificate[] certificates, final Strin
122126
if (!certMatchesOwnership) {
123127
final String errorMsg = "Certificate ownership verification failed for client: " + clientAddress;
124128
LOG.error(errorMsg);
125-
throw new CertificateException(errorMsg);
129+
exceptionMsg = (Strings.isNullOrEmpty(exceptionMsg)) ? errorMsg : (exceptionMsg + ". " + errorMsg);
126130
}
127-
if (activeCertMap != null && !Strings.isNullOrEmpty(clientAddress)) {
128-
activeCertMap.put(clientAddress, primaryClientCertificate);
131+
if (authStrictness && !Strings.isNullOrEmpty(exceptionMsg)) {
132+
throw new CertificateException(exceptionMsg);
129133
}
130134
if (LOG.isDebugEnabled()) {
131-
LOG.debug("Client/agent connection from ip=" + clientAddress + " has been validated and trusted.");
135+
if (authStrictness) {
136+
LOG.debug("Client/agent connection from ip=" + clientAddress + " has been validated and trusted.");
137+
} else {
138+
LOG.debug("Client/agent connection from ip=" + clientAddress + " accepted without certificate validation.");
139+
}
140+
}
141+
142+
if (primaryClientCertificate != null && activeCertMap != null && !Strings.isNullOrEmpty(clientAddress)) {
143+
activeCertMap.put(clientAddress, primaryClientCertificate);
132144
}
133145
}
134146

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

139151
@Override
140152
public X509Certificate[] getAcceptedIssuers() {
141-
if (!authStrictness) {
142-
return null;
143-
}
144153
return new X509Certificate[]{caCertificate};
145154
}
146155
}

‎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)