Skip to content

Conversation

@WhitWaldo
Copy link
Contributor

@WhitWaldo WhitWaldo commented May 1, 2025

Description

This PR implements three things:

  1. It splits out the cryptography building block to a separate Dapr.Cryptography package to be released with 1.16. All existing methods on Dapr.Client have an updated [Obsolete] attribute with a message telling developers that this method will be removed with the release of 1.17.
  2. An issue was reported as a PR which identified a race condition wherein either the read or write buffer could run out of content and the other stream would stall as they were filling and draining in tandem. This PR fundamentally rewrites the implementation so this is no longer the case. A separate PR will be created to fix the implementation in the DaprClient so it can be patched in a 1.15 release.
  3. I've updated the cryptography example to use the separate Dapr.Cryptography package and modernized to reflect a DI-first approach. It features three samples that demonstrate encryption and decryption of strings, streams and byte arrays of small (a dozen bytes), medium (~1 KB) and 1 GB files.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1488 dapr/dapr#8244

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@WhitWaldo WhitWaldo self-assigned this May 1, 2025
@WhitWaldo WhitWaldo requested review from a team as code owners May 1, 2025 16:04
WhitWaldo added 15 commits May 1, 2025 11:11
Signed-off-by: Whit Waldo <[email protected]>
Signed-off-by: Whit Waldo <[email protected]>
Signed-off-by: Whit Waldo <[email protected]>
WhitWaldo and others added 4 commits May 1, 2025 14:32
…-cryptography-usage.md

Co-authored-by: Christopher Watford <[email protected]>
Signed-off-by: Whit Waldo <[email protected]>
Copy link
Contributor

@watfordsuzy watfordsuzy left a comment

Choose a reason for hiding this comment

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

Couple of nits, some notes, and an unobserved task exception issue.

var duplexStream = Client.DecryptAlpha1(grpcCallOptions);

using var streamProcessor = new DecryptionStreamProcessor();
await streamProcessor.ProcessStreamAsync(ciphertextStream, duplexStream, options.StreamingBlockSizeInBytes,
Copy link
Contributor

Choose a reason for hiding this comment

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

This await doesn't really do anything given how the processor is setup.

I think a design like you see in the Azure.Data.Tables library would be amenable here: https://learn.microsoft.com/en-us/dotnet/api/azure.data.tables.tableclient.queryasync?view=azure-dotnet

Basically, instead of returning a Task the process stream returns a wrapper which includes the IAsyncEnumerable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that await doesn't really do anything, but between it and just discarding the value, C# requires that I put something there. I don't know that I want to return any additional public types just for the sake of it when I could just return the IAsyncEnumerable and leave it to consumers to decide what to do with it.

var duplexStream = Client.EncryptAlpha1(grpcCallOptions);

using var streamProcessor = new EncryptionStreamProcessor();
await streamProcessor.ProcessStreamAsync(plaintextStream, duplexStream, encryptRequestOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

One interesting thing I noticed in the dapr Go code is that it does encryption/decryption on the full buffer in memory (byte[]) rather than streaming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't speak to what happens in the runtime, but the specification is that this is implemented using a bidirectional streaming connection.

@WhitWaldo WhitWaldo merged commit 49992f4 into dapr:release-1.16 May 2, 2025
1 check passed
@WhitWaldo WhitWaldo deleted the crypto-v2 branch May 2, 2025 20:45
WhitWaldo added a commit to WhitWaldo/dapr-dotnet-sdk that referenced this pull request Aug 12, 2025
* Implementation of the new crypto client

Signed-off-by: Whit Waldo <[email protected]>
Co-authored-by: Christopher Watford <[email protected]>
WhitWaldo added a commit to WhitWaldo/dapr-dotnet-sdk that referenced this pull request Aug 13, 2025
* Implementation of the new crypto client

Signed-off-by: Whit Waldo <[email protected]>
Co-authored-by: Christopher Watford <[email protected]>
WhitWaldo added a commit to WhitWaldo/dapr-dotnet-sdk that referenced this pull request Aug 13, 2025
* Implementation of the new crypto client

Signed-off-by: Whit Waldo <[email protected]>
Co-authored-by: Christopher Watford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants