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.

Sign up for GitHub

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

Jump to bottom

PBKDF2 one-shot #48107

Merged
merged 32 commits into from
Feb 22, 2021
Merged

PBKDF2 one-shot #48107

merged 32 commits into from
Feb 22, 2021

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Feb 10, 2021 •
edited
Loading

Work left to be done:

  • Wait for API approval and feedback (and modify PR accordingly)
    • XML documentation (TBD after approval)
  • Handle other platforms accordingly (Android, Browser)
  • Comprehensive tests and edge cases
    • Zero length values (empty password bytes, etc)
    • Overlapping buffers
  • Perf numbers on throughput and allocations.
  • We should use the throwing (non-substituting) version of UTF-8 when performing the chars -> bytes
  • Use BCryptKeyDerivation for Windows 8+.

Implementation notes:

  • In Improve performance of Rfc2898DeriveBytes #24897 I (incorrectly) determined that OpenSSL 1.0 did not have PKCS5_PBKDF2_HMAC. This was a PEBCAK error while reading documentation. So no shimming required.
  • MacOS, Linux, and Windows 7 should not allocate, assuming you are not using one of the allocating helpers. Windows 8+ will mostly be non-allocating. They will allocate a handle, though it can be removed. I am doing some research to determine if removing the handle allocation makes any material impact on real world use cases.

Closes #24897

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Feb 10, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

This is a draft for PBKDF2 one-shots (an API that is yet-to-be-approved). Opening as a draft to get some feedback from the CI system.

Work left to be done:

  • Wait for API approval and feedback (and modify PR accordingly)
  • Handle other platforms accordingly (Android, Browser)
  • XML documentation
  • Comprehensive tests and edge cases
    • Zero length values (empty password bytes, etc)
    • Overlapping buffers
  • Perf numbers on throughput and allocations.

Implementation notes:

  • In Improve performance of Rfc2898DeriveBytes #24897 I (incorrectly) determined that OpenSSL 1.0 did not have PKCS5_PBKDF2_HMAC. This was a PEBCAK error while reading documentation. So no shimming required.
  • MacOS, Linux, and Windows 10 should not allocate. Windows <= 10 will mostly be non-allocating. They will use cached algorithm handles.
Author: vcsjones
Assignees: -
Labels:

area-System.Security, new-api-needs-documentation
Milestone: -

@vcsjones
Copy link
Member Author

Unrelated but exciting: All of the macOS work was done on a M1 Mac Mini without Rosetta (running as ARM64). Very cool to see dotnet/runtime in good shape for Apple Silicon.

@vcsjones
Copy link
Member Author

Hm. Trying to use the pseudo handles in Windows 7 actually crashes the process with an access violation, so we can't even get an NTSTATUS back saying it failed. Do we just do an OS version check?

@vcsjones
Cleanup native PALs.
51b77c1 
Reorder to keep alphabetic consistency.

Add comment documentation.

Tighten validation between password and salt inputs
and their length. NULL input requires a zero length.
@vcsjones
Merge remote-tracking branch 'ms/master' into 24897-pbkdf2-one-shot
61140da
@vcsjones
Fix issue with not slicing the rented buffer.
0210441 
While here, use a stack allocation for small password inputs, which
seems likely.
@runfoapp runfoapp bot mentioned this pull request Feb 12, 2021
Closed
@vcsjones vcsjones marked this pull request as ready for review February 16, 2021 23:31
@vcsjones vcsjones changed the title [WIP] PBKDF2 one-shot PBKDF2 one-shot Feb 16, 2021
vcsjones
vcsjones commented Feb 16, 2021
View reviewed changes
Copy link
Member Author

@vcsjones vcsjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartonjs @GrabYourPitchforks I think this is at a point where it can be looked at in more scrutiny and might nearly resemble something complete, barring some self-review questions.

...m.Security.Cryptography.Algorithms/src/Internal/Cryptography/Pbkdf2Implementation.Windows.cs Show resolved Hide resolved
...m.Security.Cryptography.Algorithms/src/Internal/Cryptography/Pbkdf2Implementation.Windows.cs Show resolved Hide resolved
...urity.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.OneShot.cs Show resolved Hide resolved
@vcsjones
Copy link
Member Author

Apology in advance for changing the PR title and breaking email inbox threading.

GrabYourPitchforks
GrabYourPitchforks reviewed Feb 17, 2021
View reviewed changes
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the Windows implementation and skimmed the non-Windows implementations. Generally LGTM. Some open questions about whether avoiding the allocation is worth it in the Windows case.

src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptAlgPseudoHandle.cs Outdated Show resolved Hide resolved
src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptDeriveKeyPBKDF2.cs Outdated Show resolved Hide resolved
src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptGenerateSymmetricKey.cs Show resolved Hide resolved
...stem.Security.Cryptography.Algorithms/src/Internal/Cryptography/Pbkdf2Implementation.Unix.cs Show resolved Hide resolved
...urity.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.OneShot.cs Outdated Show resolved Hide resolved
...urity.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.OneShot.cs Show resolved Hide resolved
src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898OneShotTests.cs Show resolved Hide resolved
src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898OneShotTests.cs Show resolved Hide resolved
@vcsjones vcsjones requested a review from bartonjs February 18, 2021 20:02
@bartonjs bartonjs merged commit 0c47471 into dotnet:master Feb 22, 2021
@vcsjones vcsjones deleted the 24897-pbkdf2-one-shot branch February 22, 2021 20:55
@omajid omajid mentioned this pull request Mar 15, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks left review comments

@bartonjs bartonjs bartonjs approved these changes

Assignees
No one assigned
Labels
area-System.Security new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve performance of Rfc2898DeriveBytes
3 participants
@vcsjones @GrabYourPitchforks @bartonjs