Repository: kopia/kopia
Pull request #3724 (2024-03-12 20:24:56)

fix(cli): Output error msg returned by API server payload when possible
Proposal to include the payload being returned by the API server in the case of an error in the error message logged to STDERR. If the error message payload is present in the response body, include it in the error message. Otherwise the error message will remain unchanged.

Simulated `"some error"` being thrown in a query to `/api/v1/contents/{contentID}`, resulting in an internal server error response:
Before:
![image](https://github.com/kopia/kopia/assets/12399758/2b861755-13d5-4480-bb0b-318d17a9fb04)

After:
![image](https://github.com/kopia/kopia/assets/12399758/0f2934d4-30c2-4717-a541-c4bf4a139e8e)


Patch:
diff --git a/cli/command_server_start.go b/cli/command_server_start.go
index ac6964f2..d28c7155 100644
--- a/cli/command_server_start.go
+++ b/cli/command_server_start.go
@@ -82,7 +82,7 @@ func (c *commandServerStart) setup(svc advancedAppServices, parent commandParent
 	cmd.Flag("html", "Server the provided HTML at the root URL").ExistingDirVar(&c.serverStartHTMLPath)
 	cmd.Flag("ui", "Start the server with HTML UI").Default("true").BoolVar(&c.serverStartUI)
 
-	cmd.Flag("legacy-api", "Start the legacy server API").Default("true").BoolVar(&c.serverStartLegacyRepositoryAPI)
+	cmd.Flag("legacy-api", "Start the legacy server API").Default("false").BoolVar(&c.serverStartLegacyRepositoryAPI)
 	cmd.Flag("grpc", "Start the GRPC server").Default("true").BoolVar(&c.serverStartGRPC)
 	cmd.Flag("control-api", "Start the control API").Default("true").BoolVar(&c.serverStartControlAPI)
 
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/apiclient/apiclient.go b/internal/apiclient/apiclient.go
index 5ca358ac..6a5c4adc 100644
--- a/internal/apiclient/apiclient.go
+++ b/internal/apiclient/apiclient.go
@@ -5,6 +5,7 @@
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"net"
 	"net/http"
@@ -148,9 +149,30 @@ func (e HTTPStatusError) Error() string {
 	return e.ErrorMessage
 }
 
+// serverErrorResponse is a structure that can decode the Error field
+// of a serverapi.ErrorResponse received from the API server.
+type serverErrorResponse struct {
+	Error string `json:"error"`
+}
+
+// respToErrorMessage will attempt to JSON decode the response body into
+// a structure resembling the serverapi.ErrorResponse struct. If successful,
+// the Error field will be included in the string output. Otherwise
+// only the response Status field will be returned.
+func respToErrorMessage(resp *http.Response) string {
+	errResp := serverErrorResponse{}
+
+	err := json.NewDecoder(resp.Body).Decode(&errResp)
+	if err != nil {
+		return resp.Status
+	}
+
+	return fmt.Sprintf("%s: %s", resp.Status, errResp.Error)
+}
+
 func decodeResponse(resp *http.Response, respPayload interface{}) error {
 	if resp.StatusCode != http.StatusOK {
-		return HTTPStatusError{resp.StatusCode, resp.Status}
+		return HTTPStatusError{resp.StatusCode, respToErrorMessage(resp)}
 	}
 
 	if respPayload == nil {
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/server/api_snapshots_test.go b/internal/server/api_snapshots_test.go
index 065a77b4..914ac031 100644
--- a/internal/server/api_snapshots_test.go
+++ b/internal/server/api_snapshots_test.go
@@ -116,8 +116,8 @@ func TestListAndDeleteSnapshots(t *testing.T) {
 		},
 	}, &serverapi.Empty{}))
 
-	badReq := apiclient.HTTPStatusError{HTTPStatusCode: 400, ErrorMessage: "400 Bad Request"}
-	serverError := apiclient.HTTPStatusError{HTTPStatusCode: 500, ErrorMessage: "500 Internal Server Error"}
+	badReq := apiclient.HTTPStatusError{HTTPStatusCode: 400, ErrorMessage: "400 Bad Request: unknown source"}
+	serverError := apiclient.HTTPStatusError{HTTPStatusCode: 500, ErrorMessage: "500 Internal Server Error: internal server error: source info does not match snapshot source"}
 
 	// make sure when deleting snapshot by ID the source must match
 	require.ErrorIs(t, cli.Post(ctx, "snapshots/delete", &serverapi.DeleteSnapshotsRequest{
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)
 		}
 	}
diff --git a/tests/end_to_end_test/acl_test.go b/tests/end_to_end_test/acl_test.go
index bbdd5d7f..85dbf483 100644
--- a/tests/end_to_end_test/acl_test.go
+++ b/tests/end_to_end_test/acl_test.go
@@ -77,6 +77,7 @@ func verifyACL(t *testing.T, disableGRPC bool) {
 		"--server-control-password=admin-pwd",
 		"--tls-generate-cert",
 		"--tls-generate-rsa-key-size=2048", // use shorter key size to speed up generation
+		"--legacy-api",
 	)
 
 	t.Logf("detected server parameters %#v", sp)
diff --git a/tests/end_to_end_test/api_server_repository_test.go b/tests/end_to_end_test/api_server_repository_test.go
index db1a3281..1e8b7a6e 100644
--- a/tests/end_to_end_test/api_server_repository_test.go
+++ b/tests/end_to_end_test/api_server_repository_test.go
@@ -53,7 +53,7 @@ func TestAPIServerRepository_GRPC_RepositoryUsers(t *testing.T) {
 func TestAPIServerRepository_DisableGRPC_htpasswd(t *testing.T) {
 	t.Parallel()
 
-	testAPIServerRepository(t, []string{"--no-grpc"}, false, false)
+	testAPIServerRepository(t, []string{"--no-grpc", "--legacy-api"}, false, false)
 }
 
 //nolint:thelper