Merge webcrypto into k6 #4188

olegbespalov · 2025-01-16T13:23:01Z

What?

Move xk6-webcrypto inside the k6 codebase

Why?

k6's WebCrypto soon will be promoted out of experimental, and as the team we decided that the codebase should be located in the k6

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)

https://github.com/grafana/xk6-webcrypto/

Part of #3154

Define module API to match WebCrypto's

The Web Crypto API specification has a pretty loose set of expectations towards errors. However, making our implementation pass the Web Platform Tests outlined that they're expected to be in a quite specific format. In order to provide a good support for errors in the Web Crypto API, we define its expected errors as custom errors.

Implement support for the crypto.getRandomValues operation

Implement support for the crypto.randomUUID operation

Implement support for the `crypto.subtle.digest` operation

The previous implementation of the normalization function was really thorough, complex, and close to the specification. Up to a level where the complexity was not necessarily worth it compared to the sought end result. This commit reduces the algorithm normalization to its bare minimum requirements.

The pre-existing CryptoKey type was intended as a contract-type declaration of type matching the specification's. It was not bound to any specific implementation, and as a result, not battle tested. This commit realigns its definition with the real-world use-case brought by the `subtle.crypto.generateKey` operation's implementation. The two main changes are the switch from a generic type to a `any` type for the `CryptoKey.handle` and `Cryptoattribute. This attribute is not exposed to the user, and is heavily contextual. We will in practice know from the context what type it is supposed to be castable to. The second change is to define a dedicated generic type for the algorithm field. There are a variety of Algorithm object types defined by the specs, and we need to be able to distinguish and export them to dedicated objects easily for them to be convinient to use.

js/modules/k6/webcrypto/cmd_run_test.go

inancgumus

I've reviewed as much as I could, but the PR is pretty large, so it wasn't a thorough review. I also found it a little unclear what was moved or added newly in this PR.

.github/workflows/wpt.yml

inancgumus · 2025-01-16T18:20:59Z

examples/webcrypto/README.md

+> [!IMPORTANT]
+> We do run the tests based on these examples, that's why we have a simple convention for each example:
+>
+> * It should do any `console.log`. Since we try to detect that output (log) contain `INFO` keyword.


What does "do" mean in this sentence: "It should do any console.log." I'm curious if it means "do" or "do not."

Suggested change

> * It should do any `console.log`. Since we try to detect that output (log) contain `INFO` keyword.

> * It should do any `console.log`. Since we try to detect that output (log) contains the `INFO` keyword.

In that contains that mean perform, like it's expected to have at least one console.log

I could fix this after merge if needed

inancgumus · 2025-01-16T18:22:30Z

js/modules/k6/webcrypto/tests/wpt-patches/WebCryptoAPI__derive_bits_keys__ecdh_bits.js.patch

+            // promise_test(function(test) {
+            //     return subtle.deriveBits({name: "ECDH", public: publicKeys[namedCurve]}, privateKeys[namedCurve], 8 * sizes[namedCurve] - 11)
+            //     .then(function(derivation) {
+            //         assert_true(equalBuffers(derivation, derivations[namedCurve], 8 * sizes[namedCurve] - 11), "Derived correct bits");
+            //     }, function(err) {
+            //         assert_unreached("deriveBits failed with error " + err.name + ": " + err.message);
+            //     });
+            // }, namedCurve + " non-multiple of 8 bits");


Should we consider removing that if it's not needed? I noticed there are some other parts like this commented out, but I didn't highlight all of them before understanding our reasons for keeping the code in the comments.

It's needed, it's commented because comments easier to uncomment once support will be there

olegbespalov · 2025-01-16T18:54:25Z

@inancgumus yeah, it's big, but don't worry from many aspects it's the code that has been already reviewed and this PR same as the PR of moving browser code

inancgumus

🚀

inancgumus · 2025-01-16T19:24:55Z

examples/webcrypto/README.md

+> [!IMPORTANT]
+> We do run the tests based on these examples, that's why we have a simple convention for each example:
+>
+> * It should do any `console.log`. Since we try to detect that output (log) contain `INFO` keyword.
+> * It should NOT `try/catch` exceptions. Since we try to detect if keywords like `"Uncaught"` and `"ERRO"` should not appear in the output (logs).


Based on your comment, I'd suggest the following for clarity, but I have no strong opinion. The current text is also fine, except for the typo.

Suggested change

> [!IMPORTANT]

> We do run the tests based on these examples, that's why we have a simple convention for each example:

>

> * It should do any `console.log`. Since we try to detect that output (log) contain `INFO` keyword.

> * It should NOT `try/catch` exceptions. Since we try to detect if keywords like `"Uncaught"` and `"ERRO"` should not appear in the output (logs).

> [!IMPORTANT]

> We do run the tests based on these examples; that's why we have a simple convention for each example:

>

> * Success condition: Example SHOULD output an `INFO` keyword using `console.log` at least once.

> * Failure condition: Example SHOULD NOT `try/catch` exceptions so that we can detect failures. The log output SHOULD NOT contain any keywords like `"Uncaught"` and `"ERRO"`.

joanlopez · 2025-01-17T09:32:24Z

.github/workflows/wpt.yml

@@ -18,9 +18,16 @@ jobs:
        with:
          go-version: 1.23.x
          check-latest: true
-      - name: Run tests
+      # TODO: combine WebPlatform tests checkout & patch into the singe step


Suggested change

# TODO: combine WebPlatform tests checkout & patch into the singe step

# TODO: combine WebPlatform tests checkout & patch into the single step

joanlopez · 2025-01-17T09:33:29Z

.gitignore

@@ -34,3 +34,6 @@ script.js

 /packaging/.env
 /packaging/*.gpg
+
+# web platform tests
+js/modules/k6/webcrypto/tests/wpt/


Nit; I'd move this line next to the other one related with the WPT tests.

oleiade and others added 30 commits October 7, 2022 12:12

Initial commit

a2d8b19

Add work in progress note to README

d296de7

Add unimplemented API for the module

2a95d96

Add tiny usage example of the digest function

9926f57

Merge pull request #1 from grafana/interface

fbd28a2

Define module API to match WebCrypto's

Add testing helpers to facilitate running k6 code in tests

60dc660

Address styling issue in module.go

4f563bf

Add Goja helpers for type checking

e9e9709

Implement the algorithm nomalization algorithm

3ce87a3

Improve algorithms definition and normalization

8bc1f65

Improve and make CryptoKey* generic

056c2f6

Improvements to test setup

5837a20

Remove placeholder implementation of digest

c05ac54

Move Elliptic Curve type aliases to dedicated file

07a1913

Update module dependencies

5fd1a24

Add golangci-lint configuration and fix warnings

bd46bee

Add test_setup helpers

57fafcc

Add Github Actions workflows

8817745

Implement support for the crypto.getRandomValues operation

5c1e92e

address @iMRIC comments

a01811f

Merge pull request #21 from grafana/feature/getRandomValues

d44d97e

Implement support for the crypto.getRandomValues operation

Implement support for the crypto.randomUUID operation

e498b07

Merge pull request #22 from grafana/feature/randomUUID

da963cf

Implement support for the crypto.randomUUID operation

Implement support for the crypto.digest operation

813000a

Merge pull request #23 from grafana/feature/digest

cacea29

Implement support for the `crypto.subtle.digest` operation

Upgrade to k6 v0.43.1 dependency

301bb04

Implement subtle.crypto.generateKey with support for AES algorithms

ddeda5c

olegbespalov added 2 commits January 16, 2025 13:59

webcrypto: wpt finalize

d3710c1

webcrypto: omit linter

2d638ae

olegbespalov self-assigned this Jan 16, 2025

olegbespalov added dependencies Pull requests that update a dependency file area: webcrypto labels Jan 16, 2025

olegbespalov marked this pull request as ready for review January 16, 2025 16:12

olegbespalov requested a review from a team as a code owner January 16, 2025 16:12

olegbespalov requested review from inancgumus and joanlopez and removed request for a team January 16, 2025 16:12

olegbespalov changed the title ~~WebCrypto promotion to k6~~ Merge webcrypto into k6 Jan 16, 2025

olegbespalov added this to the v0.57.0 milestone Jan 16, 2025

olegbespalov requested a review from mstoykov January 16, 2025 16:28

olegbespalov mentioned this pull request Jan 16, 2025

Derive Key grafana/xk6-webcrypto#83

Draft

mstoykov reviewed Jan 16, 2025

View reviewed changes

js/modules/k6/webcrypto/cmd_run_test.go Outdated Show resolved Hide resolved

inancgumus reviewed Jan 16, 2025

View reviewed changes

webcrypto: minor improvements

1a4aa18

olegbespalov requested review from inancgumus and mstoykov January 16, 2025 19:23

inancgumus approved these changes Jan 16, 2025

View reviewed changes

mstoykov approved these changes Jan 17, 2025

View reviewed changes

joanlopez reviewed Jan 17, 2025

View reviewed changes

joanlopez approved these changes Jan 17, 2025

View reviewed changes

olegbespalov merged commit d090c26 into master Jan 17, 2025
28 of 29 checks passed

olegbespalov deleted the feat/webcrypto-merge branch January 17, 2025 12:37

This was referenced Jan 17, 2025

Making the k6/experimental/webcrypto module part of the k6 core #3154

Open

Warning that repository in no longer maintained grafana/xk6-webcrypto#101

Merged

Clean up after merging webcrypto & making module globally avaliable #4278

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge webcrypto into k6 #4188

Merge webcrypto into k6 #4188

olegbespalov commented Jan 16, 2025 •

edited

Loading

inancgumus left a comment

inancgumus Jan 16, 2025 •

edited

Loading

olegbespalov Jan 16, 2025

olegbespalov Jan 16, 2025

inancgumus Jan 16, 2025 •

edited

Loading

olegbespalov Jan 16, 2025

olegbespalov commented Jan 16, 2025

inancgumus left a comment

inancgumus Jan 16, 2025

joanlopez Jan 17, 2025

joanlopez Jan 17, 2025

	> * It should do any `console.log`. Since we try to detect that output (log) contain `INFO` keyword.
	> * It should do any `console.log`. Since we try to detect that output (log) contains the `INFO` keyword.

	# TODO: combine WebPlatform tests checkout & patch into the singe step
	# TODO: combine WebPlatform tests checkout & patch into the single step

Merge webcrypto into k6 #4188

Merge webcrypto into k6 #4188

Conversation

olegbespalov commented Jan 16, 2025 • edited Loading

What?

Why?

Checklist

Related PR(s)/Issue(s)

inancgumus left a comment

Choose a reason for hiding this comment

inancgumus Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

olegbespalov Jan 16, 2025

Choose a reason for hiding this comment

olegbespalov Jan 16, 2025

Choose a reason for hiding this comment

inancgumus Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

olegbespalov Jan 16, 2025

Choose a reason for hiding this comment

olegbespalov commented Jan 16, 2025

inancgumus left a comment

Choose a reason for hiding this comment

inancgumus Jan 16, 2025

Choose a reason for hiding this comment

joanlopez Jan 17, 2025

Choose a reason for hiding this comment

joanlopez Jan 17, 2025

Choose a reason for hiding this comment

olegbespalov commented Jan 16, 2025 •

edited

Loading

inancgumus Jan 16, 2025 •

edited

Loading

inancgumus Jan 16, 2025 •

edited

Loading