Skip to content

Add length check for JWK key import #38

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.

Sign up for GitHub

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

Open
devgianlu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add length check for JWK key import #38

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 144 additions & 0 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,30 @@ <h4>Operations</h4>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
Copy link
Collaborator

@Frosne Frosne Aug 6, 2025

Choose a reason for hiding this comment

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

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" ?

Copy link
Author

@devgianlu devgianlu Aug 6, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@Frosne Frosne Aug 6, 2025

Choose a reason for hiding this comment

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

@panva any opinion?

Copy link
Contributor

@panva panva Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@devgianlu devgianlu Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@panva panva Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Author

@devgianlu devgianlu Aug 6, 2025

Choose a reason for hiding this comment

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

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]].

Copy link
Collaborator

@Frosne Frosne Aug 6, 2025

Choose a reason for hiding this comment

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

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..

Copy link
Collaborator

@Frosne Frosne Aug 6, 2025

Choose a reason for hiding this comment

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

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

But, obviously, it brings even more complexity :(

Copy link
Author

@devgianlu devgianlu Aug 6, 2025

Choose a reason for hiding this comment

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

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.

according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPublic| is not 256, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPrivate| be the {{JsonWebKey/d}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPrivate| is not 256, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand All @@ -599,6 +623,18 @@ <h4>Operations</h4>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPublic| is not 256, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand Down Expand Up @@ -1437,6 +1473,30 @@ <h4>Operations</h4>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPublic| is not 448, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPrivate| be the {{JsonWebKey/d}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPrivate| is not 448, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand All @@ -1462,6 +1522,18 @@ <h4>Operations</h4>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPublic| is not 448, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand Down Expand Up @@ -2321,6 +2393,30 @@ <h4>Operations</h4>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPublic| is not 256, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPrivate| be the {{JsonWebKey/d}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPrivate| is not 256, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand All @@ -2346,6 +2442,18 @@ <h4>Operations</h4>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPublic| is not 256, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand Down Expand Up @@ -3251,6 +3359,30 @@ <h4>Operations</h4>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPublic| is not 448, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPrivate| be the {{JsonWebKey/d}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPrivate| is not 448, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand All @@ -3276,6 +3408,18 @@ <h4>Operations</h4>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the <a data-cite="webcrypto#dfn-length-in-bits">length in bits</a>
of |jwkPublic| is not 448, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand Down