Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up#2301 Unable to set minimum TLS version #2304
Conversation
|
@dougwilson Do you know when we'd be able to get this merged? |
|
Apologies, I will review today. |
| - `ciphers`: Cipher suite specification, replacing the default. | ||
| - `key`: Private keys in PEM format. | ||
| - `passphrase`: Shared passphrase used for a single private key and/or a PFX. | ||
| - `minVersion`: Optionally set the minimum TLS version to allow. One of `'TLSv1.3'`, `'TLSv1.2'`, `'TLSv1.1'`, or `'TLSv1'`. |
dougwilson
Mar 3, 2020
Member
nit: we probably want to order these alphabetically (a "nit" to me is something I will change on landing, but if you want to change it earlier, even better).
nit: we probably want to order these alphabetically (a "nit" to me is something I will change on landing, but if you want to change it earlier, even better).
| passphrase : config.ssl.passphrase | ||
| passphrase : config.ssl.passphrase, | ||
| minVersion : config.ssl.minVersion, | ||
| maxVersion : config.ssl.maxVersion |
dougwilson
Mar 3, 2020
Member
nit: we probably want to order these alphabetically
nit: we probably want to order these alphabetically
|
Awesome job! I just saw a couple things in tests I noted here. Please note that nothing here to me is at a level where I won't do anything until you do something; I will liekly work on the changes in here this weekend if you don't get to them first. But if you can get to them earlier, it's that much easier & faster for me to get landed |
| var NODE_MAJOR_VERSION = process.versions.node.split('.')[0]; | ||
| if (err) throw err; | ||
| connection.ping(function(err) { | ||
| if (!err && NODE_MAJOR_VERSION >= 10) assert.fail('Expected to fail due to mismatched TLS versions for Node12+'); |
dougwilson
Mar 3, 2020
Member
I would guess that getting past this point (expecting it to fail) the test should assert that it is failing for the reason we expect it to. Found quite a few bugs a while back when I added those kinds of tests in this lib.
I would guess that getting past this point (expecting it to fail) the test should assert that it is failing for the reason we expect it to. Found quite a few bugs a while back when I added those kinds of tests in this lib.
|
|
||
| connection.ping(function(err) { | ||
| var NODE_MAJOR_VERSION = process.versions.node.split('.')[0]; | ||
| if (!err && NODE_MAJOR_VERSION >= 10) assert.fail('Expected to fail due to mismatched TLS versions for Node12+'); |
dougwilson
Mar 3, 2020
Member
I would guess that getting past this point (expecting it to fail) the test should assert that it is failing for the reason we expect it to. Found quite a few bugs a while back when I added those kinds of tests in this lib.
I would guess that getting past this point (expecting it to fail) the test should assert that it is failing for the reason we expect it to. Found quite a few bugs a while back when I added those kinds of tests in this lib.
| @@ -138,12 +138,16 @@ common.getTestConfig = function(config) { | |||
| return mergeTestConfig(config); | |||
| }; | |||
|
|
|||
| common.getSSLConfig = function() { | |||
| common.getSSLConfig = function(options) { | |||
dougwilson
Mar 3, 2020
Member
Rather than passing in the options here, the common functions in here expect the caller to use common.extend to augment after the fact.
Rather than passing in the options here, the common functions in here expect the caller to use common.extend to augment after the fact.
alexburley
Mar 4, 2020
Author
How do you mean?
How do you mean?
dougwilson
Mar 16, 2020
•
Member
common.extend(common.getSSLConfig(), { minVersion: ... })
common.extend(common.getSSLConfig(), { minVersion: ... })
|
@dougwilson Thx for review |
This reverts commit d82d3cc.
|
@dougwilson Any luck? Sorry to keep badgering, I just want to make sure I can update our service and get it running on Node12 before I forget. |
| connection.ping(function(err) { | ||
| var NODE_MAJOR_VERSION = parseInt(process.versions.node.split('.')[0], 10); | ||
| if (NODE_MAJOR_VERSION >= 10) { | ||
| if (!err) assert.fail('Expected to fail due to mismatched TLS versions for Node12+'); |
dougwilson
Mar 17, 2020
Member
I think the error message should be Node10+ (or the check should be fixed to match the error message).
I think the error message should be Node10+ (or the check should be fixed to match the error message).
| connection.ping(function(err) { | ||
| if (NODE_MAJOR_VERSION >= 10) { | ||
| if (!err) { | ||
| assert.fail('Expected to fail due to mismatched TLS versions for Node12+'); |
dougwilson
Mar 17, 2020
Member
I think the error message should be Node10+ (or the check should be fixed to match the error message).
I think the error message should be Node10+ (or the check should be fixed to match the error message).
|
@dougwilson Any update on when this can be merged? |
|
Hello, |
|
Sorry this fell off my radar. Thanks for the ping I will push it forward tonight |
|
Just found this -- excited to see the work done already! It has come up as an issue with our mysql integration in Beekeeper Studio. Think it's the cause of beekeeper-studio/beekeeper-studio#254 @dougwilson thanks so much for helping to get this merged. @alexburley thanks for doing the work! |
| - `cert`: Cert chains in PEM format. | ||
| - `ciphers`: Cipher suite specification, replacing the default. | ||
| - `key`: Private keys in PEM format. | ||
| - `maxVersion`: Optionally set the minimum TLS version to allow. |
rathboma
Jul 23, 2020
should this say Optionally set the MAXIMUM TLS version to allow?
should this say Optionally set the MAXIMUM TLS version to allow?
|
@dougwilson Is there anything I can do to help get this PR merged? |
#2301
Since Node 9 and before don't support min/max version these won't be set and the tests won't fail for those versions.
Node default TLS version is set to 1.2 for tests as Node 10 has not enabled support for 1.3 by default.
Tested against mysql 5.5 and mysql 8 servers