feat(general): add alternate key derivation algorithms #3725
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3725 +/- ##
==========================================
+ Coverage 75.86% 77.09% +1.22%
==========================================
Files 470 476 +6
Lines 37301 28784 -8517
==========================================
- Hits 28299 22190 -6109
+ Misses 7071 4672 -2399
+ Partials 1931 1922 -9 ☔ View full report in Codecov by Sentry. |
| const DefaultKeyDerivationAlgorithm = "testing-only-insecure" | ||
|
|
||
| // MasterKeyLength describes the length of the master key. | ||
| const MasterKeyLength = 32 |
There was a problem hiding this comment.
Is this used anywhere? or is it coming in a later PR?
julio-lopez
left a comment
There was a problem hiding this comment.
@bathina2 PTAL at the inline comments
|
|
||
| default: | ||
| // RegisterKeyDerivationFunc registers various key derivation functions. | ||
| func RegisterKeyDerivationFunc(name string, keyDeriver keyDerivationFunc) { |
There was a problem hiding this comment.
The name of this function should explicitly convey that it is registering a password-based KDF. Maybe RegisterPasswordBasedKeyDeriver or RegisterPBKDF
Essentially, it should be explicit that this is registering an implementation for DeriveKeyFromPassword below.
| var keyDerivers = map[string]keyDerivationFunc{} | ||
|
|
||
| default: | ||
| // RegisterKeyDerivationFunc registers various key derivation functions. |
There was a problem hiding this comment.
The comment should make it explicit that this is to be used for password-based KDFs. In other words, it seems that the separation between password-based and key-based derivation functions should be made explicit for clarity.
| func init() { | ||
| RegisterKeyDerivationFunc(ScryptAlgorithm, func(password string, salt []byte) ([]byte, error) { | ||
| if len(salt) < minScryptSha256SaltSize { | ||
| return nil, errors.Errorf("required salt size is atleast %d bytes", minPbkdfSha256SaltSize) |
There was a problem hiding this comment.
nit: fix typo.
return nil, errors.Errorf("required salt size is at least %d bytes", minPbkdfSha256SaltSize)
…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.
| h := computePasswordHashV1(password, salt) | ||
| h, err := computePasswordHashV1(password, salt, keyDerivationAlgorithm) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
This should propagate the error instead of calling panicking.
Previously, computePasswordHashV1 could panic if scrypt.Key returned an error. scrypt.Key only returns an error when "... the parameters do not satisfy the limits" https://pkg.go.dev/golang.org/x/crypto/scrypt#Key
The parameters specified to scrypt.Key were constants within the valid range, thus an error was expected to never be returned, and the panic was an "impossibility".
In this implementation, it is possible for crypto.DeriveKeyFromPassword to return an error for other conditions, such an invalid key derivation algorithm. This means, that the panic here is no longer an "impossibility", and instead the error should be handled or propagated.
This is addressed in #3907
Refer to #3670 for discussion