refactor(general): user password hashing and key derivation helpers (#…

…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.

cli/command_repository_connect_server.go

@@ -6,7 +6,6 @@ import (
 
 	"github.com/pkg/errors"
 
-	"github.com/kopia/kopia/internal/crypto"
 	"github.com/kopia/kopia/internal/passwordpersist"
 	"github.com/kopia/kopia/repo"
 )
@@ -31,14 +30,14 @@ func (c *commandRepositoryConnectServer) setup(svc advancedAppServices, parent c
 	cmd.Flag("url", "Server URL").Required().StringVar(&c.connectAPIServerURL)
 	cmd.Flag("server-cert-fingerprint", "Server certificate fingerprint").StringVar(&c.connectAPIServerCertFingerprint)
 	//nolint:lll
-	cmd.Flag("local-cache-key-derivation-algorithm", "Key derivation algorithm used to derive the local cache encryption key").Hidden().Default(repo.DefaultKeyDerivationAlgorithm).EnumVar(&c.connectAPIServerLocalCacheKeyDerivationAlgorithm, crypto.AllowedKeyDerivationAlgorithms()...)
+	cmd.Flag("local-cache-key-derivation-algorithm", "Key derivation algorithm used to derive the local cache encryption key").Hidden().Default(repo.DefaultServerRepoCacheKeyDerivationAlgorithm).EnumVar(&c.connectAPIServerLocalCacheKeyDerivationAlgorithm, repo.SupportedLocalCacheKeyDerivationAlgorithms()...)
 	cmd.Action(svc.noRepositoryAction(c.run))
 }
 
 func (c *commandRepositoryConnectServer) run(ctx context.Context) error {
 	localCacheKeyDerivationAlgorithm := c.connectAPIServerLocalCacheKeyDerivationAlgorithm
 	if localCacheKeyDerivationAlgorithm == "" {
-		localCacheKeyDerivationAlgorithm = repo.DefaultKeyDerivationAlgorithm
+		localCacheKeyDerivationAlgorithm = repo.DefaultServerRepoCacheKeyDerivationAlgorithm
 	}
 
 	as := &repo.APIServerInfo{

cli/command_repository_create.go

@@ -7,7 +7,6 @@ import (
 	"github.com/alecthomas/kingpin/v2"
 	"github.com/pkg/errors"
 
-	"github.com/kopia/kopia/internal/crypto"
 	"github.com/kopia/kopia/repo"
 	"github.com/kopia/kopia/repo/blob"
 	"github.com/kopia/kopia/repo/ecc"
@@ -54,7 +53,7 @@ func (c *commandRepositoryCreate) setup(svc advancedAppServices, parent commandP
 	cmd.Flag("retention-mode", "Set the blob retention-mode for supported storage backends.").EnumVar(&c.retentionMode, blob.Governance.String(), blob.Compliance.String())
 	cmd.Flag("retention-period", "Set the blob retention-period for supported storage backends.").DurationVar(&c.retentionPeriod)
 	//nolint:lll
-	cmd.Flag("format-block-key-derivation-algorithm", "Algorithm to derive the encryption key for the format block from the repository password").Default(crypto.DefaultKeyDerivationAlgorithm).EnumVar(&c.createBlockKeyDerivationAlgorithm, crypto.AllowedKeyDerivationAlgorithms()...)
+	cmd.Flag("format-block-key-derivation-algorithm", "Algorithm to derive the encryption key for the format block from the repository password").Default(format.DefaultKeyDerivationAlgorithm).EnumVar(&c.createBlockKeyDerivationAlgorithm, format.SupportedFormatBlobKeyDerivationAlgorithms()...)
 
 	c.co.setup(svc, cmd)
 	c.svc = svc

cli/command_user_add_set.go

@@ -7,7 +7,6 @@ import (
 	"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"
 )
@@ -37,7 +36,7 @@ func (c *commandServerUserAddSet) setup(svc appServices, parent commandParent, i
 	cmd.Flag("ask-password", "Ask for user password").BoolVar(&c.userAskPassword)
 	cmd.Flag("user-password", "Password").StringVar(&c.userSetPassword)
 	cmd.Flag("user-password-hash", "Password hash").StringVar(&c.userSetPasswordHash)
-	cmd.Flag("user-password-hashing-algorithm", "[Experimental] Password hashing algorithm").Hidden().Default(crypto.DefaultKeyDerivationAlgorithm).EnumVar(&c.userSetPasswordHashAlgorithm, crypto.AllowedKeyDerivationAlgorithms()...)
+	cmd.Flag("user-password-hashing-algorithm", "[Experimental] Password hashing algorithm").Hidden().Default(user.DefaultPasswordHashingAlgorithm).EnumVar(&c.userSetPasswordHashAlgorithm, user.PasswordHashingAlgorithms()...)
 	cmd.Arg("username", "Username").Required().StringVar(&c.userSetName)
 	cmd.Action(svc.repositoryWriterAction(c.runServerUserAddSet))
 

internal/crypto/key_derivation_nontest.go

@@ -9,25 +9,16 @@ import (
 	"github.com/pkg/errors"
 )
 
-const (
-	// MasterKeyLength describes the length of the master key.
-	MasterKeyLength = 32
-)
-
-// DefaultKeyDerivationAlgorithm is the key derivation algorithm for new configurations.
-const DefaultKeyDerivationAlgorithm = ScryptAlgorithm
-
-// KeyDeriver is an interface that contains methods for deriving a key from a password.
-type KeyDeriver interface {
-	DeriveKeyFromPassword(password string, salt []byte) ([]byte, error)
-	RecommendedSaltLength() int
+// passwordBasedKeyDeriver is an interface that contains methods for deriving a key from a password.
+type passwordBasedKeyDeriver interface {
+	deriveKeyFromPassword(password string, salt []byte, keySize int) ([]byte, error)
 }
 
 //nolint:gochecknoglobals
-var keyDerivers = map[string]KeyDeriver{}
+var keyDerivers = map[string]passwordBasedKeyDeriver{}
 
-// RegisterKeyDerivers registers various key derivation functions.
-func RegisterKeyDerivers(name string, keyDeriver KeyDeriver) {
+// registerPBKeyDeriver registers a password-based key deriver.
+func registerPBKeyDeriver(name string, keyDeriver passwordBasedKeyDeriver) {
 	if _, ok := keyDerivers[name]; ok {
 		panic(fmt.Sprintf("key deriver (%s) is already registered", name))
 	}
@@ -36,28 +27,18 @@ func RegisterKeyDerivers(name string, keyDeriver KeyDeriver) {
 }
 
 // DeriveKeyFromPassword derives encryption key using the provided password and per-repository unique ID.
-func DeriveKeyFromPassword(password string, salt []byte, algorithm string) ([]byte, error) {
+func DeriveKeyFromPassword(password string, salt []byte, keySize int, algorithm string) ([]byte, error) {
 	kd, ok := keyDerivers[algorithm]
 	if !ok {
-		return nil, errors.Errorf("unsupported key derivation algorithm: %v, supported algorithms %v", algorithm, AllowedKeyDerivationAlgorithms())
+		return nil, errors.Errorf("unsupported key derivation algorithm: %v, supported algorithms %v", algorithm, supportedPBKeyDerivationAlgorithms())
 	}
 
 	//nolint:wrapcheck
-	return kd.DeriveKeyFromPassword(password, salt)
-}
-
-// RecommendedSaltLength returns the recommended salt length of a given key derivation algorithm.
-func RecommendedSaltLength(algorithm string) (int, error) {
-	kd, ok := keyDerivers[algorithm]
-	if !ok {
-		return 0, errors.Errorf("failed to get salt length for unsupported key derivation algorithm: %v, supported algorithms %v", algorithm, AllowedKeyDerivationAlgorithms())
-	}
-
-	return kd.RecommendedSaltLength(), nil
+	return kd.deriveKeyFromPassword(password, salt, keySize)
 }
 
-// AllowedKeyDerivationAlgorithms returns a slice of the allowed key derivation algorithms.
-func AllowedKeyDerivationAlgorithms() []string {
+// supportedPBKeyDerivationAlgorithms returns a slice of the allowed key derivation algorithms.
+func supportedPBKeyDerivationAlgorithms() []string {
 	kdAlgorithms := make([]string, 0, len(keyDerivers))
 	for k := range keyDerivers {
 		kdAlgorithms = append(kdAlgorithms, k)

internal/crypto/key_derivation_testing.go

@@ -10,23 +10,13 @@ import (
 )
 
 const (
-	// MasterKeyLength describes the length of the master key.
-	MasterKeyLength = 32
-
-	V1SaltLength = 32
-
 	ScryptAlgorithm = "scrypt-65536-8-1"
 
 	Pbkdf2Algorithm = "pbkdf2-sha256-600000"
-
-	// DefaultKeyDerivationAlgorithm is the key derivation algorithm for new configurations.
-	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
-
+func DeriveKeyFromPassword(password string, salt []byte, keySize int, algorithm string) ([]byte, error) {
 	switch algorithm {
 	case ScryptAlgorithm, Pbkdf2Algorithm:
 		h := sha256.New()
@@ -41,11 +31,3 @@ func DeriveKeyFromPassword(password string, salt []byte, algorithm string) ([]by
 		return nil, errors.Errorf("unsupported key algorithm: %v", algorithm)
 	}
 }
-
-func RecommendedSaltLength(algorithm string) (int, error) {
-	return V1SaltLength, nil
-}
-
-func AllowedKeyDerivationAlgorithms() []string {
-	return []string{DefaultKeyDerivationAlgorithm}
-}

internal/crypto/pbkdf.go → internal/crypto/pb_key_deriver_pbkdf2.go

@@ -11,43 +11,37 @@ import (
 )
 
 const (
+	// Pbkdf2Algorithm is the key for the pbkdf algorithm.
+	Pbkdf2Algorithm = "pbkdf2-sha256-600000"
+
 	// The NIST recommended minimum size for a salt for pbkdf2 is 16 bytes.
 	//
-	// TBD: However, a good rule of thumb is to use a salt that is the same size
+	// 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
-	minPbkdfSha256SaltSize = 32 // size in bytes == 128 bits
+	pbkdf2Sha256MinSaltLength = 32 // 256 bits
 
 	// The NIST recommended iterations for PBKDF2 with SHA256 hash is 600,000.
 	pbkdf2Sha256Iterations = 600_000
-
-	// Pbkdf2Algorithm is the key for the pbkdf algorithm.
-	Pbkdf2Algorithm = "pbkdf2-sha256-600000"
 )
 
 func init() {
-	RegisterKeyDerivers(Pbkdf2Algorithm, &pbkdf2KeyDeriver{
-		iterations:            pbkdf2Sha256Iterations,
-		recommendedSaltLength: minPbkdfSha256SaltSize,
-		minSaltLength:         minPbkdfSha256SaltSize,
+	registerPBKeyDeriver(Pbkdf2Algorithm, &pbkdf2KeyDeriver{
+		iterations:    pbkdf2Sha256Iterations,
+		minSaltLength: pbkdf2Sha256MinSaltLength,
 	})
 }
 
 type pbkdf2KeyDeriver struct {
-	iterations            int
-	recommendedSaltLength int
-	minSaltLength         int
+	iterations    int
+	minSaltLength int
 }
 
-func (s *pbkdf2KeyDeriver) DeriveKeyFromPassword(password string, salt []byte) ([]byte, error) {
+func (s *pbkdf2KeyDeriver) deriveKeyFromPassword(password string, salt []byte, keySize int) ([]byte, error) {
 	if len(salt) < s.minSaltLength {
 		return nil, errors.Errorf("required salt size is atleast %d bytes", s.minSaltLength)
 	}
 
-	return pbkdf2.Key([]byte(password), salt, s.iterations, MasterKeyLength, sha256.New), nil
-}
-
-func (s *pbkdf2KeyDeriver) RecommendedSaltLength() int {
-	return s.recommendedSaltLength
+	return pbkdf2.Key([]byte(password), salt, s.iterations, keySize, sha256.New), nil
 }

internal/crypto/pb_key_deriver_scrypt.go

@@ -0,0 +1,52 @@
+//go:build !testing
+// +build !testing
+
+package crypto
+
+import (
+	"github.com/pkg/errors"
+	"golang.org/x/crypto/scrypt"
+)
+
+const (
+	// ScryptAlgorithm is the registration name for the scrypt algorithm instance.
+	ScryptAlgorithm = "scrypt-65536-8-1"
+
+	// The recommended minimum size for a salt to be used for scrypt.
+	// Currently set to 16 bytes (128 bits).
+	//
+	// 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
+	scryptMinSaltLength = 16 // 128 bits
+)
+
+func init() {
+	registerPBKeyDeriver(ScryptAlgorithm, &scryptKeyDeriver{
+		n:             65536, //nolint:gomnd
+		r:             8,     //nolint:gomnd
+		p:             1,
+		minSaltLength: scryptMinSaltLength,
+	})
+}
+
+type scryptKeyDeriver struct {
+	// n scryptCostParameterN is scrypt's CPU/memory cost parameter.
+	n int
+	// r scryptCostParameterR is scrypt's work factor.
+	r int
+	// p scryptCostParameterP is scrypt's parallelization parameter.
+	p int
+
+	minSaltLength int
+}
+
+func (s *scryptKeyDeriver) deriveKeyFromPassword(password string, salt []byte, keySize int) ([]byte, error) {
+	if len(salt) < s.minSaltLength {
+		return nil, errors.Errorf("required salt size is at least %d bytes", s.minSaltLength)
+	}
+	//nolint:wrapcheck
+	return scrypt.Key([]byte(password), salt, s.n, s.r, s.p, keySize)
+}

internal/servertesting/servertesting.go

@@ -79,7 +79,7 @@ func StartServer(t *testing.T, env *repotesting.Environment, tls bool) *repo.API
 		asi.BaseURL = hs.URL
 	}
 
-	asi.LocalCacheKeyDerivationAlgorithm = repo.DefaultKeyDerivationAlgorithm
+	asi.LocalCacheKeyDerivationAlgorithm = repo.DefaultServerRepoCacheKeyDerivationAlgorithm
 
 	t.Cleanup(hs.Close)
 

internal/user/password_hashings.go

@@ -0,0 +1,9 @@
+package user
+
+// DefaultPasswordHashingAlgorithm is the default password hashing scheme for user profiles.
+const DefaultPasswordHashingAlgorithm = scryptHashAlgorithm
+
+// PasswordHashingAlgorithms returns the supported algorithms for user password hashing.
+func PasswordHashingAlgorithms() []string {
+	return []string{scryptHashAlgorithm, pbkdf2HashAlgorithm}
+}

internal/user/password_hashings_test.go

@@ -0,0 +1,32 @@
+package user
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/kopia/kopia/internal/crypto"
+)
+
+// The password hashing constants defined in this package are used as "lookup
+// keys" for the register password-based key derivers in the crypto package.
+// This trivial test is a change detector to ensure that the constants defined
+// in the user package match those defined in the crypto package.
+func TestPasswordHashingConstantMatchCryptoPackage(t *testing.T) {
+	require.Equal(t, crypto.ScryptAlgorithm, scryptHashAlgorithm)
+	require.Equal(t, crypto.Pbkdf2Algorithm, pbkdf2HashAlgorithm)
+}
+
+// The passwordHashSaltLength constant defines the salt length used in this
+// package for password hashing. This trivial test ensures that this hash length
+// meets the minimum requirement for the instantiations of the registered
+// password hashers (PB key derivers in the crypto package).
+func TestSaltLengthIsSupported(t *testing.T) {
+	const badPwd = "password"
+	var salt [passwordHashSaltLength]byte
+
+	for _, h := range PasswordHashingAlgorithms() {
+		_, err := computePasswordHash(badPwd, salt[:], h)
+		require.NoError(t, err)
+	}
+}