Skip to content

tls: fix getEphemeralKeyInfo to support X25519 - #20273

Closed
shigeki wants to merge 2 commits into
nodejs:masterfrom
shigeki:fix_ephemeralkeyinfo
Closed

tls: fix getEphemeralKeyInfo to support X25519#20273
shigeki wants to merge 2 commits into
nodejs:masterfrom
shigeki:fix_ephemeralkeyinfo

Conversation

@shigeki

Copy link
Copy Markdown
Contributor

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.
@shigekishigeki added the tls Issues and PRs related to the tls subsystem. label Apr 25, 2018
@nodejs-github-botnodejs-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
Copy Markdown
ContributorAuthor

@Trott

Copy link
Copy Markdown
Member

@nodejs/crypto

@shigeki

Copy link
Copy Markdown
ContributorAuthor

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/.

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks.

Comment threadsrc/node_crypto.cc Outdated
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed.

Comment threadsrc/node_crypto.cc
info->Set(context, env->name_string(),
OneByteString(args.GetIsolate(),
OBJ_nid2sn(nid))).FromJust();
curve_name)).FromJust();

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

@TrottTrott 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
Copy Markdown
Member

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#20273Fixes: nodejs#20262
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@BridgeAR

Copy link
Copy Markdown
Member

Landed in c51b7b2 🎉

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: #20273Fixes: #20262
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorinsMylesBorins mentioned this pull request May 8, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs 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++.cryptoIssues and PRs related to the crypto subsystem.tlsIssues 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

@shigeki@Trott@BridgeAR@bnoordhuis@danbev@tniessen@nodejs-github-bot