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

Use the framework implementation of PBKDF2/RFC2898 #30941

Closed
omajid opened this issue Mar 15, 2021 · 9 comments · Fixed by #31200
Closed

Use the framework implementation of PBKDF2/RFC2898 #30941

omajid opened this issue Mar 15, 2021 · 9 comments · Fixed by #31200
Assignees
Labels
affected-most This issue impacts most of the customers area-dataprotection Includes: DataProtection Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! severity-major This label is used by an internal tool
Milestone

Comments

@omajid
Copy link
Member

omajid commented Mar 15, 2021

This is a follow up from aspnet/DataProtection#272, #2508 and dotnet/runtime#24897.

ASP.NET has its own implementation of RFC2898 because it needs top-of-the-line performance for password hashing in order to have acceptable latency for login scenarios. If we bring these performance improvements to the framework's implementation of these APIs, ASP.NET can remove its own implementation and rely on the standard classes.

This was recently fixed via dotnet/runtime#48107.

This bug/feature-request is to track that ASP.NET Core picks up and uses that API and removes its duplicate implementation.

cc @bartonjs @blowdart @GrabYourPitchforks @tmds @Tornhoof @vcsjones

@blowdart
Copy link
Contributor

@HaoK

@HaoK HaoK added this to the Next sprint planning milestone Mar 15, 2021
@ghost
Copy link

ghost commented Mar 15, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@HaoK
Copy link
Member

HaoK commented Mar 15, 2021

Cool, I'll combine this into the existing span issue

@HaoK HaoK added affected-most This issue impacts most of the customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-major This label is used by an internal tool labels Mar 15, 2021 — with ASP.NET Core Issue Ranking
@HaoK HaoK added the Working label Mar 24, 2021
@ghost ghost closed this as completed in #31200 Mar 30, 2021
@HaoK HaoK added Done This issue has been fixed ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! and removed Working labels Mar 30, 2021
@omajid
Copy link
Member Author

omajid commented Mar 31, 2021

Hi @HaoK ! Are there any plans to remove the managed implementation and/or Win7Pbkdf2Provider/Win8Pbkdf2Provider implementations? Will those be kept around for compatibility?

@HaoK
Copy link
Member

HaoK commented Mar 31, 2021

Unfortunately we still have to support netstandard and net461, we can't use the new one shot pbdkdf2 in those target frameworks right?

@HaoK
Copy link
Member

HaoK commented Mar 31, 2021

Is the new API faster than the Win8/Win7 implementations as well? We can switch the code to always use the NetCore provider when available and only use the Win8/7/Managed implementations on netstandard/net461

@vcsjones
Copy link
Member

we can't use the new one shot pbdkdf2 in those target frameworks right?

Correct. It is .NET 6.

Is the new API faster than the Win8/Win7 implementations as well?

It should be "as fast or better". It borrows many ideas from the DataProtection implementation for Windows, and has a slight allocation improvement for Windows 10+.

On MacOS it uses CommonCrypto from the underlying OS, and for Linux it uses openssl.

@HaoK
Copy link
Member

HaoK commented Mar 31, 2021

Cool, so sounds like we should just switch to always using this on netcoreapp, and keeping the old branching providers only for non netcoreapp, I'll open a PR tomorrow, thanks for pointing this out

@mkArtakMSFT mkArtakMSFT added Working and removed Done This issue has been fixed ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! labels Apr 1, 2021
@HaoK HaoK added the Done This issue has been fixed label Apr 15, 2021
@HaoK HaoK closed this as completed Apr 15, 2021
@ghost ghost removed the Working label Apr 15, 2021
@HaoK HaoK added the ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! label Apr 15, 2021
@omajid
Copy link
Member Author

omajid commented Apr 15, 2021

Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-most This issue impacts most of the customers area-dataprotection Includes: DataProtection Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! severity-major This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants