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: expose KeyObject class #26438

Closed
wants to merge 1 commit into from
Closed

crypto: expose KeyObject class #26438

wants to merge 1 commit into from

Conversation

panva
Copy link
Member

@panva panva commented Mar 4, 2019

This PR exposes the KeyObject class as per #26200 discussion.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @sam-github @tniessen

Closes #26200

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Mar 4, 2019
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

Shouldn't this touch the docs somewhere? Can you find how the other classes are documented, and duplicate the style for this one? Or are they completely undocumented? That would be unfortunate! The docs will need YAML change markers.

lib/crypto.js Outdated Show resolved Hide resolved
@panva
Copy link
Member Author

panva commented Mar 4, 2019

The KeyObject as a Class is already documented, i reworded it a bit to be in line with e.g. Hmac and added a history entry.

@sam-github
Copy link
Contributor

Thanks for adding the docs. Also, PR should touch the test code. Something like the first test in test/parallel/test-crypto-hmac.js that checks the instanceof should be in test/parallel/test-crypto-key-objects.js (for each key object type).

If using new on the class fails, should be a test asserting that, too, though I don't see one for Hmac, so perhaps it doesn't fail, its just a doced "do not do this" restriction, not a runtime restriction.

ci: https://ci.nodejs.org/job/node-test-pull-request/21194/

@panva
Copy link
Member Author

panva commented Mar 4, 2019

Thanks for adding the docs. Also, PR should touch the test code. Something like the first test in test/parallel/test-crypto-hmac.js that checks the instanceof should be in test/parallel/test-crypto-key-objects.js (for each key object type).

If using new on the class fails, should be a test asserting that, too, though I don't see one for Hmac, so perhaps it doesn't fail, its just a doced "do not do this" restriction, not a runtime restriction.

ci: ci.nodejs.org/job/node-test-pull-request/21194

I've reflected the previous review notes in the force-pushed f64f682.

Looking at the code it's not hard to fool this one. Not sure how the C binding object instanceof would look like to make sure a KeyObject instance is not coming out of new. With that in mind, do you want a runtime fail test for new?

Hmmm,

we could do if (!(handle instanceof KeyObjectHandle)) in the constructor, one more change then.

@panva
Copy link
Member Author

panva commented Mar 5, 2019

CI ✅

Any further suggestions?

@sam-github
Copy link
Contributor

If the other exposed classes aren't doing something special to make new not work, then we don't have to do it for KeyObject either.

@panva
Copy link
Member Author

panva commented Mar 5, 2019

If the other exposed classes aren't doing something special to make new not work, then we don't have to do it for KeyObject either.

The difference is that other classes do not create the handle outside of the constructor. I'd like to keep the check in place, it's insurance (see below comments), doesn't harm.

@tniessen
Copy link
Member

tniessen commented Mar 5, 2019

@tniessen tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 5, 2019
@BridgeAR
Copy link
Member

@sam-github @tniessen what is the consensus here?

@sam-github isn't it a good first step to prevent this class from being instantiated? The other classes could follow if they should not be used in userland.

@sam-github
Copy link
Contributor

  • Previously, all the classes but 1 were exposed. This is an API gotcha.
  • Now, all the classes but 1 can be instantiated. This is an API gotcha.

So, no, @BridgeAR , IMO it is isn't a good first step, its an idiosyncratic special case.

I suggest not doing it in this PR, but would encourage a follow up PR, that changes ALL the classes to consistently make them not instantiable. It could be introduced in this PR, but it doesn't seem related.

@panva
Copy link
Member Author

panva commented Mar 11, 2019

@sam-github while i have my reserves, there, force pushed just the class exposure and removed the tests for blocking, let it be a wild west as-is today.

I will try and get a sweeping revision of all exposed crypto classes in the coming days tho.

@panva
Copy link
Member Author

panva commented Mar 11, 2019

To explain my reservation for not having it part of this PR

other exports are marked as Classes but aren't

e.g. Certificate

// For backwards compatibility reasons, this cannot be converted into a
// ES6 Class.

or Cipheriv

function Cipheriv(cipher, key, iv, options) {
  if (!(this instanceof Cipheriv))
    return new Cipheriv(cipher, key, iv, options);

  createCipherWithIV.call(this, cipher, key, options, true, iv);
}

They all have one create* method which all pretty much just pass the same arguments to the constructor function so there's no harm if they get called directly, they're just not documented API (neither is the KeyObject constructor i know), KeyObject has three and they pass a handle from the create* down to the class constructor and I don't think there's a way to unify them all unless they're all classes that have their handles instantiated outside of the constructor.

It's the fact that an invalid key MAY be instantiated with the class exposed is why i first opened just an isKeyObject helper. Now there's gonna be a class that developers or attackers may push down a consuming crypto library's API (e.g. JWT). Is there an opening for a vulnerability? I don't know, but would rather have the check in place. I don't believe having it in place is a gotcha, it would a be a good hygiene.

@tniessen
Copy link
Member

other exports are marked as Classes but aren't

That's because what you call "classes" is a relatively new concept in ES.

so there's no harm if they get called directly

There are two reasons why users shouldn't use new:

  1. They would rely on internal APIs that can change without notice.
  2. Since the constructor was not designed to be called by users, it is potentially unsafe and we consider calling it a mistake.

Now there's gonna be a class that developers or attackers may push down a consuming crypto library's API (e.g. JWT). Is there an opening for a vulnerability?

We try to prevent users from making mistakes, but we cannot fully protect users from code with malicious intent. This is not a vulnerability.

@panva
Copy link
Member Author

panva commented Mar 11, 2019

All i'm saying is that by using the other exports today you don't end up with a broken object. Using this one you do and i understand why it wasn't exported in the first place. Here's a branch where you can't. Just played around a bit.

It's the fact that this is undocumented and not intended to be used directly, that I find the strive to keep a constructor that's putting potentially broken KeyObjects in place strange.
What's worse? having a quirk where one can't figure out a passed object is already an instance of a KeyObject or having a quirk where undocumented API is different between the exports. No need to answer that, it's rhetorical.

@tniessen
Copy link
Member

I am against adding such checks for KeyObject as part of this PR. They make the code more complex and less efficient. It is already difficult enough to construct a KeyObject using new due to the necessity of passing a KeyObjectHandle.

(Off topic: You don't need to use an object with a symbol as a key. You can just pass a unique value such as a Symbol() as a second parameter, if you really want to do that.)

I'll approve this if there is consensus amongst collaborators that exposing this class is the best way to go.

@sam-github
Copy link
Contributor

@nodejs/crypto @nodejs/collaborators

@panva
Copy link
Member Author

panva commented Mar 17, 2019

So, can we land this?

tniessen pushed a commit that referenced this pull request Mar 17, 2019
PR-URL: #26438
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@tniessen
Copy link
Member

Thanks for reviewing, landed in f105654. @panva, thank you for your contribution! 🎉

@nodejs/crypto, please chime in before the next semver-minor release if you think this should not have landed.

@tniessen tniessen closed this Mar 17, 2019
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
PR-URL: nodejs#26438
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
targos pushed a commit that referenced this pull request Mar 27, 2019
PR-URL: #26438
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants