DataProtection use built in AesGcm where available by HaoK · Pull Request #29814 · dotnet/aspnetcore

HaoK · 2021-02-01T09:53:51Z

Fixes #20689

Use new framework crypto apis now that they are available, this enable these apis on non windows for non net461/netcore2.

HaoK · 2021-02-01T18:56:43Z

@GrabYourPitchforks for the crypto @dotnet/aspnet-build for adding a new algorithms dependency, @dotnet/aspnet-blazor-eng for fyi/review

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs

src/DataProtection/DataProtection/src/Cng/GcmAuthenticatedEncryptor.cs

eng/Versions.props

src/DataProtection/DataProtection/src/Cng/GcmAuthenticatedEncryptor.cs

javiercn · 2021-02-02T09:33:11Z

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs

+                bytesA = new ReadOnlySpan<byte>(bufA, byteCount);
+                bytesB = new ReadOnlySpan<byte>(bufB, byteCount);
+            }
+            return CryptographicOperations.FixedTimeEquals(bytesA, bytesB);


❤️

There are a bunch of places in data protection where we could leverage new primitives introduced to the framework in the past few releases. Might be interesting to have an issue for that. Identity likely can also benefit from it.

eng/Versions.props

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs

eng/Dependencies.props

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs

Co-authored-by: Pranav K <prkrishn@hotmail.com>

src/DataProtection/DataProtection/src/Cng/GcmAuthenticatedEncryptor.cs

src/DataProtection/DataProtection/src/Managed/AesGcmAuthenticatedEncryptor.cs

HaoK · 2021-02-16T09:18:36Z

@GrabYourPitchforks when you get a chance could you review the new managed implementation and the test I added to make sure it looks legit?

GrabYourPitchforks

Only thing that really needs fixed is the comment on AuthenticatedEncryptorDescriptorTests.cs. Approved once that's done. Everything else is at your discretion.

Thanks for getting to this! 👍

GrabYourPitchforks · 2021-02-24T06:15:08Z

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs

            Assert(countA == countB, "countA == countB");

+#if NETCOREAPP
+            unsafe


Extreme nits: No need to use unsafe here. If desired, you can also put the NoInlining | NoOptimization attribute within an #if !NETCOREAPP block. Maybe those can come with the spanification PR so as not to cause noise here.

Will address this in the span PR

src/DataProtection/DataProtection/src/AuthenticatedEncryption/AuthenticatedEncryptorFactory.cs

...ion/test/AuthenticatedEncryption/ConfigurationModel/AuthenticatedEncryptorDescriptorTests.cs

src/DataProtection/DataProtection/src/Managed/AesGcmAuthenticatedEncryptor.cs

pranavkm · 2021-02-24T16:58:02Z

src/DataProtection/DataProtection/src/Managed/AesGcmAuthenticatedEncryptor.cs

+
+                // Use the KDF to generate a new symmetric block cipher key
+                // We'll need a temporary buffer to hold the symmetric encryption subkey
+                var decryptedKdk = new byte[_keyDerivationKey.Length];


@GrabYourPitchforks would it be unwise to use the shared ArrayPool here?

Going to merge this as is, can make this switch in the follow up / spanification PR if needed

Correct, because it's a cryptographic secret. Within the runtime, our crypto libraries use an internal CryptoPool type instead of the public singleton ArrayPool<T>.Shared instance.

During the spanification work, this can be switched to using stackalloc (the common case) or the pinned object heap. Then no pooling will be needed at all.

…tedEncryptor.cs Co-authored-by: Pranav K <prkrishn@hotmail.com>

GrabYourPitchforks · 2021-02-24T22:17:16Z

I finished reviewing again through the commit a752598 and everything looks great. Thanks so much Hao!

ghost · 2021-02-24T22:17:19Z

Hi @GrabYourPitchforks. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Fun with spans and pointers

65aa435

HaoK added the area-dataprotection Includes: DataProtection label Feb 1, 2021

HaoK added this to the 6.0-preview2 milestone Feb 1, 2021

Merge branch 'main' into haok/gcm

4a5d224

HaoK requested review from a team and GrabYourPitchforks February 1, 2021 18:56

HaoK marked this pull request as ready for review February 1, 2021 18:56

HaoK requested a review from dougbu as a code owner February 1, 2021 18:56

wtgodbe approved these changes Feb 1, 2021

View reviewed changes

Update Version.Details.xml

d658fcc

dougbu reviewed Feb 1, 2021

View reviewed changes

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs Outdated Show resolved Hide resolved

pranavkm reviewed Feb 1, 2021

View reviewed changes

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs Outdated Show resolved Hide resolved

src/DataProtection/DataProtection/src/Cng/GcmAuthenticatedEncryptor.cs Outdated Show resolved Hide resolved

eng/Versions.props Outdated Show resolved Hide resolved

HaoK added 3 commits February 1, 2021 13:38

Cleanup

4b43975

Flip check order

0c6d717

Update Versions.props

4ae2bdb

javiercn reviewed Feb 2, 2021

View reviewed changes

src/DataProtection/DataProtection/src/Cng/GcmAuthenticatedEncryptor.cs Outdated Show resolved Hide resolved

javiercn reviewed Feb 2, 2021

View reviewed changes

Cleanup move decls into unsafe block

fba0a99

pranavkm reviewed Feb 2, 2021

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs Show resolved Hide resolved

HaoK added 3 commits February 2, 2021 09:37

Update overload

b97ecc0

Update Version.Details.xml

2f76630

Update Versions.props

b81c9d4

pranavkm reviewed Feb 2, 2021

View reviewed changes

eng/Dependencies.props Outdated Show resolved Hide resolved

pranavkm reviewed Feb 2, 2021

View reviewed changes

src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs Outdated Show resolved Hide resolved

HaoK and others added 3 commits February 2, 2021 10:14

Update src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs

717e303

Co-authored-by: Pranav K <prkrishn@hotmail.com>

Update eng/Dependencies.props

d9f33b9

Co-authored-by: Pranav K <prkrishn@hotmail.com>

Update CryptoUtil.cs

b1a59e8

GrabYourPitchforks reviewed Feb 3, 2021

View reviewed changes

src/DataProtection/DataProtection/src/Cng/GcmAuthenticatedEncryptor.cs Outdated Show resolved Hide resolved

GrabYourPitchforks mentioned this pull request Feb 4, 2021

GCM encryption not available on linux when using DataProtection #20689

Closed

Renames and add managed aesgcm encryptor

37e4aa2

HaoK commented Feb 5, 2021

View reviewed changes

src/DataProtection/DataProtection/src/Managed/AesGcmAuthenticatedEncryptor.cs Outdated Show resolved Hide resolved

HaoK added 8 commits February 5, 2021 03:00

Cleanup

98ac949

Fixup

a85582c

Update CngGcmAuthenticatedEncryptor.cs

052296f

Fix bugs

7a932de

Fix typo in context header :(

47dcec9

Cleanup

53e3d8f

Revert print buffer

512a9f6

Cleanup

fa4ee92

HaoK mentioned this pull request Feb 8, 2021

Spanify DataProtection code #30013

Closed

GrabYourPitchforks approved these changes Feb 24, 2021

View reviewed changes

HaoK added 8 commits February 24, 2021 01:07

Code review feedback

bc1194d

Update AuthenticatedEncryptorFactory.cs

c1f8809

Update AesGcmAuthenticatedEncryptor.cs

6a866f4

Update AesGcmAuthenticatedEncryptor.cs

68f37c2

Update AesGcmAuthenticatedEncryptor.cs

a631a30

Update AesGcmAuthenticatedEncryptor.cs

4e84ecf

Update AesGcmAuthenticatedEncryptor.cs

96b1b4b

Update AuthenticatedEncryptorDescriptorTests.cs

fb78d32

pranavkm reviewed Feb 24, 2021

View reviewed changes

HaoK and others added 2 commits February 24, 2021 09:07

Update AesGcmAuthenticatedEncryptor.cs

75a1e0f

Update src/DataProtection/DataProtection/src/Managed/AesGcmAuthentica…

a752598

…tedEncryptor.cs Co-authored-by: Pranav K <prkrishn@hotmail.com>

HaoK merged commit 5be1082 into main Feb 24, 2021

HaoK deleted the haok/gcm branch February 24, 2021 22:12

Conversation

HaoK commented Feb 1, 2021

Uh oh!

HaoK commented Feb 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

javiercn Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HaoK commented Feb 16, 2021

Uh oh!

GrabYourPitchforks left a comment

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

HaoK Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pranavkm Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

HaoK Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks commented Feb 24, 2021

Uh oh!

ghost commented Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants