feat(general): User profile hashing version to algorithm translation #3816
Conversation
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
bathina2
changed the title
bathina2
changed the title
julio-lopez
reviewed
Apr 24, 2024
julio-lopez
reviewed
Apr 24, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3816 +/- ##
==========================================
+ Coverage 75.86% 77.06% +1.20%
==========================================
Files 470 473 +3
Lines 37301 28680 -8621
==========================================
- Hits 28299 22103 -6196
+ Misses 7071 4686 -2385
+ Partials 1931 1891 -40 ☔ View full report in Codecov by Sentry. |
julio-lopez
approved these changes
Apr 25, 2024
There was a problem hiding this comment.
🥇Thanks @bathina2 for doing this.
LG. Couple of minor nits. Those can be addressed separately after merging.
Thanks again.
| Pbkdf2Algorithm = "pbkdf2-sha256-600000" | ||
|
|
||
| // DefaultKeyDerivationAlgorithm is the key derivation algorithm for new configurations. | ||
| DefaultKeyDerivationAlgorithm = ScryptAlgorithm |
There was a problem hiding this comment.
nit: it seems that the default should be "testing-only-insecure"
There was a problem hiding this comment.
This breaks tests, since we have separated the hash versions from the key derivation algorithms.
I would have to handle testing-only-insecure in the internal/user package but that would mean introducing testing code in production
| // Different key derivation algorithm besides the original should fail | ||
| p.KeyDerivationAlgorithm = crypto.Pbkdf2Algorithm | ||
| // Assume the key derivation algorithm is bad. This will cause | ||
| // a panic when validating |
There was a problem hiding this comment.
nit: Is the comment accurate? does it panic?
| algorithms := crypto.AllowedKeyDerivationAlgorithms() | ||
| // if the user is invalid, return false but use the same amount of time as when we | ||
| // if the Username is invalid, return false but use the same amount of time as when we |
There was a problem hiding this comment.
nit:
Suggested change
| // if the Username is invalid, return false but use the same amount of time as when we | |
| // if the user profile is invalid (either invalid user name or password hash version, return false but use the same amount of time as when we |
This was referenced Apr 25, 2024
julio-lopez
added a commit
that referenced
this pull request
Apr 27, 2024
…3821) Code movement and simplification, no functional changes. Objectives: - Allow callers specifying the needed key (or hash) size, instead of hard-coding it in the registered PBK derivers. Conceptually, the caller needs to specify the key size, since that is a requirement of the (encryption) algorithm being used in the caller. Now, the code changes here do not result in any functional changes since the key size is always 32 bytes. - Remove a global definition for the default PB key deriver to use. Instead, each of the 3 use case sets the default value. Changes: - `crypto.DeriveKeyFromPassword` now takes a key size. - Adds new constants for the key sizes at the callers. - Removes the global `crypto.MasterKeySize` const. - Removes the global `crypto.DefaultKeyDerivationAlgorithm` const. - Adds const for the default derivation algorithms for each use case. - Adds a const for the salt length in the `internal/user` package, to ensure the same salt length is used in both hash versions. - Unexports various functions, variables and constants in the `internal/crypto` & `internal/user` packages. - Renames various constants for consistency. - Removes unused functions and symbols. - Renames files to be consistent and better reflect the structure of the code. - Adds a couple of tests to ensure the const values are in sync and supported. - Fixes a couple of typos Followups to: - #3725 - #3770 - #3779 - #3799 - #3816 The individual commits show the code transformations to simplify the review of the changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR reverts the behavior of storing the key derivation algorithm in the profile.
We will continue to use the
PasswordHashVersionand have internal tooling to translate to the key derivation algorithm (password hashing algorithm).