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.

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

crypto: assign deprecation code for setAuthTag/GCM #18017

Closed

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Jan 6, 2018

#17566 added a warning when invalid GCM tag lengths are used. As a preparation for #17825, assign a deprecation code before moving it to end-of-life.

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
Affected core subsystem(s)

crypto

@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 Jan 6, 2018
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 6, 2018
@tniessen
Copy link
Member Author

tniessen commented Jan 6, 2018

Node.js supports all GCM authentication tag lengths which are accepted by
OpenSSL when calling [`decipher.setAuthTag()`][]. This behavior will change in
a future version at which point only authentication tag lengths of 128, 120,
112, 104, 96, 64 and 32 bits will be allowed. Authentication tags whose length
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: serial comma for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thank you!

@jasnell
Copy link
Member

jasnell commented Jan 9, 2018

Ping @nodejs/tsc

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@joyeecheung
Copy link
Member

Previous CI stopped. New CI: https://ci.nodejs.org/job/node-test-pull-request/12488/

@tniessen tniessen added this to the 10.0.0 milestone Jan 11, 2018
@tniessen
Copy link
Member Author

tniessen added a commit that referenced this pull request Jan 14, 2018
PR-URL: #18017
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@tniessen
Copy link
Member Author

Landed in 858b48b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. deprecations Issues and PRs related to deprecations. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants