Migrate modernize encryption service (#6830)

Co-authored-by: Claude <claude@anthropic.com>

go.mod

@@ -58,7 +58,6 @@ require (
 	gitlab.com/gitlab-org/api/client-go/v2 v2.58.0
 	go.uber.org/multierr v1.11.0
 	go.yaml.in/yaml/v4 v4.0.0-rc.6
-	golang.org/x/crypto v0.55.0
 	golang.org/x/image v0.45.0
 	golang.org/x/net v0.58.0
 	golang.org/x/oauth2 v0.36.0
@@ -227,7 +226,8 @@ require (
 	go.yaml.in/yaml/v2 v2.4.4 // indirect
 	go.yaml.in/yaml/v3 v3.0.5 // indirect
 	golang.org/x/arch v0.22.0 // indirect
-	golang.org/x/mod v0.40.0 // indirect
+	golang.org/x/crypto v0.55.0 // indirect
+	golang.org/x/mod v0.39.0 // indirect
 	golang.org/x/sys v0.47.0 // indirect
 	golang.org/x/time v0.15.0 // indirect
 	golang.org/x/tools v0.49.0 // indirect

go.sum

@@ -674,8 +674,8 @@ golang.org/x/mod v0.0.0-20190513183733-4bf6d317e70e/go.mod h1:mXi4GBBbnImb6dmsKG
 golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee/go.mod h1:QqPTAvyqsEbceGzBzNggFXnrqF1CaUcvgkdR5Ot7KZg=
 golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
-golang.org/x/mod v0.40.0 h1:hUv+3cXcdRHz08UmSiOob7sadHig73uo5bkXxQ/tvUs=
-golang.org/x/mod v0.40.0/go.mod h1:0/weTWkPWGBikyTWAX3dkjVztMmBA5hM0DH6BElSupE=
+golang.org/x/mod v0.39.0 h1:UF5zwQdCRRUpHfyPwr7d4UrGiVeldIsogtzWVnczL74=
+golang.org/x/mod v0.39.0/go.mod h1:bvIbwjQ0HUFFf5AKukeeYQG4ZBUG9yxQbR9aEweIwYY=
 golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
 golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=

server/services/encryption/aes_encryption.go

@@ -17,12 +17,11 @@ package encryption
 import (
 	"crypto/aes"
 	"crypto/cipher"
+	"crypto/sha3"
+	"encoding/hex"
 	"errors"
 	"fmt"
 
-	"golang.org/x/crypto/bcrypt"
-	"golang.org/x/crypto/sha3"
-
 	"go.woodpecker-ci.org/woodpecker/v3/server/store/types"
 )
 
@@ -31,11 +30,11 @@ func (svc *aesEncryptionService) loadCipher(password string) error {
 	if err != nil {
 		return fmt.Errorf(errTemplateAesFailedGeneratingKey, err)
 	}
-	keyHash, err := bcrypt.GenerateFromPassword(key, bcrypt.DefaultCost)
+	keyID, err := svc.deriveKeyID(key)
 	if err != nil {
 		return fmt.Errorf(errTemplateAesFailedGeneratingKeyID, err)
 	}
-	svc.keyID = string(keyHash)
+	svc.keyID = keyID
 
 	block, err := aes.NewCipher(key)
 	if err != nil {
@@ -59,17 +58,42 @@ func (svc *aesEncryptionService) validateKey() error {
 	}
 
 	plaintext, err := svc.Decrypt(ciphertextSample, keyIDAssociatedData)
+	if err != nil {
+		return errEncryptionKeyInvalid
+	}
 	if plaintext != svc.keyID {
 		return errEncryptionKeyInvalid
-	} else if err != nil {
-		return err
 	}
 	return nil
 }
 
+// deriveKeyID derives a deterministic, non-reversible identifier for the
+// encryption key. Domain separation ensures the id differs from the key
+// derivation of the password, so storing it (encrypted) leaks nothing about
+// the key itself. It must be deterministic: it is compared against the
+// sample stored in the database to validate the key across server restarts.
+func (svc *aesEncryptionService) deriveKeyID(key []byte) (string, error) {
+	result := make([]byte, 32)
+	sha := sha3.NewSHAKE256()
+
+	_, err := sha.Write([]byte(keyIDDomainSeparation))
+	if err != nil {
+		return "", fmt.Errorf(errTemplateAesFailedCalculatingHash, err)
+	}
+	_, err = sha.Write(key)
+	if err != nil {
+		return "", fmt.Errorf(errTemplateAesFailedCalculatingHash, err)
+	}
+	_, err = sha.Read(result)
+	if err != nil {
+		return "", fmt.Errorf(errTemplateAesFailedCalculatingHash, err)
+	}
+	return hex.EncodeToString(result), nil
+}
+
 func (svc *aesEncryptionService) hash(data []byte) ([]byte, error) {
 	result := make([]byte, 32)
-	sha := sha3.NewShake256()
+	sha := sha3.NewSHAKE256()
 
 	_, err := sha.Write(data)
 	if err != nil {

server/services/encryption/aes_test.go

@@ -16,11 +16,14 @@ package encryption
 
 import (
 	"encoding/base64"
+	"encoding/hex"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/tink-crypto/tink-go/v2/subtle/random"
+
+	store_mocks "go.woodpecker-ci.org/woodpecker/v3/server/store/mocks"
 )
 
 func TestShortMessageLongKey(t *testing.T) {
@@ -162,3 +165,83 @@ func TestRandomBytesLength(t *testing.T) {
 		assert.Len(t, bytes, int(length), "random bytes should have requested length")
 	}
 }
+
+func TestKeyDerivationKnownVector(t *testing.T) {
+	// Pins the password -> AES key derivation (SHAKE256, 32 bytes) so that
+	// refactorings do not silently break decryption of existing data.
+	aes := &aesEncryptionService{}
+	key, err := aes.hash([]byte("this-is-a-test-password"))
+	require.NoError(t, err)
+	assert.Equal(t, "fd0331e5103fcd88306554e97f1e25e1b7fa73622ed18dd8a396d194f9271f6a", hex.EncodeToString(key))
+}
+
+func TestKeyIDDeterministic(t *testing.T) {
+	// The key id must be stable across service instances (server restarts),
+	// otherwise validateKey rejects the correct key after a restart.
+	password := string(random.GetRandomBytes(32))
+
+	first := &aesEncryptionService{}
+	require.NoError(t, first.loadCipher(password))
+
+	second := &aesEncryptionService{}
+	require.NoError(t, second.loadCipher(password))
+
+	assert.NotEmpty(t, first.keyID)
+	assert.Equal(t, first.keyID, second.keyID)
+}
+
+func TestKeyIDDiffersPerPassword(t *testing.T) {
+	first := &aesEncryptionService{}
+	require.NoError(t, first.loadCipher("password-one"))
+
+	second := &aesEncryptionService{}
+	require.NoError(t, second.loadCipher("password-two"))
+
+	assert.NotEqual(t, first.keyID, second.keyID)
+}
+
+func TestKeyIDDiffersFromKey(t *testing.T) {
+	// The key id is stored (encrypted) in the database and must never leak
+	// the raw AES key or the plain key derivation of the password.
+	aes := &aesEncryptionService{}
+	require.NoError(t, aes.loadCipher("some-password"))
+
+	key, err := aes.hash([]byte("some-password"))
+	require.NoError(t, err)
+
+	assert.NotEqual(t, hex.EncodeToString(key), aes.keyID)
+	assert.NotContains(t, aes.keyID, hex.EncodeToString(key))
+}
+
+func TestValidateKeyAcrossRestart(t *testing.T) {
+	password := string(random.GetRandomBytes(32))
+
+	// first service instance: enable encryption and store the sample
+	first := &aesEncryptionService{}
+	require.NoError(t, first.loadCipher(password))
+	sample, err := first.Encrypt(first.keyID, keyIDAssociatedData)
+	require.NoError(t, err)
+
+	// second service instance ("after restart"): same password, stored sample
+	s := store_mocks.NewMockStore(t)
+	s.On("ServerConfigGet", ciphertextSampleConfigKey).Return(sample, nil)
+
+	second := &aesEncryptionService{store: s}
+	require.NoError(t, second.loadCipher(password))
+	assert.NoError(t, second.validateKey())
+}
+
+func TestValidateKeyWrongPassword(t *testing.T) {
+	// sample was created with a different password -> key must be rejected
+	first := &aesEncryptionService{}
+	require.NoError(t, first.loadCipher("correct-password"))
+	sample, err := first.Encrypt(first.keyID, keyIDAssociatedData)
+	require.NoError(t, err)
+
+	s := store_mocks.NewMockStore(t)
+	s.On("ServerConfigGet", ciphertextSampleConfigKey).Return(sample, nil)
+
+	second := &aesEncryptionService{store: s}
+	require.NoError(t, second.loadCipher("wrong-password"))
+	assert.Error(t, second.validateKey())
+}

server/services/encryption/constants.go

@@ -91,6 +91,8 @@ const (
 
 // AES.
 const (
+	keyIDDomainSeparation = "woodpecker-encryption-key-id/v1"
+
 	// Error wrapping templates.
 	errTemplateAesFailedLoadingCipher   = "failed loading encryption cipher: %w"
 	errTemplateAesFailedCalculatingHash = "failed calculating hash: %w"