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.

Sign up for GitHub

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

Jump to bottom

tls: expose SSL_export_keying_material #31814

Closed
wants to merge 1 commit into from
Closed

tls: expose SSL_export_keying_material #31814

wants to merge 1 commit into from

Conversation

simllll
Copy link
Contributor

@simllll simllll commented Feb 15, 2020

closes #31802

I added one test, that just checks if the function is available and returns the requested amount of data.
Please double check if I have done the memory allocation right.

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]

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 15, 2020
@simllll simllll mentioned this pull request Feb 15, 2020
Closed
@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch from 886fb45 to e46860b Compare February 15, 2020 18:37
@simllll simllll changed the title tls-socket: expose SSL_export_keying_material tls: expose SSL_export_keying_material Feb 15, 2020
@simllll simllll requested a review from sam-github February 15, 2020 18:38
@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch from e46860b to e44ec54 Compare February 15, 2020 18:40
addaleax
addaleax reviewed Feb 15, 2020
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 15, 2020
@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch from e44ec54 to 367dcc0 Compare February 15, 2020 19:42
@addaleax
Copy link
Member

@nodejs/crypto

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29160/

bnoordhuis
bnoordhuis reviewed Feb 15, 2020
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Show resolved Hide resolved
@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch 2 times, most recently from 0a9170a to 94f7fce Compare February 17, 2020 08:14
@simllll
Copy link
Contributor Author

simllll commented Feb 17, 2020

I've adressed all comments now, thanks for it! Also linted js and c++ code correctly :)

Is there anything else todo? e.g. how gets (in the good case this gets merged ;)) the ts type definition added?

bnoordhuis
bnoordhuis reviewed Feb 17, 2020
View reviewed changes
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.

Some minor issues but LGTM when those are fixed. Thanks.

lib/_tls_wrap.js Outdated Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch from 94f7fce to 337becf Compare February 17, 2020 11:25
jasnell
jasnell reviewed Feb 17, 2020
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Feb 17, 2020
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Feb 17, 2020
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Feb 17, 2020
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch from 337becf to c98c1b5 Compare February 17, 2020 15:03
@simllll simllll mentioned this pull request Feb 20, 2020
Closed
tniessen
tniessen requested changes Feb 22, 2020
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot about one thing: Could you please document the new error code in doc/api/errors.md?

@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch from a37a144 to 119ae43 Compare February 22, 2020 19:31
@simllll simllll requested a review from tniessen February 22, 2020 22:23
tniessen
tniessen approved these changes Feb 23, 2020
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch from 119ae43 to 176f4e6 Compare February 23, 2020 00:45
@simllll simllll force-pushed the feat/expose-SSL_export_keying_material branch from 176f4e6 to c2b23f9 Compare February 23, 2020 00:51
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29289/

tniessen pushed a commit that referenced this pull request Feb 23, 2020
@simllll @tniessen
tls: expose SSL_export_keying_material
341c06f 
Fixes: #31802

PR-URL: #31814
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@tniessen
Copy link
Member

Landed in 341c06f, thank you, @simllll, and congratulations on becoming a contributor! 🎉

@tniessen tniessen closed this Feb 23, 2020
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 23, 2020
@simllll
Copy link
Contributor Author

simllll commented Feb 23, 2020

Thanks from my side for the great input on this PR and for the quick and easy process :-) absolutely love the team behind node js! Great work! 😍

@simllll simllll deleted the feat/expose-SSL_export_keying_material branch February 23, 2020 12:21
codebytere pushed a commit that referenced this pull request Feb 27, 2020
@simllll @codebytere
tls: expose SSL_export_keying_material
acb3aff 
Fixes: #31802

PR-URL: #31814
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@codebytere codebytere mentioned this pull request Feb 29, 2020
Merged
codebytere added a commit that referenced this pull request Mar 1, 2020
@codebytere
2020-03-03 Version 13.10.0 (Current)
165ecaa 
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
@codebytere
2020-03-03 Version 13.10.0 (Current)
e39b006 
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
@codebytere
2020-03-03 Version 13.10.0 (Current)
471590e 
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
@codebytere
2020-03-04 Version 13.10.0 (Current)
f6ffdc2 
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 4, 2020
@codebytere
2020-03-04 Version 13.10.0 (Current)
7546686 
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
@amclin amclin mentioned this pull request Mar 11, 2020
Closed
@targos targos mentioned this pull request Apr 20, 2020
Closed
2 tasks
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@simllll @targos
tls: expose SSL_export_keying_material
b4c431e 
Fixes: nodejs#31802

PR-URL: nodejs#31814
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Apr 28, 2020
@simllll @targos
tls: expose SSL_export_keying_material
6348fae 
Fixes: #31802

PR-URL: #31814
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@sam-github sam-github Awaiting requested review from sam-github

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

expose SSL_export_keying_material via Node API (e.g. like SSL_get_shared_sigalgs)
6 participants
@simllll @addaleax @nodejs-github-bot @tniessen @bnoordhuis @jasnell