crypto: reject public keys properly

[tniessen]

If the underlying operation requires a private key, isPublic must be set to false instead of undefined. (The latter means that both public and private keys are accepted.)

Fixes: #29904

Checklist

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

[sam-github]

There are other formats for public keys, perhaps its worth testing them all?

[tniessen]

I think the only other supported type is pkcs1 which behaves differently, but I added a test case for that as well.

[sam-github]

node/src/node_crypto.cc

Line 3349 in 7f22aaf

CHECK_EQ(config.type_.ToChecked(), kKeyEncodingSEC1);

I think is still a problem, its not necessarily true, the conditionals don't handle new values being added to the enum PKEncodingType. I suggest changing the if..if else ..else to a switch(config.type_.ToChecked()) { case .... with either a default that returns ParseKeyResult::kParseKeyFailed, or probably better, no default. I think gcc will warn when a switch doesn't handle all values of an enum in its case statements, so the code will be more future proof. Just a suggestion.

[tniessen]

I understand your point. My main problem with a switch without a default case is that relying on a compiler warning seems dangerous to me, the process would not abort anymore.

In my opinion, the current behavior of ParsePrivateKey is correct, it handles all valid values (PKCS#1, PKCS#8, SEC1) and aborts on invalid values (SPKI). The only problem is that we won't notice the possibility of passing invalid values to CreatePrivateKey without running into the situation (which is exactly what happened in #29904), but that wouldn't be fixed by a switch that handles those cases.

[nodejs-github-bot]

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

[sam-github]

Aborts at the js-to-c++ layer make a lot of sense to me, where its easy to add type check calls in js just before calling C++ it should be very difficult to reach those abort arg checks.

But since users are managing to get unexpected things to happen multiple layers down in the C++, then actual error returns make more sense. I don't feel strongly about it, but I notice that changing the fallback in the code in question to an error return, see #29904 (comment), prevented the abort. The user would still get an error, of course, but that was already possible for reasons of malformed input. That was exploratory code, I'm not suggesting using it as-is, but having the return be the default/final-else case would make the code more robust.

[tniessen]

But since users are managing to get unexpected things to happen multiple layers down in the C++, then actual error returns make more sense. I don't feel strongly about it, but I notice that changing the fallback in the code in question to an error return, see #29904 (comment), prevented the abort. The user would still get an error, of course, but that was already possible for reasons of malformed input. That was exploratory code, I'm not suggesting using it as-is, but having the return be the default/final-else case would make the code more robust.

It would be more robust in the sense that the process would not abort, but what kind of error should we throw here? Reaching this point is definitely a bug, and an error code that indicates otherwise will only be misleading. So the error should very clearly say: "Hey, this is a bug in node!". However, if we just throw the error, it might get lost, whereas aborting the process will send a very clear message.

[Trott]

Landed in c64ed10