From 4498a2622276d8fd812107c76f993be0f213e73f Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Sun, 7 May 2023 22:12:32 +0800
Subject: [PATCH] Improve decryption failure message (#24573) (#24575)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

Backport #24573

Help some users like #16832 #1851

There are many users reporting similar problem: if the SECRET_KEY
mismatches, some operations (like 2FA login) only reports unclear 500
error and unclear "base64 decode error" log （some maintainers ever spent
a lot of time on debugging such problem)

The SECRET_KEY was not well-designed and it is also a kind of technical
debt. Since it couldn't be fixed easily, it's good to add clearer error
messages, then at least users could know what the real problem is.
---
 modules/secret/secret.go      | 21 +++++++++++----------
 modules/secret/secret_test.go | 22 +++++++++++++++-------
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/modules/secret/secret.go b/modules/secret/secret.go
index b84d1cfea8..e70ae1839c 100644
--- a/modules/secret/secret.go
+++ b/modules/secret/secret.go
@@ -11,6 +11,7 @@ import (
 	"encoding/base64"
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"io"
 )
 
@@ -18,13 +19,13 @@ import (
 func AesEncrypt(key, text []byte) ([]byte, error) {
 	block, err := aes.NewCipher(key)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("AesEncrypt invalid key: %v", err)
 	}
 	b := base64.StdEncoding.EncodeToString(text)
 	ciphertext := make([]byte, aes.BlockSize+len(b))
 	iv := ciphertext[:aes.BlockSize]
-	if _, err := io.ReadFull(rand.Reader, iv); err != nil {
-		return nil, err
+	if _, err = io.ReadFull(rand.Reader, iv); err != nil {
+		return nil, fmt.Errorf("AesEncrypt unable to read IV: %w", err)
 	}
 	cfb := cipher.NewCFBEncrypter(block, iv)
 	cfb.XORKeyStream(ciphertext[aes.BlockSize:], []byte(b))
@@ -38,7 +39,7 @@ func AesDecrypt(key, text []byte) ([]byte, error) {
 		return nil, err
 	}
 	if len(text) < aes.BlockSize {
-		return nil, errors.New("ciphertext too short")
+		return nil, errors.New("AesDecrypt ciphertext too short")
 	}
 	iv := text[:aes.BlockSize]
 	text = text[aes.BlockSize:]
@@ -46,7 +47,7 @@ func AesDecrypt(key, text []byte) ([]byte, error) {
 	cfb.XORKeyStream(text, text)
 	data, err := base64.StdEncoding.DecodeString(string(text))
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("AesDecrypt invalid decrypted base64 string: %w", err)
 	}
 	return data, nil
 }
@@ -57,21 +58,21 @@ func EncryptSecret(key, str string) (string, error) {
 	plaintext := []byte(str)
 	ciphertext, err := AesEncrypt(keyHash[:], plaintext)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed to encrypt by secret: %w", err)
 	}
 	return hex.EncodeToString(ciphertext), nil
 }
 
 // DecryptSecret decrypts a previously encrypted hex string
-func DecryptSecret(key, cipherhex string) (string, error) {
+func DecryptSecret(key, cipherHex string) (string, error) {
 	keyHash := sha256.Sum256([]byte(key))
-	ciphertext, err := hex.DecodeString(cipherhex)
+	ciphertext, err := hex.DecodeString(cipherHex)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed to decrypt by secret, invalid hex string: %w", err)
 	}
 	plaintext, err := AesDecrypt(keyHash[:], ciphertext)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed to decrypt by secret, the key (maybe SECRET_KEY?) might be incorrect: %w", err)
 	}
 	return string(plaintext), nil
 }
diff --git a/modules/secret/secret_test.go b/modules/secret/secret_test.go
index 0a189ecebf..d4fb46955b 100644
--- a/modules/secret/secret_test.go
+++ b/modules/secret/secret_test.go
@@ -10,14 +10,22 @@ import (
 )
 
 func TestEncryptDecrypt(t *testing.T) {
-	var hex string
-	var str string
+	hex, err := EncryptSecret("foo", "baz")
+	assert.NoError(t, err)
+	str, _ := DecryptSecret("foo", hex)
+	assert.Equal(t, "baz", str)
 
-	hex, _ = EncryptSecret("foo", "baz")
+	hex, err = EncryptSecret("bar", "baz")
+	assert.NoError(t, err)
 	str, _ = DecryptSecret("foo", hex)
-	assert.Equal(t, str, "baz")
+	assert.NotEqual(t, "baz", str)
 
-	hex, _ = EncryptSecret("bar", "baz")
-	str, _ = DecryptSecret("foo", hex)
-	assert.NotEqual(t, str, "baz")
+	_, err = DecryptSecret("a", "b")
+	assert.ErrorContains(t, err, "invalid hex string")
+
+	_, err = DecryptSecret("a", "bb")
+	assert.ErrorContains(t, err, "the key (maybe SECRET_KEY?) might be incorrect: AesDecrypt ciphertext too short")
+
+	_, err = DecryptSecret("a", "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef")
+	assert.ErrorContains(t, err, "the key (maybe SECRET_KEY?) might be incorrect: AesDecrypt invalid decrypted base64 string")
 }
-- 
2.39.5