Skip to content

Commit bbaaf0b

Browse files
tsaarnitsaarni
authored
ZOOKEEPER-4992: Avoid overriding same-subject certs in PEM trust store
Reviewers: anmolnar Author: tsaarni Closes #2336 from tsaarni/fix-zookeeper-4992
1 parent a3c478b commit bbaaf0b

4 files changed

Lines changed: 34 additions & 18 deletions

File tree

‎zookeeper-server/src/main/java/org/apache/zookeeper/util/PemReader.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@
4949
import javax.crypto.SecretKey;
5050
import javax.crypto.SecretKeyFactory;
5151
import javax.crypto.spec.PBEKeySpec;
52-
import javax.security.auth.x500.X500Principal;
5352

5453
/**
5554
* Note: this class is copied from io.airlift.security.pem.PemReader (see
@@ -93,8 +92,13 @@ public static KeyStore loadTrustStore(File certificateChainFile) throws IOExcept
9392

9493
List<X509Certificate> certificateChain = readCertificateChain(certificateChainFile);
9594
for (X509Certificate certificate : certificateChain) {
96-
X500Principal principal = certificate.getSubjectX500Principal();
97-
keyStore.setCertificateEntry(principal.getName("RFC2253"), certificate);
95+
String subject = certificate.getSubjectX500Principal().getName("RFC2253");
96+
String alias = subject;
97+
// Append a suffix alias-1, alias-2 ... if the same alias (subject) already exists.
98+
for (int i = 1; keyStore.containsAlias(alias); i++) {
99+
alias = subject + "-" + i;
100+
}
101+
keyStore.setCertificateEntry(alias, certificate);
98102
}
99103
return keyStore;
100104
}

‎zookeeper-server/src/test/java/org/apache/zookeeper/common/PEMFileLoaderTest.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import static org.junit.jupiter.api.Assertions.assertEquals;
2222
import static org.junit.jupiter.api.Assertions.assertThrows;
23+
import static org.junit.jupiter.api.Assertions.assertTrue;
2324
import java.io.IOException;
2425
import java.security.KeyStore;
2526
import java.security.KeyStoreException;
@@ -108,7 +109,7 @@ public void testLoadKeyStoreWithWrongFileType(
108109

109110
@ParameterizedTest
110111
@MethodSource("data")
111-
public void testLoadTrustStore(
112+
public void testLoadTrustStoreFromPemBundle(
112113
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
113114
throws Exception {
114115
init(caKeyType, certKeyType, keyPassword, paramIndex);
@@ -118,7 +119,9 @@ public void testLoadTrustStore(
118119
.setTrustStorePassword(x509TestContext.getTrustStorePassword())
119120
.build()
120121
.loadTrustStore();
121-
assertEquals(1, ts.size());
122+
assertEquals(2, ts.size());
123+
assertTrue(ts.containsAlias("cn=org.apache.zookeeper.common.x509testcontext root ca"));
124+
assertTrue(ts.containsAlias("cn=org.apache.zookeeper.common.x509testcontext root ca-1"));
122125
}
123126

124127
@ParameterizedTest

‎zookeeper-server/src/test/java/org/apache/zookeeper/common/X509TestContext.java‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.security.Security;
3030
import java.security.cert.X509Certificate;
3131
import java.util.Arrays;
32+
import java.util.List;
3233
import org.apache.commons.io.FileUtils;
3334
import org.bouncycastle.asn1.x500.X500NameBuilder;
3435
import org.bouncycastle.asn1.x500.style.BCStyle;
@@ -48,7 +49,7 @@ public class X509TestContext {
4849
private final X509KeyType trustStoreKeyType;
4950
private final KeyPair trustStoreKeyPair;
5051
private final long trustStoreCertExpirationMillis;
51-
private final X509Certificate trustStoreCertificate;
52+
private final List<X509Certificate> trustStoreCertificates;
5253
private final String trustStorePassword;
5354
private File trustStoreJksFile;
5455
private File trustStorePemFile;
@@ -99,11 +100,18 @@ private X509TestContext(File tempDir, KeyPair trustStoreKeyPair, long trustStore
99100

100101
X500NameBuilder caNameBuilder = new X500NameBuilder(BCStyle.INSTANCE);
101102
caNameBuilder.addRDN(BCStyle.CN, MethodHandles.lookup().lookupClass().getCanonicalName() + " Root CA");
102-
trustStoreCertificate = X509TestHelpers.newSelfSignedCACert(caNameBuilder.build(), trustStoreKeyPair, trustStoreCertExpirationMillis);
103+
// Create two CA certs to test multiple certs in PEM bundles.
104+
// Use same subject name to simulate multiple CA certs from the same CA (e.g. reissued cert with different key type).
105+
trustStoreCertificates = Arrays.asList(
106+
X509TestHelpers.newSelfSignedCACert(caNameBuilder.build(), trustStoreKeyPair,
107+
trustStoreCertExpirationMillis),
108+
X509TestHelpers.newSelfSignedCACert(caNameBuilder.build(), trustStoreKeyPair,
109+
trustStoreCertExpirationMillis)
110+
);
103111

104112
X500NameBuilder nameBuilder = new X500NameBuilder(BCStyle.INSTANCE);
105113
nameBuilder.addRDN(BCStyle.CN, MethodHandles.lookup().lookupClass().getCanonicalName() + " Zookeeper Test");
106-
keyStoreCertificate = X509TestHelpers.newCert(trustStoreCertificate, trustStoreKeyPair, nameBuilder.build(), keyStoreKeyPair.getPublic(), keyStoreCertExpirationMillis);
114+
keyStoreCertificate = X509TestHelpers.newCert(trustStoreCertificates.get(0), trustStoreKeyPair, nameBuilder.build(), keyStoreKeyPair.getPublic(), keyStoreCertExpirationMillis);
107115
trustStorePkcs12File = trustStorePemFile = trustStoreJksFile = null;
108116
keyStorePkcs12File = keyStorePemFile = keyStoreJksFile = null;
109117

@@ -139,8 +147,8 @@ public long getTrustStoreCertExpirationMillis() {
139147
return trustStoreCertExpirationMillis;
140148
}
141149

142-
public X509Certificate getTrustStoreCertificate() {
143-
return trustStoreCertificate;
150+
public List<X509Certificate> getTrustStoreCertificates() {
151+
return trustStoreCertificates;
144152
}
145153

146154
public String getTrustStorePassword() {
@@ -159,7 +167,7 @@ public File getTrustStoreFile(KeyStoreFileType storeFileType) throws IOException
159167
case JKS:
160168
return getTrustStoreJksFile();
161169
case PEM:
162-
return getTrustStorePemFile();
170+
return getTrustStorePemBundleFile();
163171
case PKCS12:
164172
return getTrustStorePkcs12File();
165173
case BCFKS:
@@ -177,7 +185,7 @@ private File getTrustStoreJksFile() throws IOException {
177185
File trustStoreJksFile = File.createTempFile(TRUST_STORE_PREFIX, KeyStoreFileType.JKS.getDefaultFileExtension(), tempDir);
178186
trustStoreJksFile.deleteOnExit();
179187
try (final FileOutputStream trustStoreOutputStream = new FileOutputStream(trustStoreJksFile)) {
180-
byte[] bytes = X509TestHelpers.certToJavaTrustStoreBytes(trustStoreCertificate, trustStorePassword);
188+
byte[] bytes = X509TestHelpers.certToJavaTrustStoreBytes(trustStoreCertificates.get(0), trustStorePassword);
181189
trustStoreOutputStream.write(bytes);
182190
trustStoreOutputStream.flush();
183191
} catch (GeneralSecurityException e) {
@@ -188,11 +196,13 @@ private File getTrustStoreJksFile() throws IOException {
188196
return trustStoreJksFile;
189197
}
190198

191-
private File getTrustStorePemFile() throws IOException {
199+
private File getTrustStorePemBundleFile() throws IOException {
192200
if (trustStorePemFile == null) {
193201
File trustStorePemFile = File.createTempFile(TRUST_STORE_PREFIX, KeyStoreFileType.PEM.getDefaultFileExtension(), tempDir);
194202
trustStorePemFile.deleteOnExit();
195-
FileUtils.writeStringToFile(trustStorePemFile, X509TestHelpers.pemEncodeX509Certificate(trustStoreCertificate), StandardCharsets.US_ASCII, false);
203+
for (X509Certificate cert : trustStoreCertificates) {
204+
FileUtils.writeStringToFile(trustStorePemFile, X509TestHelpers.pemEncodeX509Certificate(cert), StandardCharsets.US_ASCII, true);
205+
}
196206
this.trustStorePemFile = trustStorePemFile;
197207
}
198208
return trustStorePemFile;
@@ -203,7 +213,7 @@ private File getTrustStorePkcs12File() throws IOException {
203213
File trustStorePkcs12File = File.createTempFile(TRUST_STORE_PREFIX, KeyStoreFileType.PKCS12.getDefaultFileExtension(), tempDir);
204214
trustStorePkcs12File.deleteOnExit();
205215
try (final FileOutputStream trustStoreOutputStream = new FileOutputStream(trustStorePkcs12File)) {
206-
byte[] bytes = X509TestHelpers.certToPKCS12TrustStoreBytes(trustStoreCertificate, trustStorePassword);
216+
byte[] bytes = X509TestHelpers.certToPKCS12TrustStoreBytes(trustStoreCertificates.get(0), trustStorePassword);
207217
trustStoreOutputStream.write(bytes);
208218
trustStoreOutputStream.flush();
209219
} catch (GeneralSecurityException e) {
@@ -220,7 +230,7 @@ private File getTrustStoreBcfksFile() throws IOException {
220230
TRUST_STORE_PREFIX, KeyStoreFileType.BCFKS.getDefaultFileExtension(), tempDir);
221231
trustStoreBcfksFile.deleteOnExit();
222232
try (final FileOutputStream trustStoreOutputStream = new FileOutputStream(trustStoreBcfksFile)) {
223-
byte[] bytes = X509TestHelpers.certToBCFKSTrustStoreBytes(trustStoreCertificate, trustStorePassword);
233+
byte[] bytes = X509TestHelpers.certToBCFKSTrustStoreBytes(trustStoreCertificates.get(0), trustStorePassword);
224234
trustStoreOutputStream.write(bytes);
225235
trustStoreOutputStream.flush();
226236
} catch (GeneralSecurityException e) {

‎zookeeper-server/src/test/java/org/apache/zookeeper/util/PemReaderTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,7 @@ public void testLoadCertificateFromTrustStore(
135135
throws Exception {
136136
init(caKeyType, certKeyType, keyPassword, paramIndex);
137137
List<X509Certificate> certs = PemReader.readCertificateChain(x509TestContext.getTrustStoreFile(KeyStoreFileType.PEM));
138-
assertEquals(1, certs.size());
139-
assertEquals(x509TestContext.getTrustStoreCertificate(), certs.get(0));
138+
assertEquals(x509TestContext.getTrustStoreCertificates(), certs);
140139
}
141140

142141
}

0 commit comments

Comments
 (0)