feat(general): Add alternate key derivation algorithms #3670
Conversation
|
this is interesting, I'm curious if there's any particular reason for this? is it to support memory-constrained environments? compliance? |
|
@jkowalski FIPS compliance. Besides the key derivation, Kopia can mostly be run in a FIPS compliant manner (supports selecting encryption and hashing etc). |
|
I mean to follow up this PR with changes that let you select the key derivation algorithm. |
|
@jkowalski I've simplified the PR a bit. What do you think of this approach? Are there any other changes you would like to see? |
internal/crypto/pbkdf.go
Outdated
| const pbkdf2Sha256Iterations = 1<<20 - 1<<18 // 786,432 | ||
|
|
||
| // The NIST recommended minimum size for a salt to be used for pbkdf2 | ||
| const minPbkdfSaltSize = 16 // size in bytes == 128 bits |
There was a problem hiding this comment.
A good rule of thumb is to use a salt that is the same size as the output of the hash function. For example, the output of SHA256 is 256 bits (32 bytes), so the salt should be at least 32 random bytes.
REF: https://crackstation.net/hashing-security.htm
Should not this be 32 instead for both scrypt and pbkdf2 ?
There was a problem hiding this comment.
I believe throughout the code we pass in a 32 byte salt so I can change this to make that the minimum. I picked 16 because it was the minimum recommended for NIST. I will update the comments with this information.
There was a problem hiding this comment.
Also, changing the salt size will break existing users, so it is required to be consistent with the existing code unless we have a key migration path forward.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3670 +/- ##
==========================================
+ Coverage 75.86% 77.10% +1.23%
==========================================
Files 470 476 +6
Lines 37301 28784 -8517
==========================================
- Hits 28299 22193 -6106
+ Misses 7071 4669 -2402
+ Partials 1931 1922 -9 ☔ View full report in Codecov by Sentry. |
Shrekster
left a comment
There was a problem hiding this comment.
LGTM, we can merge once the linters pass.
Looks like GitHub is misbehaving on this PR. @bathina2 and I attempted to push commits to this branch which are not showing up here. No further changes are visible, so we'll be reopening this PR. |
|
@jkowalski note that we had to duplicate the |
This PR adds the ability to select a different key derivation algorithm instead of
scrypt. We add thepbkdf2algorithm as an example. However, the default is stillscryptand we are not yet using thepbkdf2algorithm to generate keys yet.