Support for 25519 algos #4556

McMastS · 2025-02-18T03:44:07Z

What?

Adds support for Ed25519 and X25519 algorithms in K6's webcrypto offering.

This PR is still in draft, and I will add the following as I have time:

Why?

Checklist

I have performed a self-review of my code.
I have added tests for my changes.
I have run linter locally (make lint) and all checks pass.
I have run tests locally (make tests) and all tests pass.
I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #4263

McMastS · 2025-04-07T23:02:58Z

Two minor TODOs are left, but the bulk of the PR is ready for review. Sorry for the big one!

I'm also looking into why the WebCrypto platform tests aren't running properly (see #4555 )

inancgumus

Thanks for your contribution 🙇 I have some suggestions and questions.

inancgumus · 2025-04-08T13:44:40Z

internal/js/modules/k6/webcrypto/subtle_crypto.go

+		if normalizeAlgorithmName == ECDH {
+			if err := ensureKeysUseSameCurve(privateKey, publicKey); err != nil {
+				return NewError(InvalidAccessError, err.Error())
+			}


What's the reason for using ensureKeysUseSameCurve only for ECDH? How does this change the previous behavior?

Previously, we only had ECDH algorithms going through the DeriveKey, which need that ensureKeysUseSameCurve function, but the X25519 keys always use the X25519 curve. Inside ensureKeysUseSameCurve we assert that the Algorithm types attached to the keys are both EcKeyAlgorithm but X25519 keys use the base Algorithm type.

Would a comment be helpful here to explain the new path?

Yes, a comment would be helpful 🙇

inancgumus · 2025-04-08T13:45:38Z

internal/js/modules/k6/webcrypto/tests/wpt-patches/WebCryptoAPI__generateKey__failures.js.patch

-+        // {name: "Ed25519",  resultType: "CryptoKeyPair", usages: ["sign", "verify"], mandatoryUsages: ["sign"]},
 +        // {name: "Ed448",    resultType: "CryptoKeyPair", usages: ["sign", "verify"], mandatoryUsages: ["sign"]},
-+        // {name: "X25519",   resultType: "CryptoKeyPair", usages: ["deriveKey", "deriveBits"], mandatoryUsages: ["deriveKey", "deriveBits"]},
+         {name: "X25519",   resultType: "CryptoKeyPair", usages: ["deriveKey", "deriveBits"], mandatoryUsages: ["deriveKey", "deriveBits"]},
+-        {name: "X448",     resultType: "CryptoKeyPair", usages: ["deriveKey", "deriveBits"], mandatoryUsages: ["deriveKey", "deriveBits"]},
 +        // {name: "X448",     resultType: "CryptoKeyPair", usages: ["deriveKey", "deriveBits"], mandatoryUsages: ["deriveKey", "deriveBits"]},


Since we implemented Ed25519 and X448, we can remove the commented-out lines.

Ed25519 and X25519 are uncommented in here (I had to read this in the split diff view, it's very hard to read otherwise). Is X448 implemented? I didn't implement it in this PR.

Sorry, I mean X25519, not X448. Ed25519 and X25519 are commented in the diff I see.

inancgumus · 2025-04-08T13:45:53Z

...rnal/js/modules/k6/webcrypto/tests/wpt-patches/WebCryptoAPI__generateKey__successes.js.patch

-+        // {name: "Ed25519",  resultType: "CryptoKeyPair", usages: ["sign", "verify"], mandatoryUsages: ["sign"]},
+        {name: "Ed25519",  resultType: "CryptoKeyPair", usages: ["sign", "verify"], mandatoryUsages: ["sign", "wrapKey"]},
 +        // {name: "Ed448",    resultType: "CryptoKeyPair", usages: ["sign", "verify"], mandatoryUsages: ["sign"]},
-+        // {name: "X25519",   resultType: "CryptoKeyPair", usages: ["deriveKey", "deriveBits"], mandatoryUsages: ["deriveKey", "deriveBits"]},
+         {name: "X25519",   resultType: "CryptoKeyPair", usages: ["deriveKey", "deriveBits"], mandatoryUsages: ["deriveKey", "deriveBits"]},
+-        {name: "X448",     resultType: "CryptoKeyPair", usages: ["deriveKey", "deriveBits"], mandatoryUsages: ["deriveKey", "deriveBits"]},
 +        // {name: "X448",     resultType: "CryptoKeyPair", usages: ["deriveKey", "deriveBits"], mandatoryUsages: ["deriveKey", "deriveBits"]},


Also, here, we can remove the commented-out lines.

inancgumus · 2025-04-08T13:48:49Z

Makefile

+build-debug:
+	go build  -gcflags="all=-N -l"
+


Do we need to keep this? I believe this was for debugging your code.

I found it helpful while running the debugger and thought others may find it useful, but I'm happy to remove if you want to keep the Makefile cleaner.

I think I'd prefer to remove this, as it looks like tool/use-case specific, like people versioning files like the configuration to run the debugger with VSCode. I understand both parties, and I'm no strong positioned, but if k6 have been kept with none of these, I'd prefer to keep it "clean".

I know in this case isn't exactly like that, because it's "purely Go", but for instance, in my particular case that I'm used to JetBrains IDEs and their debugger, I have never ever had the need to run this for debugging.

…rth the benefit

mstoykov · 2025-04-17T09:34:08Z

👋 Please rebase on latest master as #4701 is needed to see if the test are working.

Sorry for the inconvience 🙇

McMastS · 2025-04-17T17:24:01Z

👋 Please rebase on latest master as #4701 is needed to see if the test are working.

Sorry for the inconvience 🙇

Looks like there are a few test failures here that these tests have caught! Thanks for fixing that @mstoykov

mstoykov · 2025-04-18T11:18:24Z

Well that what tests are for :)

You can do it again as I decided to drop the need for buildign with tags. IME this helps a lot also with other tools as gopls wasn't really happy working with the tags being required. Unless you also set it up with those tags, which is extra steps.

joanlopez · 2025-05-15T11:39:30Z

Hi @McMastS,

I see some WPTs are still failing, while there has been no new activity on this PR/branch since almost a month.
Are you planning to continue this work? If not, I'd suggest closing the PR, and opening it again if anytime you're interested again in finishing this contribution.

Thanks! 🙇🏻

McMastS added 18 commits February 17, 2025 20:37

Add initial Ed25519 key generation code

1210307

Restore RSASsaPkcs1v15 to algorithm switch

be916e6

add sign/verify functionality for ed25519

48e5ba2

Update comment to reference Ed25519

266f040

Add import key functionality for Ed25519 keys

8746772

Merge branch 'master' into support-for-25519-algos

24f9816

Add support for Ed25519 raw format imports

2bf57f9

Add Ed25519 example files and fix bugs found

b6efce4

Generate x25519 keys

e704670

Revert x25519 changes

a1e258d

implement import and export for X25519

1a08b7f

implement derive bits for X25519

e178528

add X25519 JWK import export example file

1808ab8

fix linting errors

2743fd0

Merge branch 'master' into support-for-25519-algos

d902eef

Merge branch 'master' into support-for-25519-algos

ef92dab

Fix example file error

f41f22c

Fix reference error in import ed25519 example

8fe2658

McMastS marked this pull request as ready for review April 7, 2025 22:54

McMastS requested a review from a team as a code owner April 7, 2025 22:54

McMastS requested review from inancgumus and joanlopez and removed request for a team April 7, 2025 22:54

inancgumus reviewed Apr 8, 2025

View reviewed changes

McMastS added 3 commits April 9, 2025 10:49

use correct casing for Ed25519 identifier

9d27f22

verified that ed25519 library conducts small order checks

946cb32

remove todo to pass extractable as the refactoring required is not wo…

1705db1

…rth the benefit

McMastS requested a review from inancgumus April 9, 2025 17:16

Merge branch 'master' into support-for-25519-algos

8e13f8c

Merge branch 'master' into support-for-25519-algos

d01d1b9

McMastS added 2 commits April 18, 2025 19:13

remove debug command from Makefile

2fe95b7

fix failing WPT tests

aaf05be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for 25519 algos #4556

Support for 25519 algos #4556

McMastS commented Feb 18, 2025 •

edited

Loading

McMastS commented Apr 7, 2025

inancgumus left a comment

inancgumus Apr 8, 2025

McMastS Apr 8, 2025

inancgumus Apr 17, 2025

inancgumus Apr 8, 2025

McMastS Apr 8, 2025

inancgumus Apr 17, 2025 •

edited

Loading

inancgumus Apr 8, 2025

inancgumus Apr 8, 2025

McMastS Apr 8, 2025

joanlopez Apr 14, 2025

mstoykov commented Apr 17, 2025

McMastS commented Apr 17, 2025

mstoykov commented Apr 18, 2025

joanlopez commented May 15, 2025

Support for 25519 algos #4556

Are you sure you want to change the base?

Support for 25519 algos #4556

Conversation

McMastS commented Feb 18, 2025 • edited Loading

What?

Why?

Checklist

Related PR(s)/Issue(s)

McMastS commented Apr 7, 2025

inancgumus left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inancgumus Apr 17, 2025 • edited Loading

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

Choose a reason for hiding this comment

mstoykov commented Apr 17, 2025

McMastS commented Apr 17, 2025

mstoykov commented Apr 18, 2025

joanlopez commented May 15, 2025

McMastS commented Feb 18, 2025 •

edited

Loading

inancgumus Apr 17, 2025 •

edited

Loading