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: add simple getCipherInfo #35368

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 27, 2020

This builds on #35093 which has to land first. (the first two commits are from that PR)

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

Review requested:

  • @nodejs/crypto
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 27, 2020
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.

I only looked at the last commit but it LGTM with minor comments.

src/crypto/crypto_cipher.cc Outdated Show resolved Hide resolved
src/crypto/crypto_cipher.cc Show resolved Hide resolved
Copy link

@crypt096 crypt096 left a 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.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Looks good @jasnell. #26612 contains a relevant discussion.

src/crypto/crypto_cipher.cc Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Sep 27, 2020

Ok, @tniessen, I've extended this API to make it a bit more useful for variable key length and iv length ciphers. Specifically, the getCipherInfo() can include keyLength and ivLength options that will be tested against the given cipher. If they are acceptable, then the info is returned, if they are not undefined is returned. The tests are applied for OCB, GCM, and CCM mode ciphers.

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:

  1. I've removed the type property
  2. For stream ciphers, the blockSize property is omitted
  3. For ciphers that do not use an iv, the ivLength property is omitted

@bnoordhuis
Copy link
Member

@jasnell That kind of duplicates the existing functionality of calling crypto.createCipheriv() and checking if it throws an exception. I've used that technique to good effect for some years now. :-)

@jasnell
Copy link
Member Author

jasnell commented Sep 28, 2020

@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 createCipherInfo() return object with the known acceptable ranges for key and iv length for ciphers that are known to be variable.

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.

@bnoordhuis
Copy link
Member

would it be better for the lengths to be expressed in terms of bits?

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 Buffer.alloc() calls because now you have to worry about values that aren't multiples of 8. Not super complicated - (bits + 7) >>> 3 - but still.

@jasnell jasnell force-pushed the subcrypto-cipherinfo branch 3 times, most recently from 1c67df2 to f9ff966 Compare October 5, 2020 19:23
@jasnell jasnell marked this pull request as ready for review October 8, 2020 00:46
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
@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 10, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 10, 2020

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2020
@jasnell
Copy link
Member Author

jasnell commented Oct 13, 2020

Landed in 095be6a

@jasnell jasnell closed this Oct 13, 2020
jasnell added a commit that referenced this pull request Oct 13, 2020
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: #22304

PR-URL: #35368
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Member

This builds on a semver-major commit so I've added the dont-land labels for older release lines

joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants