Implementing Cryptography building block in .NET

[WhitWaldo]

Description

This adds support for Cryptography building block (alpha) in .NET supporting both the encrypt and decrypt streaming APIs.

Examples work and I've updated the README to reflect use with Azure Key Vault (RBAC, environment variable install, etc.).

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.

This closes #1072 - per recommendations in Discord, this streams the requests in configurable blocks (defaulting to 4 KB), sending only the request metadata payload in the first message and sending the plaintext/ciphertext blocks in subsequent messages. I manually updated the relevant sections of the dapr.proto pulling from the latest in dapr/dapr.

Submitted update for docs at dapr/docs#3928.
Also needs a Quickstart to show how this is used dapr/quickstarts#969

I still need to figure out how to write any tests for this, but I confirmed that my example (properly configured) runs without issue locally.

Checklist

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

• Code compiles correctly
• Created/updated tests (some tests written - presumably room for more)
• Extended the documentation

[codecov]

Codecov Report

Attention: 122 lines in your changes are missing coverage. Please review.

Comparison is base (348d143) 68.48% compared to head (48bd541) 67.17%.

Files	Patch %	Lines
src/Dapr.Client/DaprClientGrpc.cs	6.95%	103 Missing and 4 partials ⚠️
src/Dapr.Client/CryptographyOptions.cs	27.77%	13 Missing ⚠️
src/Dapr.Client/EnumExtensions.cs	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1217      +/-   ##
==========================================
- Coverage   68.48%   67.17%   -1.32%     
==========================================
  Files         172      174       +2     
  Lines        5848     5986     +138     
  Branches      648      667      +19     
==========================================
+ Hits         4005     4021      +16     
- Misses       1681     1798     +117     
- Partials      162      167       +5     

Flag	Coverage Δ
net6	67.15% <11.59%> (-1.32%)	⬇️
net7	67.15% <11.59%> (-1.32%)	⬇️
net8	67.15% <11.59%> (-1.32%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[WhitWaldo]

/assign

[philliphoff]

Should there be an explicit endeavour to make the default cipher consistent across SDKs? (Is this consistent with other SDKs today?)

[WhitWaldo]

This is the default per the comments in the dapr.proto so I simply selected it, but in case there was a reason for users to use the alternative (right now just "chacha20-poly1305"), I wanted to leave users the option to select the other one. That said, this issue suggested that a third should be added and used as the default (perhaps completely replacing the alternatives) and by setting it as the default it would promote better security.

[ItalyPaleAle]

This is awesome, thanks for writing this as I know it wasn't the simplest code :) I mentioned to you before that I'm really bad at C#, especially when streaming is involved, so please take my comments with a grain of salt!

[philliphoff]

ReadOnlyMemory<byte> should by implicitly convertible to byte[], no? ToArray() will make a copy which is probably not what we want.

[WhitWaldo]

byte[] is implicitly convertible to a ReadOnlyMemory<byte>, but not vice versa. The best alternative I can find is using a MemoryMarshal to create a reference to the byte array and pass that in instead of copying it, but as this is exposed via TryGetArray, it begs the new question of what the exception message should be if it fails for some reason. Is there any documented guidance on the shape of exceptions for the SDK?

Guidance needed for here and here.

[philliphoff]

I see. Ach! The purpose of accepting ReadOnlyMemory<byte> was to be more flexible for callers, but if we have to essentially restrict it to just array-like memory (or else make a copy), I'm wondering if that flexibility is really worth it. (Note that there is a performant Stream implementation over ReadOnlyMemory<byte> in the .NET "high efficiency" community toolkit. That said, I don't think we want to go down that path of taking on a new dependency or having our own implementation just for this purpose.)

I'm starting to feel like accepting (for now) byte[] as the better approach for now. (And also returning byte[] to keep the symmetry of the method.)

[WhitWaldo]

It looks like their implementation to make a memory stream from a ReadOnlyMemory<byte> is awfully similar to what I've got, using the MemoryMarshal to access the byte array.

I did copy over their argument exception text as it's richer than what I had.

As such, I vote leaving this as-is. It's about as high-performant as it'll probably get with this version of .NET and there's nothing that stops the developer from simply passing in a byte[] to the method if that's what they have or a ReadOnlyMemory<byte> if they happen to have that as well (as opposed to making this the developer's problem by reverting to only accepting byte[] and leaving them having to do this same research to convert their own ReadOnlyMemory<byte> accordingly).

[WhitWaldo]

@philliphoff Would appreciate your thoughts on this: Because of the method signature conflict, I renamed the encrypt/decrypt methods that took a stream and returned an IAsyncEnumerable<byte[]> to EncryptStreamAsync and DecryptStreamAsync.

Now that the Stream -> ReadOnlyMemory method is out of the picture, do you think it makes the API more accessible to keep these different names or instead expose just the EncryptAsync and DecryptAsync methods again with their various overloads?

[philliphoff]

Now that the Stream -> ReadOnlyMemory method is out of the picture, do you think it makes the API more accessible to keep these different names or instead expose just the EncryptAsync and DecryptAsync methods again with their various overloads?

I would try to use just a single method name (EncryptAsync() and DecryptAsync()), if possible, as that keeps the API completion lists shorter and groups related methods together better.

[WhitWaldo]

If the terminal only supported multi-line commit messages, that would have been much easier to deal with. As it is, I believe I've properly merged this and the DCO check is validating as it should. @philliphoff

[philliphoff]

@WhitWaldo Would you be willing to look at the differences in code coverage and see if there are some tests that could be added to bring it back to parity with the baseline?

[WhitWaldo]

@philliphoff I can give it a look, but I don't know that there's a whole lot that can be added on the .NET side since it's largely just shuffling the bytes out and vice versa, as all the magic happens on the Dapr sidecar. Give me a few days and I'll see what I can add to it that I don't already have in there. What's the baseline we're aiming for?

[philliphoff]

What's the baseline we're aiming for?

@WhitWaldo Ideally we'd end up close to the same test coverage as the baseline (i.e. prior to the changes). The coverage maps show a good portion of the new logic that isn't covered. This may just be a false-negative, but something to look at to see if that's the case or, if not, how easy it is to get it covered.

[philliphoff]

Merging this to ensure it's in 1.13; more test coverage would be nice prior to the APIs being considered stable but can wait.