search

dapr / dotnet-sdk

Dapr SDK for .NET
Apache License 2.0
1.12k stars 340 forks source link

Implementing Cryptography building block in .NET #1217

Closed WhitWaldo closed 9 months ago

WhitWaldo commented 11 months ago

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 https://github.com/dapr/docs/pull/3928. Also needs a Quickstart to show how this is used https://github.com/dapr/quickstarts/issues/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:

codecov[bot] commented 11 months ago

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 :warning:
src/Dapr.Client/CryptographyOptions.cs 27.77% 13 Missing :warning:
src/Dapr.Client/EnumExtensions.cs 60.00% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1217/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | Coverage Δ | | |---|---|---| | [net6](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `67.15% <11.59%> (-1.32%)` | :arrow_down: | | [net7](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `67.15% <11.59%> (-1.32%)` | :arrow_down: | | [net8](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1217/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `67.15% <11.59%> (-1.32%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

WhitWaldo commented 11 months ago

/assign

WhitWaldo commented 11 months ago

@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 commented 11 months ago

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.

msfussell commented 10 months ago

To support this code, best to also add a quickstart example at the same. I have raised the issue here https://github.com/dapr/quickstarts/issues/969

WhitWaldo commented 10 months ago

To support this code, best to also add a quickstart example at the same. I have raised the issue here dapr/quickstarts#969

There is a working sample provided in the .NET SDK that the documentation is synced with, but the only reason I didn't add a quickstart to pair with this is because it uses a NuGet reference to Dapr instead of a package reference and since these bits aren't a part of Dapr yet, I wouldn't be able to test it end-to-end. Rather, I figured that once this was included in the SDK, that'd be the very next piece to tackle as it could be merged into a the docs/repo in a patch release.

WhitWaldo commented 10 months ago

@philliphoff Apparently I somehow dismissed your review. Mind giving it another pass?

philliphoff commented 10 months ago

Apparently I somehow dismissed your review. Mind giving it another pass?

@WhitWaldo Re-approved. Not sure why the DCO check isn't passing; might try to re-sign the commits?

WhitWaldo commented 10 months ago

@philliphoff Looks like it's the fourth commit (2e1e6b335cc1421dc71556fea48de2c0e133011f) is missing the DCO. I know how to revert the last commit and amend it, but this is 41 commits in the past. Any chance you know the commands necessary to amend that commit?

philliphoff commented 10 months ago

@WhitWaldo What I've done in the past is rebase and squash the changes into a single commit, then amend that commit with the signoff. Or, rebase and amend each commit with the signoff (if needed). Then force push the change. I don't know which way you'd find easier.

WhitWaldo commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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 commented 9 months ago

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.