Support additional tls.connect() options

[jgeurts]

No description provided.

[jgeurts]

The build error seems unrelated. Please let me know if it is related and there is something for me to fix.

[jgeurts]

@brianc any suggestions for how to get this merged?

[jgeurts]

@brianc Can you please help me with what needs to be done to get this merged? This allows for more secure connections to the db server

[brianc]

I will take a look tomorrow and let you know

…

On Sun, Nov 10, 2019 at 12:11 PM Jim Geurts ***@***.***> wrote: @brianc <https://github.com/brianc> Can yo please help me with what needs to be done to get this merged? This allows for more secure connections to the db server — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1996?email_source=notifications&email_token=AAAMHII23QTZSW5KN7J73JTQTAQCPA5CNFSM4JDT3RYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDU7III#issuecomment-552203297>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIIJAKUYVVSUWHDWHRDQTAQCPANCNFSM4JDT3RYA> .

[charmander]

Tests need to be fixed (#1946), then this should probably have tests.

How about copying all of the remaining properties from self.ssl instead of listing each one, for automatic compatibility with most future additions?

[brianc]

I merged the test fix so the current tests should pass. I agree w/ @charmander we should just splat in the ssl options into the object now as there are so many ssl options and they might change in the future....there's no real need to enumerate them all by hand here other than than when this code was initially written there were only a handful.

Also would be good to see a test (at least a unit test that checks the options are set on the socket) so we can be sure this works in the future.

[brianc]

what if we just did

tls.connect(Object.assign({
  // all the non-ssl options here
}, self.ssl))

[jgeurts]

Works for me! I'll push an update shortly

[jgeurts]

@brianc should be all set. Let me know if there are any changes needed

[charmander]

This does change the behaviour for the existing properties being read from self.ssl when they aren’t own/enumerable, but it probably won’t affect anyone.

[brianc]

Awesome! Currently waiting to take off. Will likely be destroyed tired tomorrow but i have some time on Wednesday booked for this and doing some more work as well. Thanks again!

…

On Mon, Nov 11, 2019 at 9:29 PM Charmander ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1996?email_source=notifications&email_token=AAAMHIIJH4DPQHFE3CEYYK3QTH2H5A5CNFSM4JDT3RYKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLFBINI#pullrequestreview-315233333>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIOOU3PUL2ISBPKHLQLQTH2H5ANCNFSM4JDT3RYA> .

[brianc]

awesome! I'll release a new minor version with this today. Got a bit destroyed by the travel so went afk for a few days.

[teleranek]

This does change the behaviour for the existing properties being read from self.ssl when they aren’t own/enumerable, but it probably won’t affect anyone.

This already broke our server, I think there will be more -_- because you only need to init pg with connectionString for this to fail.

[charmander]

@teleranek Broke it how? #2009? If so: that it worked was a bug. You can get the previous behaviour back with rejectUnauthorized: false, but it’s best to actually make it validate the certificate.

[sh-developers]

This broke our server(s) as well, I just forced the version to 7.12.
Not a great update to do in a minor version update, would have preferred breaking change like this to be done in Major versions like semver recommends.

[teleranek]

Sorry @charmander have no time to investigate this, but it just suffice to pass only connectionString to proper ssl server with legit certificate and leave all options default. Most common way to connect. No matter if that's a bug or not, what matters that the default behaviour was changed in non-major version, not cool.

[brianc]

Was an unintended mistake to change default behavior in a minor version. I'm going to fix this within the hour. Thanks for the patience.

[charmander]

Sorry @charmander have no time to investigate this, but it just suffice to pass only connectionString to proper ssl server with legit certificate and leave all options default.

I doubt that it actually has a legit certificate, and that you think it does is why this is a dangerous bug. But yes, it was introduced a long time ago, so I guess a major version is helpful to people.