Support additional tls.connect() options #1996
Conversation
|
The build error seems unrelated. Please let me know if it is related and there is something for me to fix. |
|
@brianc any suggestions for how to get this merged? |
|
@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 |
|
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>
.
|
|
Tests need to be fixed (#1946), then this should probably have tests. How about copying all of the remaining properties from |
There was a problem hiding this comment.
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.
lib/connection.js
Outdated
| secureOptions: self.ssl.secureOptions, | ||
| ALPNProtocols: self.ssl.ALPNProtocols, |
There was a problem hiding this comment.
what if we just did
tls.connect(Object.assign({
// all the non-ssl options here
}, self.ssl))There was a problem hiding this comment.
Works for me! I'll push an update shortly
# Conflicts: # lib/connection.js
|
@brianc should be all set. Let me know if there are any changes needed |
|
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>
.
|
This already broke our server, I think there will be more -_- because you only need to init pg with |
|
@teleranek Broke it how? #2009? If so: that it worked was a bug. You can get the previous behaviour back with |
|
This broke our server(s) as well, I just forced the version to 7.12. |
|
Sorry @charmander have no time to investigate this, but it just suffice to pass only |
|
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. |
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. |
…" (brianc#2010)" This reverts commit 510a273.
…" (brianc#2010)" This reverts commit 510a273.
* Drop support for EOL versions of node (#2062) * Drop support for EOL versions of node * Re-add testing for [email protected] * Revert changes to .travis.yml * Update packages/pg-pool/package.json Co-Authored-By: Charmander <[email protected]> Co-authored-by: Charmander <[email protected]> * Remove password from stringified outputs (#2066) * Remove password from stringified outputs Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password. To widen the pit of success I'm making that field non-enumerable. You can still get at it...it just wont show up "by accident" when you're logging things now. The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0. * Implement feedback * Fix more whitespace the autoformatter changed * Simplify code a bit * Remove password from stringified outputs (#2070) * Keep ConnectionParameters’s password property writable `Client` writes to it when `password` is a function. * Avoid creating password property on pool options when it didn’t exist previously. * Allow password option to be non-enumerable to avoid breaking uses like `new Pool(existingPool.options)`. * Make password property definitions consistent in formatting and configurability. Co-authored-by: Charmander <[email protected]> * Make `native` non-enumerable (#2065) * Make `native` non-enumerable Making it non-enumerable means less spurious "Cannot find module" errors in your logs when iterating over `pg` objects. `Object.defineProperty` has been available since Node 0.12. See #1894 (comment) * Add test for `native` enumeration Co-authored-by: Gabe Gorelick <[email protected]> * Use class-extends to wrap Pool (#1541) * Use class-extends to wrap Pool * Minimize diff * Test `BoundPool` inheritance Co-authored-by: Charmander <[email protected]> Co-authored-by: Brian C <[email protected]> * Continue support for creating a pg.Pool from another instance’s options (#2076) * Add failing test for creating a `BoundPool` from another instance’s settings * Continue support for creating a pg.Pool from another instance’s options by dropping the requirement for the `password` property to be enumerable. * Use user name as default database when user is non-default (#1679) Not entirely backwards-compatible. * Make native client password property consistent with others i.e. configurable. * Make notice messages not an instance of Error (#2090) * Make notice messages not an instance of Error Slight API cleanup to make a notice instance the same shape as it was, but not be an instance of error. This is a backwards incompatible change though I expect the impact to be minimal. Closes #1982 * skip notice test in travis * Pin [email protected] for regression in async iterators * Check and see if node 13.8 is still borked on async iterator * Yeah, node still has changed edge case behavior on stream * Emit notice messages on travis * Revert "Revert "Support additional tls.connect() options (#1996)" (#2010)" (#2113) This reverts commit 510a273. * Fix ssl tests (#2116) * Convert Query to an ES6 class (#2126) The last missing `new` deprecation warning for pg 8. Co-authored-by: Charmander <[email protected]> Co-authored-by: Gabe Gorelick <[email protected]> Co-authored-by: Natalie Wolfe <[email protected]>
No description provided.