Repository: kopia/kopia
Pull request #3725 (2024-03-13 00:18:15)

feat(general): Add alternate key derivation algorithms
Refer to https://github.com/kopia/kopia/pull/3670 for discussion

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kopia/kopia/pull/3725?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) Report
Attention: Patch coverage is `86.95652%` with `3 lines` in your changes are missing coverage. Please review.
> Project coverage is 77.09%. Comparing base [(`cb455c6`)](https://app.codecov.io/gh/kopia/kopia/commit/cb455c699a04e54cb47e25a676d7d65063ce883e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) to head [(`b903583`)](https://app.codecov.io/gh/kopia/kopia/pull/3725?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).
> Report is 55 commits behind head on master.

| [Files](https://app.codecov.io/gh/kopia/kopia/pull/3725?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) | Patch % | Lines |
|---|---|---|
| [cli/command\_user\_add\_set.go](https://app.codecov.io/gh/kopia/kopia/pull/3725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-Y2xpL2NvbW1hbmRfdXNlcl9hZGRfc2V0Lmdv) | 0.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/3725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |
| [internal/user/user\_profile\_hash\_v1.go](https://app.codecov.io/gh/kopia/kopia/pull/3725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-aW50ZXJuYWwvdXNlci91c2VyX3Byb2ZpbGVfaGFzaF92MS5nbw==) | 92.30% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/3725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            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     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/kopia/kopia/pull/3725?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).


Patch:
diff --git a/cli/command_user_add_set.go b/cli/command_user_add_set.go
index f98b51e3..14589db5 100644
--- a/cli/command_user_add_set.go
+++ b/cli/command_user_add_set.go
@@ -7,6 +7,7 @@
 	"github.com/alecthomas/kingpin/v2"
 	"github.com/pkg/errors"
 
+	"github.com/kopia/kopia/internal/crypto"
 	"github.com/kopia/kopia/internal/user"
 	"github.com/kopia/kopia/repo"
 )
@@ -74,7 +75,7 @@ func (c *commandServerUserAddSet) runServerUserAddSet(ctx context.Context, rep r
 	if p := c.userSetPassword; p != "" {
 		changed = true
 
-		if err := up.SetPassword(p); err != nil {
+		if err := up.SetPassword(p, crypto.DefaultKeyDerivationAlgorithm); err != nil {
 			return errors.Wrap(err, "error setting password")
 		}
 	}
@@ -107,7 +108,7 @@ func (c *commandServerUserAddSet) runServerUserAddSet(ctx context.Context, rep r
 
 		changed = true
 
-		if err := up.SetPassword(pwd); err != nil {
+		if err := up.SetPassword(pwd, crypto.DefaultKeyDerivationAlgorithm); err != nil {
 			return errors.Wrap(err, "error setting password")
 		}
 	}
diff --git a/internal/auth/authn_repo.go b/internal/auth/authn_repo.go
index 9df1373d..67466e65 100644
--- a/internal/auth/authn_repo.go
+++ b/internal/auth/authn_repo.go
@@ -6,6 +6,7 @@
 	"time"
 
 	"github.com/kopia/kopia/internal/clock"
+	"github.com/kopia/kopia/internal/crypto"
 	"github.com/kopia/kopia/internal/user"
 	"github.com/kopia/kopia/repo"
 )
@@ -51,7 +52,7 @@ func (ac *repositoryUserAuthenticator) IsValid(ctx context.Context, rep repo.Rep
 
 	// IsValidPassword can be safely called on nil and the call will take as much time as for a valid user
 	// thus not revealing anything about whether the user exists.
-	return ac.userProfiles[username].IsValidPassword(password)
+	return ac.userProfiles[username].IsValidPassword(password, crypto.DefaultKeyDerivationAlgorithm)
 }
 
 func (ac *repositoryUserAuthenticator) Refresh(ctx context.Context) error {
diff --git a/internal/crypto/pbkdf.go b/internal/crypto/pbkdf.go
new file mode 100644
index 00000000..812bafe3
--- /dev/null
+++ b/internal/crypto/pbkdf.go
@@ -0,0 +1,31 @@
+//go:build !testing
+// +build !testing
+
+package crypto
+
+import (
+	"crypto/sha256"
+
+	"github.com/pkg/errors"
+	"golang.org/x/crypto/pbkdf2"
+)
+
+// The NIST recommended iterations for PBKDF2 with SHA256 hash is 600,000.
+const pbkdf2Sha256Iterations = 1<<20 - 1<<18 // 786,432
+
+// The NIST recommended minimum size for a salt for pbkdf2 is 16 bytes.
+// However, 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.
+// See: https://crackstation.net/hashing-security.htm
+const minPbkdfSha256SaltSize = 32 // size in bytes == 256 bits
+
+func init() {
+	RegisterKeyDerivationFunc(Pbkdf2Algorithm, func(password string, salt []byte) ([]byte, error) {
+		if len(salt) < minPbkdfSha256SaltSize {
+			return nil, errors.Errorf("required salt size is atleast %d bytes", minPbkdfSha256SaltSize)
+		}
+
+		return pbkdf2.Key([]byte(password), salt, pbkdf2Sha256Iterations, MasterKeyLength, sha256.New), nil
+	})
+}
diff --git a/internal/crypto/scrypt.go b/internal/crypto/scrypt.go
new file mode 100644
index 00000000..0ad3e932
--- /dev/null
+++ b/internal/crypto/scrypt.go
@@ -0,0 +1,28 @@
+//go:build !testing
+// +build !testing
+
+package crypto
+
+import (
+	"github.com/pkg/errors"
+	"golang.org/x/crypto/scrypt"
+)
+
+// The recommended minimum size for a salt to be used for scrypt
+// 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.
+// Scrypt uses a SHA256 hash function.
+// https://crackstation.net/hashing-security.htm
+const minScryptSha256SaltSize = 32 // size in bytes == 256 bits
+
+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)
+		}
+
+		//nolint:wrapcheck,gomnd
+		return scrypt.Key([]byte(password), salt, 65536, 8, 1, MasterKeyLength)
+	})
+}
diff --git a/internal/user/user_profile.go b/internal/user/user_profile.go
index 6001a048..709eaaf0 100644
--- a/internal/user/user_profile.go
+++ b/internal/user/user_profile.go
@@ -14,23 +14,23 @@ type Profile struct {
 }
 
 // SetPassword changes the password for a user profile.
-func (p *Profile) SetPassword(password string) error {
-	return p.setPasswordV1(password)
+func (p *Profile) SetPassword(password, keyDerivationAlgorithm string) error {
+	return p.setPasswordV1(password, keyDerivationAlgorithm)
 }
 
 // IsValidPassword determines whether the password is valid for a given user.
-func (p *Profile) IsValidPassword(password string) bool {
+func (p *Profile) IsValidPassword(password, keyDerivationAlgorithm string) bool {
 	if p == nil {
 		// if the user is invalid, return false but use the same amount of time as when we
 		// compare against valid user to avoid revealing whether the user account exists.
-		isValidPasswordV1(password, dummyV1HashThatNeverMatchesAnyPassword)
+		isValidPasswordV1(password, dummyV1HashThatNeverMatchesAnyPassword, keyDerivationAlgorithm)
 
 		return false
 	}
 
 	switch p.PasswordHashVersion {
 	case hashVersion1:
-		return isValidPasswordV1(password, p.PasswordHash)
+		return isValidPasswordV1(password, p.PasswordHash, keyDerivationAlgorithm)
 
 	default:
 		return false
diff --git a/internal/user/user_profile_hash_v1.go b/internal/user/user_profile_hash_v1.go
index a0cc949f..ab8c3514 100644
--- a/internal/user/user_profile_hash_v1.go
+++ b/internal/user/user_profile_hash_v1.go
@@ -6,54 +6,56 @@
 	"io"
 
 	"github.com/pkg/errors"
-	"golang.org/x/crypto/scrypt"
+
+	"github.com/kopia/kopia/internal/crypto"
 )
 
 // parameters for v1 hashing.
 const (
 	hashVersion1 = 1
 
-	v1ScryptN    = 65536
-	v1ScryptR    = 8
-	v1ScryptP    = 1
 	v1SaltLength = 32
-	v1KeyLength  = 32
 )
 
 //nolint:gochecknoglobals
-var dummyV1HashThatNeverMatchesAnyPassword = make([]byte, v1KeyLength+v1SaltLength)
+var dummyV1HashThatNeverMatchesAnyPassword = make([]byte, crypto.MasterKeyLength+v1SaltLength)
 
-func (p *Profile) setPasswordV1(password string) error {
+func (p *Profile) setPasswordV1(password, keyDerivationAlgorithm string) error {
 	salt := make([]byte, v1SaltLength)
 	if _, err := io.ReadFull(rand.Reader, salt); err != nil {
 		return errors.Wrap(err, "error generating salt")
 	}
 
+	var err error
+
 	p.PasswordHashVersion = 1
-	p.PasswordHash = computePasswordHashV1(password, salt)
+	p.PasswordHash, err = computePasswordHashV1(password, salt, keyDerivationAlgorithm)
 
-	return nil
+	return err
 }
 
-func computePasswordHashV1(password string, salt []byte) []byte {
-	key, err := scrypt.Key([]byte(password), salt, v1ScryptN, v1ScryptR, v1ScryptP, v1KeyLength)
+func computePasswordHashV1(password string, salt []byte, keyDerivationAlgorithm string) ([]byte, error) {
+	key, err := crypto.DeriveKeyFromPassword(password, salt, keyDerivationAlgorithm)
 	if err != nil {
-		panic("unexpected scrypt error")
+		return nil, errors.Wrap(err, "error deriving key from password")
 	}
 
 	payload := append(append([]byte(nil), salt...), key...)
 
-	return payload
+	return payload, nil
 }
 
-func isValidPasswordV1(password string, hashedPassword []byte) bool {
-	if len(hashedPassword) != v1SaltLength+v1KeyLength {
+func isValidPasswordV1(password string, hashedPassword []byte, keyDerivationAlgorithm string) bool {
+	if len(hashedPassword) != v1SaltLength+crypto.MasterKeyLength {
 		return false
 	}
 
 	salt := hashedPassword[0:v1SaltLength]
 
-	h := computePasswordHashV1(password, salt)
+	h, err := computePasswordHashV1(password, salt, keyDerivationAlgorithm)
+	if err != nil {
+		panic(err)
+	}
 
 	return subtle.ConstantTimeCompare(h, hashedPassword) != 0
 }
diff --git a/repo/open.go b/repo/open.go
index 2186e2d8..3eef8bf9 100644
--- a/repo/open.go
+++ b/repo/open.go
@@ -10,7 +10,6 @@
 	"time"
 
 	"github.com/pkg/errors"
-	"golang.org/x/crypto/scrypt"
 
 	"github.com/kopia/kopia/internal/cache"
 	"github.com/kopia/kopia/internal/cacheprot"
@@ -141,11 +140,10 @@ func getContentCacheOrNil(ctx context.Context, opt *content.CachingOptions, pass
 		return nil, errors.Wrap(err, "error opening storage")
 	}
 
-	// derive content cache key from the password & HMAC secret using scrypt.
-	salt := append([]byte("content-cache-protection"), opt.HMACSecret...)
+	// derive content cache key from the password & HMAC secret
+	saltWithPurpose := append([]byte("content-cache-protection"), opt.HMACSecret...)
 
-	//nolint:gomnd
-	cacheEncryptionKey, err := scrypt.Key([]byte(password), salt, 65536, 8, 1, 32)
+	cacheEncryptionKey, err := crypto.DeriveKeyFromPassword(password, saltWithPurpose, crypto.DefaultKeyDerivationAlgorithm)
 	if err != nil {
 		return nil, errors.Wrap(err, "unable to derive cache encryption key from password")
 	}


Test patch:
diff --git a/internal/auth/authn_repo_test.go b/internal/auth/authn_repo_test.go
index 8324bcf7..27cd9e45 100644
--- a/internal/auth/authn_repo_test.go
+++ b/internal/auth/authn_repo_test.go
@@ -7,6 +7,7 @@
 	"github.com/stretchr/testify/require"
 
 	"github.com/kopia/kopia/internal/auth"
+	"github.com/kopia/kopia/internal/crypto"
 	"github.com/kopia/kopia/internal/repotesting"
 	"github.com/kopia/kopia/internal/user"
 	"github.com/kopia/kopia/repo"
@@ -22,7 +23,7 @@ func(ctx context.Context, w repo.RepositoryWriter) error {
 				Username: "user1@host1",
 			}
 
-			p.SetPassword("password1")
+			p.SetPassword("password1", crypto.DefaultKeyDerivationAlgorithm)
 
 			return user.SetUserProfile(ctx, w, p)
 		}))
diff --git a/internal/crypto/key_derivation_nontest.go b/internal/crypto/key_derivation_nontest.go
index 7ce781e1..9e30c825 100644
--- a/internal/crypto/key_derivation_nontest.go
+++ b/internal/crypto/key_derivation_nontest.go
@@ -5,22 +5,37 @@
 
 import (
 	"github.com/pkg/errors"
-	"golang.org/x/crypto/scrypt"
+)
+
+const (
+	// MasterKeyLength describes the length of the master key.
+	MasterKeyLength = 32
+
+	// ScryptAlgorithm is the key for the scrypt algorithm.
+	ScryptAlgorithm = "scrypt-65536-8-1"
+	// Pbkdf2Algorithm is the key for the pbkdf algorithm.
+	Pbkdf2Algorithm = "pbkdf2"
 )
 
 // DefaultKeyDerivationAlgorithm is the key derivation algorithm for new configurations.
-const DefaultKeyDerivationAlgorithm = "scrypt-65536-8-1"
+const DefaultKeyDerivationAlgorithm = ScryptAlgorithm
 
-// DeriveKeyFromPassword derives encryption key using the provided password and per-repository unique ID.
-func DeriveKeyFromPassword(password string, salt []byte, algorithm string) ([]byte, error) {
-	const masterKeySize = 32
+type keyDerivationFunc func(password string, salt []byte) ([]byte, error)
 
-	switch algorithm {
-	case "scrypt-65536-8-1":
-		//nolint:wrapcheck,gomnd
-		return scrypt.Key([]byte(password), salt, 65536, 8, 1, masterKeySize)
+//nolint:gochecknoglobals
+var keyDerivers = map[string]keyDerivationFunc{}
 
-	default:
+// RegisterKeyDerivationFunc registers various key derivation functions.
+func RegisterKeyDerivationFunc(name string, keyDeriver keyDerivationFunc) {
+	keyDerivers[name] = keyDeriver
+}
+
+// DeriveKeyFromPassword derives encryption key using the provided password and per-repository unique ID.
+func DeriveKeyFromPassword(password string, salt []byte, algorithm string) ([]byte, error) {
+	kdFunc, ok := keyDerivers[algorithm]
+	if !ok {
 		return nil, errors.Errorf("unsupported key algorithm: %v", algorithm)
 	}
+
+	return kdFunc(password, salt)
 }
diff --git a/internal/crypto/key_derivation_testing.go b/internal/crypto/key_derivation_testing.go
index 04ea1d93..c56bbb55 100644
--- a/internal/crypto/key_derivation_testing.go
+++ b/internal/crypto/key_derivation_testing.go
@@ -12,6 +12,9 @@
 // DefaultKeyDerivationAlgorithm is the key derivation algorithm for new configurations.
 const DefaultKeyDerivationAlgorithm = "testing-only-insecure"
 
+// MasterKeyLength describes the length of the master key.
+const MasterKeyLength = 32
+
 // DeriveKeyFromPassword derives encryption key using the provided password and per-repository unique ID.
 func DeriveKeyFromPassword(password string, salt []byte, algorithm string) ([]byte, error) {
 	const masterKeySize = 32
diff --git a/internal/user/user_profile_test.go b/internal/user/user_profile_test.go
index 4c6c1dad..1497f25c 100644
--- a/internal/user/user_profile_test.go
+++ b/internal/user/user_profile_test.go
@@ -3,31 +3,42 @@
 import (
 	"testing"
 
+	"github.com/kopia/kopia/internal/crypto"
 	"github.com/kopia/kopia/internal/user"
 )
 
 func TestUserProfile(t *testing.T) {
 	p := &user.Profile{}
 
-	if p.IsValidPassword("bar") {
+	if p.IsValidPassword("bar", crypto.DefaultKeyDerivationAlgorithm) {
 		t.Fatalf("password unexpectedly valid!")
 	}
 
-	p.SetPassword("foo")
+	p.SetPassword("foo", crypto.DefaultKeyDerivationAlgorithm)
 
-	if !p.IsValidPassword("foo") {
+	if !p.IsValidPassword("foo", crypto.DefaultKeyDerivationAlgorithm) {
 		t.Fatalf("password not valid!")
 	}
 
-	if p.IsValidPassword("bar") {
+	if p.IsValidPassword("bar", crypto.DefaultKeyDerivationAlgorithm) {
 		t.Fatalf("password unexpectedly valid!")
 	}
 }
 
+func TestBadKeyDerivationAlgorithmPanic(t *testing.T) {
+	defer func() { _ = recover() }()
+	func() {
+		p := &user.Profile{}
+		p.SetPassword("foo", crypto.DefaultKeyDerivationAlgorithm)
+		p.IsValidPassword("foo", "bad algorithm")
+	}()
+	t.Errorf("should have panicked")
+}
+
 func TestNilUserProfile(t *testing.T) {
 	var p *user.Profile
 
-	if p.IsValidPassword("bar") {
+	if p.IsValidPassword("bar", crypto.DefaultKeyDerivationAlgorithm) {
 		t.Fatalf("password unexpectedly valid!")
 	}
 }
@@ -40,7 +51,7 @@ func TestInvalidPasswordHash(t *testing.T) {
 
 	for _, tc := range cases {
 		p := &user.Profile{PasswordHash: tc}
-		if p.IsValidPassword("some-password") {
+		if p.IsValidPassword("some-password", crypto.DefaultKeyDerivationAlgorithm) {
 			t.Fatalf("password unexpectedly valid for %v", tc)
 		}
 	}