Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: set maximum supported TLS version to 1.2 #1471

Merged
merged 3 commits into from Jul 26, 2022

Conversation

mShan0
Copy link
Collaborator

@mShan0 mShan0 commented Jul 20, 2022

Modifies tests to pass on Node 18.6.0+

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #1471 (bce54cb) into master (7ed1661) will increase coverage by 0.05%.
The diff coverage is 85.71%.

Current head bce54cb differs from pull request most recent head a6a5a11. Consider uploading reports for the commit a6a5a11 to get more accurate results

@@            Coverage Diff             @@
##           master    #1471      +/-   ##
==========================================
+ Coverage   80.45%   80.50%   +0.05%     
==========================================
  Files          91       91              
  Lines        4667     4669       +2     
  Branches      855      856       +1     
==========================================
+ Hits         3755     3759       +4     
  Misses        634      634              
+ Partials      278      276       -2     
Impacted Files Coverage Δ
src/connection.ts 65.14% <75.00%> (-0.04%) ⬇️
src/message-io.ts 91.42% <100.00%> (+0.38%) ⬆️
src/token/handler.ts 55.62% <0.00%> (+1.18%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@arthurschreiber arthurschreiber changed the title test: set max tls version to 1.2 fix: set maximum supported TLS version to 1.2 Jul 20, 2022
@@ -289,6 +289,7 @@ describe('Initiate Connect Test', function() {

// Specify a cipher that should never be supported by SQL Server
config.options.cryptoCredentialsDetails = {
maxVersion: 'TLSv1.2',
Copy link
Collaborator

@arthurschreiber arthurschreiber Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move this out of the tests somewhere else. This is going to fix the test failures, but I think we should make sure that we only ever use TLSv1.2 and never TLSv1.3 inside tedious when TDS < 8.0 is used - and only support TLSv1.3 once we add TDS 8.0 support later.

Copy link
Collaborator

@arthurschreiber arthurschreiber Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might make sense to specify the max tls version when we create the tls socket here:

cleartext: tls.connect({
socket: duplexpair.socket1 as Socket,
servername: hostname,
secureContext: secureContext,
rejectUnauthorized: !trustServerCertificate
}),

To do that, we also need to pass move the creation of the secure context object from

tedious/src/connection.ts

Lines 1700 to 1701 in 85a2b89

this.secureContext = createSecureContext(credentialsDetails);
over into the startTls method.

This will also require checking if the user has specified a maxVersion in the secure context options, and overwriting it unless it's set to TLSv1.2, TLSv1.1, or TLSv1.

Copy link
Collaborator Author

@mShan0 mShan0 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just committed some changes. Was this closer to what you had in mind?

@arthurschreiber arthurschreiber merged commit 5015634 into master Jul 26, 2022
13 checks passed
@github-actions
Copy link

github-actions bot commented Jul 26, 2022

🎉 This PR is included in version 15.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mShan0 mShan0 deleted the fix-tls-tests-node18 branch Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants