Add length check for JWK key import

[devgianlu]

The PR adds length validation of JWK fields provided during key import.

See #33 (comment)

---

Preview | Diff

[Frosne]

What do you think - would it improve reading if we move the public key part of the patch to right before "If the d field is present" ?

[devgianlu]

It makes sense to factor the public key part out of the switch instead of duplicating it. However, I think we should factor out the If |jwk| does not meet the requirements of part too.

Does it make sense to you?

[Frosne]

@panva any opinion?

[panva]

I tend to agree with the comment in #33 (comment) in that this is already covered by having a "valid" JWK for a given sub type. Given that implementations already reject with exactly what is described here I find the entire addition superfluous.

[devgianlu]

Given that implementations already reject with exactly what is described here I find the entire addition superfluous.

Isn't this the exact reason why it should be added to the spec? Implementors (and WPT) are already obeying this un-written rule, so why not make it more explicit.

[panva]

I think it's superfluous and unnecessary. And in order to do it right we'd have to bring in base64url decoding machinery to the spec because the |length in bits| on a base64url encoded field isn't going to be 256.

Please don't take this as blocking. I was asked for my opinion :)

[devgianlu]

And in order to do it right we'd have to bring in base64url decoding machinery to the spec because the |length in bits| on a base64url encoded field isn't going to be 256.

That's why I tried to convey the base64 decoding with interpreted according to Section 2 of [[RFC8037]].

[Frosne]

you're talking about this, right?

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

from

interpreted according to Section 2 of [[RFC8037]]

I would expect that the Section 2 contains the rules of decoding..

[Frosne]

Maybe "decoded using the base64url [RFC4648] decoding"?

But, obviously, it brings even more complexity :(

[devgianlu]

Maybe "decoded using the base64url [RFC4648] decoding"?

I agree with @panva that brining in the definition of what's base64url is to avoid. But since Section 2 of RFC8037 already tells you that it must be base64url encoded, interpreted according to felt like a good trade off.