Improve decryption failure message (go-gitea#24573) (go-gitea#24575)

Backport go-gitea#24573

Help some users like go-gitea#16832 go-gitea#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.

Author: wxiaoguang

modules/secret/secret.go

@@ -11,20 +11,21 @@ import (
 	"encoding/base64"
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"io"
 )
 
 // AesEncrypt encrypts text and given key with AES.
 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,15 +39,15 @@ 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:]
 	cfb := cipher.NewCFBDecrypter(block, iv)
 	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
 }

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")
 }