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

crypto: add support for OCB mode for AEAD #21447

Closed
wants to merge 3 commits into from
Closed

crypto: add support for OCB mode for AEAD #21447

wants to merge 3 commits into from

Conversation

tniessen
Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

@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

@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 Jun 21, 2018
@tniessen
Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Jun 22, 2018

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.

@jasnell jasnell added the blocked PRs that are blocked by other issues or PRs. label Jun 22, 2018
bnoordhuis
bnoordhuis approved these changes Jun 25, 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.

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>

src/node_crypto.cc Outdated
@@ -2668,6 +2668,9 @@ void CipherBase::Init(const FunctionCallbackInfo<Value>& args) {
cipher->Init(*cipher_type, key_buf, key_buf_len, auth_tag_len);
}

#define IS_SUPPORTED_AUTHENTICATED_MODE(mode) ((mode) == EVP_CIPH_CCM_MODE || \
(mode) == EVP_CIPH_GCM_MODE || \
(mode) == EVP_CIPH_OCB_MODE)
Copy link
Member

Choose a reason for hiding this comment

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

Why not make this a function?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please.

src/node_crypto.cc Outdated
max_message_size_ = (1 << (8 * (15 - iv_len))) - 1;
} else {
max_message_size_ = INT_MAX;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but thanks for pointing it out 😃

@tniessen
Copy link
Member Author

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.

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

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

@jasnell
Copy link
Member

jasnell commented Jun 29, 2018

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

@tniessen
Copy link
Member Author

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/

@jasnell
Copy link
Member

jasnell commented Jun 29, 2018

That's something @MylesBorins needs to do

@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Jul 1, 2018
@tniessen tniessen force-pushed the crypto-add-ocb-support branch from 64efc01 to dce91f9 Compare July 13, 2018 00:02
@tniessen
Copy link
Member Author

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.

@Trott
Copy link
Member

Trott commented Jul 13, 2018

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

@BridgeAR
Copy link
Member

This needs a rebase.

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

tniessen commented Jul 17, 2018 •
edited
Loading

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.

tniessen added a commit to tniessen/node that referenced this pull request Jul 18, 2018
@tniessen
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
Copy link
Member Author

Landed in b3f459e.

@tniessen tniessen closed this Jul 18, 2018
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 19, 2018
@targos
Copy link
Member

targos commented Jul 19, 2018

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

@targos targos mentioned this pull request Jul 24, 2018
Closed
3 tasks
@targos
Copy link
Member

targos commented Aug 7, 2018

#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

@rvagg
Copy link
Member

rvagg commented Aug 13, 2018

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.

@tniessen
Copy link
Member Author

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

@targos
Copy link
Member

targos commented Aug 19, 2018

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

@tniessen
Copy link
Member Author

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

tniessen added a commit to tniessen/node that referenced this pull request Aug 23, 2018
@tniessen
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 tniessen mentioned this pull request Aug 23, 2018
Closed
targos pushed a commit that referenced this pull request Aug 29, 2018
@tniessen @targos
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
@tniessen @targos
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 targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@tniessen @targos
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
Closed
Closed
@panva panva mentioned this pull request Apr 8, 2022
Merged
7 tasks
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

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 @nodejs-github-bot @jasnell @Trott @BridgeAR @targos @rvagg @bnoordhuis