-
Notifications
You must be signed in to change notification settings - Fork 22
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
Stateless password hash derivation with header specificity #13
Open
michaeljgray-sfdc
wants to merge
2
commits into
viniciuschiele:master
Choose a base branch
from
michaeljgray-sfdc:feature-header-based-derivation
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,4 +28,5 @@ nuget.exe | |
#*.sln | ||
#*.csproj | ||
#*.kproj | ||
*.lock.json | ||
*.lock.json | ||
*.vs |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is
_salt
a class attribute? This will makeEncode
not thread-safe anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @viniciuschiele! Thank you for the comment. The
Encode
method wasn't thread safe to begin with becauseRandomNumberGenerator.GetBytes
is not thread safe. A general practice most follow is to make the caller responsible for resource locking for thread safety related to individual methods, unless the class itself is running multiple threads internally or using overlapping callbacks of some kind. The exception to this rule is usually for methods markedstatic
, which should ensure their own thread safety. Let me know if you have any other feedback. Thanks!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, based on this,
RNGCryptoServiceProvider
is thread-safe andRandomNumberGenerator.Create()
also returns aRNGCryptoServiceProvider
by default based on that.My concern is that this change can potentially break existing apps, that's why I think this shouldn't be part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification. After some research based on your comment, I see the issue now. I agree on the point of it breaking existing apps. Even though this varies by implementation, it shouldn't cause a behavioral change that breaks existing consumers of the library who use older versions of .NET, since you still support them. I'll go ahead and remove that change and return it to the original with a new commit tomorrow.
I did want to point out that in .NET Core and .NET Framework, the implementation now just invokes Windows API's BCryptGenRandom on Windows and OpenSSL's GetRandomBytes on Unix, so new consumers should not rely on thread safety of this method in modern codebases.
I really appreciate your thoughtful response and for exercising care around breaking your users. Thank you again and you can expect the commit tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing the links to the source code, for me both implementations seem thread-safe and I couldn't find anything to say otherwise.
I agree that this may vary based on the implementation, I wonder if it would be better to instantiate a new
RandomNumberGenerator
every time theEncode
is called.