-
Notifications
You must be signed in to change notification settings - Fork 94
feat(crypto): add encrypt and decrypt APIs #491
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #491 +/- ##
==========================================
- Coverage 36.14% 35.84% -0.31%
==========================================
Files 82 85 +3
Lines 9927 10047 +120
Branches 371 394 +23
==========================================
+ Hits 3588 3601 +13
- Misses 6280 6387 +107
Partials 59 59
|
|
Hi @ItalyPaleAle ! First of all, thank you so much for contributing this, it is shaping up to be an amazing PR!! As for my thoughts, I do have some questions:
(adding your example below to illustrate what you proposed in the PR here more easily) await pipeline(
await client.crypto.encrypt(createReadStream("plaintext.txt"), {
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),
createWriteStream("ciphertext.out"),
);
// Decrypt the message
await pipeline(
await client.crypto.decrypt(createReadStream("ciphertext.out"), {
componentName: "crypto-local",
}),
createWriteStream("plaintext. Out"),
); |
|
Hi @XavierGeerinck thanks for the initial review. On subtle APIs:
Because of the above, my suggestion would be to keep the subtle crypto APIs (when they are implemented for Dapr 1.12 or later) into a separate object, for example
Yes, this is how the high-level encrypt/decrypt methods are meant to use. Except they're not just for files: anything that is a Readable stream can be encrypted, including The current API design mirrors what's implemented with the Go SDK: first parameter is a Readable stream with the input, and the result is a (Promise that resolves with) a Readable stream as output. This requires using something like the above: await pipeline(
await client.crypto.encrypt(createReadStream("plaintext.txt"), {
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),
createWriteStream("ciphertext.out"),
);Where the const cryptoStream = await client.crypto.encrypt(createReadStream("plaintext.txt"), {
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
})
const outStream = createWriteStream("ciphertext.out")
cryptoStream.pipe(outStream)
// Plus code to `await` on cryptoStream's endI am thinking that for JS, maybe this could be changed to use a Duplex stream, where data is written into the writable part of the stream, and the result is read from the readable part. This would allow for something like this: await pipeline(
createReadStream("plaintext.txt"),
// Note "await" is missing here; errors are returned into the stream
client.crypto.encrypt({
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),
createWriteStream("ciphertext.out"),
);This seems nicer when used with Another thing to consider specifically for JS is perhaps we could offer a wrapper around const message: = Buffer.from(...)
const encrypted: Buffer = client.crypto.encrypt(
message,
{
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),(In this case I'm suggesting the use of overloading rather than a separate method, but a separate method could be fine too) In this case, internally we still use streams, but users don't need to worry about them. Of course this is not efficient especially when encrypting large files, but it removes the pain that it is to work with streams in JS. |
|
Thanks for adding your extensive remarks! I had a read on the proposal document as well to try to grasp better on what is happening here. Your latest remarks is what sums it up for me personal (looking at @shubham1172 and @DeepanshuA their opinion as well): "user's shouldn't worry about how things work". This is the vision of the JS SDK specifically as well, providing value over the normal Dapr APIs to ensure that everything is done for the user behind the scenes. Looking at your last example, I think this is shaping up to be an API I would love to expose to the users. As I fear that the "streaming" implementation that is being exposed to users will be either to complex or difficult to explain in documentation. Boiling it down in use cases, this is what I could think of (please feel free to add more use cases):
Using those, I would like to refrain from having the stream API as the main entry point for users. Thus your example below: const message: = Buffer.from(...)
const encrypted: Buffer = client.crypto.encrypt(
message,
{
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),would definitely be the most interesting for me! Could we add some tests around how this API could be used to support:
Let me know what you think! |
|
Thanks for your thoughts @XavierGeerinck Yes, what you list there is correct, but many (most?) of those things require the subtle APIs, which are currently not available. However, what you say about designing this to be "non-stream" first makes sense. I have a complicated relationship with streams myself, as I both (came to) love them and hate them as they're not very easy to use :) I can update the APIs. What do you think about making this overloaded? I'm using // This variant uses buffers
// Buffer = Node.js buffer
// BufferSource = ArrayBuffer or a view (like Uint8Array)
// string = if passed, will be interpreted as UTF-8
encrypt(data: Buffer | BufferSource | string, opts: EncryptRequest): Promise<Buffer>;
// This variant uses streams
// Duplex = duplex streams
encrypt(opts: EncryptRequest): Promise<Duplex>;The version with duplex streams can be used as: await pipeline(
createReadStream("plaintext.txt"),
// Note "await" is missing here; errors are returned into the stream
client.crypto.encrypt({
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),
createWriteStream("ciphertext.out"),
); |
|
What would you think about making an API spec that can as well future wise merge in the Subtle API? I.e., based on encrypt and what we pass to it, it will use the correct underlying API P.S. Let's be careful of method overloading in Typescript as it's not natively as supported as we want it to So what do we have then?
What about something like the below? const bufferOut = await client.crypto.encrypt(componentName, msg: string | Buffer, {
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
type: "file | message" // <-- default to message, if file, it will take the message string as file name and read from the file
})Under the hood we then translate to the correct Dapr API and initially don't use the Subtle API This would ensure that in the future we can adapt it without breaking changes, while still adopting all the use cases |
No, the subtle encrypt and high-level encrypt are very different.
The subtle API is also very low level. Users must know what they're doing. For example, with the high-level API, they just need to tell Dapr "encrypt this, using this key, and the key type is RSA". With the low-level API they need to worry about generating their own nonces/IVs if needed, they need to deal with authentication tags, etc. I strongly recommend keeping the subtle APIs, when available, in its own namespace. For example
I'm ok with passing a readable stream explicitly. But if you don't use a duplex stream, you can't do like: await pipeline(
createReadStream("plaintext.txt"),
client.crypto.encrypt({
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),
createWriteStream("ciphertext.out"),
);Instead you must do this which is a bit more awkward: await pipeline(
client.crypto.encrypt(createReadStream("plaintext.txt"), {
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),
createWriteStream("ciphertext.out"),
); |
|
The following API spec might make sense then: const streamIn = createReadStream("plaintext.txt");
const streamOut = createWriteStream("encrypted.txt");
const out = await client.crypto.encrypt(streamIn, { ...config });
// If stream
streamOut.pipe(out);
// If string or Buffer
const res = out; Which then would translate to async function encrypt(componentName: string, in: string | Readable | Buffer, config): Promise<string | Writable> {
// Handle each of the datatypes
}For the subtle API, as agreed on, we will publish this under a separate namespace if it - ever - gets released on the Dapr core
|
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
|
Code has been updated. The new interfaces are: interface IClientCrypto {
encrypt(opts: EncryptRequest): Promise<Duplex>;
encrypt(inData: Buffer | ArrayBuffer | ArrayBufferView | string, opts: EncryptRequest): Promise<Buffer>;
decrypt(opts: DecryptRequest): Promise<Duplex>;
decrypt(inData: Buffer | ArrayBuffer | ArrayBufferView, opts: DecryptRequest): Promise<Buffer>;
}An example of the usage (using // Using streams
// Here `encrypt` returns a `Duplex` stream that can be used in a pipeline
await pipeline(
createReadStream("plaintext.txt"),
await client.crypto.encrypt({
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}),
createWriteStream("ciphertext.out"),
);
// Using buffers (or strings)
// Here `ciphertext` is a `Buffer`
const ciphertext = await client.crypto.encrypt(plaintext, {
componentName: "crypto-local",
keyName: "symmetric256",
keyWrapAlgorithm: "A256KW",
}); |
|
Awesome! Just checking something as well, the |
|
Hi @ItalyPaleAle can you check for the tests? Would LOVE to get this in! :) |
|
@XavierGeerinck sorry, i was on vacation till yesterday. I think I fixed the CI failures. I need to look into adding E2E tests, for now there's only a sample |
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
|
@XavierGeerinck added E2E tests too |
shubham1172
left a comment
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.
Ye, just one last thing - could you please add docs as well that go in here https://v1-11.docs.dapr.io/developing-applications/sdks/js/js-client ?
Co-authored-by: Shubham Sharma <[email protected]> Signed-off-by: Xavier Geerinck <[email protected]>
XavierGeerinck
left a comment
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.
Small comments
| // Handle overloading | ||
| // If we have a single argument, assume the user wants to use the Duplex stream-based approach | ||
| let inData: Buffer | undefined; | ||
| if (opts === undefined) { |
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.
Shouldn't we use !opts?
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 like explicitly checking for "undefined" in these cases as it's not ambiguous.
Signed-off-by: ItalyPaleAle <[email protected]>
|
Amazing! Merged it!! Thank you for this amazing contribution! |
Description
This PR implements the
crypto.encryptandcrypto.decryptmethods in the JS SDK. These allow using the crypto APIs in Dapr 1.11 to encrypt and decrypt data.TODO
Issue reference
Fixes #474
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: