Skip to content

Commit 7d1a95c

Browse files
committed
feat: Update TLS validation to use both SAN and CN fields.
1 parent 1042972 commit 7d1a95c

File tree

5 files changed

+317
-24
lines changed

5 files changed

+317
-24
lines changed

core/src/main/java/com/google/cloud/sql/core/InstanceCheckingTrustManagerFactory.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
*/
5353
class InstanceCheckingTrustManagerFactory extends TrustManagerFactory {
5454

55-
static TrustManagerFactory newInstance(InstanceMetadata instanceMetadata)
55+
static InstanceCheckingTrustManagerFactory newInstance(InstanceMetadata instanceMetadata)
5656
throws NoSuchAlgorithmException, KeyStoreException, CertificateException, IOException {
5757

5858
TrustManagerFactory delegate = TrustManagerFactory.getInstance("X.509");

core/src/main/java/com/google/cloud/sql/core/InstanceCheckingTrustManger.java

Lines changed: 49 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,33 @@
3030
import javax.net.ssl.X509ExtendedTrustManager;
3131

3232
/**
33-
* This is a workaround for a known bug in Conscrypt crypto in how it handles X509 auth type.
34-
* OpenJDK interpres the X509 certificate auth type as "UNKNOWN" while Conscrypt interpret the same
35-
* certificate as auth type "GENERIC". This incompatibility causes problems in the JDK.
33+
* InstanceCheckingTrustManger implements custom TLS verification logic to gracefully and securely
34+
* handle deviations from standard TLS hostname verification in existing Cloud SQL instance server
35+
* certificates.
3636
*
37-
* <p>This adapter works around the issue by creating wrappers around all TrustManager instances. It
38-
* replaces "GENERIC" auth type with "UNKNOWN" auth type before delegating calls.
37+
* <p>This is the verification algorithm:
3938
*
40-
* <p>See https://github.com/google/conscrypt/issues/1033#issuecomment-982701272
39+
* <ol>
40+
* <li>Verify the server cert CA, using the CA certs from the instance metadata. Reject the
41+
* certificate if the CA is invalid.
42+
* <li>Check that the server cert contains a SubjectAlternativeName matching the DNS name in the
43+
* connector configuration OR the DNS Name from the instance metadata
44+
* <li>If the SubjectAlternativeName does not match, and if the server cert Subject.CN field is
45+
* not empty, check that the Subject.CN field contains the instance name. Reject the
46+
* certificate if both the #2 SAN check and #3 CN checks fail.
47+
* </ol>
48+
*
49+
* <p>To summarize the deviations from standard TLS hostname verification:
50+
*
51+
* <p>Historically, Cloud SQL creates server certificates with the instance name in the Subject.CN
52+
* field in the format "my-project:my-instance". The connector is expected to check that the
53+
* instance name that the connector was configured to dial matches the server certificate Subject.CN
54+
* field. Thus, the Subject.CN field for most Cloud SQL instances does not contain a well-formed DNS
55+
* Name. This breaks standard TLS hostname verification.
56+
*
57+
* <p>Also, there are times when the instance metadata reports that an instance has a DNS name, but
58+
* that DNS name does not yet appear in the SAN records of the server certificate. The client should
59+
* fall back to validating the hostname using the instance name in the Subject.CN field.
4160
*/
4261
class InstanceCheckingTrustManger extends X509ExtendedTrustManager {
4362
private final X509ExtendedTrustManager tm;
@@ -96,27 +115,31 @@ private void checkCertificateChain(X509Certificate[] chain) throws CertificateEx
96115
throw new CertificateException("Subject is missing");
97116
}
98117

99-
// If the instance metadata does not contain a domain name, use legacy CN validation
100-
if (Strings.isNullOrEmpty(instanceMetadata.getDnsName())) {
101-
checkCn(chain);
102-
} else {
103-
// If there is a DNS name, check the Subject Alternative Names.
104-
checkSan(chain);
105-
}
106-
}
107-
108-
private void checkSan(X509Certificate[] chain) throws CertificateException {
109118
final String dns;
110119
if (!Strings.isNullOrEmpty(instanceMetadata.getInstanceName().getDomainName())) {
111120
// If the connector is configured using a DNS name, validate the DNS name from the connector
112121
// config.
113122
dns = instanceMetadata.getInstanceName().getDomainName();
114-
} else {
123+
} else if (!Strings.isNullOrEmpty(instanceMetadata.getDnsName())) {
115124
// If the connector is configured with an instance name, validate the DNS name from
116125
// the instance metadata.
117126
dns = instanceMetadata.getDnsName();
127+
} else {
128+
dns = null;
118129
}
119130

131+
// If the instance metadata does not contain a domain name, and the connector was not
132+
// configured with a domain name, use legacy CN validation.
133+
if (dns == null) {
134+
checkCn(chain);
135+
} else {
136+
// If there is a DNS name, check the Subject Alternative Names.
137+
checkSan(dns, chain);
138+
}
139+
}
140+
141+
private void checkSan(String dns, X509Certificate[] chain) throws CertificateException {
142+
120143
if (Strings.isNullOrEmpty(dns)) {
121144
throw new CertificateException(
122145
"Instance metadata for " + instanceMetadata.getInstanceName() + " has an empty dnsName");
@@ -128,11 +151,15 @@ private void checkSan(X509Certificate[] chain) throws CertificateException {
128151
return;
129152
}
130153
}
131-
throw new CertificateException(
132-
"Server certificate does not contain expected name '"
133-
+ dns
134-
+ "' for Cloud SQL instance "
135-
+ instanceMetadata.getInstanceName());
154+
try {
155+
checkCn(chain);
156+
} catch (CertificateException e) {
157+
throw new CertificateException(
158+
"Server certificate does not contain expected name '"
159+
+ dns
160+
+ "' for Cloud SQL instance "
161+
+ instanceMetadata.getInstanceName());
162+
}
136163
}
137164

138165
private List<String> getSans(X509Certificate cert) throws CertificateException {

core/src/test/java/com/google/cloud/sql/core/ConnectorTest.java

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ public void create_throwsErrorForDomainNameDoesntMatchServerCert() throws Except
452452
config.getConnectorConfig(),
453453
port,
454454
"db.example.com",
455-
"myProject:myRegion:myInstance",
455+
"myProject:myRegion:otherInstance",
456456
true);
457457

458458
SSLHandshakeException ex =
@@ -461,6 +461,28 @@ public void create_throwsErrorForDomainNameDoesntMatchServerCert() throws Except
461461
assertThat(ex).hasMessageThat().contains("Server certificate does not contain expected name");
462462
}
463463

464+
@Test
465+
public void create_fallbackToInstanceWhenDomainNameDoesntMatchServerCert() throws Exception {
466+
FakeSslServer sslServer = new FakeSslServer();
467+
ConnectionConfig config =
468+
new ConnectionConfig.Builder()
469+
.withDomainName("not-in-san.example.com")
470+
.withIpTypes("PRIMARY")
471+
.build();
472+
473+
int port = sslServer.start(PUBLIC_IP);
474+
475+
Connector c =
476+
newConnector(
477+
config.getConnectorConfig(),
478+
port,
479+
"db.example.com",
480+
"myProject:myRegion:myInstance",
481+
true);
482+
483+
c.connect(config, TEST_MAX_REFRESH_MS);
484+
}
485+
464486
@Test
465487
public void create_successfulPublicCasConnection() throws IOException, InterruptedException {
466488
PrivateKey privateKey = TestKeys.getServerKeyPair().getPrivate();
Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
/*
2+
* Copyright 2025 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.cloud.sql.core;
18+
19+
import static org.junit.Assert.assertThrows;
20+
21+
import java.security.cert.Certificate;
22+
import java.security.cert.CertificateException;
23+
import java.security.cert.X509Certificate;
24+
import java.util.ArrayList;
25+
import java.util.Arrays;
26+
import java.util.Collections;
27+
import java.util.List;
28+
import org.junit.BeforeClass;
29+
import org.junit.Test;
30+
import org.junit.runner.RunWith;
31+
import org.junit.runners.Parameterized;
32+
import org.junit.runners.Parameterized.Parameters;
33+
34+
@RunWith(Parameterized.class)
35+
public class InstanceCheckingTrustManagerFactoryTest {
36+
37+
private static TestCertificateGenerator generator;
38+
private final TestCase tc;
39+
40+
@BeforeClass
41+
public static void beforeClass() {
42+
generator = new TestCertificateGenerator();
43+
}
44+
45+
public InstanceCheckingTrustManagerFactoryTest(TestCase tc) {
46+
this.tc = tc;
47+
}
48+
49+
@Test
50+
public void testValidateCertificate() throws Exception {
51+
52+
List<Certificate> caCerts;
53+
X509Certificate[] serverCert;
54+
if (tc.cas) {
55+
caCerts = Arrays.asList(generator.getCasServerCertificateChain());
56+
caCerts = caCerts.subList(1, caCerts.size());
57+
serverCert = generator.createServerCertificate(tc.cn, tc.san, true);
58+
} else {
59+
caCerts = Collections.singletonList(generator.getServerCaCert());
60+
serverCert = generator.createServerCertificate(tc.cn, tc.san, false);
61+
}
62+
63+
InstanceMetadata instanceMetadata =
64+
new InstanceMetadata(
65+
new CloudSqlInstanceName(tc.icn, tc.serverName),
66+
Collections.emptyMap(),
67+
caCerts,
68+
false,
69+
null,
70+
false);
71+
72+
InstanceCheckingTrustManagerFactory f =
73+
InstanceCheckingTrustManagerFactory.newInstance(instanceMetadata);
74+
InstanceCheckingTrustManger tm = (InstanceCheckingTrustManger) f.getTrustManagers()[0];
75+
76+
if (tc.valid) {
77+
tm.checkServerTrusted(serverCert, "UNKNOWN");
78+
} else {
79+
assertThrows(
80+
CertificateException.class,
81+
() -> {
82+
tm.checkServerTrusted(serverCert, "UNKNOWN");
83+
});
84+
}
85+
}
86+
87+
@Parameters(name = "{index}: {0}")
88+
public static List<TestCase> testCases() {
89+
List<TestCase> cases =
90+
Arrays.asList(
91+
new TestCase(
92+
"cn match",
93+
null,
94+
"myProject:myRegion:myInstance",
95+
"myProject:myInstance",
96+
null,
97+
true),
98+
new TestCase(
99+
"cn no match",
100+
null,
101+
"myProject:myRegion:badInstance",
102+
"myProject:myInstance",
103+
null,
104+
false),
105+
new TestCase(
106+
"cn empty", null, "myProject:myRegion:myInstance", "db.example.com", null, false),
107+
new TestCase(
108+
"san match",
109+
"db.example.com",
110+
"myProject:myRegion:myInstance",
111+
null,
112+
"db.example.com",
113+
true),
114+
new TestCase(
115+
"san no match",
116+
"bad.example.com",
117+
"myProject:myRegion:myInstance",
118+
null,
119+
"db.example.com",
120+
false),
121+
new TestCase(
122+
"san empty match",
123+
"empty.example.com",
124+
"myProject:myRegion:myInstance",
125+
"",
126+
null,
127+
false),
128+
new TestCase(
129+
"san match with cn present",
130+
"db.example.com",
131+
"myProject:myRegion:myInstance",
132+
"myProject:myInstance",
133+
"db.example.com",
134+
true),
135+
new TestCase(
136+
"san no match fallback to cn",
137+
"db.example.com",
138+
"myProject:myRegion:myInstance",
139+
"myProject:myInstance",
140+
"other.example.com",
141+
true),
142+
new TestCase(
143+
"san empty match fallback to cn",
144+
"db.example.com",
145+
"myProject:myRegion:myInstance",
146+
"myProject:myInstance",
147+
null,
148+
true),
149+
new TestCase(
150+
"san no match fallback to cn and fail",
151+
"db.example.com",
152+
"myProject:myRegion:badInstance",
153+
"other.example.com",
154+
"myProject:myInstance",
155+
false));
156+
List<TestCase> casesWithCas = new ArrayList<>(cases);
157+
for (TestCase tc : cases) {
158+
casesWithCas.add(tc.withCas(true));
159+
}
160+
return casesWithCas;
161+
}
162+
163+
private static class TestCase {
164+
/** Testcase description. */
165+
private final String desc;
166+
/** connector configuration domain name. */
167+
private final String serverName;
168+
/** connector configuration instance name. */
169+
private final String icn;
170+
/** server cert CN field value. */
171+
private final String cn;
172+
/** server cert SAN field value. */
173+
private final String san;
174+
/** wants validation to succeed. */
175+
private final boolean valid;
176+
177+
private final boolean cas;
178+
179+
public TestCase(
180+
String desc, String serverName, String icn, String cn, String san, boolean valid) {
181+
this(desc, serverName, icn, cn, san, valid, false);
182+
}
183+
184+
public TestCase(
185+
String desc,
186+
String serverName,
187+
String icn,
188+
String cn,
189+
String san,
190+
boolean valid,
191+
boolean cas) {
192+
this.desc = desc;
193+
this.serverName = serverName;
194+
this.icn = icn;
195+
this.cn = cn;
196+
this.san = san;
197+
this.valid = valid;
198+
this.cas = cas;
199+
}
200+
201+
@Override
202+
public String toString() {
203+
return desc;
204+
}
205+
206+
private TestCase withCas(boolean cas) {
207+
return new TestCase(this.desc, this.serverName, this.icn, this.cn, this.san, this.valid, cas);
208+
}
209+
}
210+
}

0 commit comments

Comments
 (0)