crypto: forbid NODE-ED25519 and NODE-ED448 "raw" key export

[panva]

closes #38655

Similar to ECDH and ECDSA "raw" export is not allowed for private keys.

This is a breaking change but webcrypto is still an experimental module.

cc @nodejs/crypto

[nodejs-github-bot]

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

[vlovich]

Raw import is similarly affected & why I provided the pkcs8 key equivalents: https://github.com/nodejs/node/pull/36879/files#diff-15c1a60d165e73c0aa21f0aff84493a257a577a97e9c8d2f33779b003193e338R126

This should be removed? https://github.com/nodejs/node/pull/36879/files#diff-15c1a60d165e73c0aa21f0aff84493a257a577a97e9c8d2f33779b003193e338R236

It's also unclear to me if X25519/X448 is similarly impacted, but I think so given the logic in the test (at least for import?):
https://github.com/nodejs/node/pull/36879/files#diff-6287c7ab5132602cd7901483437a6b5ca4cb3b0596395854be8eccf3e0d9774aR46-R53

Here's the import of private EdDH keys:
https://github.com/nodejs/node/pull/36879/files#diff-6287c7ab5132602cd7901483437a6b5ca4cb3b0596395854be8eccf3e0d9774aR174-R175

[panva]

Raw X/Ed key import was the reason why @jasnell implemented this node-specific webcrypto extension in the first place.

I'll leave the rest to him to comment. I'm having a hard time understanding what is it you are after.

[devsnek]

raw private import and raw public import/export should still work i hope?

[panva]

@devsnek with this PR in place that is still the case, yes.

[panva]

I'm not so sure we need to forbid this in the first place.

[vlovich]

@pavna I'm from the Cloudflare Workers team working on WebCrypto updates (context here is I'm adding Ed25519 support). I was originally going to use a custom name on our end for the algorithms/curves as I didn't understand why Node went with NODE-ED25519/NODE-ED448/ECDH for the algorithm names instead of what seemed more natural to me (NODE-EDDSA/NODE-EDDH).

I was told to use the same ones as Node to avoid unnecessary incompatibility for users who may write code for both platforms. I wanted to highlight an area of incompatibility we'll have. It seems like we're aligned that raw export of private keys shouldn't be allowed & for imports I'll just make a note in our developer docs for the difference.

[jasnell]

@vlovich ... that's excellent to hear re: Cloudflare Workers WebCrypto updates! Making sure we're aligned here makes perfect sense! Keep in mind that the entire WebCrypto module is still considered to be "Experimental" so we can change things as it makes sense to do so.

Regarding the naming, I just went with something that would be descriptive and would make it clearer to someone who is less familiar with the EDD... naming pattern in reference to the 25519/448 bits.

I think we're definitely in agreement on the raw export of the private keys. Limiting that makes perfect sense. If there are other incompatibilities definitely let us know! While the module is still experimental we can make breaking changes without worrying about semver rules.

[jasnell]

@panva:

I'm not so sure we need to forbid this in the first place.

"Need* is a bit strong. Restricting export of the private keys is consistent with the rest of WebCrypto and making it more difficult to get to the private key data just makes sense Just In Case (tm).

[vlovich]

Yup. I'm definitely noting that it's non-standard & might experience breaking changes. We'll have more constraints on our end in terms of breakages since this is a production system & we will have at least one customer that will be using Ed25519 when it ships. We're only going to ship Ed25519 to start with & we'll be unable to support Curve448 since BoringSSL ripped it out. Hopefully that limits our exposure & if issues come up we'll try to co-ordinate if we think there's a problem. The spec seems simple enough that I'm hopeful any changes won't actually be wholly backwards incompatible.

On the naming, it was just a bit jarring and as an implementer new to WebCrypto & Curve25519, it would have saved me a few cycles to see NODE-EDDSA/NODE-EDDH to realize it's similar to but not the same as elliptic curves (at first I thought I just needed to update the lookup table & that was it). Not a big deal although I think I would prefer seeing EDDSA/EDDH as the algorithm names for standardization because the spec will just be more self-consistent.

[jasnell]

I'm happy to change the names to whatever may eventually become the "Standard" :-) ...

We'll have more constraints on our end in terms of breakages since this is a production system & we will have at least one customer that will be using Ed25519 when it ship

Once this happens we can make sure we're being careful about avoiding breaking changes on these to avoid issues.

[devsnek]

I'm actually the customer (or well, i work for the customer 😄)

happy to track whatever changes happen in the name of a nice standardized ecosystem.

[panva]

Landed in 2130598

[armfazh]

On the naming, it was just a bit jarring and as an implementer new to WebCrypto & Curve25519, it would have saved me a few cycles to see NODE-EDDSA/NODE-EDDH to realize it's similar to but not the same as elliptic curves (at first I thought I just needed to update the lookup table & that was it). Not a big deal although I think I would prefer seeing EDDSA/EDDH as the algorithm names for standardization because the spec will just be more self-consistent.

Regarding the name I suggest do not change the original algorithm's name: for signatures are Ed25519 and Ed448, for Diffie-Hellman operations are: X25519 and X448. There are historical reasons for these names and there is no need to change them.
Also these names are in line with RFC7748 and RFC8032 IETF documents.