Unclear whether key valiation should be performed on import

[devgianlu]

Citing w3c/webcrypto#399:

Looking at the import key operation for Ed25519 (also applies to X25519, Ed448 and X448) there does not seem to be any explicit mention of the necessity of validating the imported key. However, multiple WPT tests are present to test for invalid key lengths.

This is especially relevant for the raw type which could simply be a byte-sequence copy (implementation wise), but in reality needs to validate (at least?) the length of the key. Similary, the jwk type should additionally check for the validity of the keypair according to the WPT tests.

[Frosne]

Let data be keyData.
If the length in bits of data is not 256 then throw a DataError.

Something like this for raw keys?

[Frosne]

We can also reject small-order points if we want

[twiss]

Checking small-order points on key import was hotly debated in #13 and ultimately decided against in favor of doing it on verification in #21.

I agree though that we should have a check for the length during key import, at least, as WPT already expects this and implementations already do so. A PR would be welcome :)

[Frosne]

One more thing -

I see 'invalid key pair' WPT tests only for *25519 and *448, should not they also be implemented for all EC?
Also, PKCS8 format (I believe) could include public key as well, thus we can also test that the public key corresponds to the private key.

On the other hand, I believe that this check is not super important and could be optional?

[twiss]

The original report has been fixed by #35, which added length checks for raw key imports. (Thanks @Frosne!)

For JWK imports, we already had the following check:

If jwk does not meet the requirements of the JWK private key format described in Section 2 of [RFC8037], then throw a DataError.

@Frosne:

I see 'invalid key pair' WPT tests only for *25519 and *448, should not they also be implemented for all EC?

Yeah, that would be good. This is probably more for an issue in wpt, though.

Also, PKCS8 format (I believe) could include public key as well, thus we can also test that the public key corresponds to the private key.

As currently written, the spec says to parse a PKCS8 key according to the PrivateKeyInfo structure of RFC5208, rather than the OneAsymmetricKey structure of RFC8410. Though I think they're mutually compatible, this means that the public key, if present, will be parsed as an abstract Attribute rather than a PublicKey. So I think this implies that the public key isn't checked. If we want to change that, we could change the text to parse the data as per RFC8410 instead, but I'd classify this as a possible future improvement and make a separate issue for that, if you want to see it happen 😊

[devgianlu]

@twiss Looking at Section 2 of [RFC8037], I don't see anywhere mentioned that there has to be checks on the key length.

[twiss]

@devgianlu It's not written very explicitly, but RFC8037 says:

o  The parameter "x" MUST be present and contain the public key
   encoded using the base64url [RFC4648] encoding.

o  The parameter "d" MUST be present for private keys and contain the
   private key encoded using the base64url encoding.

and RFC7748 and RFC8032 specify that X25519/Ed25519 and X448/Ed448 public and private keys are 256 bits and 448 bits, respectively. So, if they are a different length (when base64-decoded) I think it's defensible to say that it's not a valid JWK key.

But, if it's not clear enough we could clarify this in the spec, of course.

[devgianlu]

But, if it's not clear enough we could clarify this in the spec, of course.

Yes, I think that requires to be more explicit. I'll try to put together a PR.

[devgianlu]

I have filed #38