Skip to content
This repository has been archived by the owner on Oct 17, 2018. It is now read-only.

Consider adding a parameterless DataProtectionProvider.Create overload #130

Closed
kevinchalet opened this issue Mar 17, 2016 · 13 comments
Closed
Assignees
Milestone

Comments

@kevinchalet
Copy link

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

@GrabYourPitchforks
Copy link
Contributor

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

@blowdart
Copy link
Member

blowdart commented Apr 5, 2016

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?

@kevinchalet
Copy link
Author

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

@kevinchalet
Copy link
Author

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.

@blowdart
Copy link
Member

blowdart commented Apr 5, 2016

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

@blowdart
Copy link
Member

blowdart commented Apr 5, 2016

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

@ajaybhargavb
Copy link
Contributor

Already updated 😄

@kevinchalet
Copy link
Author

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.

@blowdart
Copy link
Member

blowdart commented Apr 5, 2016

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

@kevinchalet
Copy link
Author

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.

@blowdart
Copy link
Member

blowdart commented Apr 5, 2016

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

@kevinchalet
Copy link
Author

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.

@kevinchalet
Copy link
Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants