aspnet/DataProtection

Consider adding a parameterless DataProtectionProvider.Create overload

Closed this issue · 13 comments

Currently, DataProtectionProvider.Create always overrides the default key repository by a file system repository pointing to the keyDirectory parameter.

That would be great if we had a parameterless overload that let the data protection block decide itself what's the most appropriate place to store the crypto keys (file system, registry, in-memory) instead of overriding it.

https://github.com/aspnet/DataProtection/blob/dev/src/Microsoft.AspNetCore.DataProtection.Extensions/DataProtectionProvider.cs

Having such an overload would make the authentication ticket serializer compat' package much easier to use in non-vNext apps.

@Eilon any chance we could have that for RC2? I'd really like to use that in the OWIN/Katana version of ASOS (see aspnet-contrib/AspNet.Security.OpenIdConnect.Server#188).

/cc @blowdart @javiercn

FYI, the reason this overload didn't exist in the first place is because part of the security of the system hinges on the ability to isolate applications from one another, and outside the context of the ASP.NET hosting environment there's no good way to get this information automatically from the ambient environment. Forcing the directory to be specified manually provides this isolation. If you want an overload that doesn't require specifying a directory, then you really should at least force the developer to provide an application name or similar to regain the isolation that was lost. In the source code this is referred to as the "discriminator".

The "using DataProtection in ASP.NET 4.x applications" sample works around this by using the IIS metabase path as a unique identifier so that the developer doesn't need to worry about specifying an isolation identifier. Unfortunately this trick doesn't work for self-hosted ASP.NET 4.x apps.

/cc @blowdart

Oh dear. I did not realise this.

@ajaybhargavb time to change your work I'm afraid.

@PinpointTownes you're not going to get what you want. Is it worth doing this at all?

@PinpointTownes you're not going to get what you want. Is it worth doing this at all?

I've been asked to provide interoperability between ASOS vNext and the OWIN/Katana version. For that, I need to use the same ticket format and the same data protection stack.

Why not adding a discriminator parameter, as suggested by @GrabYourPitchforks?

TBH, if Microsoft.AspNetCore.DataProtection.SystemWeb offered a way to retrieve an IDataProtectionProvider, I'd happily use it. Sadly, pretty much everything in this package is internal, and AFAICT, there's no easy way to achieve what I want.

Would you prefer if Microsoft.AspNetCore.DataProtection.SystemWeb exposed a public (static) Create method, instead of having such an extension on DataProtectionProvider?

BTW, it's not a demand specific to ASOS. People who'll want to share vNext's authentication cookies with a Katana app will be facing the same situation.

OK, if the discriminator works for you - @ajaybhargavb can you make it so?

As for sharing cookies, we have a package for that anyway.

Already updated 😄

As for sharing cookies, we have a package for that anyway.

But the whole problem is that this package doesn't handle the data protector instantiation. It's still up to the developer to create it. And with the existing stuff, it's too painful.

@ajaybhargavb Dear lord. You young people with your eagerness.

Ideally, we'd also need a SystemWebDataProtectionProvider.Create() method that would do the same thing as DataProtectionProvider.Create but would internally use SystemWebApplicationDiscriminator to provide isolation.

I've said it before to you @PinpointTownes that the more options we add the more we move away from the "Fire and forget" nature of the data protector shrug

On the other hand, the less options you provide, the more people will have to roll their own thing to achieve what their want, which seems far more risky than providing a built-in method.

Personally, I'm fine with just DataProtectionProvider.Create as it should cover most "use DP vNext in Katana apps" scenarios. Requiring an explicit application name to provide isolation shouldn't be too painful.