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

tls: fix getEphemeralKeyInfo to support X25519 #20273

Closed
wants to merge 2 commits into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Apr 25, 2018

EVP_PKEY_EC only covers ANSI X9.62 curves not IETF ones(curve25519 and curve448).
This fixes to add support of X25519 in tlsSocket.getEphemeralKeyInfo().
X448 should be added in the future upgrade to OpenSSL-1.1.1.

Fixes: #20262

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

CC @bnoordhuis

`EVP_PKEY_EC` only covers ANSI X9.62 curves not IETF ones(curve25519
and curve448). This fixes to add support of X25519 in
`tlsSocket.getEphemeralKeyInfo()`.
X448 should be added in the future upgrade to OpenSSL-1.1.1.
@shigeki shigeki added the tls Issues and PRs related to the tls subsystem. label Apr 25, 2018
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Apr 25, 2018
@shigeki
Copy link
Contributor Author

shigeki commented Apr 25, 2018

@Trott
Copy link
Member

Trott commented Apr 25, 2018

@nodejs/crypto

@shigeki
Copy link
Contributor Author

shigeki commented Apr 25, 2018

Only on AIX, something wrong in test-https-agent-additional-options.
Run CI again in https://ci.nodejs.org/job/node-test-pull-request/14485/.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

EC_KEY* ec = EVP_PKEY_get1_EC_KEY(key);
int nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec));
EC_KEY_free(ec);
const char *curve_name;
Copy link
Member

Choose a reason for hiding this comment

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

Style: star leans left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

info->Set(context, env->type_string(),
FIXED_ONE_BYTE_STRING(env->isolate(), "ECDH")).FromJust();
info->Set(context, env->name_string(),
OneByteString(args.GetIsolate(),
OBJ_nid2sn(nid))).FromJust();
curve_name)).FromJust();
Copy link
Member

Choose a reason for hiding this comment

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

Since you're here, can you insert a break; statement on line 2151? That's a subtle footgun waiting to go off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks.

@shigeki
Copy link
Contributor Author

shigeki commented Apr 25, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2018
@Trott
Copy link
Member

Trott commented Apr 25, 2018

node-test-commit-aix re-run: https://ci.nodejs.org/job/node-test-commit-aix/14495/

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 28, 2018
`EVP_PKEY_EC` only covers ANSI X9.62 curves not IETF ones(curve25519
and curve448). This fixes to add support of X25519 in
`tlsSocket.getEphemeralKeyInfo()`.
X448 should be added in the future upgrade to OpenSSL-1.1.1.

PR-URL: nodejs#20273
Fixes: nodejs#20262
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@BridgeAR
Copy link
Member

Landed in c51b7b2 🎉

@BridgeAR BridgeAR closed this Apr 28, 2018
MylesBorins pushed a commit that referenced this pull request May 4, 2018
`EVP_PKEY_EC` only covers ANSI X9.62 curves not IETF ones(curve25519
and curve448). This fixes to add support of X25519 in
`tlsSocket.getEphemeralKeyInfo()`.
X448 should be added in the future upgrade to OpenSSL-1.1.1.

PR-URL: #20273
Fixes: #20262
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tlsSocket.getEphemeralKeyInfo() returns empty object on PFS connection
7 participants