From ddcdfdb55a91d70823297bcf20a6f1c778d23cc5 Mon Sep 17 00:00:00 2001 From: Sirish Bathina Date: Tue, 12 Mar 2024 14:34:46 -1000 Subject: [PATCH] changes for switching key derivation (#3725) --- cli/command_user_add_set.go | 5 ++-- internal/auth/authn_repo.go | 3 +- internal/auth/authn_repo_test.go | 3 +- internal/crypto/key_derivation_nontest.go | 35 +++++++++++++++------- internal/crypto/key_derivation_testing.go | 3 ++ internal/crypto/pbkdf.go | 31 +++++++++++++++++++ internal/crypto/scrypt.go | 28 ++++++++++++++++++ internal/user/user_profile.go | 10 +++---- internal/user/user_profile_hash_v1.go | 36 ++++++++++++----------- internal/user/user_profile_test.go | 23 +++++++++++---- repo/open.go | 8 ++--- 11 files changed, 138 insertions(+), 47 deletions(-) create mode 100644 internal/crypto/pbkdf.go create mode 100644 internal/crypto/scrypt.go diff --git a/cli/command_user_add_set.go b/cli/command_user_add_set.go index f98b51e3e..14589db56 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 9df1373dd..67466e655 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/auth/authn_repo_test.go b/internal/auth/authn_repo_test.go index 8324bcf79..27cd9e458 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 7ce781e19..9e30c8252 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 + +type keyDerivationFunc func(password string, salt []byte) ([]byte, error) + +//nolint:gochecknoglobals +var keyDerivers = map[string]keyDerivationFunc{} + +// 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) { - const masterKeySize = 32 - - switch algorithm { - case "scrypt-65536-8-1": - //nolint:wrapcheck,gomnd - return scrypt.Key([]byte(password), salt, 65536, 8, 1, masterKeySize) - - default: + 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 04ea1d930..c56bbb55c 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/crypto/pbkdf.go b/internal/crypto/pbkdf.go new file mode 100644 index 000000000..812bafe3d --- /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 000000000..0ad3e9323 --- /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 6001a048c..709eaaf05 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 a0cc949fd..ab8c3514d 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") } - p.PasswordHashVersion = 1 - p.PasswordHash = computePasswordHashV1(password, salt) + var err error - return nil + p.PasswordHashVersion = 1 + p.PasswordHash, err = computePasswordHashV1(password, salt, keyDerivationAlgorithm) + + 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/internal/user/user_profile_test.go b/internal/user/user_profile_test.go index 4c6c1dad2..1497f25cb 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) } } diff --git a/repo/open.go b/repo/open.go index 2186e2d81..3eef8bf92 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") }