diff --git a/go.mod b/go.mod index 42b3a770c..4c911ffff 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 68a86f37e..4c671f84d 100644 --- a/go.sum +++ b/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= diff --git a/server/services/encryption/aes_encryption.go b/server/services/encryption/aes_encryption.go index 2f27868ec..53289ad3c 100644 --- a/server/services/encryption/aes_encryption.go +++ b/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 { diff --git a/server/services/encryption/aes_test.go b/server/services/encryption/aes_test.go index cd2eb995c..638e2ee68 100644 --- a/server/services/encryption/aes_test.go +++ b/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()) +} diff --git a/server/services/encryption/constants.go b/server/services/encryption/constants.go index d3926653c..cb8d1da47 100644 --- a/server/services/encryption/constants.go +++ b/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"