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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for the crypto.getRandomValues operation #21

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Nov 3, 2022

This PR addresses #19

It adds an implementation for the crypto.getRandomValues operation to the extension.

import { crypto } from "k6/x/webcrypto";

export default function () {
  const array = new Uint32Array(10);
  crypto.getRandomValues(array);

  for (const num of array) {
    console.log(num);
  }
}

The underlying implementation depends on the Go rand/crypto.Read method as a PRNG. This standard functionality relies on the underlying OS' pseudo-random number generator, such as /dev/urandom on Linux.

This implementation suffers a few limitations that shouldn't be a deal-breaker, but for which I couldn't find any acceptable workaround at this day:

  • The specification indicates that only certain views (Int8Array, Uint8Array, Int16Array, Uint16Array, Int32Array, Uint32Array or UInt8ClampedArray) on the underlying ArrayBuffer should be accepted. I couldn't figure out how to do that in Goja. I'd be really grateful if you have any idea on that front 馃檱馃徎
  • The specification indicates that certain specific errors such as QuotaExceededError should be returned under certain conditions. At the moment I have defined specific errors the Go way, and I throw them using common.Throw; but that doesn't allow me to, for instance, switch on them in the context of a catch statement, as in:
switch (err) {
  case (err isinstanceof QuotaExceededError) {

Let me know what you think 馃檱馃徎

@oleiade oleiade self-assigned this Nov 3, 2022
@oleiade oleiade added the enhancement New feature or request label Nov 3, 2022
@oleiade oleiade force-pushed the feature/getRandomValues branch 3 times, most recently from 20931bc to 18a8241 Compare November 3, 2022 15:25
@oleiade oleiade force-pushed the feature/getRandomValues branch 4 times, most recently from 6604769 to 988b42d Compare November 21, 2022 08:53
webcrypto/crypto.go Outdated Show resolved Hide resolved
webcrypto/crypto.go Outdated Show resolved Hide resolved
webcrypto/crypto.go Outdated Show resolved Hide resolved
@oleiade oleiade requested a review from imiric November 28, 2022 09:46
@oleiade oleiade force-pushed the feature/getRandomValues branch 2 times, most recently from 82f7022 to 6085a79 Compare November 28, 2022 12:23
Copy link

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM 馃憦 Just left some nitpicks.

As for the limitations you mentioned, I see you've resolved the first one. 馃憤

I think the second one is possible, but you'd need to first make the error object constructor available to the goja VM, and then use it in Go to create an instance. I took a glance at goja tests, but my goja-fu is not great, so I summon the wizardry of @mstoykov.

But technically, these are DOMException errors, and exposing that wouldn't make sense for k6. We could expose them as separate constructors I guess, and choose to not follow the spec slightly. I think that would be better than the currently thrown plain object. Let's maybe create an issue for it and fix it later?

Comment on lines 38 to 57
var (
isInt8Array = IsInstanceOf(c.vu.Runtime(), typedArray, Int8ArrayConstructor)
isUint8Array = IsInstanceOf(c.vu.Runtime(), typedArray, Uint8ArrayConstructor)
isUint8ClampedArray = IsInstanceOf(c.vu.Runtime(), typedArray, Uint8ClampedArrayConstructor)
isInt16Array = IsInstanceOf(c.vu.Runtime(), typedArray, Int16ArrayConstructor)
isUint16Array = IsInstanceOf(c.vu.Runtime(), typedArray, Uint16ArrayConstructor)
isInt32Array = IsInstanceOf(c.vu.Runtime(), typedArray, Int32ArrayConstructor)
isUint32Array = IsInstanceOf(c.vu.Runtime(), typedArray, Uint32ArrayConstructor)
)

// 1.
if !isInt8Array &&
!isUint8Array &&
!isInt16Array &&
!isUint16Array &&
!isInt32Array &&
!isUint32Array &&
!isUint8ClampedArray {
common.Throw(c.vu.Runtime(), NewError(0, TypeMismatchError, "typedArray parameter isn't a TypedArray instance"))
}
Copy link

@imiric imiric Nov 29, 2022

Choose a reason for hiding this comment

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

What do you think about looping over the valid types instead? This way you can short-circuit and avoid the unnecessary IsInstanceOf() calls if a valid type is found. A negligible optimization, to be sure, but it's slightly neater IMO.

Consider:

func (c *Crypto) validateTypedArray(val goja.Value) bool {
	validTypes := []JSType{
		Int8ArrayConstructor, Uint8ArrayConstructor, Uint8ArrayConstructor,
		Uint8ClampedArrayConstructor, Int16ArrayConstructor, Uint16ArrayConstructor,
		Int32ArrayConstructor, Uint32ArrayConstructor,
	}

	var valid bool
	for _, t := range validTypes {
		if valid = IsInstanceOf(c.vu.Runtime(), val, t); valid {
			break
		}
	}

	return valid
}

Since GetRandomValues() is already broken up in steps, it might make it a bit more readable to turn each step into a separate smaller unexported method (or maybe just the validation?). Technically, step 2 below is also part of validation, so you might want to return an error from the validate method depending on if it fails the type or length validation. That way you can call common.Throw() only once here.

None of this is a big deal, so feel free to disregard if you prefer it the way it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot for this, it sparked new ideas on how to approach it indeed 馃檱馃徎 What I ended up doing was:

Regarding the structure of the function itself, I'd rather keep it as is. At least during the first implementation run. Considering the small scope of the function, I find it more convenient to track the implementation's correctness to have inlined and mirroring the specs. I find that splitting in smaller functions will prove useful in later functionalities (encrypt, decrypt, etc), but for the functions in the crypto namespace, I would judge it overkill abstraction for now 馃憤馃徎

webcrypto/crypto_test.go Outdated Show resolved Hide resolved
@mstoykov
Copy link
Contributor

The specification indicates that certain specific errors such as QuotaExceededError should be returned under certain conditions. At the moment I have defined specific errors the Go way, and I throw them using common.Throw; but that doesn't allow me to, for instance, switch on them in the context of a catch statement, as in:

AFAIK this will require that you actually define this error and then make instances of them.

@oleiade oleiade merged commit d44d97e into main Dec 8, 2022
@oleiade oleiade deleted the feature/getRandomValues branch December 8, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants