-
Notifications
You must be signed in to change notification settings - Fork 3
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
[3/3] Implement the encrypt
and decrypt
operations with support for AES algorithms
#32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oleiade Why are we doing it in this PR and not refactoring directly from the PR it is adding?
Hey @codebien I intend to address this in the 1/3 PR indeed, but wanted to finish this one before I iterate over the 1/1. Expect the CryptoKey refactor to go away indeed 👍🏻 |
9060976
to
b30f6cc
Compare
9d11db4
to
00ccbbf
Compare
@codebien The PR is now fully rebased on the PR chain and should be clean enough to review again, although I would recommend starting with 1/3 and 2/3 🙇🏻 (unless your strategy was to preempt 3/3 to help me gain some time as there are other reviewers for the previous PRs already, in which case, I'm grateful 😇 ) |
Yeah, it was already covered by 2 reviewers, I started where it was uncovered. |
d97e6d8
to
3adcc9f
Compare
9de2272
to
a2a2a71
Compare
d17809c
to
d7d680c
Compare
a2a2a71
to
f5585b0
Compare
d7d680c
to
98a08db
Compare
beb229a
to
cc22372
Compare
93c75d6
to
aebb22b
Compare
b1aeba2
to
56ef51c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have comments around the new traverseObject
, but we can move them in a new issue if you want.
56ef51c
to
032b36f
Compare
032b36f
to
38c0660
Compare
What this is
This PR implements to support the
encrypt
anddecrypt
operations. To keep Pull Requests as small and manageable as possible, It only implements support for the AES family of algorithms.main
✋🏻The
encrypt
operation allows encrypting a plaintext payload using an existingCryptoKey
object. Note that although the common nomenclature refers to them as "text", those actually consist inArrayBuffer
andTypedArray
in practice. The supported algorithms in this PR are only AES, and the supported AES ciphers are: CBC, CTR, and GCM as per the specification. Note that as a result of the Go standard library limitations, we had to bypass certain specification requirements: for instance, when using the GCM cipher, we only support iv sizes of 96bits, although the specs targets a much wider range. This is a result of the Go AES GCM implementation supporting 12 bytes nonces.The
decrypt
operation does the opposite. It allows decrypting a ciphertext into its original plaintext representation. Note that although the common nomenclature refers to them as "text", those actually consist inArrayBuffer
andTypedArray
in practice. Same as with encryption, the supported algorithms in this PR are only AES, and the supported AES ciphers are: CBC, CTR, and GCM as per the specification. For the same reasons as with encryption, we are supporting only a subset of the specs in certain areas.🟢 While the PR shows ~2000Loc changes, most of them are from the official test suites, which do not need to be reviewed, and the effective changes is more around ~600Loc 🟢
Review
Test suite
While the PR might look big at first glance, you can safely disregard and ignore the files in the
webcrypto/tests
folder. 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).References
This PR is the second 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.