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(Server): set tls.DEFAULT_ECDH_CURVE to 'auto' #1531

Merged
merged 2 commits into from Oct 23, 2018

Conversation

nekolab
Copy link
Contributor

@nekolab nekolab commented Oct 17, 2018

  • This is a bugfix
  • This is a code refactor
  • This is a test update
  • This is a typo fix
  • This is a metadata update

The default value of tls.DEFAULT_ECDH_CURVE is 'prime256v1',
it breaks the connection when certificate is not compatible
with the default curve since node^8.6.0.

To fix this issue, we need set it to 'auto', makes OpenSSL
select the curve automatically.

@jsf-clabot
Copy link

jsf-clabot commented Oct 17, 2018

CLA assistant check
All committers have signed the CLA.

@alexander-akait
Copy link
Member

alexander-akait commented Oct 17, 2018

@nekolab on node@10 all works fine, right?

@codecov
Copy link

codecov bot commented Oct 18, 2018

Codecov Report

Merging #1531 into master will increase coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1531      +/-   ##
==========================================
+ Coverage   74.02%   74.02%   +<.01%     
==========================================
  Files          10       10              
  Lines         666      670       +4     
==========================================
+ Hits          493      496       +3     
- Misses        173      174       +1
Impacted Files Coverage Δ
lib/Server.js 81.36% <80%> (-0.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e719959...78f5452. Read the comment docs.

@nekolab
Copy link
Contributor Author

nekolab commented Oct 18, 2018

@evilebottnawi
Yes, node@10 default tls. DEFAULT_ECDH_CURVE to auto
Seems this change breaks node@6, so I've added some version check

@alexander-akait
Copy link
Member

alexander-akait commented Oct 18, 2018

@nekolab thanks!

@michael-ciniawsky michael-ciniawsky changed the title fix(Server): Set tls.DEFAULT_ECDH_CURVE to 'auto' fix(Server): set tls.DEFAULT_ECDH_CURVE to 'auto' Oct 18, 2018
lib/Server.js Outdated
// breaking connection when certificate is not signed with prime256v1
// change it to auto allows OpenSSL to select the curve automatically
// See https://github.com/nodejs/node/issues/16196 for more infomation
const verMatch = process.version.match(/^v(\d+).(\d+)/);
Copy link
Member

@michael-ciniawsky michael-ciniawsky Oct 18, 2018

Choose a reason for hiding this comment

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

Is the version check required here or could this be defaulted to 'auto' in general? E.g was this being 'reverted' for node >= 10.0.0 for some reason? If so, why?

Suggested change
const verMatch = process.version.match(/^v(\d+).(\d+)/);
const version = process.version.match(/^v(\d+).(\d+)/);

Copy link
Member

@michael-ciniawsky michael-ciniawsky Oct 18, 2018

Choose a reason for hiding this comment

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

node v9.0.0 is EOL

Copy link
Member

@michael-ciniawsky michael-ciniawsky Oct 18, 2018

Choose a reason for hiding this comment

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

Just saw #1531 (comment) Hmm....

Copy link
Member

@alexander-akait alexander-akait Oct 18, 2018

Choose a reason for hiding this comment

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

@michael-ciniawsky node@8 also affected

Copy link
Member

@michael-ciniawsky michael-ciniawsky Oct 18, 2018

Choose a reason for hiding this comment

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

const version = parseFloat(process.version.slice(1))

if (10 > version && version >= 8.6) {
  ...
}

Copy link
Member

@alexander-akait alexander-akait Oct 18, 2018

Choose a reason for hiding this comment

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

@michael-ciniawsky node@10 is not affected

Copy link
Contributor Author

@nekolab nekolab Oct 19, 2018

Choose a reason for hiding this comment

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

Thanks for guiding, I've update the code and please take a look again.

There is another place using regex to match the version, I've changed it too

The default value of tls.DEFAULT_ECDH_CURVE is 'prime256v1',
it breaks the connection when certificate is not compatible
with the default curve since node 8.6.0.

To fix this issue, we need set it to 'auto', makes OpenSSL
select the curve automatically.
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

@nekolab Thx

This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants