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] adds support for multiple ECDH curves #11016

Closed
wants to merge 1 commit into from

Conversation

aramix
Copy link

@aramix aramix commented Jun 6, 2018

Closes #9669

Closing this with a fix as there was no support for versions of NodeJS < v8.6.0. This is already fixed in the newer versions of NodeJS.

@geekgonecrazy
Copy link
Contributor

@aramix is this still valid?

@aramix
Copy link
Author

aramix commented Sep 27, 2018

@aramix is this still valid?

@geekgonecrazy yes if the node version on server is < v8.6.0

@dlundgren
Copy link
Contributor

The semver check should target < 10.0.0.

The DEFAULT_ECDH_CURVE is not 'auto' until that version, so this will catch the 8.11.4 that the snaps and docker images are currently using.

@geekgonecrazy
Copy link
Contributor

Unfortunately this PR is so old that this PR is no longer mergeable as is. I believe the file has been moved and likely renamed during a reorg pre 1.0

// https://github.com/nodejs/node/pull/16853
// This is fixed in Node 10, but this woraround also supports LTS versions
// https://github.com/nodejs/node/pull/15206
if (semver.gte(process.version, '8.6.0') && tls.DEFAULT_ECDH_CURVE === 'prime256v1') {
Copy link
Member

Choose a reason for hiding this comment

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

we don't need to test nodejs version, as we run only on 8.11.4 .. once we upgrade to a higher Node version, we can remove this code completely (from my understanding on code the comments)

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird Error: Valid Curve Name
5 participants