crypto: add support for OCB mode for AEAD by tniessen · Pull Request #21447 · nodejs/node · GitHub

This change adds support for OCB, the newest and fastest AEAD mode offered by OpenSSL. There are relatively few changes required within the native code, the OCB implementation behaves somewhat like CCM, except that some limitations of CCM don't apply.

As OCB is patented, we will need to check whether this change has any legal implications. The holder of the patents, Phillip Rogaway, has summarized the situation here and I am pretty sure there won't be any problems, but I wouldn't want to be the one to make the decision.

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

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/113/pipeline

CC @nodejs/crypto. Not sure who to ping for the legal stuff.

As OCB is patented, we will need to check whether this change has any legal implications.

We should marked this blocked until we can get a legal review.

@MylesBorins ... this may be one to bring up to the legal committee.

LGTM modulo comment.

Apropos the legal ramifications, note that we're already shipping OCB:

$ node
> var c = crypto.createCipheriv('aes-128-ocb', 'x'.repeat(16), 'x'.repeat(12)); c.update('boom'); c.final()
<Buffer 4e 83 df 21>

Why not make this a function?

Mhhh I think in my head it still looks like "less overhead". Would you like me to change it?

Yes, please.

This will conflict with #21462 when it lands (but no doubt you knew that since you reviewed it. :-))

Yes, but thanks for pointing it out 😃

Thanks for reviewing, @bnoordhuis!

Apropos the legal ramifications, note that we're already shipping OCB:

And we are already shipping OpenSSL which supports OCB, that's why I assume hope there won't be any problems.

Comment addressed and rebased (old HEAD was 46178187be645f7451279382467c6799b0e3b8cc).

I doubt there will be any issues here and it's likely safe to proceed, but it would still be good to get a legal committee review (assuming that doesn't take forever lol). Let's not block on it tho

Thanks @jasnell. Sadly, I literally know nothing about our legal committee, I can't even find a list of members or documented processes involving them.

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

That's something @MylesBorins needs to do

Rebased, old HEAD was 64efc012ccb9fcdf2f619f1f9cbcf0691fae5009.
New CI: https://ci.nodejs.org/job/node-test-pull-request/15839/

I am pinging @nodejs/tsc to make sure that they know of this change, I would like to land it next week.

Since adding stuff to crypto has been A Thing lately, I'm going to cc @nodejs/security-wg...

This needs a rebase.

Thank you, @BridgeAR! Rebased, old HEAD was dce91f94f146bbf73b8a059ba30be4f9defa84bc.
CI: https://ci.nodejs.org/job/node-test-pull-request/15912/
Only failure seems to be unrelated, resuming: https://ci.nodejs.org/job/node-test-pull-request/15914/

I will land this tomorrow unless someone objects.

PR-URL: nodejs#21447 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in b3f459e.

Depends on #21782 to land on v10.x-staging

#21782 landed but there are still conflicts in the C++ code.

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.

/cc @tniessen

Yes, it should be backported to v10.x-staging. I started working on it but decided to cut my losses. @tniessen the conflicts are around semver-major changes you've introduced in master so you'll probably have a much easier time putting a PR against v10.x-staging to backport this.

I'll be home in a couple of days and will give it a try.

@tniessen I see that you pushed a backport branch to your fork. Is it ready for a PR?

@targos The code itself should work, but I am afraid that it might have unintended side effects for CCM. I'll try to verify that within the next few days.

PR-URL: nodejs#21447 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #22473 PR-URL: #21447 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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 Jun 21, 2018

jasnell added the blocked PRs that are blocked by other issues or PRs. label Jun 22, 2018

bnoordhuis approved these changes Jun 25, 2018

View reviewed changes

tniessen force-pushed the crypto-add-ocb-support branch from 4617818 to 64efc01 Compare June 26, 2018 15:28

jasnell approved these changes Jun 29, 2018

View reviewed changes

tniessen added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Jul 1, 2018

tniessen force-pushed the crypto-add-ocb-support branch from 64efc01 to dce91f9 Compare July 13, 2018 00:02

tniessen added 3 commits July 17, 2018 19:51

crypto: add support for OCB mode for AEAD

bbc8da2

doc: adapt crypto docs to explain OCB mode

501017a

fixup! crypto: add support for OCB mode for AEAD

3322a3f

tniessen force-pushed the crypto-add-ocb-support branch from dce91f9 to 3322a3f Compare July 17, 2018 17:53

tniessen added a commit to tniessen/node that referenced this pull request Jul 18, 2018

crypto: add support for OCB mode for AEAD

b3f459e

PR-URL: nodejs#21447 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen closed this Jul 18, 2018

targos removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 19, 2018

targos added the backport-requested-v10.x label Jul 19, 2018

targos mentioned this pull request Jul 24, 2018

test: move AEAD test vectors out of script #21873

Closed

3 tasks

tniessen added a commit to tniessen/node that referenced this pull request Aug 23, 2018

crypto: add support for OCB mode for AEAD

7ae8ab4

PR-URL: nodejs#21447 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen mentioned this pull request Aug 23, 2018

(v10.x backport) crypto: add support for OCB mode for AEAD #22473

Closed

targos added backported-to-v10.x and removed backport-requested-v10.x labels Aug 29, 2018

targos pushed a commit that referenced this pull request Aug 29, 2018

crypto: add support for OCB mode for AEAD

bc7dbc1

Backport-PR-URL: #22473 PR-URL: #21447 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request Sep 3, 2018

crypto: add support for OCB mode for AEAD

59a6c60

Backport-PR-URL: #22473 PR-URL: #21447 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

targos pushed a commit that referenced this pull request Sep 6, 2018

crypto: add support for OCB mode for AEAD

6465623

Backport-PR-URL: #22473 PR-URL: #21447 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

panva mentioned this pull request Apr 8, 2022

fix(node): add ocb modes to crypto, fix ccm and gcm iv DefinitelyTyped/DefinitelyTyped#59761

Merged

7 tasks

crypto: add support for OCB mode for AEAD by tniessen · Pull Request #21447 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: add support for OCB mode for AEAD - #21447

crypto: add support for OCB mode for AEAD#21447tniessen wants to merge 3 commits intonodejs:masternodejs/node:masterfrom tniessen:crypto-add-ocb-supporttniessen/node:crypto-add-ocb-supportCopy head branch name to clipboard

Conversation

tniessen commented Jun 21, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jun 21, 2018

Uh oh!

tniessen commented Jun 21, 2018

Uh oh!

jasnell commented Jun 22, 2018

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

tniessen Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

tniessen Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

tniessen commented Jun 25, 2018

Uh oh!

tniessen commented Jun 26, 2018

Uh oh!

jasnell commented Jun 29, 2018

Uh oh!

tniessen commented Jun 29, 2018

Uh oh!

jasnell commented Jun 29, 2018

Uh oh!

tniessen commented Jul 13, 2018

Uh oh!

Trott commented Jul 13, 2018

Uh oh!

BridgeAR commented Jul 17, 2018

Uh oh!

tniessen commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Jul 18, 2018

Uh oh!

targos commented Jul 19, 2018

Uh oh!

targos commented Aug 7, 2018

Uh oh!

rvagg commented Aug 13, 2018

Uh oh!

tniessen commented Aug 13, 2018

Uh oh!

targos commented Aug 19, 2018

Uh oh!

tniessen commented Aug 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

crypto: add support for OCB mode for AEAD#21447
tniessen wants to merge 3 commits into
nodejs:masterfrom
tniessen:crypto-add-ocb-support

tniessen commented Jul 17, 2018 •
edited
Loading