Derive Key

[catdevman]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[catdevman]

@oleiade I have sat down to write this a couple times and seems that I haven't gotten it all out at once... so here I go again.

I have been looking at the implementation in node written in c++ to get an idea of what implementation of the spec looks like. I believe I understand what the spec desires to be implemented but I have a few questions still:

• How to implement the spec per type within the framework that already be created in this repo
• I also believe that I need some additional understanding of what util functions might be required to implement this for ECDH, HKDF as well as PBKDF2 (which I believe are the ones that are needed). Specifically it looks like getKeyLength which seems to return null for HKDF and PBKDF2 which lead me to questions about be implementation for this in go. I think it could be pointers and returning nil but wanted to get others thoughts.

For the moment those are the main 2 but I think others might present themselves in discussion of these.

[oleiade]

Shouldn't it be OperationIdentifierDeriveKey instead? 🙇‍♂️ (note that maybe deriveKey might rely on the bits derivation under the hood and I'm not just aware of it)

[catdevman]

Yeah that is what I found the step 2 in the spec here for deriveKey requires deriveBits. So I think the issues for deriveBits need worked on first before I can continue so that is my current side mission 😄 I also realized that step 6 requires a "get key length" which was the util function I was referring to in my part 2 of the question from the other day. I think that is also going to be needed for the deriveBits issues so 2 birds 1 stone type of thing going on. Either way I am fairly sure that function will need to be our own implementation because I am not seeing it in any of the libraries that you mentioned.

[oleiade]

Hi @catdevman 👋🏻

This looks good and as if it's going in the right direction, great work so far 👏🏻

1

If I understood your question correctly (feel free to clarify if you think I did not 🙂):, I believe we've been doing this to have the same types as described in the specs and the operations described on them as methods.

For instance, the AESCBCParams implements the encrypt and decrypt operations defined by the spec as methods. The general idea is to try to stick to what the spec describes as closely as possible. In these cases, I believe the initial intent was to avoid having super-long function names.

2

Regarding implementing algorithms, please do not implement those yourselves. This library has a strict policy to stick to either the standard library or a proven set of community libraries.

Here are the libraries I recommend you use:

• HKDF
• ECDH
• PBKDF2

I have also needed clarification in the initial implementation by how to match the algorithms the specification defines, and how to stick to it. After consulting with some of Grafana's security team members, my key learning is that, in this case, one does not need to understand the steps of each algorithm but rather match the algorithm described to a concrete operation in an existing library implementation of the algorithm.

For instance, where the specification might enter into details about the different steps of AES encryption and include those in their steps, we, as implementers, can most likely stick to the encrypt operation of the AES go package to perform all those operations in one go. The same goes for other algorithms.

If you end up implementing the concrete steps of ECDH key derivation, you're most likely doing something wrong and should instead look into figuring out how to use one of the ECDH package's operations directly instead 🙂

Moving on

Let me know if this is helpful, and don't hesitate to ask further questions, happy to help 🙇🏻

[catdevman]

@oleiade Do you think you could help me come up with some test use cases for deriveKey and deriveBits?

I started with one here(but I don't think that is quite right and the spec hasn't really helped me figure out what my test should look like):
examples/derive_key/derive-key-pbkdf2.js

[catdevman]

oh never mind I just found some good ones from the node repo.

https://github.com/nodejs/node/blob/73414f34e8f3e1f4dac3bb35e137c4030afb4267/test/fixtures/wpt/WebCryptoAPI/derive_bits_keys/ecdh_bits.js#L6-L28

https://github.com/nodejs/node/blob/73414f34e8f3e1f4dac3bb35e137c4030afb4267/test/fixtures/wpt/WebCryptoAPI/derive_bits_keys/hkdf_vectors.js#L31

https://github.com/nodejs/node/blob/73414f34e8f3e1f4dac3bb35e137c4030afb4267/test/fixtures/wpt/WebCryptoAPI/derive_bits_keys/pbkdf2_vectors.js#L30

[oleiade]

Hey @catdevman 👋🏻

We actually use an existing and official set of tests for testing the WebCrypto module implementation: https://github.com/web-platform-tests/wpt

More specifically, we need the implementation of deriveKey and deriveBits to pass this specific test suite: https://github.com/web-platform-tests/wpt/tree/0b9590a78d353217ae0bc6321ecc456f2da197ec/WebCryptoAPI/derive_bits_keys
(notice the hash in the URL is important as those tests change often and all our existing ones are based on the same version of them, I believe (correct me if I'm wrong and we'll advise)).

If you take a look at the webcrypto/tests folder you'll see that for most methods we have imported the test files, and adapted them to k6. The Web Platform Tests have a lot of helpers and mechanisms specific to them that we either don't need or don't have available in k6 itself. I recommend you take a look at the existing tests and try to stick to the same strategy and helpers we have defined there.

The concrete steps in your case would probably look like the following:

1. Create a derive_bits_key folder in webcrypto/tests
2. For each file in: https://github.com/web-platform-tests/wpt/tree/0b9590a78d353217ae0bc6321ecc456f2da197ec/WebCryptoAPI/derive_bits_keys
   • Copy it in the folder created above
   • Adapt the test file using our helpers located in the webcrypto/tests/util folder. If any function the test itself defines that's mandatory for you to use in the test, feel free to port it in the util folder existing files, or a new one if it doesn't exist yet.
   • Run the tests (if my memory is correct, the Go package tests are setup to auto-detect every test files, so if you add any they would be picked up automatically)
3. Repeat until complete

Once all (if not most) of the tests pass, we'll be good to go 👍🏻

I'll admit this can be somewhat tedious work, so don't hesitate to let us know if we can help in any way, along the implementation. Happy to help, and even jump on a call if that's useful at any point in time 🙇🏻

[catdevman]

@oleiade Can you also share how to run these tests. I have been do go test ./... (sometimes adding a -v when needed) should I just be running make test instead?

[catdevman]

@oleiade I edited my above comment... but from what I can tell the tests run from the examples directory and none of the js files in the webcrypto/tests get ran at all. Any additional help you can give me on the where would be help as well... I am looking at the only go test file and seeing:

// it check that output contains/not contains cetane things
// it's not a real test, but it's a good way to check that examples are working
// between changes
//
// We also do use a convention that successful output should contain `level=info` (at least one info message from console.log), e.g.:
// INFO[0000] deciphered text == original text:  true       source=console
// and should not contain `level=error` or "Uncaught", e.g. outputs like:
// ERRO[0000] Uncaught (in promise) OperationError: length is too large  executor=per-vu-iterations scenario=default
func TestExamplesInputOutput(t *testing.T) {
	t.Parallel()

	outputShouldContain := []string{
		"output: -",
		"default: 1 iterations for each of 1 VUs",
		"1 complete and 0 interrupted iterations",
		"level=info", // at least one info message
	}

	outputShouldNotContain := []string{
		"Uncaught",
		"level=error", // no error messages
	}

	// List of the directories containing the examples
	// that we should run and check that they produce the expected output
	// and not the unexpected one
	// it could be a file (ending with .js) or a directory
	examples := []string{
		"../../examples/digest.js",
		"../../examples/getRandomValues.js",
		"../../examples/randomUUID.js",
		"../../examples/generateKey",
		"../../examples/derive_bits",
		"../../examples/derive_key",
		"../../examples/encrypt_decrypt",
		"../../examples/sign_verify",
		"../../examples/import_export",
	}

	for _, path := range examples {
		list := getFiles(t, path)

		for _, file := range list {
			name := filepath.Base(file)
			file := file

			t.Run(name, func(t *testing.T) {
				t.Parallel()

				script, err := os.ReadFile(filepath.Clean(file)) //nolint:forbidigo // we read an example directly
				require.NoError(t, err)

				ts := getSingleFileTestState(t, string(script), []string{"-v", "--log-output=stdout"}, 0)

				cmd.ExecuteWithGlobalState(ts.GlobalState)

				stdout := ts.Stdout.String()

				for _, s := range outputShouldContain {
					assert.Contains(t, stdout, s)
				}
				for _, s := range outputShouldNotContain {
					assert.NotContains(t, stdout, s)
				}

				assert.Empty(t, ts.Stderr.String())
			})
		}
	}
}

[catdevman]

For now I will add them into the examples directory just to get moving but let me know if that isn't correct

[oleiade]

@catdevman

Can you also share how to run these tests. I have been do go test ./... (sometimes adding a -v when needed) should I just be running make test instead?
go test -v ./... is the way indeed 🙇🏻

Regarding the tests directory, my apology, my knowledge of the code was a bit rusty indeed. After looking into it, it turns out that at the top-level of the package, our _test.go test files define test cases which directly run against the test files. See subtle_crypto_test.go for instance; where we define a TestSubtleDigest function, which in turns uses the CompileFile helper to compile the test file and run it.

Can you give a shot to this approach for dervive key, and let me know if that works out? 🙇🏻

[olegbespalov]

Hey @catdevman !

Just a bit of context for tests in this repository.

We mostly use two types of tests here.

The first is against the WebAPI test suite, which aims to test the compatibility of k6's implementation of the WebCrypto API with the standard. It's the one that @oleiade mentioning here.

However, it usually isn't ideal as a standard. Unfortunately, the WebAPI test suite didn't cover all the cases we had, especially considering that K6 was specific. That's why we came up with the idea of golden tests, which run against the examples and validate that they work (or at least do not throw exceptions) with every commit.

Hope that explains!

[catdevman]

Yes that is very helpful explain @olegbespalov

[olegbespalov]

@catdevman please be aware of #85

[catdevman]

@catdevman please be aware of #85

Thanks for the heads up, I'll check out that and see if I can gain some nuggets of wisdom. That'll be a fun one to resolve looks like a lot of the same files get touched 😂

[olegbespalov]

One more update here 🙈 since it was a massive day for the webcrypt 😅

That'll be a fun one to resolve looks like a lot of the same files get touched 😂

So, likely, as you can see, the only two files have conflicts. And I don't think that they are complicated to resolve. However, I'd like to comment on another change chat affects the PR/branch.

First, recently there was a change in k6 that forced us to update the signatures and adjust the way how we handle resolve/reject, see for details #88

Second, I hope it would be more beneficial for the external contributors if we stopped embedding the static web platform test files (that we manually adjusted) into the repository. Instead, we would do the checkout part of the repo and apply some patches. Looking at patch files, you can learn key differences between k6 runtime and what should be adjusted. It is always clear what the difference is.

These changes you can find in:

• Web Platform Tests Checkout #87
• Web Platform Tests part 2 #90 (this is not yet merged, but probably will be soonish)

If you have any questions, feel free to ping me. I could try to help!

[catdevman]

Am I free to start on this again once I pull in the changes?

[olegbespalov]

Hi @catdevman

Thanks for returning to us and checking!

One possible inconvenience that I see that inside the @grafana/k6-core team, we decided to graduate the WebCrypto API module and back merge the code to the k6 core (https://github.com/grafana/k6 repository) the issue is grafana/k6#3154. We'll preserve the git history, like we did for the browser module (grafana/k6#4056), but still, all paths will be updated and the contribution PR should be open against grafana/k6.

We don't have a cetane ETA for that change, but hopefully, I'll be able to open and merge this PR this/next week. So once I'll do that, I can comment and update you again. How does that sound?

[catdevman]

@olegbespalov that's perfect, shouldn't be hard to make a patch from what I've done and update the paths inside the patch so they're correct. Let me know when the promotion is done and I'll take another crack at it.