feat: Update TLS validation to use both SAN and CN fields. #979
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
hessjcg
force-pushed
the
feat-new-tls-logic
branch
2 times, most recently
from
3b282f0 to
07ca9f7
Compare
hessjcg
force-pushed
the
feat-new-tls-logic
branch
from
07ca9f7 to
c132292
Compare
Collaborator
Author
|
/gcbrun |
jackwotherspoon
requested changes
Apr 22, 2025
Contributor
jackwotherspoon
left a comment
jackwotherspoon
left a comment
There was a problem hiding this comment.
Can we please make the description of this PR more clear on what this is solving, I thought we went forward with our previous PR for TLS validation because no more customers should require this CN validation when they have DNS names, now it seems like we are falling back to CN validation even with a DNS name present?
internal/cloudsql/tls_verify.go
Outdated
Comment on lines
27
to
31
| // verifyPeerCertificateFunc creates a VerifyPeerCertificate func that | ||
| // verifies that the peer certificate is in the cert pool. We need to define | ||
| // our own because CloudSQL instances use the instance name (e.g., | ||
| // my-project:my-instance) instead of a valid domain name for the certificate's | ||
| // Common Name. |
Contributor
There was a problem hiding this comment.
these comments are out of date...
hessjcg
force-pushed
the
feat-new-tls-logic
branch
5 times, most recently
from
d7f512a to
6b1443a
Compare
enocom
reviewed
Apr 23, 2025
internal/cloudsql/instance.go
Outdated
| Certificates: []tls.Certificate{c.ClientCertificate}, | ||
| RootCAs: pool, | ||
| MinVersion: tls.VersionTLS13, | ||
| // We need to set InsecureSkipVerify to true due to |
Member
There was a problem hiding this comment.
I think we should update this comment.
Historically, Cloud SQL server certificates had my-instance:my-project in the common name, which broke TLS validation and required InsecureSkipVerify being set to true. There are still instances in the Cloud SQL fleet with those certificates. So this comment is still correct for those instances. It's worth explaining why we need it for all instances now (plus any future work to improve the situation).
Collaborator
Author
There was a problem hiding this comment.
I've updated the comment here and on the verifyPeerCertificateFunc() in tls_verify.go to provide a better explanation.
hessjcg
force-pushed
the
feat-new-tls-logic
branch
2 times, most recently
from
eddfae8 to
3db5f73
Compare
hessjcg
force-pushed
the
feat-new-tls-logic
branch
from
3db5f73 to
3713046
Compare
jackwotherspoon
approved these changes
Apr 24, 2025
Contributor
jackwotherspoon
left a comment
jackwotherspoon
left a comment
There was a problem hiding this comment.
LGTM ✅ Very nice comments and description 👏
hessjcg
added a commit
to GoogleCloudPlatform/cloud-sql-jdbc-socket-factory
that referenced
this pull request
Apr 28, 2025
This updates the logic used by the connector to validate server certificates. When connecting to the instance, the connector's TLS validator will first check the SAN field, and then if that fails check the CN field in the certificate for the instance name. This will enable the connector to work smoothly with both legacy and newer instances. To summarize the deviations from standard TLS hostname verification: Historically, Cloud SQL creates server certificates with the instance name in the Subject.CN field in the format "my-project:my-instance". The connector is expected to check that the instance name that the connector was configured to dial matches the server certificate Subject.CN field. Thus, the Subject.CN field for most Cloud SQL instances does not contain a well-formed DNS Name. This breaks standard TLS hostname verification. Also, there are times when the instance metadata reports that an instance has a DNS name, but that DNS name does not yet appear in the SAN records of the server certificate. The client should fall back to validating the hostname using the instance name in the Subject.CN field. See also: GoogleCloudPlatform/cloud-sql-go-connector#979
hessjcg
added a commit
to GoogleCloudPlatform/cloud-sql-nodejs-connector
that referenced
this pull request
Apr 28, 2025
This updates the logic used by the connector to validate server certificates. When connecting to the instance, the connector's TLS validator will first check the SAN field, and then if that fails check the CN field in the certificate for the instance name. This will enable the connector to work smoothly with both legacy and newer instances. To summarize the deviations from standard TLS hostname verification: Historically, Cloud SQL creates server certificates with the instance name in the Subject.CN field in the format "my-project:my-instance". The connector is expected to check that the instance name that the connector was configured to dial matches the server certificate Subject.CN field. Thus, the Subject.CN field for most Cloud SQL instances does not contain a well-formed DNS Name. This breaks standard TLS hostname verification. Also, there are times when the instance metadata reports that an instance has a DNS name, but that DNS name does not yet appear in the SAN records of the server certificate. The client should fall back to validating the hostname using the instance name in the Subject.CN field. See also: GoogleCloudPlatform/cloud-sql-go-connector#979
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the logic used by the connector to validate server certificates.
When connecting to the instance, the connector's TLS validator will first check the SAN field,
and then if that fails check the CN field in the certificate for the instance name. This will enable
the connector to work smoothly with both legacy and newer instances.
In the Go TLS library, the only way to replace the TLS hostname verification check is to
set
TLSConfig.InsecureSkipVerify = true, and then replace the entire TLS verificationfunction by setting
TLSConfig.VerifyPeerCertificate.To summarize the deviations from standard TLS hostname verification:
Historically, Cloud SQL creates server certificates with the instance name in the Subject.CN field in
the format "my-project:my-instance". The connector is expected to check that the instance name
that the connector was configured to dial matches the server certificate Subject.CN field. Thus,
the Subject.CN field for most Cloud SQL instances does not contain a well-formed DNS Name. This
breaks standard TLS hostname verification.
Also, there are times when the instance metadata reports that an instance has a DNS name, but
that DNS name does not yet appear in the SAN records of the server certificate. The client should
fall back to use the instance name in the Subject.CN field to validate the server certificate's identity.