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

Set SNI for TSL connections #1088

Merged
merged 1 commit into from Sep 6, 2022
Merged

Set SNI for TSL connections #1088

merged 1 commit into from Sep 6, 2022

Conversation

kelvich
Copy link
Contributor

@kelvich kelvich commented Aug 20, 2022

This allows an SNI-aware proxy to route connections. Patch adds a new
connection option (sslsni) for opting out of the SNI, to have the same
behavior as libpq does. See more in sslsni sections at
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PARAMKEYWORDS.

@kelvich
Copy link
Contributor Author

kelvich commented Aug 20, 2022

Relevant discussion: #488
libpq sets SNI since v14

Copy link
Contributor

@cbandy cbandy left a comment

Choose a reason for hiding this comment

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

lgtm

ssl_test.go Outdated Show resolved Hide resolved
ssl_test.go Outdated Show resolved Hide resolved
@kelvich
Copy link
Contributor Author

kelvich commented Aug 29, 2022

@rafiss, any chance you can take a look? This PR is hanging approved for some amount of time.

ssl.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

errors from linting:

Error: ssl_test.go:347:4: this value of err is never used (SA4006)
Error: ssl_test.go:348:4: this value of err is never used (SA4006)
Error: ssl_test.go:392:5: should use !bytes.Equal(startupMessage, []byte{0, 0, 0, 0x8, 0x4, 0xd2, 0x16, 0x2f}) instead (S1004)

ssl.go Outdated Show resolved Hide resolved
@kelvich kelvich force-pushed the sni_support branch 2 times, most recently from 9927d34 to 0c4600d Compare August 29, 2022 20:48
@kelvich
Copy link
Contributor Author

kelvich commented Aug 29, 2022

Thanks for the review, I've fixed mentioned issues

This allows an SNI-aware proxy to route connections. Patch adds a new
connection option (`sslsni`) for opting out of the SNI, to have the same
behavior as `libpq` does. See more in `sslsni` sections at
<https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PARAMKEYWORDS>.
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

thanks for your contribution!

@pschultz
Copy link

pschultz commented Mar 23, 2023

This seems to have broken connections with sslmode=verify-ca. This setting is supposed to ignore the DNS names in the server certificate, but now connections fail with "x509: certificate is valid for x, y, z, not a".

From what I can tell, to preserve backward compatibility tlsConf.ServerName should not be assigned if sslsni is absent and sslmode is anything other than verify-full.

@cbandy
Copy link
Contributor

cbandy commented Mar 24, 2023

@pschultz That may be #1106.

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.

None yet

6 participants