Don't capture instance in Select for DataProtection.

[jkotalik]

Fixes #13696.

Description
Customers using the Microsoft.AspNetCore.DataProtection.EntityFrameworkCore package would hit an exception when trying to access any data. This is due to changes in EntityFramework to detect memory leaks. This memory leak is actually

System.Security.Cryptography.CryptographicException
  HResult=0x80131501
  Message=An error occurred while trying to encrypt the provided data. Refer to the inner exception for more information.
  Source=Microsoft.AspNetCore.DataProtection
  StackTrace:
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.Protect(Byte[] plaintext) in src\DataProtection\DataProtection\src\KeyManagement\KeyRingBasedDataProtector.cs:line 141
   at Microsoft.AspNetCore.DataProtection.DataProtectionCommonExtensions.Protect(IDataProtector protector, String plaintext) in src\DataProtection\Abstractions\src\DataProtectionCommonExtensions.cs:line 200
   at EntityFrameworkCoreSample.Program.Main(String[] args) in src\DataProtection\samples\EntityFrameworkCoreSample\Program.cs:line 38


Inner Exception 1:
InvalidOperationException: Client projection contains reference to constant expression of type: Microsoft.AspNetCore.DataProtection.EntityFrameworkCore.EntityFrameworkCoreXmlRepository<EntityFrameworkCoreSample.DataProtectionKeyContext>. This could potentially cause memory leak.

Customer impact
In 3.0, this renders the package completely unusable.

Regression
Yes. The is a regression from 2.1 and 2.2 with regards to functionality. The memory leak still exists in 2.1 and 2.2 though, see below.

Risk
Low. This changes the call to TryParseKeyXml from an instance method to static.

cc @Pilchie @ajcvickers this change would also need to go into 2.1 and 2.2 as there is a memory leak. This memory leak shouldn't be hit often according to @blowdart , so I believe we need to publicly patch it.

[ajcvickers]

Approving from my side, although as was stated elsewhere, we need some test coverage for this.

[jkotalik]

@ajcvickers there is coverage, however the tests are using InMemory rather than SqlServer. Should that be switched to use SqlLite?

[ajcvickers]

@jkotalik No, in-memory should be okay in this case. Smit corrected me. Due to an issue we have with the in-memory provider it wouldn't catch this. So, switching to SQLite would have helped, but I don't think it is high priority.

[Pilchie]

Thanks for jumping on this @jkotalik. I'll get it in front of Tactics tomorrow.

[jkotalik]

This was approved by shiproom. I'm going ahead with merging.

[jkotalik]

So I took a look back and saw this was added in 2.2, so it doesn't apply to 2.1. @blowdart as this only applies to 2.2 and that it isn't a very frequent memory leak (leaks once every 6 months), do you still think we should consider servicing?

[blowdart]

Yea we should just to remove the risk.

…

________________________________ From: Justin Kotalik <[email protected]> Sent: Monday, September 9, 2019 2:30:24 PM To: aspnet/AspNetCore <[email protected]> Cc: Barry Dorrans <[email protected]>; Mention <[email protected]> Subject: Re: [aspnet/AspNetCore] Don't capture instance in Select for DataProtection. (#13743) So I took a look back and saw this was added in 2.2, so it doesn't apply to 2.1. @blowdart<https://github.com/blowdart> as this only applies to 2.2 and that it isn't a very frequent memory leak (leaks once every 6 months), do you still think we should consider servicing? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#13743>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAGCNCQ4PHP36VNZGVIVUULQI257BANCNFSM4IUDAWFA>.

[smuthiya]

The latest RC1 does not have this fix in it - still the DataProtection package in unusable with RC1.
Following is the latest package released which does not have this fix

https://www.nuget.org/packages/Microsoft.AspNetCore.DataProtection/3.0.0-rc1.19457.4

When this fix will be included and released?

[jkotalik]

I’ll verify tomorrow. Thanks for letting us know.

[davidfowl]

What is this?

[jkotalik]

@smuthiya for some reason, this commit wasn't included in the 3.0.0-rc1 release: https://github.com/aspnet/AspNetCore/commits/49e84ee5ff04b17f35cacb9c1d6ccf52d8328dad. This should be present in the next release, but I'll make sure it gets in.

[jkotalik]

Ah I see. 3.0.0-rc1 was based on the 3.0.0-preview9 branch, while this commit was merged into release/3.0. Confusing 😢 .

This fix will be available in the next release.

[Pilchie]

Tagging @dougbu here to see if this change flowed properly?

[Pilchie]

Oh - it looks like there was a comment deleted that I was replying to. Never mind.

[smitpatel]

Been through the same in the morning. 😄

[jkotalik]

I got a feeling people are going to hit this issue fairly frequently (in their own code). @smitpatel do we have documented guidance here in EF?

[smitpatel]

We have updated the exception message and added some easy way to deal with it in the message as per dotnet/efcore#17623
We have also added fwlink to the message which will point to guidance in EF docs, we have tracking issue to add detailed guidance in the docs.