-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
crypto: add simple getCipherInfo #35368
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
Conversation
|
Review requested:
|
bnoordhuis
left a comment
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.
crypt096
left a comment
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.
tniessen
left a comment
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 undefinedThis 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 <jasnell@gmail.com> Fixes: nodejs#22304
a885bd7 to
fec7c9a
Compare
This comment has been minimized.
This comment has been minimized.
|
Landed in 095be6a |
|
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 <jasnell@gmail.com> Fixes: nodejs#22304 PR-URL: nodejs#35368 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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