Skip to content
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.

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

Derive Key #83

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft

Derive Key #83

wants to merge 26 commits into from

Conversation

catdevman
Copy link

No description provided.

@catdevman catdevman requested a review from a team as a code owner July 16, 2024 15:51
@catdevman catdevman requested review from mstoykov and olegbespalov and removed request for a team July 16, 2024 15:51
@CLAassistant
Copy link

CLAassistant commented Jul 16, 2024

CLA assistant check
All committers have signed the CLA.

@catdevman catdevman marked this pull request as draft July 16, 2024 16:55
@catdevman
Copy link
Author

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

@mstoykov mstoykov requested review from oleiade and removed request for mstoykov September 26, 2024 07:24
@@ -178,6 +187,8 @@ func isRegisteredAlgorithm(algorithmName string, forOperation string) bool {
return isAesAlgorithm(algorithmName)
case OperationIdentifierSign, OperationIdentifierVerify:
return algorithmName == HMAC || algorithmName == ECDSA
case OperationIdentifierDeriveBits, OperationGetKeyLength:
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

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
Copy link
Member

oleiade commented Sep 26, 2024

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:

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
Copy link
Author

catdevman commented Oct 16, 2024

@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

@oleiade
Copy link
Member

oleiade commented Oct 17, 2024

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
Copy link
Author

catdevman commented Oct 19, 2024

@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
Copy link
Author

@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
Copy link
Author

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

@oleiade
Copy link
Member

oleiade commented Oct 21, 2024

@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
Copy link
Contributor

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
Copy link
Author

Yes that is very helpful explain @olegbespalov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants