feat: dynamic TLS configuration support#4191
feat: dynamic TLS configuration support#4191jeengbe wants to merge 3 commits intosidorares:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4191 +/- ##
==========================================
+ Coverage 90.64% 90.66% +0.01%
==========================================
Files 86 86
Lines 14245 14267 +22
Branches 1798 1804 +6
==========================================
+ Hits 12913 12935 +22
Misses 1332 1332
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks, @jeengbe! Does this PR resolve some issue? To fix the website's lint/format, you can run |
|
Thanks, @wellwelwel, fixed! The issue it resovles it that you currently have to recreate your connection (pool) when client credentials change. Now, the same pool instance can continue to be used and new connections can simply fetch the latest available certificate. The docs should maybe mention this (feel free to change stuff yourself too) that this will most commonly be used to fetch If you mean GitHub issue, then no 🙂 The change is small enough I figured I'd just give it a go |
|
Could you please help me with getting this out if it looks alright? |
I'm stopping for today, but I'll definitely take a closer look tomorrow 🙋🏻♂️ |
|
Hi @wellwelwel, please excuse the personal mention, but could you give this another look? Thank you for your time and maintaining this package 🙇🏻♂️ |
|
Hi @jeengbe sorry for the delay! I'll add my review comments, overall looks good, lets try to release this |
|
|
||
| const sslConfig = | ||
| typeof this.config.ssl === 'function' | ||
| ? this.config.ssl(this.config) |
There was a problem hiding this comment.
what happens if this throws synchronously? The caller does not catches error, and unlike async error we'll have connection in a broken state
| typeof this.config.ssl === 'function' | ||
| ? this.config.ssl(this.config) | ||
| : this.config.ssl; | ||
|
|
There was a problem hiding this comment.
worth adding extra guard
if (typeof sslConfig !== 'object' || sslConfig === null) {
onSecure(new TypeError('SSL factory must return an object'));
return;
}so that if user supplied factory returns garbage its much easier to debug ( the typings do cover that, but won't hurt imo )
3 participants
Changes the
ssloption to accept a function that returns TLS options or a promise thereof. This is useful for ever more popular environments that run short-lived certificates that might expire during the lifetime of an application. The docs mention now SPIFFE for some SEO juice for people searching "node spiffe" 🙂The only current alternative I can see is the
streamoption, but that's undocumented and would require manually sending the SSLRequest packet during the handshake. And it doesn't appear to support asynchronous creation.All contributions except for the test spec written by humans.