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.

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

enable SNI #953

Merged
merged 1 commit into from
Apr 2, 2024
Merged

enable SNI #953

merged 1 commit into from
Apr 2, 2024

Conversation

conradludgate
Copy link
Contributor

Adding the ServerName config allows TLS to include the ServerNameIdentification (SNI) extension. We use this at Neon to determine which database endpoint to connect to: https://neon.tech/docs/connect/connection-errors#the-endpoint-id-is-not-specified

I have tested that this works for sslmode=require, but I need to still confirm that this doesn't break the insecure modes from being insecure

@eu-erwin
Copy link

Hi @conradludgate, I currently face the same problem on neon too.
however it works with sslmode=verify-full. does this have some impact? I am not sure, what the different is.
but, at lease it works for now.

@conradludgate
Copy link
Contributor Author

sslmode=require does not verify that the certificate is valid, it only ensures that TLS is used. I would recommend verify-full of you can (neon supports this perfectly)

@@ -283,6 +287,10 @@ func parseDSN(dsn string) ([]Option, error) {
// (verify chain, but skip server name).
// See https://github.com/golang/go/issues/21971 .
Copy link
Member

Choose a reason for hiding this comment

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

Please update the comment and explain why this is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment is still correct. InsecureSkipVerify = true will not verify either the certificate chain nor verify the server name. The ServerName field only adds the server name to the TLS ClientHello but doesn't impact the verification if insecure is set.

Copy link
Member

@vmihailenco vmihailenco Apr 2, 2024

Choose a reason for hiding this comment

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

Good to know, thanks.

@vmihailenco vmihailenco merged commit 4071ffb into uptrace:master Apr 2, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants