Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: have ssl handshake check hostname #283

Closed
wants to merge 14 commits into from
Closed

fix: have ssl handshake check hostname #283

wants to merge 14 commits into from

Conversation

jackwotherspoon
Copy link
Collaborator

@jackwotherspoon jackwotherspoon commented Mar 22, 2024 •
edited by nancynh
Loading

For Cloud SQL we need to disable checking hostname of ssl handshake because IP address does not match SAN. However, for AlloyDB this is not the case and the IP address can be validated as the host name.

Fixes #324

@jackwotherspoon jackwotherspoon self-assigned this Mar 22, 2024
@jackwotherspoon jackwotherspoon requested review from cthumuluru, nancynh and a team as code owners March 22, 2024 21:05
@jackwotherspoon jackwotherspoon changed the title WIP: check hostname fix: check hostname Mar 28, 2024
@jackwotherspoon jackwotherspoon changed the title fix: check hostname fix: have ssl handshake check hostname Mar 28, 2024
@enocom enocom added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 2, 2024
@enocom
Copy link
Member

enocom commented May 2, 2024

We need to hold off on this until PSC DNS is populated in all existing certificates.

@enocom enocom removed request for cthumuluru and nancynh May 17, 2024 17:55
@enocom enocom removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 17, 2024
nancynh
nancynh approved these changes Jun 24, 2024
View reviewed changes
Copy link
Collaborator

@nancynh nancynh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this is also good to merge in now that the PSC DNS is in all existing certificates

@enocom enocom mentioned this pull request Jun 24, 2024
Closed
enocom
enocom reviewed Jun 25, 2024
View reviewed changes
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks perfectly correct, but obviously is missing something. Thanks for all the work on this @jackwotherspoon .

@enocom
Copy link
Member

enocom commented Jun 25, 2024

Via @jackwotherspoon

"HostName" contains the fully qualified DNS hostname of the server,
as understood by the client. The hostname is represented as a byte
string using ASCII encoding without a trailing dot.

https://datatracker.ietf.org/doc/html/rfc6066#section-3

@jackwotherspoon
Copy link
Collaborator Author

TLDR;

For wrap_socket and passing it to SSLContext, the trailing dot is invalid. However, the hostname match requires a perfect match to the DNS entry in the SAN which has the trailing dot. Thus, here we are, stuck with the hostname match failing.

@enocom
Copy link
Member

enocom commented Jul 19, 2024

Going to close this for now. We'll need to revisit some server settings I think.

@enocom enocom closed this Jul 19, 2024
@enocom enocom deleted the jackwotherspoon-patch-1 branch July 19, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enocom enocom enocom left review comments

@nancynh nancynh nancynh approved these changes

Assignees

@jackwotherspoon jackwotherspoon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verify PSC DNS name is present as a SAN in the server certificate
3 participants
@jackwotherspoon @enocom @nancynh