-
Notifications
You must be signed in to change notification settings - Fork 30k
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: add simple getCipherInfo #35368
Conversation
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only looked at the last commit but it LGTM with minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build failed. Please investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
655f798
to
0245e9d
Compare
Ok, @tniessen, I've extended this API to make it a bit more useful for variable key length and iv length ciphers. Specifically, the So, for instance, crypto.getCipherInfo('aes-192-ocb', { ivLength: 10 }); // works!
crypto.getCipherInfo('aes-192-ocb', { ivLength: 18 }); // returns undefined
crypto.getCipherInfo('rc4', { keyLength: 10 }); // works!
crypto.getCipherInfo('aes-192-ccm', { keyLength: 18 }); // returns undefined This way we can at least test to see if given parameters are usable for the given cipher. Other changes:
|
@jasnell That kind of duplicates the existing functionality of calling |
@bnoordhuis ... yeah, I've done the same before. I think the key difference here is just fewer possible allocations but you're right that it duplicates the behavior just a bit. The key question is whether that is ok. Another approach we can take here is to populate the For instance, something like... {
name: 'rc4',
// ...
keyLength: { min: 1 } // no max
} or {
name: 'aes-192-ocb',
// ...
ivLength: { min: 7, max: 15 }
} The other question I would have is: would it be better for the lengths to be expressed in terms of bits? For instance, max iv for ocb modes is anything less than 128 bits.... so a 127 bit iv is allowed but not expressed properly in the info object if we align everything on number of bytes. |
Since that info comes from openssl, and because openssl always expresses it in bytes, I'd stick with that unit. One more reason: expressing it in bits complicates |
1c67df2
to
f9ff966
Compare
f9ff966
to
a2d6e07
Compare
a2d6e07
to
a885bd7
Compare
Simple method for retrieving basic information about a cipher (such as block length, expected or default iv length, key length, etc) Signed-off-by: James M Snell <[email protected]> Fixes: nodejs#22304
a885bd7
to
fec7c9a
Compare
This comment has been minimized.
This comment has been minimized.
Landed in 095be6a |
Simple method for retrieving basic information about a cipher (such as block length, expected or default iv length, key length, etc) Signed-off-by: James M Snell <[email protected]> Fixes: #22304 PR-URL: #35368 Reviewed-By: Ben Noordhuis <[email protected]>
This builds on a semver-major commit so I've added the dont-land labels for older release lines |
Simple method for retrieving basic information about a cipher (such as block length, expected or default iv length, key length, etc) Signed-off-by: James M Snell <[email protected]> Fixes: nodejs#22304 PR-URL: nodejs#35368 Reviewed-By: Ben Noordhuis <[email protected]>
This builds on #35093 which has to land first. (the first two commits are from that PR)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes