[1/3] Implement generateKey with support for AES algorithms
#30
Conversation
There was a problem hiding this comment.
LGTM, great work all around! 馃憦 Just left minor suggestions.
A couple of general thoughts:
-
I would've preferred to see the refactoring commits in a separate PR to make reviewing easier, but it's not a big deal.
-
I suppose key generation is currently not that useful, even after they're exportable in [2/3] Implement the
exportKeyandimportKeyoperations with support for AES algorithms聽#31, since they can't be written to local disk without an extension like xk6-file. We are planning to make this possible via the newly planned File API, right? Until then, users would have to transfer them over the network, which is not ideal.
There was a problem hiding this comment.
LGTM in general.
I would argue this is mostly blocked on the current access to goja values off the event loop.
The majority of my other comments can be done in later PRs and arguably are about the tests that IMO should follow a lot more what we do for tc39:
- we check out the original test (from a specific version)
- try to run them with as little changes as possible
- if needed (and possible) edit them in some codified way. Either directly before running them or thrugh
sed/edor applying.patchfiles
This likely will take a few tries and might a bit hard to get right, so it likely is better as separate PR. But if you have time this likely should be done before we continue to add functionality
| // Expected error is either a number, tested against the error code, | ||
| // or a string, tested against the error name. | ||
| function testError(algorithm, extractable, usages, expectedError, testTag) { | ||
| return crypto.subtle.generateKey(algorithm, extractable, usages) |
There was a problem hiding this comment.
Here you removed promise_test invocation.
Wouldn't it better if this is not the case and we actually make promise_test work?
This might be done after some initial PR merging, but I do really prefer if we are checking ou the actual tests and running them without changes as much as possible.
If we need to change something, I guess it might be better to have the changes scripts with something like ed/sed or .patch files so those are better handled.
There was a problem hiding this comment.
I gave it at the very beginning, but if I remember correctly, promise_test depended on quite some other dependencies, and the whole thing was rapidly becoming exponential (forcing me to also import stuff that either doesn't work, or is irrelevant to our specific subset of the test suite).
As I ported the tests, when they failed, I wish I had promise_test actually. Thus, I'll give another try at importing it and report here 馃憤馃徎
There was a problem hiding this comment.
I gave another try at porting the original promise_test, but failed. The reason is that it relies on browser constructs, and on a Test class which itself relies on a bunch of global state that we do not have access to, because we do not run inside the larger WebPlatform tests setup.
I could try to scrap most of that behavior, just so we have a promise_test that does roughly the same thing, but doesn't have the same code though, like I did for the assert functions.
oleiade
force-pushed
the
feature/generateKey-symmetric
branch
3 times, most recently
from
781c2a6
to
68abb91
Compare
|
|
@mstoykov regarding the test suite, in an ideal world, I'd love to do that indeed. I even attempted it at the beginning, but unfortunately the reality check quickly hit, hard. The WebPlatformTests are huge, filled with global states, and dependencies to browsers. The effort to be able to do that would be Cornelian, and if we were to attempt it, I would definitely do it as a separate, non-immediate PR. If you've got the time, and energy in the future to support me with trying it, that would much appreciated too 馃憤馃徎 |
oleiade
force-pushed
the
feature/generateKey-symmetric
branch
from
68abb91
to
a2cd5eb
Compare
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.
oleiade
force-pushed
the
feature/generateKey-symmetric
branch
from
a2cd5eb
to
34fa553
Compare
|
As requested in #31 I added a README file with more information regarding the current state of supported operations and algorithms 馃檱馃徎 |
There was a problem hiding this comment.
LGTM!
I would prefer to merge PRs and fix random stylistic problems later, as we are definitely going to be rewrrting this quite a bit as we add more and more functionality. But we need something to build on top.
And as was experienced - building everything in one go makes things even worse.
A stylistic note: In some places you use switch to check that a variable has one of many values, in some placse you use isSomething variables and and if.
For the majority of this particular cases I prefer the switch variant. It is both more concise and IMO is easier to read in almost all cases except the ones where for example we are counting bits. But even there it likely is good enough and we can have inline comment to help with that.
|
@mstoykov thanks for your feedback, I'm on your side with addressing stylistic issues separately. Regarding switch vs if/is I'm also on board. I'll keep in mind to be more consistent moving forward, and also expect that I will naturally rewrite most of the ifs as switch statements as more algorithms are supported 馃憤馃徎 |
oleiade
force-pushed
the
feature/generateKey-symmetric
branch
from
575c685
to
86e837f
Compare
What this is
This PR implements to support the
subtle.crypto.generateKeyoperation. To keep Pull Requests as small and manageable as possible, It only implements support for the AES family of algorithms.The operation allows the generation of a new random cryptographic key, which can later be used to perform encryption/decryption, or signature/verification operations (implemented in further PRs).
馃煝 While the PR shows +1000 changes, 700 of them are from the official test suites, which do not need to be reviewed 馃煝
Review
Test suite
While the PR might look big at first glance, you can safely disregard and ignore the files in the
webcrypto/testsfolder. Those files are directly imported and slightly adapted from the official test suite so that they are runnable in k6, but we haven't written those originally. We won't make any functional changes to them, as they are our safety-net, ensuring that our implementation matches the specifications expectation (and that what we do is 馃 safe).Implementation
We have revamped quite a bit of the initial types, and data structure laid out during the first phases of the project. We decided to rely much more on
goja.Valueat the interface level than before and have refactored our types to limit their use of generics to where they were strictly useful/necessary/convenient.References
This PR is the first in a chain of 3 implementing support for key generation (1.), import/export (2.), and encryption/decryption using them. We intend to merge these in order in master: 1->2->3.