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: migrate crypto sign to internal/errors #15757

Closed
wants to merge 2 commits into from

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Oct 3, 2017

Improve argument type checking and move into js, use internal/errors

Improve tests.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@lpinca
lpinca approved these changes Oct 5, 2017
lib/internal/crypto/sig.js Outdated
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'key',
['string', 'Buffer', 'TypedArray', 'DataView']);
}

var ret = this._handle.sign(toBuf(key), passphrase, rsaPadding,

This comment has been minimized.

@lpinca

lpinca Oct 5, 2017
Member

Nit: toBuf() seems redundant here.

lib/internal/crypto/sig.js Outdated
const { Writable } = require('stream');
const { inherits } = require('util');

function Sign(algorithm, options) {
if (typeof algorithm !== 'string')

This comment has been minimized.

@lpinca

lpinca Oct 5, 2017
Member

Nit: maybe it's better to move this after the instanceof check to only run it once when Sign is called without new.

lib/internal/crypto/sig.js Outdated
@@ -80,6 +94,8 @@ Sign.prototype.sign = function sign(options, encoding) {


function Verify(algorithm, options) {
if (typeof algorithm !== 'string')

This comment has been minimized.

@lpinca

lpinca Oct 5, 2017
Member

Ditto.

@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 13, 2017

@nodejs/tsc ... ping? Please review?

@jasnell jasnell requested a review from nodejs/tsc Oct 13, 2017
Copy link
Member

@joyeecheung joyeecheung left a comment

LGTM. Next step would be to migrate SignBase::CheckThrow...

Copy link
Member

@joyeecheung joyeecheung left a comment

Checked the wrong box before

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 14, 2017

Also after this patch, sign.sign() and verify.verify() take array buffer views as key, the documentation need to be updated (not necessarily in this PR)

@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 16, 2017

re: SignBase::CheckThrow and the doc updates... yes, I intend to do both in separate PRs.

@jasnell jasnell force-pushed the jasnell:crypto-sign-internal-errors branch Oct 16, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 16, 2017

@nodejs/tsc ... need at least one more TSC signoff on this please

Copy link
Member

@mcollina mcollina left a comment

LGTM

Copy link
Member

@mhdawson mhdawson left a comment

LGTM

jasnell added 2 commits Oct 3, 2017
Improve argument type checking and move into js, use internal/errors
@jasnell jasnell force-pushed the jasnell:crypto-sign-internal-errors branch to cee5974 Oct 20, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 23, 2017

CI was a bit too red, trying again: https://ci.nodejs.org/job/node-test-commit/13401/

jasnell added a commit that referenced this pull request Oct 24, 2017
Improve argument type checking and move into js, use internal/errors

PR-URL: #15757
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 24, 2017

Landed in b8bc652

@jasnell jasnell closed this Oct 24, 2017
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
Improve argument type checking and move into js, use internal/errors

PR-URL: nodejs/node#15757
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Improve argument type checking and move into js, use internal/errors

PR-URL: nodejs/node#15757
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.