From 02463ab118c391c520995cc907dc070e944054cb Mon Sep 17 00:00:00 2001 From: Sirish Bathina Date: Wed, 24 Apr 2024 14:50:26 -1000 Subject: [PATCH] feat(general): user profile hashing version to algorithm translation (#3816) Reverts to using the `PasswordHashVersion` in the user profile. Adds a simple mechanism for translating between password hash version and the corresponding password hashing algorithm (key derivation algorithm). --- cli/command_user_add_set.go | 21 ++++--- internal/auth/authn_repo_test.go | 11 ++-- internal/crypto/key_derivation_testing.go | 15 ++--- internal/crypto/scrypt.go | 4 -- internal/user/user_profile.go | 73 ++++++++++++++++++----- internal/user/user_profile_hash_v1.go | 16 ++--- internal/user/user_profile_test.go | 64 ++++++-------------- 7 files changed, 107 insertions(+), 97 deletions(-) diff --git a/cli/command_user_add_set.go b/cli/command_user_add_set.go index 3f95976b1..abc7ff2b5 100644 --- a/cli/command_user_add_set.go +++ b/cli/command_user_add_set.go @@ -13,11 +13,11 @@ ) type commandServerUserAddSet struct { - userAskPassword bool - userSetName string - userSetPassword string - keyDerivationAlgorithm string - userSetPasswordHash string + userAskPassword bool + userSetName string + userSetPassword string + userSetPasswordHashAlgorithm string + userSetPasswordHash string isNew bool // true == 'add', false == 'update' out textOutput @@ -37,7 +37,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.keyDerivationAlgorithm, crypto.AllowedKeyDerivationAlgorithms()...) + cmd.Flag("user-password-hashing-algorithm", "[Experimental] Password hashing algorithm").Hidden().Default(crypto.DefaultKeyDerivationAlgorithm).EnumVar(&c.userSetPasswordHashAlgorithm, crypto.AllowedKeyDerivationAlgorithms()...) cmd.Arg("username", "Username").Required().StringVar(&c.userSetName) cmd.Action(svc.repositoryWriterAction(c.runServerUserAddSet)) @@ -53,9 +53,14 @@ func (c *commandServerUserAddSet) getExistingOrNewUserProfile(ctx context.Contex return nil, errors.Errorf("user %q already exists", username) case errors.Is(err, user.ErrUserNotFound): + passwordHashVersion, err := user.GetPasswordHashVersion(c.userSetPasswordHashAlgorithm) + if err != nil { + return nil, errors.Wrap(err, "failed to get password hash version") + } + return &user.Profile{ - Username: username, - KeyDerivationAlgorithm: c.keyDerivationAlgorithm, + Username: username, + PasswordHashVersion: passwordHashVersion, }, nil } } diff --git a/internal/auth/authn_repo_test.go b/internal/auth/authn_repo_test.go index d145884b2..50761312c 100644 --- a/internal/auth/authn_repo_test.go +++ b/internal/auth/authn_repo_test.go @@ -7,7 +7,6 @@ "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" @@ -26,14 +25,14 @@ func(ctx context.Context, w repo.RepositoryWriter) error { { profile: &user.Profile{ Username: "user1@host1", - PasswordHashVersion: crypto.HashVersion1, + PasswordHashVersion: user.ScryptHashVersion, }, password: "password1", }, { profile: &user.Profile{ - Username: "user2@host2", - KeyDerivationAlgorithm: crypto.ScryptAlgorithm, + Username: "user2@host2", + PasswordHashVersion: user.ScryptHashVersion, }, password: "password2", }, @@ -45,8 +44,8 @@ func(ctx context.Context, w repo.RepositoryWriter) error { }, { profile: &user.Profile{ - Username: "user4@host4", - KeyDerivationAlgorithm: crypto.Pbkdf2Algorithm, + Username: "user4@host4", + PasswordHashVersion: user.Pbkdf2HashVersion, }, password: "password4", }, diff --git a/internal/crypto/key_derivation_testing.go b/internal/crypto/key_derivation_testing.go index acbbe74b3..9a30554e0 100644 --- a/internal/crypto/key_derivation_testing.go +++ b/internal/crypto/key_derivation_testing.go @@ -10,16 +10,17 @@ ) const ( - // DefaultKeyDerivationAlgorithm is the key derivation algorithm for new configurations. - DefaultKeyDerivationAlgorithm = "testing-only-insecure" - // MasterKeyLength describes the length of the master key. MasterKeyLength = 32 - V1SaltLength = 32 - HashVersion1 = 1 // this translates to Scrypt KeyDerivationAlgorithm + V1SaltLength = 32 + ScryptAlgorithm = "scrypt-65536-8-1" - Pbkdf2Algorithm = "pbkdf2" + + 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. @@ -27,7 +28,7 @@ func DeriveKeyFromPassword(password string, salt []byte, algorithm string) ([]by const masterKeySize = 32 switch algorithm { - case DefaultKeyDerivationAlgorithm, ScryptAlgorithm, Pbkdf2Algorithm: + case ScryptAlgorithm, Pbkdf2Algorithm: h := sha256.New() // Adjust password so that we get a different key for each algorithm if _, err := h.Write([]byte(password + algorithm)); err != nil { diff --git a/internal/crypto/scrypt.go b/internal/crypto/scrypt.go index 1758215dc..b7e71eb9b 100644 --- a/internal/crypto/scrypt.go +++ b/internal/crypto/scrypt.go @@ -24,10 +24,6 @@ // Legacy hash version salt length. V1SaltLength = 32 - - // Legacy hash version system translates to KeyDerivationAlgorithm. - HashVersion1 = 1 // this translates to Scrypt KeyDerivationAlgorithm - ) func init() { diff --git a/internal/user/user_profile.go b/internal/user/user_profile.go index 53659cdf9..5065385ea 100644 --- a/internal/user/user_profile.go +++ b/internal/user/user_profile.go @@ -5,16 +5,29 @@ "github.com/kopia/kopia/internal/crypto" "github.com/kopia/kopia/repo/manifest" + + "github.com/pkg/errors" +) + +const ( + // ScryptHashVersion is the version representation of the scrypt algorithm. + ScryptHashVersion = 1 + // ScryptHashAlgorithm is the scrypt password hashing algorithm. This must match crypto.ScryptAlgorithm. + ScryptHashAlgorithm = "scrypt-65536-8-1" + + // Pbkdf2HashVersion is the version representation of the pbkdf2 algorithm. + Pbkdf2HashVersion = 2 + // Pbkdf2HashAlgorithm is the pbkdf2 password hashing algorithm. This must match crypto.Pbkdf2Algorithm. + Pbkdf2HashAlgorithm = "pbkdf2-sha256-600000" ) // Profile describes information about a single user. type Profile struct { ManifestID manifest.ID `json:"-"` - Username string `json:"username"` - PasswordHashVersion int `json:"passwordHashVersion,omitempty"` // indicates how password is hashed, deprecated in favor of KeyDerivationAlgorithm - KeyDerivationAlgorithm string `json:"keyDerivationAlgorithm,omitempty"` - PasswordHash []byte `json:"passwordHash"` + Username string `json:"username"` + PasswordHashVersion int `json:"passwordHashVersion,omitempty"` + PasswordHash []byte `json:"passwordHash"` } // SetPassword changes the password for a user profile. @@ -24,23 +37,53 @@ func (p *Profile) SetPassword(password string) error { // IsValidPassword determines whether the password is valid for a given user. func (p *Profile) IsValidPassword(password string) bool { + var invalidProfile bool + + var passwordHashAlgorithm string + + var err error + if p == nil { + invalidProfile = true + } else { + passwordHashAlgorithm, err = GetPasswordHashAlgorithm(p.PasswordHashVersion) + if err != nil { + invalidProfile = true + } + } + + if invalidProfile { algorithms := crypto.AllowedKeyDerivationAlgorithms() - // if the user is invalid, return false but use the same amount of time as when we + // if the Username 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. isValidPassword(password, dummyV1HashThatNeverMatchesAnyPassword, algorithms[rand.Intn(len(algorithms))]) //nolint:gosec return false } - // Legacy case where password hash version is set - if p.PasswordHashVersion == crypto.HashVersion1 { - return isValidPassword(password, p.PasswordHash, crypto.ScryptAlgorithm) - } - - if p.KeyDerivationAlgorithm != "" { - return isValidPassword(password, p.PasswordHash, p.KeyDerivationAlgorithm) - } - - return false + return isValidPassword(password, p.PasswordHash, passwordHashAlgorithm) +} + +// GetPasswordHashAlgorithm returns the password hash algorithm given a version. +func GetPasswordHashAlgorithm(passwordHashVersion int) (string, error) { + switch passwordHashVersion { + case ScryptHashVersion: + return ScryptHashAlgorithm, nil + case Pbkdf2HashVersion: + return Pbkdf2HashAlgorithm, nil + default: + return "", errors.Errorf("unsupported hash version (%d)", passwordHashVersion) + } +} + +// GetPasswordHashVersion returns the password hash version given an algorithm. +func GetPasswordHashVersion(passwordHashAlgorithm string) (int, error) { + switch passwordHashAlgorithm { + case ScryptHashAlgorithm: + return ScryptHashVersion, nil + case Pbkdf2HashAlgorithm: + return Pbkdf2HashVersion, nil + default: + return 0, errors.Errorf("unsupported hash algorithm (%s)", passwordHashAlgorithm) + } } diff --git a/internal/user/user_profile_hash_v1.go b/internal/user/user_profile_hash_v1.go index 4a11757b9..379604e08 100644 --- a/internal/user/user_profile_hash_v1.go +++ b/internal/user/user_profile_hash_v1.go @@ -14,18 +14,12 @@ var dummyV1HashThatNeverMatchesAnyPassword = make([]byte, crypto.MasterKeyLength+crypto.V1SaltLength) func (p *Profile) setPassword(password string) error { - keyDerivationAlgorithm := p.KeyDerivationAlgorithm - if keyDerivationAlgorithm == "" { - if p.PasswordHashVersion == 0 { - return errors.New("key derivation algorithm and password hash version not set") - } - // Setup to handle legacy hashVersion. - if p.PasswordHashVersion == crypto.HashVersion1 { - keyDerivationAlgorithm = crypto.ScryptAlgorithm - } + passwordHashAlgorithm, err := GetPasswordHashAlgorithm(p.PasswordHashVersion) + if err != nil { + return err } - saltLength, err := crypto.RecommendedSaltLength(keyDerivationAlgorithm) + saltLength, err := crypto.RecommendedSaltLength(passwordHashAlgorithm) if err != nil { return errors.Wrap(err, "error getting recommended salt length") } @@ -35,7 +29,7 @@ func (p *Profile) setPassword(password string) error { return errors.Wrap(err, "error generating salt") } - p.PasswordHash, err = computePasswordHash(password, salt, keyDerivationAlgorithm) + p.PasswordHash, err = computePasswordHash(password, salt, passwordHashAlgorithm) return err } diff --git a/internal/user/user_profile_test.go b/internal/user/user_profile_test.go index e9088ad55..476189a0c 100644 --- a/internal/user/user_profile_test.go +++ b/internal/user/user_profile_test.go @@ -3,41 +3,12 @@ import ( "testing" - "github.com/kopia/kopia/internal/crypto" "github.com/kopia/kopia/internal/user" ) -func TestLegacyUserProfile(t *testing.T) { - p := &user.Profile{ - PasswordHashVersion: 1, // hashVersion1 - } - - if p.IsValidPassword("bar") { - t.Fatalf("password unexpectedly valid!") - } - - p.SetPassword("foo") - - if !p.IsValidPassword("foo") { - t.Fatalf("password not valid!") - } - - if p.IsValidPassword("bar") { - t.Fatalf("password unexpectedly valid!") - } - - // Setting the key derivation to scrypt and unsetting PasswordHashVersion - // Legacy profile should translate to scrypt - p.KeyDerivationAlgorithm = crypto.ScryptAlgorithm - p.PasswordHashVersion = 0 - if !p.IsValidPassword("foo") { - t.Fatalf("password not valid!") - } -} - func TestUserProfile(t *testing.T) { p := &user.Profile{ - KeyDerivationAlgorithm: crypto.ScryptAlgorithm, + PasswordHashVersion: user.ScryptHashVersion, } if p.IsValidPassword("bar") { @@ -55,26 +26,24 @@ func TestUserProfile(t *testing.T) { } // Different key derivation algorithm besides the original should fail - p.KeyDerivationAlgorithm = crypto.Pbkdf2Algorithm + p.PasswordHashVersion = user.Pbkdf2HashVersion if p.IsValidPassword("foo") { t.Fatalf("password unexpectedly valid!") } } -func TestBadKeyDerivationAlgorithmPanic(t *testing.T) { - defer func() { _ = recover() }() - func() { - // mock a valid password - p := &user.Profile{ - KeyDerivationAlgorithm: crypto.ScryptAlgorithm, - } - p.SetPassword("foo") - // Assume the key derivation algorithm is bad. This will cause - // a panic when validating - p.KeyDerivationAlgorithm = "some bad algorithm" - p.IsValidPassword("foo") - }() - t.Errorf("should have panicked") +func TestBadPasswordHashVersion(t *testing.T) { + // mock a valid password + p := &user.Profile{ + PasswordHashVersion: user.ScryptHashVersion, + } + p.SetPassword("foo") + // Assume the key derivation algorithm is bad. This will cause + // a panic when validating + p.PasswordHashVersion = 0 + if p.IsValidPassword("foo") { + t.Fatalf("password unexpectedly valid!") + } } func TestNilUserProfile(t *testing.T) { @@ -92,7 +61,10 @@ func TestInvalidPasswordHash(t *testing.T) { } for _, tc := range cases { - p := &user.Profile{PasswordHash: tc} + p := &user.Profile{ + PasswordHash: tc, + PasswordHashVersion: 1, + } if p.IsValidPassword("some-password") { t.Fatalf("password unexpectedly valid for %v", tc) }