Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider moving ASP.NET Core Data Protection to Microsoft.Extensions.* #3774

Closed
kevinchalet opened this issue Oct 31, 2018 · 67 comments
Closed
Assignees
Labels
area-dataprotection Includes: DataProtection area-identity Includes: Identity and providers Done This issue has been fixed

Comments

@kevinchalet
Copy link
Contributor

kevinchalet commented Oct 31, 2018

@davidfowl asked for concrete examples of things people would like to use outside of ASP.NET Core, that will be limited to .NET Core starting with 3.0.

Some of the OWIN/Katana apps I've worked on rely on the Microsoft.Owin.Security.Interop backcompat' package to be able to share things like cookies and authentication tokens with more recent ASP.NET Core apps. This package depends on the shiny ASP.NET Core Data Protection, which currently lives in the Microsoft.AspNetCore.DataProtection namespace.

My request is simple: please consider moving the DP components to Microsoft.Extensions.DataProtection and keeping them compatible with .NET Standard (and .NET Framework).

@blowdart
Copy link
Contributor

I'm confused why you, of all people, don't just share logins via OIDC, you wrote an entire system for that. The back compat shim was meant as a temporary backstop, and honestly I'd like to kill it off in 3.0.

While there's no technical reason not to, data protection was meant for asp.net core use, and for ephemeral data at that. Moving it out would encourage uses outside of what we'd be willing to support, and when that use can end in data loss it seems to me to be a bad idea to encourage it.

@blowdart
Copy link
Contributor

(Tagging @davidfowl because he started this)

@kevinchalet
Copy link
Contributor Author

I'm confused why you, of all people, don't just share logins via OIDC, you wrote an entire system for that.

I do use OIDC to "share logins", but this standard alone is not the only one involved in the token authentication dance. Not everyone wants to always use JWT for their access tokens: I’m one of those guys.

In the "system I wrote" tokens are by default created using ASP.NET Core Data Protection, which has many advantages over JWT, like an almost configuration-less experience, a very high security level and a huge perf boost (between +15% and +20% req/sec compared to HMACed JWTs encrypted using a symmetric AES key).

With the compat package, we can have OWIN/Katana/ASP.NET 4.x API projects that accept DP access tokens issued by an ASP.NET Core app.

@natemcmaster natemcmaster added the area-dataprotection Includes: DataProtection label Oct 31, 2018
@philmcmillan
Copy link

I work with a couple of shops that have a number of existing legacy investments along with new Core based work. Their upgrade cycles don't keep up with your release cycles, and they've been relying upon OWIN/Katana interop for a single sign-on experience as they work through their tech upgrade cycles. What will happen if the interop is killed with 3.0 is that they'll end up in another set of delayed upgrades of their Core investments while they upgrade in their legacy apps to Core. As much as it's meant as a temporary backstop, at least one of my customers cycle is such that you'll probably be well into planning/dev cycles 4.0 before we get the core of the legacy investments updated to Core and off of 4.6/4.7. And that's if we can prioritize architectural updates over new functionality, which is not a given.

@blowdart
Copy link
Contributor

(An aside, just because I'd like to kill the back compat doesn't mean it'll happen)

@philmcmillan
Copy link

@blowdart - Thanks - and I get that it might not happen - but it is a concern. But I support @PinpointTownes suggestion of moving DP out of the ASP.NET Core API for 3.0 (especially if there are underlying API changes to the DataProtector and DataProtectionProvider classes). Since the legacy stack has to import the packages that support DP, if they fold into the core API package, and we want to update Core apps to 3.0 we would end up having to reference the entire Core API package in our legacy apps to maintain compatibility.

Not sure I'd put them in the Extensions space, but the DP packages are really useful, and not just for ticket/token and other classic web scenarios. I was considering DP as a solution for protecting shared sensitive data in a non-ASP.NET environment, but I'm reconsidering that as with the 3.0 announcements, I can't depend on those packages existing in the future, and I really do not want to import the whole API into the solution just to get DP.

@kevinchalet
Copy link
Contributor Author

The good news is that this change should be extremely easy, as DP only depends on the rest of ASP.NET Core for the application discriminator thing, that you'll be able to move to the hosting layer just like you tentatively did for 2.0 before the "let's ditch .NET Framework" plan was abandoned.

Related posts:

@natemcmaster natemcmaster self-assigned this Nov 21, 2018
@natemcmaster natemcmaster added this to the 3.0.0-preview2 milestone Nov 21, 2018
@natemcmaster
Copy link
Contributor

I'm assigning to myself to review this proposal for 3.0.0 preview2. DataProtection will be shared-framework-only for 3.0 preview1, but I expect we will adjust before 3.0 is released.

@kevinchalet
Copy link
Contributor Author

@natemcmaster great, thanks!

If you think an external contribution (e.g a PR with a prototype) may make that happen sooner, please let me know. Some of the packages I maintain and develop depend on DP (and the OWIN backcompat' package) so I'd love to start porting them to 3.0 to be able to submit feedback very early in the process.

@natemcmaster
Copy link
Contributor

Thanks Kévin. Half the team is out for holiday break. When they're back, we'll take a closer look at this and let you know if PRs would help.

@natemcmaster natemcmaster added the ✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. label Nov 28, 2018
@natemcmaster
Copy link
Contributor

I'm closing this because we are not planning on moving DataProtection to extensions, and do not plan to ship a version of DataProtection 3.0 with .NET Framework support. We do not have any plans for changes for the API surface or behavior of Data Protection in 3.0. We believe the 2.1 DataProtection packages on NuGet.org should continue to be sufficent for customers who want to use DataProtection outside of .NET Core.

Our recommendation for Owin/Katana users who want interop with AspNetCore is to use Microsoft.Owin.Security.Interop 2.1. This package is covered under the LTS policy for ASP.NET Core 2.1. If there are issues in the future with interop between ASP.NET Core and Katana, we would fix this by servicing 2.1.

I want to really clear, however. Closing this issue does not mean that we will drop support for the scenarios mentioned in the original post. We will continue to support interop between ASP.NET Core 3.0+ and Owin/Katana apps. I've opened this issue #4074 to add test coverage to ensure this continues to work into the future. This means ensuring we maintain compatibility in dataprotection key formats, cookie formats, storage locations, etc.

@kevinchalet
Copy link
Contributor Author

@natemcmaster what's the motivation for not moving DP outside ASP.NET Core? It's disheartening to hear that such announcements are never justified by technical motivations in the first place, giving us the impression they are arbitrary decisions made without any technical ground.

Referencing old packages comes with very annoying downsides, like incompatibilities caused by API changes introduced in future versions. In my OpenIddict project, I'd like the core package to stay netstandard20-compatible (so that it can be used on .NET Framework). It depends on DP: if I keep referencing an old build, chances are high we'll see runtime exceptions at some point.

There's also crypto agility: a critical library like DP can't be stuck in the past: supporting newer algorithms is necessary as soon as flaws are discovered in existing algorithms. Supporting new algorithms would likely require extensive code and new configuration settings, making it a poor candidate for a minor version/patch.

@kevinchalet
Copy link
Contributor Author

Oh, and a 3.0 version of DP that wouldn't depend on Microsoft.AspNetCore.* would help OWIN/Katana reduce the number of unnecessary dependencies (DP depends on Hosting, which brings a looooot of packages)

@natemcmaster
Copy link
Contributor

The technical grounds for this decision was that ASP.NET Core 3.0 will not support .NET Framework (#3753). If we had decided to continue supporting DP 3.0 and .NET Framework, we also have to support all of its dependencies on .NET Framework. In our cost/benefit estimates, we don't think there is enough benefit to incur the cost of continued .NET Framework support for this scenario. As I stated, we believe the 2.1 DataProtection packages should continue to function well enough to provide interop between Owin/Katana and ASP.NET Core.

Referencing old packages comes with very annoying downsides, like incompatibilities caused by API changes introduced in future versions.

Should something like this become a problem in the future, let me know and I'll be happy to help. I suspect this would be solved the same way you would solve differences in API between .NET Core and .NET Framework -- multi-target.

Supporting new algorithms would likely require extensive code and new configuration settings, making it a poor candidate for a minor version/patch.

This is a hypothetical scenario, right?

@natemcmaster
Copy link
Contributor

a 3.0 version of DP that wouldn't depend on ... hosting

This isn't something we plan to do at the moment.

@kevinchalet
Copy link
Contributor Author

If we had decided to continue supporting DP 3.0 and .NET Framework, we also have to support all of its dependencies on .NET Framework

If you had decided to keep supporting it, I wouldn't have opened this ticket in the first place and we wouldn't have this discussion 😄

What dependencies are you referring to, concretely? I took a look at the dependencies, and Hosting is basically the only problematic one. Everything else is either Microsoft.Extensions.* dependencies or Windows Compat' packages (e.g Microsoft.Win32.Registry).

In our cost/benefit estimates, we don't think there is enough benefit to incur the cost of continued .NET Framework support for this scenario.

It's not just .NET Framework, it's any .NET Standard platform. DP is an excellent way to encrypt/decrypt payloads without having to write a single code of crypto stuff and dealing with key management (which is hard to get right). As @philmcmillan said, it could be extremely useful in non-web scenarios.

If HR is the only blocking factor, I'm sure the community (me included) would love to dedicate hours - or even days, if necessary - making that possible.

I suspect this would be solved the same way you would solve differences in API between .NET Core and .NET Framework -- multi-target.

So you mean applications targeting netstandard20 and referencing DP 2.1 should also be updated to target netcoreapp30 and reference a different version (3.0) if an API incompatibility was discovered? Isn't referencing different versions of the same package considered an antipattern, which has caused many issues in the past?

This is a hypothetical scenario, right?

Deprecation of vulnerable algos is certainly not a hypothetical scenario (tho' I agree it's a rare occurrence, thankfully). In April 2019, Windows Update will stop supporting SHA1 for code signing and Windows 7/2008 will have to be updated to support newer algos: https://support.microsoft.com/en-us/help/4472027/2019-sha-2-code-signing-support-requirement-for-windows-and-wsus.

@natemcmaster
Copy link
Contributor

I appreciate you opening the issue to discuss. Backcompat with OWIN/Katana apps is the primary scenario we're interested in supporting right now, and also the only concrete scenario that has been considered so far. If you have other concrete scenarios that require .NET Standard support and DataProtection 3.0, please let me know.

What dependencies are you referring to, concretely?

Microsoft.AspNetCore.Hosting.Abstractions
Microsoft.AspNetCore.Hosting.Server.Abstractions
Microsoft.AspNetCore.Http.Abstractions
Microsoft.AspNetCore.Http.Features

So you mean applications targeting netstandard20 and referencing DP 2.1 should also be updated to target netcoreapp30 and reference a different version (3.0) if an API incompatibility was discovered? Isn't referencing different versions of the same package considered an antipattern, which has caused many issues in the past?

Let me clarify, I'm talking about scenario A below. Let's say (again, hypothetically) we added a new, super useful api in DP 3.0, IDataProtection3.Something(). I see two outcomes of this.

A. Your app or library should use the new API if DP >= 3.0, but gracefully falls back to something else when DP < 3.0
Solution: multi-target.

<TargetFrameworks>netstandard2.0;netcoreapp3.0</TargetFrameworks>
#if NETCOREAPP3_0
   IDataProtection3.Something()
#else
   // ignore or do something else
#endif

B. Your app/library absolutely requires the new API and there is no way you can target .NET Core 3.0.

In this case, yes, you are stuck. This is the scenario we are looking for but haven't found. We don't believe Owin/Katana falls under this scenario.

@kevinchalet
Copy link
Contributor Author

Backcompat with OWIN/Katana apps is the primary scenario we're interested in supporting right now, and also the only concrete scenario that has been considered so far. If you have other concrete scenarios that require .NET Standard support and DataProtection 3.0, please let me know.

I can certainly understand you have limited resources, but there are people interested in using DP in non-web scenarios. The one that immediately comes to mind - as I've done it multiple times - is DP access token validation in non-ASP.NET Core services (e.g gRPC .NET services on .NET Framework).

I believe it's worth discussing these scenarios, specially since the community would be willing to contribute to make DP support more platforms (and honestly, the cost seems fairly low).

Maybe you should at least re-open this ticket so that interested people can list the scenarios they'd like to use DP for?

Microsoft.AspNetCore.Hosting.Abstractions
Microsoft.AspNetCore.Hosting.Server.Abstractions
Microsoft.AspNetCore.Http.Abstractions
Microsoft.AspNetCore.Http.Features

Don't all these dependencies come because of Hosting, which we all can agree is a weird dependency?

Let me clarify, I'm talking about scenario A below.

Oh yeah, that's not the scenario I had in mind. I was not talking about additive changes, but changes that consist in removing public or internal APIs in future major versions, that may cause missing methods exceptions when loading a newer version than the one officially referenced.

@natemcmaster natemcmaster removed the ✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. label Nov 29, 2018
@natemcmaster natemcmaster modified the milestones: 3.0.0-preview2, Backlog Nov 29, 2018
@natemcmaster natemcmaster removed their assignment Nov 29, 2018
@natemcmaster
Copy link
Contributor

Maybe you should at least re-open this ticket so that interested people can list the scenarios they'd like to use DP for?

Sure, fair enough. Let’s see if we get more feedback. I’m going to leave this in the Backlog milestone. We are still not planning to make changes, but are open to hearing from more customers.

@natemcmaster natemcmaster reopened this Nov 29, 2018
@dlandi
Copy link

dlandi commented Dec 8, 2018

@blowdart @davidfowl
<<While there's no technical reason not to, data protection was meant for asp.net core use, and for ephemeral data at that. Moving it out would encourage uses outside of what we'd be willing to support, and when that use can end in data loss it seems to me to be a bad idea to encourage it.>>

Too Late!!! I am already using the DataProtection API in this library:
https://github.com/dlandi/MemStache

I use it in a Xamarin Forms context, and it works great. First, you say "there's no technical reason not to" use it, and then you state your opinion on why others shouldn't use it. Don't care.

I have tested it in a Xamarin Context, and it works like gangbusters.

@natemcmaster natemcmaster removed their assignment May 2, 2019
@Eilon
Copy link
Member

Eilon commented May 2, 2019

I have no objection.

@HaoK
Copy link
Member

HaoK commented Jun 1, 2019

Need to do this early in preview 7

@HaoK
Copy link
Member

HaoK commented Jun 7, 2019

First issue, after changing DataProtection to netstandard 2.0, there's an issue since it references Microsoft.Win32.Registry which isn't in the shared framework

@natemcmaster @blowdart ideas?

@HaoK
Copy link
Member

HaoK commented Jun 7, 2019

Identity also has an issue, where a fairly recent PR added usage of a netcore API in passwordhasher here:

https://github.com/aspnet/AspNetCore/pull/9798/files

@natemcmaster
Copy link
Contributor

there's an issue since it references Microsoft.Win32.Registry which isn't in the shared framework

What's the error you are seeing? If my hunch is right, you just need to add Microsoft.Win32.Registry to this list: https://github.com/aspnet/AspNetCore/blob/d3d6e0e7090ae4a45e9e616e92f3395650471436/eng/SharedFramework.External.props#L82-L91

@HaoK
Copy link
Member

HaoK commented Jun 7, 2019

Ah nice, yeah looks like I needed to add all 3 of the ones in the netcoreapp3.0 group:

  <ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.0'">
    <_CompilationOnlyReference Include="Microsoft.Win32.Registry" />
    <_CompilationOnlyReference Include="System.Security.Cryptography.Cng" />
    <_CompilationOnlyReference Include="System.Security.Principal.Windows" />
  </ItemGroup>

That gets things to the main thing we need to resolve DP depending on Hosting.Abstractions which is netcoreapp3.0 for things like IStartupFilter

@natemcmaster
Copy link
Contributor

Here are some solutions for fixing this.

  1. Cross-compile DP for netstandard2.0 and netcoreapp3.0. The NS version does not implement an IStartupFilter (excluded via cross compilation), and the netcore version has a <frameworkReference> to Microsoft.AspNetCore.App
  2. Invert the dependency order. Move DataProtectionStartupFilter to Microsoft.AspNetCore.Hosting, make it a filter that always runs, but change DataProtectionStartupFilter so it only does something if an implementation of IKeyRingProvider is in the service provider.
  3. Reimplement the behavior of DataProtectionStartupFilter in a different layer of the stack. The important thing DataProtectionStartupFilter does is initialize the key ring before the server begins accepting HTTP requests.

Thoughts @davidfowl on layering?

@kevinchalet
Copy link
Contributor Author

Cross-compile DP for netstandard2.0 and netcoreapp3.0. The NS version does not implement an IStartupFilter (excluded via cross compilation), and the netcore version has a to Microsoft.AspNetCore.App

This sounds really fragile. Wouldn't that force "non-ASP.NET" .NET Core apps (e.g consoles or services) to reference the Hosting/Http assemblies, making self-deployed apps bigger than what they should be?

FWIW, I personally really like the second option, which should be trivial and super clean to implement (likely by having an empty constructor and a constructor accepting a IKeyRingProvider?)

@blowdart
Copy link
Contributor

Would you rather they were big, or wouldn't be possible at all?

@davidfowl please review the options, this is your call.

@Tratcher
Copy link
Member

We've started this project before and did option 2 last time (invert the dependency). It was reverted for unrelated reasons.

@davidfowl
Copy link
Member

We've started this project before and did option 2 last time (invert the dependency). It was reverted for unrelated reasons.

I'm not a fan of pushing DataProtection into every ASP.NET Core project but maybe the optional dependency is a reasonable compromise.

Another option might be an interface like IHostedService? We can change the WebHost to make sure these run before the server starts (I believe it doesn't today).

@davidfowl
Copy link
Member

Did some more thinking and I don't like 2 anymore, I think it's wrong to have data protection as part of hosting. Can we have the components that use it call the shared method that adds the startup filter (cookies etc).

@Tratcher
Copy link
Member

Let's get a room for 15 minutes and deal with this.

@HaoK
Copy link
Member

HaoK commented Jun 14, 2019

Fowler is out for NDC for a while.

So are we closing in on 3, where we have a new DataProtectionStartupFilter package that targets netcoreapp3.0, and consumers of data protection would call AddDataProtectionStartup as part of their AddXyz? That doesn't sound too bad to do.

@Tratcher
Copy link
Member

If DataProtectionStartupFilter is the only outlier then you might as well move everything else first. It doesn't even look like required functionality, it only moves latency from first request to startup.

@Tratcher
Copy link
Member

And yes it does look like it could be converted to an IHostedService, IHostedService.StartAsync is called about the same time as IStartupFilter.Configure. If converted then it could still be moved to Extensions.

@natemcmaster
Copy link
Contributor

natemcmaster commented Jun 14, 2019

It doesn't even look like required functionality, it only moves latency from first request to startup.

It's required. See aspnet/DataProtection#233. Key ring initialization needs to happen before the server begins accepting requires. It doesn't matter if this is implemented as an IStartFilter or a service or something else because the API is internal. I would strongly favor not creating a new assembly just for one internal-only API. If Microsoft.Extensions.Hosting.IHostedService is a suitable fit, that seems better.

Bear in mind -- moving it up the stack isn't really an option because you need to continue to ensure anywhere .AddDataProtection is called that the keyring initializing behavior is also added. If we move the API, it can only move down, such as into hosting. For example, moving it up the stack into the default WebHost builder wouldn't work because not everyone uses the default WebHost builder.

@HaoK
Copy link
Member

HaoK commented Jun 18, 2019

Okay I was able to move everything over to netstandard2.0 except the the EF based DP provider which requires netstandard 2.1 from EF (cc @ajcvickers)
There is one failure as a result which seems problematic, hosted service doesn't start early enough?

[xUnit.net 00:00:01.28]     Microsoft.AspNetCore.DataProtection.Test.HostingTests.LoadsKeyRingBeforeServerStarts [FAIL]
Failed   Microsoft.AspNetCore.DataProtection.Test.HostingTests.LoadsKeyRingBeforeServerStarts
Error Message:
 System.InvalidOperationException : Server was started before key ring was initialized
Stack Trace:
   at Microsoft.AspNetCore.Testing.TaskExtensions.TimeoutAfter[T](Task`1 task, TimeSpan timeout, String filePath, Int32 lineNumber)
   at Microsoft.AspNetCore.DataProtection.Test.HostingTests.LoadsKeyRingBeforeServerStarts() in C:\Github\AspNetCore\src\DataProtection\DataProtection\test\HostingTests.cs:line 45
--- End of stack trace from previous location where exception was thrown ---

@Tratcher
Copy link
Member

@HaoK See #11008 (comment)

@HaoK
Copy link
Member

HaoK commented Jun 21, 2019

DataProtection and extensions identity packages are targeting netstandard via #11008

@HaoK HaoK closed this as completed Jun 21, 2019
@HaoK HaoK added the Done This issue has been fixed label Jun 21, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-dataprotection Includes: DataProtection area-identity Includes: Identity and providers Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests