feat: add ed25519 support to JWK (public keys) #452

edudobay · 2022-08-24T00:11:13Z

Ed25519 key support has been added in #343 for the "standard" key format, but not yet for JWKs. (I'm opening a new PR, as I had previously opened #414 against a different base branch.)

Reference documentation: RFC8037

bshaffer

Thank you for submitting this! I have some technical questions, as well as some issues with the implementation here. Hopefully we can get this feature merged!

tests/JWKTest.php

bshaffer · 2022-10-05T20:15:17Z

src/JWT.php

+     * @return string A Base64 encoded string with standard characters (+/) and padding (=), when
+     * needed.
+     */
+    public static function urlsafeToStandardB64(string $input): string


This naming is confusing to me. For key type OKP, the spec says

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

This links to another spec which says

This encoding may be referred to as "base64url". This encoding
should not be regarded as the same as the "base64" encoding and
should not be referred to as only "base64"

I think it would be better to call this base64UrlDecode

I've renamed the function to convertBase64urlToBase64. Do you think it's clearer?

It doesn't actually decode base64 data, it just translates from base64url to base64 so that the PHP base64_decode function can be used.

bshaffer · 2022-10-05T20:19:42Z

src/JWK.php

+        $ktyNotRequiringAlg = [
+            // In Octet Key Pair (OKP) keys, the signing algorithm (alg) will not be read from the
+            // JWK, as it can be inferred directly from the curve type (crv).
+            // @see https://datatracker.ietf.org/doc/html/rfc8037#section-3.1
+            'OKP',
+        ];
+        if (!isset($jwk['alg']) && !\in_array($jwk['kty'], $ktyNotRequiringAlg, true)) {


I am unfamiliar with OKPs, but by looking at this example, those values contain an "alg" of EdDSA instead of using the clunky mapping. I'd prefer to keep the algorithm explicit, and require it like we do everywhere else.

I've reverted to the original version where the algorithm is always required. I see that the library opted for always requiring an explicit algorithm.

src/JWK.php

bshaffer · 2022-10-05T20:22:47Z

src/JWK.php

@@ -30,6 +30,11 @@ class JWK
        // 'P-521' => '1.3.132.0.35', // Len: 132 (not supported)
    ];

+    // 'crv' identifier => JWT 'alg'
+    private const OKP_CURVES = [


This is strange to me, as the values here aren't curves, but algorithms. I am not convinced this is the correct implementation.

I've rephrased the concepts to call these "key subtypes", as section 2 of RFC 8037 calls it.

bshaffer · 2022-10-05T20:36:38Z

src/JWK.php

+                $publicKey = JWT::urlsafeToStandardB64($jwk['x']);
+                $alg = self::OKP_CURVES[$jwk['crv']];


I am confused because there is no action taken as far as implementing decoding a key with ed25519. The JWK class is base64url-decoding the key, and (I am assuming) relying on the to default behavior of sodium_crypto to accept this as ed25519. There are other possible values here, however, so I think at the very least this needs to throw an exception if crv is not ed25519.

Yes, the sodium_crypto functions that are used by the library work specifically with ed25519.

If an unsupported crv is provided, an exception will be thrown a few lines above with the message Unrecognised or unsupported OKP key subtype.

alecsammon · 2023-05-15T15:37:33Z

This PR would be really useful for us.

@edudobay - do you have time to get this updated and the questions responded to, or instead would you be happy if I took it forward?

I've tested the branch with our code bases and everything looks to work correctly.

Thank you!

Reference documentation: https://datatracker.ietf.org/doc/html/rfc8037

edudobay · 2023-05-21T17:22:32Z

Hi! Sorry for the delay, I've addressed the issues you commented, and I think it's clearer and cleaner now.

I was wondering if it would be useful to throw an exception if the provided alg is not supported for the given kty (for example, alg=RS256 and kty=OKP). But this verification does not yet exist for the other key types, so I guess this would be the scope of a future PR.

In that case, the mismatch would fall down to the crypto function layer — it would either give an error because the given key is invalid for the given algorithm, or encrypt with the wrong algorithm if by chance the same key is valid for different algorithms.

bshaffer

This looks great! Just one minor change and we can merge this!

Thanks again for your contribution.

src/JWT.php

bshaffer · 2023-05-22T18:09:48Z

tests/data/ed25519-jwkset.json

+            "kid": "jwk1",
+            "kty": "OKP",
+            "crv": "Ed25519",
+            "x": "uOSJMhbKSG4V5xUHS7B9YHmVg_1yVd-G-Io6oBFhSfY"


The tests say this needs to contain an "alg" parameter! Either that or you can refactor the tests to use the $defaultAlg argument

Added the parameter.

edudobay · 2023-05-23T01:43:24Z

@bshaffer I've addressed the final comments, so I think this is ready now :)

edudobay force-pushed the add-ed25519-jwk-support branch from 54c6024 to 9914472 Compare August 24, 2022 00:14

bshaffer reviewed Oct 5, 2022

View reviewed changes

feat: add ed25519 support to JWK (public keys)

c55b88d

Reference documentation: https://datatracker.ietf.org/doc/html/rfc8037

edudobay force-pushed the add-ed25519-jwk-support branch from 43c3860 to c55b88d Compare May 21, 2023 17:07

bshaffer reviewed May 22, 2023

View reviewed changes

src/JWT.php Outdated Show resolved Hide resolved

bshaffer reviewed May 22, 2023

View reviewed changes

fix required "alg" in test + correct method name casing

a61a0b0

bshaffer approved these changes Jun 14, 2023

View reviewed changes

bshaffer merged commit e53979a into firebase:main Jun 14, 2023
7 checks passed

release-please bot mentioned this pull request Jun 14, 2023

chore(main): release 6.7.0 #518

Merged

edudobay deleted the add-ed25519-jwk-support branch June 14, 2023 15:28

edudobay mentioned this pull request Aug 28, 2023

feat: add ed25519 support to JWKs #450

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ed25519 support to JWK (public keys) #452

feat: add ed25519 support to JWK (public keys) #452

edudobay commented Aug 24, 2022

bshaffer left a comment

bshaffer Oct 5, 2022

edudobay May 21, 2023

bshaffer Oct 5, 2022 •

edited

edudobay May 21, 2023

bshaffer Oct 5, 2022 •

edited

edudobay May 21, 2023

bshaffer Oct 5, 2022

edudobay May 21, 2023

alecsammon commented May 15, 2023

edudobay commented May 21, 2023 •

edited

bshaffer left a comment

bshaffer May 22, 2023

edudobay May 23, 2023

edudobay commented May 23, 2023

		$publicKey = JWT::urlsafeToStandardB64($jwk['x']);
		$alg = self::OKP_CURVES[$jwk['crv']];

feat: add ed25519 support to JWK (public keys) #452

feat: add ed25519 support to JWK (public keys) #452

Conversation

edudobay commented Aug 24, 2022

bshaffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshaffer Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshaffer Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecsammon commented May 15, 2023

edudobay commented May 21, 2023 • edited

bshaffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edudobay commented May 23, 2023

bshaffer Oct 5, 2022 •

edited

bshaffer Oct 5, 2022 •

edited

edudobay commented May 21, 2023 •

edited