ssl: only set "sslrootcert" once #573

tamird · 2017-03-06T17:35:46Z

Fixes #570, though I have not been able to write a test for this.

Remove methods on values. These methods are not a useful abstraction
and only serve to obscure that the underlying structure is a map. Their
presence is particularly inconvenient in that they promote confusing
empty values with the lack of value.

This change is

cbandy · 2017-03-06T23:18:11Z

I can't tell if it's appropriate to treat o.Get(...) != "" the same as _, ok := o[...]; ok.

cbandy · 2017-03-06T23:16:30Z

ssl.go

+ errorf("couldn't parse pem in sslrootcert")
+ }
+ } else {
+ delete(o, "sslrootcert")


iiuc, this has the same problem described in #570. We shouldn't be writing to o.

I don't think so, because it's now guarded by the presence of a non-empty value, which should only happen once at Dial and not during cancellation.

I agree that this is safe and fixes the bug there because when this function is re-invoked during cancel, sslrootcert will have been deleted, and so this branch will not be entered.

cbandy · 2017-03-06T23:17:48Z

ssl.go

 case "disable":
 return nil
 default:
 errorf(`unsupported sslmode %q; only "require" (default), "verify-full", "verify-ca", and "disable" supported`, mode)
 }

 sslClientCertificates(&tlsConf, o)
- sslCertificateAuthority(&tlsConf, o)


This function (or its contents) still need to be invoked when sslmode is verify-ca or verify-full.

See failing tests TestSSLVerifyCA and TestSSLVerifyFull.

tamird · 2017-03-06T23:23:17Z

All good points, thanks for your review. I'll update it shortly.

…

On Mar 6, 2017 6:20 PM, "Chris Bandy" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ssl.go <#573 (comment)>: > case "disable": return nil default: errorf(`unsupported sslmode %q; only "require" (default), "verify-full", "verify-ca", and "disable" supported`, mode) } sslClientCertificates(&tlsConf, o) - sslCertificateAuthority(&tlsConf, o) See failing tests TestSSLVerifyCA and TestSSLVerifyFull. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPHcUIxAZ1Syz8oTqNghxzLpAIwf6ks5rjJTWgaJpZM4MUdXd> .

tamird · 2017-03-07T03:26:34Z

@cbandy addressed your concrete comments; anything else you'd like to see done before merging?

cbandy

The logic/fix for #570 looks sound.

However, I'm convinced we should not change the conditionals that test for empty string. I found (unless I'm mistaken) that the SSL options in this PR are affected negatively, and I suspect other options are affected similarly.

cbandy · 2017-03-07T04:16:48Z

ssl.go

- verifyCaOnly = true
- } else {
- o.Set("sslrootcert", "")
+ // From http://www.postgresql.org/docs/current/static/libpq- ssl.html:


Spaces have appeared after some of the dashes in this paragraph. On this line, it broke this link.

cbandy · 2017-03-07T05:06:37Z

ssl.go


 var cinfo, kinfo os.FileInfo
 var err error

- if sslcert != "" && sslkey != "" {
+ if sslkeyOK && sslcertOK {


As I understand it, this change breaks compatibility with libpq connection strings.

If I'm reading libpq source correctly, these settings fallback to the user/home paths when the supplied value is blank:

if (conn->sslcert && strlen(conn->sslcert) > 0) ... else ... "%s/%s", homedir, USER_CERT_FILE

if (conn->sslkey && strlen(conn->sslkey) > 0) ... else ... "%s/%s", homedir, USER_KEY_FILE

The old code is also incompatible with that libpq logic in that libpq allows either the certificate or the key to be blank, while this only does the right thing if both are blank.

I'm inclined to "break" compatibility in this way until someone reports that they need it, at which time we should add tests and implement the actual libpq behaviour.

Fixed this, I think, though I think tests are going to break and I've not yet explicitly tested the empty string case.

cbandy · 2017-03-07T05:16:19Z

ssl.go

@@ -105,16 109,15 @@ func sslClientCertificates(tlsConf *tls.Config, o values) {

 // sslCertificateAuthority adds the RootCA specified in the "sslrootcert" setting.
 func sslCertificateAuthority(tlsConf *tls.Config, o values) {
- if sslrootcert := o.Get("sslrootcert"); sslrootcert != "" {
+ if sslrootcert, ok := o["sslrootcert"]; ok {


As I understand it, this change breaks compatibility with libpq connection strings.

If I'm reading libpq source correctly, the CA certificate is only loaded when this setting is not blank:

if (conn->sslrootcert && strlen(conn->sslrootcert) > 0)

I think you're right, but as mentioned above, I think this is OK until we get a report of someone relying on this behaviour. WDYT?

Fixed and added a test.

cbandy · 2017-03-07T05:35:09Z

If compatibility with libpq connection strings is a goal, then maybe we should also support the home directory fallback for sslrootcert, ~/.postgresql/root.crt.

tamird

I do think compatibility with libpq connection strings is a goal of this library, but not of this particular PR.

tamird · 2017-03-07T10:47:33Z

ssl.go

- verifyCaOnly = true
- } else {
- o.Set("sslrootcert", "")
+ // From http://www.postgresql.org/docs/current/static/libpq- ssl.html:


tamird · 2017-03-07T10:47:59Z

ssl.go


 var cinfo, kinfo os.FileInfo
 var err error

- if sslcert != "" && sslkey != "" {
+ if sslkeyOK && sslcertOK {


The old code is also incompatible with that libpq logic in that libpq allows either the certificate or the key to be blank, while this only does the right thing if both are blank.

I'm inclined to "break" compatibility in this way until someone reports that they need it, at which time we should add tests and implement the actual libpq behaviour.

tamird · 2017-03-07T10:48:00Z

ssl.go

@@ -105,16 109,15 @@ func sslClientCertificates(tlsConf *tls.Config, o values) {

 // sslCertificateAuthority adds the RootCA specified in the "sslrootcert" setting.
 func sslCertificateAuthority(tlsConf *tls.Config, o values) {
- if sslrootcert := o.Get("sslrootcert"); sslrootcert != "" {
+ if sslrootcert, ok := o["sslrootcert"]; ok {


I think you're right, but as mentioned above, I think this is OK until we get a report of someone relying on this behaviour. WDYT?

cbandy · 2017-03-07T15:51:18Z

I do think compatibility with libpq connection strings is a goal of this library, but not of this particular PR.

I didn't mean to imply that we should improve compatibility in this PR. I mean that we should not regress compatibility.

I'm inclined to "break" compatibility in this way until someone reports that they need it, at which time we should add tests and implement the actual libpq behaviour.

I'm not sure what you're trying to imply with the quotes here. I think I sufficiently (?) demonstrated that the behavior of SSL options should be based on blank values rather than the presence of keys in the map.

I think this is OK until we get a report of someone relying on this behaviour. WDYT?

🤔 I'm still not convinced... Maybe you see some other benefits to these values changes that I do not. Why these changes?

tamird · 2017-03-07T17:04:22Z

Well, in general, values definitely promotes bad practices. That said, fixing the regressions you pointed out and adding tests.

tamird · 2017-03-08T18:11:06Z

@cbandy OK, I think this is good to go with new tests exercising the treatment of empty sslcert and sslkey options.

Fixes #570, though I have not been able to write a test for this. Remove methods on `values`. These methods are not a useful abstraction and only serve to obscure that the underlying structure is a map. Their presence is particularly inconvenient in that they promote confusing empty values with the lack of value. While I'm here, also improve conformance with libpq's treatment of blank sslcert and sslkey option values. See the test changes for specific changes.

maddyblue · 2017-03-08T19:38:28Z

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

ssl_test.go, line 44 at r1 (raw file):

func checkSSLSetup(t *testing.T, conninfo string) {
	_, err := openSSLConn(t, conninfo)

Why don't we need to close the db here?

Comments from Reviewable

tamird · 2017-03-08T19:52:35Z

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

ssl_test.go, line 44 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Why don't we need to close the db here?

I mean, we do, but it doesn't really matter since a nil error will result in a test failure, and the leaked database handle won't turn up anywhere.

Comments from Reviewable

maddyblue · 2017-03-08T19:54:33Z

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

ssl_test.go, line 44 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I mean, we do, but it doesn't really matter since a nil error will result in a test failure, and the leaked database handle won't turn up anywhere.

Will it remain open on the server until a timeout? When running go test -count 100 -race trying to find race conditions I've had the pg server complain about too many connections, so I want to make sure we're closing things correctly.

Comments from Reviewable

tamird · 2017-03-08T19:55:20Z

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

ssl_test.go, line 44 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Will it remain open on the server until a timeout? When running go test -count 100 -race trying to find race conditions I've had the pg server complain about too many connections, so I want to make sure we're closing things correctly.

There will never be a non-nil connection here in a passing test run.

Comments from Reviewable

maddyblue · 2017-03-08T20:02:58Z

Well then LGTM, again. I agree that there is (was?) a problem since empty strings had their meaning changed in this PR. It seems like this code handles those in at least a well documented way, and if there are missing combinations we can add those as test cases.

tamird · 2017-03-08T20:32:40Z

Thanks, @mjibson. I'll wait for final approval from @cbandy or else merge this tomorrow.

cbandy

🎈

tamird requested a review from maddyblue March 6, 2017 17:35

maddyblue approved these changes Mar 6, 2017

View reviewed changes

tamird changed the title ~~Remove methods on values~~ ssl: only set "sslrootcert" once Mar 6, 2017

maddyblue approved these changes Mar 6, 2017

View reviewed changes

cbandy suggested changes Mar 6, 2017

View reviewed changes

cbandy suggested changes Mar 7, 2017

View reviewed changes

tamird commented Mar 7, 2017

View reviewed changes

cbandy approved these changes Mar 9, 2017

View reviewed changes

tamird merged commit a6140c9 into lib:master Mar 9, 2017

tamird deleted the remove-values-methods branch March 9, 2017 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ssl: only set "sslrootcert" once #573

ssl: only set "sslrootcert" once #573

tamird commented Mar 6, 2017 •

edited

Loading

cbandy commented Mar 6, 2017

cbandy Mar 6, 2017

tamird Mar 7, 2017

maddyblue Mar 7, 2017

cbandy Mar 6, 2017

cbandy Mar 6, 2017

tamird Mar 7, 2017

tamird commented Mar 6, 2017 via email

tamird commented Mar 7, 2017

cbandy left a comment

cbandy Mar 7, 2017

tamird Mar 7, 2017

cbandy Mar 7, 2017

tamird Mar 7, 2017

tamird Mar 7, 2017

cbandy Mar 7, 2017

tamird Mar 7, 2017

tamird Mar 7, 2017

cbandy commented Mar 7, 2017

tamird left a comment

tamird Mar 7, 2017

tamird Mar 7, 2017

tamird Mar 7, 2017

cbandy commented Mar 7, 2017

tamird commented Mar 7, 2017

tamird commented Mar 8, 2017

maddyblue commented Mar 8, 2017

tamird commented Mar 8, 2017

maddyblue commented Mar 8, 2017

tamird commented Mar 8, 2017

maddyblue commented Mar 8, 2017

tamird commented Mar 8, 2017

cbandy left a comment

ssl: only set "sslrootcert" once #573

ssl: only set "sslrootcert" once #573

Conversation

tamird commented Mar 6, 2017 • edited Loading

cbandy commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamird commented Mar 6, 2017 via email

tamird commented Mar 7, 2017

cbandy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbandy commented Mar 7, 2017

tamird left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbandy commented Mar 7, 2017

tamird commented Mar 7, 2017

tamird commented Mar 8, 2017

maddyblue commented Mar 8, 2017

tamird commented Mar 8, 2017

maddyblue commented Mar 8, 2017

tamird commented Mar 8, 2017

maddyblue commented Mar 8, 2017

tamird commented Mar 8, 2017

cbandy left a comment

Choose a reason for hiding this comment

tamird commented Mar 6, 2017 •

edited

Loading