Skip to content

crypto: use kNoAuthTagLength in InitAuthenticated #20225

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
Closed
tniessen wants to merge 3 commits into from
Closed

crypto: use kNoAuthTagLength in InitAuthenticated #20225

tniessen wants to merge 3 commits into from

Conversation

@tniessen
Copy link
Member

@tniessen tniessen commented Apr 23, 2018

As suggested by @bnoordhuis in #20039, this changes InitAuthenticated to accept an unsigned int auth_tag_len.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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++. crypto Issues and PRs related to the crypto subsystem. labels Apr 23, 2018
@tniessen
Copy link
Member Author

tniessen commented Apr 23, 2018

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

bnoordhuis
bnoordhuis approved these changes Apr 24, 2018
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.

LGTM modulo comment.

src/node_crypto.cc Outdated
int auth_tag_len = args[2].As<v8::Int32>()->Value();
unsigned int auth_tag_len;
if (args[2]->IsUint32()) {
auth_tag_len = args[2]->Uint32Value();
Copy link
Member

@bnoordhuis bnoordhuis Apr 24, 2018

Choose a reason for hiding this comment

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

Can you use the overload that takes a Local<Context> or use the args[2].As<Uint32>()->Value() idiom?

@BridgeAR
Copy link
Member

BridgeAR commented Apr 24, 2018

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2018
bnoordhuis
bnoordhuis approved these changes Apr 24, 2018
View reviewed changes
src/node_crypto.cc Outdated
unsigned int auth_tag_len;
if (args[2]->IsUint32()) {
auth_tag_len = args[2]->Uint32Value();
auth_tag_len = args[2].As<v8::Uint32>()->Value();
Copy link
Member

@bnoordhuis bnoordhuis Apr 24, 2018

Choose a reason for hiding this comment

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

Can you add using v8::Uint32; and using v8::Int32; at the top?

@tniessen
Copy link
Member Author

tniessen commented Apr 24, 2018

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

tniessen added a commit that referenced this pull request Apr 25, 2018
@tniessen
crypto: use kNoAuthTagLength in InitAuthenticated
4809db9 
PR-URL: #20225
Refs: #20039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
tniessen added a commit that referenced this pull request Apr 25, 2018
@tniessen
crypto: add using directives for v8::Int32, Uint32
9d35f69 
PR-URL: #20225
Refs: #20039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@tniessen
Copy link
Member Author

tniessen commented Apr 25, 2018

Landed in 4809db9, 9d35f69.

@tniessen tniessen closed this Apr 25, 2018
@MylesBorins
Copy link
Contributor

MylesBorins commented May 4, 2018

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@targos targos mentioned this pull request May 12, 2018
Closed
tniessen added a commit to tniessen/node that referenced this pull request May 13, 2018
@tniessen
crypto: use kNoAuthTagLength in InitAuthenticated
3bd5db3 
PR-URL: nodejs#20225
Refs: nodejs#20039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
tniessen added a commit to tniessen/node that referenced this pull request May 13, 2018
@tniessen
crypto: add using directives for v8::Int32, Uint32
f57543b 
PR-URL: nodejs#20225
Refs: nodejs#20039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@tniessen tniessen mentioned this pull request May 13, 2018
Closed
addaleax pushed a commit that referenced this pull request May 14, 2018
@tniessen @addaleax
crypto: use kNoAuthTagLength in InitAuthenticated
f5e7010 
Backport-PR-URL: #20706
PR-URL: #20225
Refs: #20039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit that referenced this pull request May 14, 2018
@tniessen @addaleax
crypto: add using directives for v8::Int32, Uint32
1e5de6f 
Backport-PR-URL: #20706
PR-URL: #20225
Refs: #20039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax addaleax mentioned this pull request May 14, 2018
Merged
@targos targos added backported-to-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v10.x labels May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@danbev danbev danbev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tniessen @BridgeAR @MylesBorins @bnoordhuis @danbev @jasnell @targos @nodejs-github-bot