dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.3k stars 9.97k forks source link

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

Closed kevinchalet closed 5 years ago

kevinchalet commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

(Tagging @davidfowl because he started this)

kevinchalet commented 5 years ago

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.

philmcmillan commented 5 years ago

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 commented 5 years ago

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

philmcmillan commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/aspnet/AspNetCore/issues/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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

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

kevinchalet commented 5 years ago

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 :smile:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

dlandi commented 5 years ago

@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.

thuannguy commented 5 years ago

DP has become my favorite tool whenever I have a need to protect data, regardless of what environment it is used for 😟 As Murphy's law said: "whatever can happen will happen" 😈

clairernovotny commented 5 years ago

We've encountered use of DP API in desktop applications from different clients. In that context, it's generally been used to secure credentials or other secrets entered by the user in the application settings.

I'd suggest that DP API has a lot of uses in the interactive desktop scenarios as is in scope for .NET Core 3.

davidfowl commented 5 years ago

Maybe worth making a proper System.* API for it.

kevinchalet commented 5 years ago

@davidfowl it's certainly an option, but it also means you won't be able to make breaking changes to DP if it's part of CoreFX, even in major releases (and the review process will be fairly complicated, as the API surface is quite large).

I'm not sure it's worth going that far: moving DP to Microsoft.Extensions.*, removing the hosting dependency and keeping it .NET Standard-compatible would be an excellent compromise. The cost of doing that would be minimal and you could still introduce breaking changes in future versions if needed.

Tratcher commented 5 years ago

The bar for breaking changes isn't as different as you imply. You can also expect the APIs to get heavily reviewed if we're going to expand the supported scenarios.

kevinchalet commented 5 years ago

The bar for breaking changes isn't as different as you imply.

Each major version of ASP.NET Core was released with many breaking changes (in 2.0, the most important one was authentication, in 3.0, it will be the hosting stuff). I'm not aware of any API removal in .NET Core/CoreFX itself. Have I missed something?

You can also expect the APIs to get heavily reviewed if we're going to expand the supported scenarios.

Correct me if I'm wrong, but the review process you use for ASP.NET Core seems way different (much simpler) than the one used for .NET Core (each API change must come with a formal API proposal, which is heavily reviewed - potentially during a meeting - before one can even start working on the implementation).

It's worth mentioning that no one here has asked for features that are not already supported by DP. We just want it to keep supporting .NET Standard, which is already the case.

Tratcher commented 5 years ago

You've asked for support of non-asp.net scenarios. The biggest difference is that most asp.net scenarios protect ephimeral data, the risks for data loss are minimal. It's not clear the current APIs and implementation are suitable for other scenarios.

kevinchalet commented 5 years ago

You've asked for support of non-asp.net scenarios. The biggest difference is that most asp.net scenarios protect ephimeral data, the risks for data loss are minimal.

Using DP outside of ASP.NET Core doesn't mean using it for long-lived data (in my specific case, I want to use it for access tokens, that typically expire after a few hours max).

Anyway the documentation clearly states that you can already use DP to protect long-lived data, tho' it's not the best option for that:

The ASP.NET Core data protection APIs are not primarily intended for indefinite persistence of confidential payloads. Other technologies like Windows CNG DPAPI and Azure Rights Management are more suited to the scenario of indefinite storage, and they have correspondingly strong key management capabilities. That said, there's nothing prohibiting a developer from using the ASP.NET Core data protection APIs for long-term protection of confidential data.

https://docs.microsoft.com/en-us/aspnet/core/security/data-protection/introduction?view=aspnetcore-2.2#design-philosophy

If DP moves to Microsoft.Extensions.* as-is, the same design principles will still apply: you'll be able to encrypt/decrypt long-lived data, but for that, you'll have to keep the key ring intact (DP itself never deletes keys, even revoked ones).

philmcmillan commented 5 years ago

You've asked for support of non-asp.net scenarios. The biggest difference is that most asp.net scenarios protect ephimeral data, the risks for data loss are minimal. It's not clear the current APIs and implementation are suitable for other scenarios.

Yes - but I have a scenario in which we exchange messages internally in our systems via queuing and APIs. These messages have the holy trinity of PII at times. While we can protect the transport - we also want to protect the payload when at rest in a queue or serialized to a file system drop for pickup by another endpoint. The various apps/services that participate are a set of full framework .NET services, legacy ASP.NET apps on full framework, WCF services, and Core MVC apps. The fact that I could potentially create a data protection instance that uses a shared key repository would allow me to encrypt those payloads for protection at rest, but be able decrypt and read them as they get passed along in various queues and servers.

This is ephemeral data, it only exists at for a few minutes at most, and generally for less than 5 seconds. I don't want to do some kind of hugely complex PKI solution due to the operational headache of maintaining and distributing keys and certificates and dealing with key revocation/cert renewal. DPAPI would force us to run all of our apps and services as the same domain account to have access to a shared key. Untenable, we need to be able to assign least privilege to different app pools, service accounts, etc, so that we limit each app and service to its appropriate scope of access to resources.

DP is a near perfect solution, but not at the cost of having to import the whole ASP.NET Core platform, and not at the expense of potential issues with support for netstandard and the full framework that could potentially cause runtime errors in the future due to compatibility issues.

Tagging @natemcmaster and @davidfowl in hopes they see this as well; while I understand concerns about use in scenarios for which DP was not designed/envisioned and support issues related to key management and long term data encryption, this is a specific scenario for ephemeral data that exists outside of just ASP.NET and Core.

muratg commented 5 years ago

I don't expect we'll make this change at this point, but CCing @davidfowl @DamianEdwards. Let's make the call here.

kevinchalet commented 5 years ago

It's important to note that Identity depends on DP packages. If the Identity core packages stay in the Microsoft.Extensions.* namespace like in 2.x, DP will have to be netstandard-compatible.

More context:

blowdart commented 5 years ago

So summary time.

We discussed this at length, and yes, some of you have used dataprotection in really weird ways. But that's ok, we're not shaming you for.

Both Data Protection and Identity will remain targeted at .NET Standard 2.0 to enable you to continue being weird (with the caveat that the UI components for identity might not, depending on if we need 3.0 features there)

HOWEVER there will not be a namespace renaming because that would be horrible for consumers, although we might move the repos to extensions.

kevinchalet commented 5 years ago

natemcmaster commented 5 years ago

although we might move the repos to extensions.

I vote we keep it as is. No compelling reason to move it.

We will need to remove DP's dependency on Microsoft.AspNetCore.Hosting (FYI @Tratcher @davidfowl). We need to work through the options for replacing this dependency.

HaoK commented 5 years ago

Yeah splitting identity sucks, lets not do that unless there's a reason we have to

blowdart commented 5 years ago

@natemcmaster is the dependency for the discriminator and for the automatic guess at where to put things?

natemcmaster commented 5 years ago

is the dependency for the discriminator ?

IIRC, yes. We tried to fix this a while ago by flipping the dependency direction so Hosting -> DataProtection, but that upset @davidfowl.

blowdart commented 5 years ago

That's a bonus then surely?

kevinchalet commented 5 years ago

If having Hosting depend on DP is a problem, couldn't the IApplicationDiscriminator be registered by the default host builder? (the static WebHost class, or whatever it's called in 3.0)

Tratcher commented 5 years ago

I've coincidentally improved this by changing HostingApplicationDiscriminator to use Microsoft.Extensions.Hosting.IHostEnvironment instead of Microsoft.AspNetCore.Hosting.IHostingEnvironment.

kevinchalet commented 5 years ago

@Tratcher that's certainly an improvement, tho' in an ideal world, you may probably still want to decouple DP from Hosting, so that libs that don't need hosting abstractions don't have to reference it transitively (e.g OWIN apps that use the compat' package).

Tratcher commented 5 years ago

Understood, but that makes the decoupling easier when we get to it.

HaoK commented 5 years ago

@ajcvickers

ajcvickers commented 5 years ago

@Tratcher @blowdart @Eilon Is there anything blocking this from happening? Could we get it in soon so that we can react in Identity and we can get feedback from customers?

natemcmaster commented 5 years ago

I don’t believe anything is blocking this. I see both @HaoK and I are still assigned. It’s unclear which of us is actually taking point on this, so first order of business should be to clarify that. I think I can probably squeeze this in to my workload over the next few weeks, but no promises…a lot of other work is happening for P6 that’s higher priority for me than this.

ajcvickers commented 5 years ago

Thanks Nate. I think @HaoK can do it, if we're good to pull the trigger.

Tratcher commented 5 years ago

Go ahead.

Eilon commented 5 years ago

I have no objection.

HaoK commented 5 years ago

Need to do this early in preview 7

HaoK commented 5 years ago

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?