-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Use standard TLS hostname validation for instances with DNS names #954
Conversation
2656648
to
23b578d
Compare
23b578d
to
08c5e88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one small suggestion to add a comment, otherwise LGTM
if dnm.Name != "" && | ||
dnm.ConnectionType == "PRIVATE_SERVICE_CONNECT" && dnm.DnsScope == "INSTANCE" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a comment here to explain this logic, especially since DnsScope
may not be trivial
@hessjcg looks like you re-requested a review here but my two nits still have not been addressed... |
08c5e88
to
c05654f
Compare
2228dc9
to
94492b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
ab9dbd7
to
a97f945
Compare
internal/mock/cloudsql.go
Outdated
DNSName string | ||
DNSNames []*sqladmin.DnsNameMapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe a comment here explaining the two differences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case an external contributor is trying to update tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
7c953e0
to
3126f9a
Compare
…er certificate's server name.
3126f9a
to
c746d06
Compare
…es. (#2125) When the Cloud SQL Instance reports that it has a DNS Name, the connector will use standard TLS hostname validation when checking the server certificate. Now, the server's TLS certificate must contain a SAN record with the instance's DNS name. The ConnectSettings API added a field dns_names which contains all of the valid DNS names for an instance. See also GoogleCloudPlatform/cloud-sql-go-connector#954
…me of the instance. (#1242) The Cloud SQL Instance ConnectSettings added a new field `dns_names` which contains a list of valid DNS names for an instance. The Python Connector will use these DNS names, falling back to the old `dns_name` field if `dns_names` is not populated. Other connectors use this DNS name for hostname validation for the instance's TLS server certificate. However, the python connector does not perform hostname validation due to limitations of python's TLS library. See also: GoogleCloudPlatform/cloud-sql-go-connector#954
…es (#428) When the the Cloud SQL Instance reports that it has a DNS Name, the connector will use standard TLS hostname validation when checking the server certificate. Now, the server's TLS certificate must contain a SAN record with the instance's DNS name. The ConnectSettings API added a field dns_names which contains all of the valid DNS names for an instance. See also: GoogleCloudPlatform/cloud-sql-go-connector#954
When the the Cloud SQL Instance reports that it has a DNS Name, the connector will use standard TLS hostname validation when checking the server certificate. Now, the server's TLS certificate must contain a SAN record with the instance's DNS name.
The ConnectSettings API added a field
dns_names
which contains all of the valid DNS names foran instance.