feat: Support CAS server certificate authority to validate instance connections. #2060

hessjcg · 2024-08-26T17:59:41Z

For Cloud SQL instances with CAS enabled, the connector will use the certificate chain of trust reported by the
SQL Admin API to validate the instance connection.

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

jackwotherspoon

I believe we want to update the PSC check now that dnsName can be for CAS-based instances as well as PSC just like Go did: https://github.com/GoogleCloudPlatform/cloud-sql-go-connector/pull/850/files#diff-5097a430f11cfd2c79c874367e2f96cf048d9e47112b01f7650d961d8a677ff8R104

Not sure where exactly in the Java code base this would be, maybe here?

cloud-sql-jdbc-socket-factory/core/src/main/java/com/google/cloud/sql/core/DefaultConnectionInfoRepository.java

Line 282 in e13fa46

ipAddrs.put(IpType.PSC, instanceMetadata.getDnsName());

jackwotherspoon · 2024-09-09T21:05:46Z

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

+  InstanceMetadata(
+      Map<IpType, String> ipAddrs,
+      List<Certificate> instanceCaCertificates,
+      boolean casManagedCertificate,
+      String dnsName) {
+    this.ipAddrs = ipAddrs;
+    this.instanceCaCertificates = instanceCaCertificates;
+    this.casManagedCertificate = casManagedCertificate;
+    this.dnsName = dnsName;


do we really need two constructors for InstanceMetadata ? seems like we should be able to just use one...

Fixed. I consolidated this to 1 constructor.

hessjcg · 2024-09-10T20:28:57Z

Java has this requirement covered without needing additional code:

I believe we want to update the PSC check now that dnsName can be for CAS-based instances as well as PSC just like Go did: https://github.com/GoogleCloudPlatform/cloud-sql-go-connector/pull/850/files#diff-5097a430f11cfd2c79c874367e2f96cf048d9e47112b01f7650d961d8a677ff8R104

The Java connector calls SSLSocket.connect() at Connector.java:118 with either the instance IP or the PSC domain name. When the instance is a CAS instance, the TLS context will be configured to use the built-in logic to check certificate trust. When SSLSocket.connect() is called with the PSC domain name, the built-in TLS logic will check the certificate's SAN records as expected.

feng-zhe · 2024-09-09T22:35:18Z

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

+      return delegate;
+    }
+
+    // Otherwise, Use a custom trust manager factory that checks the CN against the instance name
    InstanceCheckingTrustManagerFactory tmf =
        new InstanceCheckingTrustManagerFactory(instanceName, delegate);


Looks like the dnsName is not actually used to verify the server identity? Can we enhance this part and pass in the dnsName to verify it? I think we can use getSubjectAlternativeNames() to get the SAN fields. Thanks.

We should not need to explicitly program the TrustManagerFactory to check the subjectAlternateName records, that behavior should be built in. I will add some unit tests with correct and incorrect server certificates to demonstrate this behavior.

+1 we need to actually check the dnsName against the SAN fields

Fixed. I also moved some of the code from this into 2 other PRs for clarity.

chore(refactor): Add CAS related fields to the InstanceMetadata. #2067

(this one) feat: Support CAS server certificate authority to validate instance connections

test: Add integration test for public IP instance with CAS enabled. #2066

feng-zhe · 2024-09-13T20:12:52Z

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

+    }
+    throw new CertificateException(
+        "Server certificate does not contain expected name '"
+            + instanceMetadata.getInstanceName()


Use instanceMetadata.getDnsName() instead.

feng-zhe · 2024-09-13T20:16:28Z

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

+
+    for (List item : sanAsn1Field) {
+      Integer type = (Integer) item.get(0);
+      if (type == 0 || type == 2) {


Can we add a comment here for the 0 and 2? I assume that 0 is the otherName and 2 is the dNSName according to the RFC 5280.

You are correct. I added a comment to clarify.

jackwotherspoon · 2024-09-16T15:06:27Z

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

+      SSLParameters sslParams = new SSLParameters();
+      socket.setSSLParameters(sslParams);


can you add a comment in the code here explaining this new piece?

Removed. This was unnecessary.

jackwotherspoon · 2024-09-16T15:08:02Z

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

@@ -367,7 +374,7 @@ private Certificate fetchEphemeralCertificate(
    // Parse the certificate from the response.
    Certificate ephemeralCertificate;
    try {
-      ephemeralCertificate = createCertificate(response.getEphemeralCert().getCert());
+      ephemeralCertificate = createCertificate(response.getEphemeralCert().getCert()).get(0);


Let's add a comment in the code about which cert .get(0) is gettiing...

jackwotherspoon · 2024-09-16T15:09:11Z

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

+      // If this is CAS-managed, then use the HTTPS algorithm to check the subjectAlternateNames
+      // against the certificate.


should we not also be doing this for PSC? It also has the DNS name as a SAN entry...

Actually, this is wrong. I'm removing it. It won't work because unlike Go we cannot override the hostname passed to Socket.connect().

I built custom hostname verification logic in InstanceCheckingTrustManager.

The built-in HTTPS hostname verification algorithm won't work because the connector cannot override the hostname when Socket.connect(addr) is called with an IP Address.

jackwotherspoon · 2024-09-16T15:10:05Z

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

- * checks that the Subject CN field contains the Cloud SQL instance ID.
+ * does custom hostname checking in accordance with these rules:
+ *
+ * <p>If an instanceMetadata.casEnabled Check that the subjectAlternativeNames in the server


nit: this sentence seems off to me, reads a bit funny...

Good catch. I fixed it.

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

jackwotherspoon

LGTM ✅

hessjcg requested a review from a team as a code owner August 26, 2024 17:59

blunderbuss-gcf bot assigned jackwotherspoon Aug 26, 2024

hessjcg force-pushed the cas-support branch from a3bc64b to 1ab1ce5 Compare August 26, 2024 21:01

hessjcg requested review from jackwotherspoon and feng-zhe August 26, 2024 21:01

hessjcg marked this pull request as draft August 26, 2024 21:11

hessjcg marked this pull request as ready for review September 3, 2024 20:00

jackwotherspoon reviewed Sep 9, 2024

View reviewed changes

core/src/main/java/com/google/cloud/sql/core/DefaultConnectionInfoRepository.java Outdated Show resolved Hide resolved

jackwotherspoon requested changes Sep 9, 2024

View reviewed changes

hessjcg force-pushed the cas-support branch 2 times, most recently from 408d64e to 668aa58 Compare September 10, 2024 20:19

hessjcg requested a review from jackwotherspoon September 10, 2024 20:29

hessjcg force-pushed the cas-support branch from 668aa58 to 8de81ea Compare September 10, 2024 20:38

feng-zhe reviewed Sep 10, 2024

View reviewed changes

hessjcg force-pushed the cas-support branch 2 times, most recently from 0ef54c0 to 689b27f Compare September 13, 2024 19:20

hessjcg changed the base branch from main to cas-support-refactor September 13, 2024 19:24

hessjcg requested a review from feng-zhe September 13, 2024 19:25

hessjcg force-pushed the cas-support branch 2 times, most recently from 26e21b7 to fe76396 Compare September 13, 2024 19:37

hessjcg changed the title ~~feat: Support CAS server certificate authority to validate instance connections~~ feat: Support CAS server certificate authority to validate instance connections. Sep 13, 2024

feng-zhe approved these changes Sep 13, 2024

View reviewed changes

jackwotherspoon requested changes Sep 16, 2024

View reviewed changes

hessjcg force-pushed the cas-support branch from fe76396 to 55d227f Compare September 16, 2024 15:48

hessjcg force-pushed the cas-support-refactor branch from a9815d8 to 12de4d4 Compare September 16, 2024 15:59

hessjcg force-pushed the cas-support branch 2 times, most recently from 647e946 to f2fefc0 Compare September 16, 2024 16:10

hessjcg force-pushed the cas-support-refactor branch 2 times, most recently from 87a8a3f to 096f8e7 Compare September 16, 2024 16:11

hessjcg requested a review from a team as a code owner September 16, 2024 16:11

hessjcg requested a review from jackwotherspoon September 16, 2024 19:07

hessjcg force-pushed the cas-support-refactor branch from 096f8e7 to 4ee3acc Compare September 17, 2024 17:54

jackwotherspoon reviewed Sep 17, 2024

View reviewed changes

core/src/main/java/com/google/cloud/sql/core/InstanceCheckingTrustManger.java Show resolved Hide resolved

jackwotherspoon approved these changes Sep 17, 2024

View reviewed changes

Base automatically changed from cas-support-refactor to main September 18, 2024 19:34

feat: Support for CAS server certificate authority.

15e9584

hessjcg force-pushed the cas-support branch from f2fefc0 to 15e9584 Compare September 18, 2024 19:35

hessjcg merged commit 4332ffc into main Sep 18, 2024
17 checks passed

hessjcg deleted the cas-support branch September 18, 2024 19:50

release-please bot mentioned this pull request Sep 20, 2024

chore(main): release 1.21.0 #2075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support CAS server certificate authority to validate instance connections. #2060

feat: Support CAS server certificate authority to validate instance connections. #2060

hessjcg commented Aug 26, 2024

jackwotherspoon left a comment

jackwotherspoon Sep 9, 2024

hessjcg Sep 10, 2024

hessjcg commented Sep 10, 2024

feng-zhe Sep 9, 2024

hessjcg Sep 10, 2024

jackwotherspoon Sep 11, 2024

hessjcg Sep 13, 2024 •

edited

Loading

feng-zhe Sep 13, 2024

hessjcg Sep 16, 2024

feng-zhe Sep 13, 2024

hessjcg Sep 16, 2024

jackwotherspoon Sep 16, 2024

hessjcg Sep 16, 2024

jackwotherspoon Sep 16, 2024

hessjcg Sep 16, 2024

jackwotherspoon Sep 16, 2024

hessjcg Sep 16, 2024

jackwotherspoon Sep 16, 2024

hessjcg Sep 16, 2024

jackwotherspoon left a comment

		SSLParameters sslParams = new SSLParameters();
		socket.setSSLParameters(sslParams);

		// If this is CAS-managed, then use the HTTPS algorithm to check the subjectAlternateNames
		// against the certificate.

feat: Support CAS server certificate authority to validate instance connections. #2060

feat: Support CAS server certificate authority to validate instance connections. #2060

Conversation

hessjcg commented Aug 26, 2024

jackwotherspoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hessjcg commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hessjcg Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackwotherspoon left a comment

Choose a reason for hiding this comment

hessjcg Sep 13, 2024 •

edited

Loading