fix(general): avoid panic on computing password hash error (#3907)

Rationale: this code path is primarily executed from the server. A
potential error, say from a corrupt, unsupported or otherwise invalid
user profile should not cause the server to panic (and crash).

It is possible for `computePasswordHash` to return an error, not just an
impossibility.

Test refactoring:
- use 'require' in user profile tests;
- move test case to TestBadPasswordHashVersion;
- update comments in test.

internal/auth/authn_repo.go

@@ -51,7 +51,14 @@ 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)
+	valid, err := ac.userProfiles[username].IsValidPassword(password)
+	if err != nil {
+		log(ctx).Debugf("password error for user '%s': %v", username, err)
+
+		return false
+	}
+
+	return valid
 }
 
 func (ac *repositoryUserAuthenticator) Refresh(ctx context.Context) error {

internal/user/user_profile.go

@@ -38,7 +38,7 @@ 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 {
+func (p *Profile) IsValidPassword(password string) (bool, error) {
 	var invalidProfile bool
 
 	var passwordHashAlgorithm string
@@ -59,9 +59,9 @@ func (p *Profile) IsValidPassword(password string) bool {
 		// if the user profile is invalid, either a non-existing user name or password
 		// hash version, then 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, dummyHashThatNeverMatchesAnyPassword, algorithms[rand.Intn(len(algorithms))]) //nolint:gosec
+		_, err := isValidPassword(password, dummyHashThatNeverMatchesAnyPassword, algorithms[rand.Intn(len(algorithms))]) //nolint:gosec
 
-		return false
+		return false, err
 	}
 
 	return isValidPassword(password, p.PasswordHash, passwordHashAlgorithm)

internal/user/user_profile_pw_hash.go

@@ -50,17 +50,17 @@ func computePasswordHash(password string, salt []byte, keyDerivationAlgorithm st
 	return payload, nil
 }
 
-func isValidPassword(password string, hashedPassword []byte, keyDerivationAlgorithm string) bool {
+func isValidPassword(password string, hashedPassword []byte, keyDerivationAlgorithm string) (bool, error) {
 	if len(hashedPassword) != passwordHashSaltLength+passwordHashLength {
-		return false
+		return false, nil
 	}
 
 	salt := hashedPassword[0:passwordHashSaltLength]
 
 	h, err := computePasswordHash(password, salt, keyDerivationAlgorithm)
 	if err != nil {
-		panic(err)
+		return false, err
 	}
 
-	return subtle.ConstantTimeCompare(h, hashedPassword) != 0
+	return subtle.ConstantTimeCompare(h, hashedPassword) != 0, nil
 }

internal/user/user_profile_test.go

@@ -3,6 +3,8 @@
 import (
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/kopia/kopia/internal/user"
 )
 
@@ -11,25 +13,22 @@ func TestUserProfile(t *testing.T) {
 		PasswordHashVersion: user.ScryptHashVersion,
 	}
 
-	if p.IsValidPassword("bar") {
-		t.Fatalf("password unexpectedly valid!")
-	}
+	isValid, err := p.IsValidPassword("bar")
+
+	require.False(t, isValid, "password unexpectedly valid!")
+	require.NoError(t, err)
 
 	p.SetPassword("foo")
 
-	if !p.IsValidPassword("foo") {
-		t.Fatalf("password not valid!")
-	}
+	isValid, err = p.IsValidPassword("foo")
 
-	if p.IsValidPassword("bar") {
-		t.Fatalf("password unexpectedly valid!")
-	}
+	require.True(t, isValid, "password not valid!")
+	require.NoError(t, err)
 
-	// Different key derivation algorithm besides the original should fail
-	p.PasswordHashVersion = user.Pbkdf2HashVersion
-	if p.IsValidPassword("foo") {
-		t.Fatalf("password unexpectedly valid!")
-	}
+	isValid, err = p.IsValidPassword("bar")
+
+	require.False(t, isValid, "password unexpectedly valid!")
+	require.NoError(t, err)
 }
 
 func TestBadPasswordHashVersion(t *testing.T) {
@@ -37,21 +36,37 @@ func TestBadPasswordHashVersion(t *testing.T) {
 	p := &user.Profile{
 		PasswordHashVersion: user.ScryptHashVersion,
 	}
+
 	p.SetPassword("foo")
-	// Assume the key derivation algorithm is bad. This will cause
-	// a panic when validating
+
+	isValid, err := p.IsValidPassword("foo")
+
+	require.True(t, isValid, "password not valid!")
+	require.NoError(t, err)
+
+	// A password hashing algorithm different from the original should fail
+	p.PasswordHashVersion = user.Pbkdf2HashVersion
+	isValid, err = p.IsValidPassword("foo")
+
+	require.False(t, isValid, "password unexpectedly valid!")
+	require.NoError(t, err)
+
+	// Invalid password hashing algorithm
 	p.PasswordHashVersion = 0
-	if p.IsValidPassword("foo") {
-		t.Fatalf("password unexpectedly valid!")
-	}
+
+	isValid, err = p.IsValidPassword("foo")
+
+	require.False(t, isValid, "password unexpectedly valid!")
+	require.NoError(t, err)
 }
 
 func TestNilUserProfile(t *testing.T) {
 	var p *user.Profile
 
-	if p.IsValidPassword("bar") {
-		t.Fatalf("password unexpectedly valid!")
-	}
+	isValid, err := p.IsValidPassword("bar")
+
+	require.False(t, isValid, "password unexpectedly valid!")
+	require.NoError(t, err)
 }
 
 func TestInvalidPasswordHash(t *testing.T) {
@@ -63,10 +78,11 @@ func TestInvalidPasswordHash(t *testing.T) {
 	for _, tc := range cases {
 		p := &user.Profile{
 			PasswordHash:        tc,
-			PasswordHashVersion: 1,
-		}
-		if p.IsValidPassword("some-password") {
-			t.Fatalf("password unexpectedly valid for %v", tc)
+			PasswordHashVersion: user.ScryptHashVersion,
 		}
+		isValid, err := p.IsValidPassword("some-password")
+
+		require.False(t, isValid, "password unexpectedly valid for %v", tc)
+		require.NoError(t, err)
 	}
 }