Skip to content

Commit c5ec704

Browse files
sagnghossagnghos
committed
fix(spanner): Detect PKCS#1 keys with actionable error and clean up test certs
1 parent 8473bb7 commit c5ec704

4 files changed

Lines changed: 267 additions & 186 deletions

File tree

‎java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/omni/DynamicKeyManager.java‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,15 @@ private void reloadMaterial() {
139139
PrivateKey key = parsePrivateKey(keyBytes);
140140

141141
this.currentMaterial = new KeyMaterial(certMod, certLen, keyMod, keyLen, chain, key);
142+
} catch (IllegalArgumentException e) {
143+
if (this.currentMaterial != null) {
144+
logger.log(
145+
Level.WARNING,
146+
"Error reloading client certificate or key, falling back to cached credentials",
147+
e);
148+
} else {
149+
throw e;
150+
}
142151
} catch (Exception e) {
143152
if (this.currentMaterial != null) {
144153
logger.log(
@@ -162,7 +171,13 @@ private static X509Certificate[] parseCertificates(byte[] certBytes) throws Cert
162171
}
163172

164173
private static PrivateKey parsePrivateKey(byte[] keyBytes) throws Exception {
165-
String keyStr = new String(keyBytes, StandardCharsets.US_ASCII);
174+
String keyStr = new String(keyBytes, StandardCharsets.UTF_8);
175+
if (keyStr.contains("-----BEGIN RSA PRIVATE KEY-----")
176+
|| keyStr.contains("-----BEGIN EC PRIVATE KEY-----")) {
177+
throw new IllegalArgumentException(
178+
"PKCS#1 private keys are not supported. Please convert your key to PKCS#8 format using: "
179+
+ "openssl pkcs8 -topk8 -nocrypt -in <key> -out <key_pkcs8>");
180+
}
166181
byte[] der;
167182
if (keyStr.contains("-----BEGIN PRIVATE KEY-----")) {
168183
der = extractPemContent(keyStr, "-----BEGIN PRIVATE KEY-----", "-----END PRIVATE KEY-----");

‎java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java‎

Lines changed: 39 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1692,46 +1692,51 @@ public void testUseClientCertAndTrustCertificate() throws Exception {
16921692
io.grpc.netty.shaded.io.netty.handler.ssl.util.SelfSignedCertificate ca =
16931693
new io.grpc.netty.shaded.io.netty.handler.ssl.util.SelfSignedCertificate("spanner.ca");
16941694

1695-
String certPath = ssc.certificate().getAbsolutePath();
1696-
String keyPath = ssc.privateKey().getAbsolutePath();
1697-
String caPath = ca.certificate().getAbsolutePath();
1695+
try {
1696+
String certPath = ssc.certificate().getAbsolutePath();
1697+
String keyPath = ssc.privateKey().getAbsolutePath();
1698+
String caPath = ca.certificate().getAbsolutePath();
16981699

1699-
SpannerOptions options =
1700-
SpannerOptions.newBuilder()
1701-
.setProjectId("test-project")
1702-
.setCredentials(NoCredentials.getInstance())
1703-
.setHost("https://localhost:1234")
1704-
.useClientCert(certPath, keyPath)
1705-
.setCaCertificate(caPath)
1706-
.build();
1700+
SpannerOptions options =
1701+
SpannerOptions.newBuilder()
1702+
.setProjectId("test-project")
1703+
.setCredentials(NoCredentials.getInstance())
1704+
.setHost("https://localhost:1234")
1705+
.useClientCert(certPath, keyPath)
1706+
.setCaCertificate(caPath)
1707+
.build();
17071708

1708-
assertNotNull(options.getChannelConfigurator());
1709+
assertNotNull(options.getChannelConfigurator());
17091710

1710-
SpannerOptions fromBuilder = options.toBuilder().build();
1711-
assertNotNull(fromBuilder.getChannelConfigurator());
1711+
SpannerOptions fromBuilder = options.toBuilder().build();
1712+
assertNotNull(fromBuilder.getChannelConfigurator());
17121713

1713-
// Test standalone setCaCertificate
1714-
SpannerOptions caOnlyOptions =
1715-
SpannerOptions.newBuilder()
1716-
.setProjectId("test-project")
1717-
.setCredentials(NoCredentials.getInstance())
1718-
.setHost("https://localhost:1234")
1719-
.setCaCertificate(caPath)
1720-
.build();
1714+
// Test standalone setCaCertificate
1715+
SpannerOptions caOnlyOptions =
1716+
SpannerOptions.newBuilder()
1717+
.setProjectId("test-project")
1718+
.setCredentials(NoCredentials.getInstance())
1719+
.setHost("https://localhost:1234")
1720+
.setCaCertificate(caPath)
1721+
.build();
17211722

1722-
assertNotNull(caOnlyOptions.getChannelConfigurator());
1723+
assertNotNull(caOnlyOptions.getChannelConfigurator());
17231724

1724-
// Test setCaCertificate combined with login (username/password)
1725-
SpannerOptions loginWithCaOptions =
1726-
SpannerOptions.newBuilder()
1727-
.setProjectId("test-project")
1728-
.setType(SpannerOptions.InstanceType.OMNI)
1729-
.setHost("https://localhost:1234")
1730-
.setCaCertificate(caPath)
1731-
.login("test-user", "test-pass".toCharArray())
1732-
.build();
1725+
// Test setCaCertificate combined with login (username/password)
1726+
SpannerOptions loginWithCaOptions =
1727+
SpannerOptions.newBuilder()
1728+
.setProjectId("test-project")
1729+
.setType(SpannerOptions.InstanceType.OMNI)
1730+
.setHost("https://localhost:1234")
1731+
.setCaCertificate(caPath)
1732+
.login("test-user", "test-pass".toCharArray())
1733+
.build();
17331734

1734-
assertTrue(loginWithCaOptions.getCredentials() instanceof SpannerOmniCredentials);
1735-
assertNotNull(loginWithCaOptions.getChannelConfigurator());
1735+
assertTrue(loginWithCaOptions.getCredentials() instanceof SpannerOmniCredentials);
1736+
assertNotNull(loginWithCaOptions.getChannelConfigurator());
1737+
} finally {
1738+
ssc.delete();
1739+
ca.delete();
1740+
}
17361741
}
17371742
}

‎java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/omni/DynamicKeyManagerTest.java‎

Lines changed: 124 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package com.google.cloud.spanner.omni;
1818

19+
import static com.google.common.truth.Truth.assertThat;
1920
import static org.junit.Assert.assertEquals;
2021
import static org.junit.Assert.assertNotNull;
2122
import static org.junit.Assert.assertNull;
@@ -41,99 +42,138 @@ public class DynamicKeyManagerTest {
4142
@Test
4243
public void testInitialLoadAndDynamicRotation() throws Exception {
4344
SelfSignedCertificate ssc1 = new SelfSignedCertificate("spanner.test.1");
44-
File certFile = tempFolder.newFile("client.crt");
45-
File keyFile = tempFolder.newFile("client.key");
45+
SelfSignedCertificate ssc2 = new SelfSignedCertificate("spanner.test.2");
46+
try {
47+
File certFile = tempFolder.newFile("client.crt");
48+
File keyFile = tempFolder.newFile("client.key");
4649

47-
Files.write(certFile.toPath(), Files.readAllBytes(ssc1.certificate().toPath()));
48-
Files.write(keyFile.toPath(), Files.readAllBytes(ssc1.privateKey().toPath()));
50+
Files.write(certFile.toPath(), Files.readAllBytes(ssc1.certificate().toPath()));
51+
Files.write(keyFile.toPath(), Files.readAllBytes(ssc1.privateKey().toPath()));
4952

50-
DynamicKeyManager keyManager = new DynamicKeyManager(certFile, keyFile, 0L);
53+
DynamicKeyManager keyManager = new DynamicKeyManager(certFile, keyFile, 0L);
5154

52-
String alias1 = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
53-
assertNotNull(alias1);
54-
assertEquals(alias1, keyManager.chooseEngineClientAlias(new String[] {"RSA"}, null, null));
55+
String alias1 = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
56+
assertNotNull(alias1);
57+
assertEquals(alias1, keyManager.chooseEngineClientAlias(new String[] {"RSA"}, null, null));
5558

56-
X509Certificate[] chain1 = keyManager.getCertificateChain(alias1);
57-
assertNotNull(chain1);
58-
assertEquals(1, chain1.length);
59-
assertEquals(ssc1.cert().getSubjectDN(), chain1[0].getSubjectDN());
59+
X509Certificate[] chain1 = keyManager.getCertificateChain(alias1);
60+
assertNotNull(chain1);
61+
assertEquals(1, chain1.length);
62+
assertEquals(ssc1.cert().getSubjectDN(), chain1[0].getSubjectDN());
6063

61-
PrivateKey pk1 = keyManager.getPrivateKey(alias1);
62-
assertNotNull(pk1);
63-
assertEquals(ssc1.key().getAlgorithm(), pk1.getAlgorithm());
64+
PrivateKey pk1 = keyManager.getPrivateKey(alias1);
65+
assertNotNull(pk1);
66+
assertEquals(ssc1.key().getAlgorithm(), pk1.getAlgorithm());
6467

65-
String[] aliases1 = keyManager.getClientAliases("RSA", null);
66-
assertNotNull(aliases1);
67-
assertEquals(1, aliases1.length);
68-
assertEquals(alias1, aliases1[0]);
68+
String[] aliases1 = keyManager.getClientAliases("RSA", null);
69+
assertNotNull(aliases1);
70+
assertEquals(1, aliases1.length);
71+
assertEquals(alias1, aliases1[0]);
6972

70-
// Ensure lastModified timestamp changes upon rotation
71-
Thread.sleep(1100);
73+
// Ensure lastModified timestamp changes upon rotation
74+
Thread.sleep(1100);
7275

73-
SelfSignedCertificate ssc2 = new SelfSignedCertificate("spanner.test.2");
74-
Files.write(certFile.toPath(), Files.readAllBytes(ssc2.certificate().toPath()));
75-
Files.write(keyFile.toPath(), Files.readAllBytes(ssc2.privateKey().toPath()));
76+
Files.write(certFile.toPath(), Files.readAllBytes(ssc2.certificate().toPath()));
77+
Files.write(keyFile.toPath(), Files.readAllBytes(ssc2.privateKey().toPath()));
7678

77-
String alias2 = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
78-
assertNotNull(alias2);
79+
String alias2 = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
80+
assertNotNull(alias2);
7981

80-
X509Certificate[] chain2 = keyManager.getCertificateChain(alias2);
81-
assertNotNull(chain2);
82-
assertEquals(ssc2.cert().getSubjectDN(), chain2[0].getSubjectDN());
82+
X509Certificate[] chain2 = keyManager.getCertificateChain(alias2);
83+
assertNotNull(chain2);
84+
assertEquals(ssc2.cert().getSubjectDN(), chain2[0].getSubjectDN());
8385

84-
PrivateKey pk2 = keyManager.getPrivateKey(alias2);
85-
assertNotNull(pk2);
86+
PrivateKey pk2 = keyManager.getPrivateKey(alias2);
87+
assertNotNull(pk2);
88+
} finally {
89+
ssc1.delete();
90+
ssc2.delete();
91+
}
8692
}
8793

8894
@Test
8995
public void testFileCheckThrottling() throws Exception {
9096
SelfSignedCertificate ssc1 = new SelfSignedCertificate("spanner.test.throttle1");
91-
File certFile = tempFolder.newFile("client-throttle.crt");
92-
File keyFile = tempFolder.newFile("client-throttle.key");
93-
94-
Files.write(certFile.toPath(), Files.readAllBytes(ssc1.certificate().toPath()));
95-
Files.write(keyFile.toPath(), Files.readAllBytes(ssc1.privateKey().toPath()));
96-
97-
// 60-second check interval
98-
DynamicKeyManager keyManager = new DynamicKeyManager(certFile, keyFile, 60000L);
99-
100-
String alias1 = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
101-
assertEquals(
102-
ssc1.cert().getSubjectDN(), keyManager.getCertificateChain(alias1)[0].getSubjectDN());
103-
104-
// Rotate files immediately on disk
10597
SelfSignedCertificate ssc2 = new SelfSignedCertificate("spanner.test.throttle2");
106-
Files.write(certFile.toPath(), Files.readAllBytes(ssc2.certificate().toPath()));
107-
Files.write(keyFile.toPath(), Files.readAllBytes(ssc2.privateKey().toPath()));
108-
109-
// Within the throttle interval, the manager should retain and return previous certificate
110-
assertEquals(
111-
ssc1.cert().getSubjectDN(), keyManager.getCertificateChain(alias1)[0].getSubjectDN());
98+
try {
99+
File certFile = tempFolder.newFile("client-throttle.crt");
100+
File keyFile = tempFolder.newFile("client-throttle.key");
101+
102+
Files.write(certFile.toPath(), Files.readAllBytes(ssc1.certificate().toPath()));
103+
Files.write(keyFile.toPath(), Files.readAllBytes(ssc1.privateKey().toPath()));
104+
105+
// 60-second check interval
106+
DynamicKeyManager keyManager = new DynamicKeyManager(certFile, keyFile, 60000L);
107+
108+
String alias1 = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
109+
assertEquals(
110+
ssc1.cert().getSubjectDN(), keyManager.getCertificateChain(alias1)[0].getSubjectDN());
111+
112+
// Rotate files immediately on disk
113+
Files.write(certFile.toPath(), Files.readAllBytes(ssc2.certificate().toPath()));
114+
Files.write(keyFile.toPath(), Files.readAllBytes(ssc2.privateKey().toPath()));
115+
116+
// Within the throttle interval, the manager should retain and return previous certificate
117+
assertEquals(
118+
ssc1.cert().getSubjectDN(), keyManager.getCertificateChain(alias1)[0].getSubjectDN());
119+
} finally {
120+
ssc1.delete();
121+
ssc2.delete();
122+
}
112123
}
113124

114125
@Test
115126
public void testCorruptRotationFallsBackToPrevious() throws Exception {
116127
SelfSignedCertificate ssc = new SelfSignedCertificate("spanner.test.fallback");
117-
File certFile = tempFolder.newFile("client-fallback.crt");
118-
File keyFile = tempFolder.newFile("client-fallback.key");
119-
120-
Files.write(certFile.toPath(), Files.readAllBytes(ssc.certificate().toPath()));
121-
Files.write(keyFile.toPath(), Files.readAllBytes(ssc.privateKey().toPath()));
122-
123-
DynamicKeyManager keyManager = new DynamicKeyManager(certFile, keyFile, 0L);
124-
String aliasBefore = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
125-
assertNotNull(aliasBefore);
126-
127-
Thread.sleep(1100);
128-
129-
// Overwrite certFile with corrupt bytes
130-
Files.write(certFile.toPath(), "NOT A CERTIFICATE CONTENT".getBytes(StandardCharsets.UTF_8));
128+
try {
129+
File certFile = tempFolder.newFile("client-fallback.crt");
130+
File keyFile = tempFolder.newFile("client-fallback.key");
131+
132+
Files.write(certFile.toPath(), Files.readAllBytes(ssc.certificate().toPath()));
133+
Files.write(keyFile.toPath(), Files.readAllBytes(ssc.privateKey().toPath()));
134+
135+
DynamicKeyManager keyManager = new DynamicKeyManager(certFile, keyFile, 0L);
136+
String aliasBefore = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
137+
assertNotNull(aliasBefore);
138+
139+
Thread.sleep(1100);
140+
141+
// Overwrite certFile with corrupt bytes
142+
Files.write(certFile.toPath(), "NOT A CERTIFICATE CONTENT".getBytes(StandardCharsets.UTF_8));
143+
144+
// DynamicKeyManager should catch reload error and retain previous material
145+
String aliasAfter = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
146+
assertEquals(aliasBefore, aliasAfter);
147+
assertNotNull(keyManager.getCertificateChain(aliasAfter));
148+
assertNotNull(keyManager.getPrivateKey(aliasAfter));
149+
} finally {
150+
ssc.delete();
151+
}
152+
}
131153

132-
// DynamicKeyManager should catch reload error and retain previous material
133-
String aliasAfter = keyManager.chooseClientAlias(new String[] {"RSA"}, null, null);
134-
assertEquals(aliasBefore, aliasAfter);
135-
assertNotNull(keyManager.getCertificateChain(aliasAfter));
136-
assertNotNull(keyManager.getPrivateKey(aliasAfter));
154+
@Test
155+
public void testPkcs1KeyThrowsIllegalArgumentException() throws Exception {
156+
SelfSignedCertificate ssc = new SelfSignedCertificate("spanner.test.pkcs1");
157+
try {
158+
File certFile = tempFolder.newFile("client-pkcs1.crt");
159+
File keyFile = tempFolder.newFile("client-pkcs1.key");
160+
161+
Files.write(certFile.toPath(), Files.readAllBytes(ssc.certificate().toPath()));
162+
Files.write(
163+
keyFile.toPath(),
164+
("-----BEGIN RSA PRIVATE KEY-----\n"
165+
+ "MIIEowIBAAKCAQEA0Y3...\n"
166+
+ "-----END RSA PRIVATE KEY-----\n")
167+
.getBytes(StandardCharsets.UTF_8));
168+
169+
IllegalArgumentException exception =
170+
assertThrows(
171+
IllegalArgumentException.class, () -> new DynamicKeyManager(certFile, keyFile));
172+
assertThat(exception.getMessage()).contains("PKCS#1 private keys are not supported");
173+
assertThat(exception.getMessage()).contains("openssl pkcs8");
174+
} finally {
175+
ssc.delete();
176+
}
137177
}
138178

139179
@Test
@@ -148,15 +188,19 @@ public void testNonExistentFileFailsInitialization() {
148188
@Test
149189
public void testServerAliasesReturnNull() throws Exception {
150190
SelfSignedCertificate ssc = new SelfSignedCertificate("spanner.test.server");
151-
File certFile = tempFolder.newFile("server-test.crt");
152-
File keyFile = tempFolder.newFile("server-test.key");
153-
154-
Files.write(certFile.toPath(), Files.readAllBytes(ssc.certificate().toPath()));
155-
Files.write(keyFile.toPath(), Files.readAllBytes(ssc.privateKey().toPath()));
156-
157-
DynamicKeyManager keyManager = new DynamicKeyManager(certFile, keyFile);
158-
assertNull(keyManager.getServerAliases("RSA", null));
159-
assertNull(keyManager.chooseServerAlias("RSA", null, null));
160-
assertNull(keyManager.chooseEngineServerAlias("RSA", null, null));
191+
try {
192+
File certFile = tempFolder.newFile("server-test.crt");
193+
File keyFile = tempFolder.newFile("server-test.key");
194+
195+
Files.write(certFile.toPath(), Files.readAllBytes(ssc.certificate().toPath()));
196+
Files.write(keyFile.toPath(), Files.readAllBytes(ssc.privateKey().toPath()));
197+
198+
DynamicKeyManager keyManager = new DynamicKeyManager(certFile, keyFile);
199+
assertNull(keyManager.getServerAliases("RSA", null));
200+
assertNull(keyManager.chooseServerAlias("RSA", null, null));
201+
assertNull(keyManager.chooseEngineServerAlias("RSA", null, null));
202+
} finally {
203+
ssc.delete();
204+
}
161205
}
162206
}

0 commit comments

Comments
 (0)