Merge pull request #4980 from gofogo/fix-aes-4976

fix(aes-encryption): support plain txt and url safe base64 strings

Author: k8s-ci-robot

docs/registry/txt.md

@@ -26,11 +26,11 @@ wildcard domains will have invalid domain syntax and be rejected by most provide
 
 ## Encryption
 
-Registry TXT records may contain information, such as the internal ingress name or namespace, considered sensitive, , which attackers could exploit to gather information about your infrastructure. 
+Registry TXT records may contain information, such as the internal ingress name or namespace, considered sensitive, , which attackers could exploit to gather information about your infrastructure.
 By encrypting TXT records, you can protect this information from unauthorized access.
 
-Encryption is enabled by using the `--txt-encrypt-enabled` flag. The 32-byte AES-256-GCM encryption
-key must be specified in URL-safe base64 form, using the `--txt-encrypt-aes-key` flag.
+Encryption is enabled by setting the `--txt-encrypt-enabled`. The 32-byte AES-256-GCM encryption
+key must be specified in URL-safe base64 form (recommended) or be a plain text, using the `--txt-encrypt-aes-key=<key>` flag.
 
 Note that the key used for encryption should be a secure key and properly managed to ensure the security of your TXT records.
 
@@ -78,14 +78,32 @@ import (
 )
 
 func main() {
-	key := []byte("testtesttesttesttesttesttesttest")
-	encrypted, _ := endpoint.EncryptText(
-		"heritage=external-dns,external-dns/owner=example,external-dns/resource=ingress/default/example",
-		key,
-		nil,
-	)
-	decrypted, _, _ := endpoint.DecryptText(encrypted, key)
-	fmt.Println(decrypted)
+	keys := []string{
+		"ZPitL0NGVQBZbTD6DwXJzD8RiStSazzYXQsdUowLURY=", // safe base64 url encoded 44 bytes and 32 when decoded
+		"01234567890123456789012345678901",             // plain txt 32 bytes
+		"passphrasewhichneedstobe32bytes!",             // plain txt 32 bytes
+	}
+
+	for _, k := range keys {
+		key := []byte(k)
+		if len(key) != 32 {
+			// if key is not a plain txt let's decode
+			var err error
+			if key, err = b64.StdEncoding.DecodeString(string(key)); err != nil || len(key) != 32 {
+				fmt.Errorf("the AES Encryption key must have a length of 32 byte")
+			}
+		}
+		encrypted, _ := endpoint.EncryptText(
+			"heritage=external-dns,external-dns/owner=example,external-dns/resource=ingress/default/example",
+			key,
+			nil,
+		)
+		decrypted, _, err := endpoint.DecryptText(encrypted, key)
+		if err != nil {
+			fmt.Println("Error decrypting:", err, "for key:", k)
+		}
+		fmt.Println(decrypted)
+	}
 }
 ```
 

endpoint/crypto_test.go

@@ -17,8 +17,12 @@ limitations under the License.
 package endpoint
 
 import (
+	"encoding/base64"
+	"io"
 	"testing"
 
+	"crypto/rand"
+
 	"github.com/stretchr/testify/require"
 )
 
@@ -56,3 +60,33 @@ func TestEncrypt(t *testing.T) {
 		t.Error("Decryption of text didn't result in expected plaintext result.")
 	}
 }
+
+func TestGenerateNonceSuccess(t *testing.T) {
+	nonce, err := GenerateNonce()
+	require.NoError(t, err)
+	require.NotEmpty(t, nonce)
+
+	// Test nonce length
+	decodedNonce, err := base64.StdEncoding.DecodeString(string(nonce))
+	require.NoError(t, err)
+	require.Equal(t, standardGcmNonceSize, len(decodedNonce))
+}
+
+func TestGenerateNonceError(t *testing.T) {
+	// Save the original rand.Reader
+	originalRandReader := rand.Reader
+	defer func() { rand.Reader = originalRandReader }()
+
+	// Replace rand.Reader with a faulty reader
+	rand.Reader = &faultyReader{}
+
+	nonce, err := GenerateNonce()
+	require.Error(t, err)
+	require.Nil(t, nonce)
+}
+
+type faultyReader struct{}
+
+func (f *faultyReader) Read(p []byte) (n int, err error) {
+	return 0, io.ErrUnexpectedEOF
+}

endpoint/labels.go

@@ -93,8 +93,8 @@ func NewLabelsFromStringPlain(labelText string) (Labels, error) {
 func NewLabelsFromString(labelText string, aesKey []byte) (Labels, error) {
 	if len(aesKey) != 0 {
 		decryptedText, encryptionNonce, err := DecryptText(strings.Trim(labelText, "\""), aesKey)
-		//in case if we have decryption error, just try process original text
-		//decryption errors should be ignored here, because we can already have plain-text labels in registry
+		// in case if we have decryption error, just try process original text
+		// decryption errors should be ignored here, because we can already have plain-text labels in registry
 		if err == nil {
 			labels, err := NewLabelsFromStringPlain(decryptedText)
 			if err == nil {
@@ -152,8 +152,8 @@ func (l Labels) Serialize(withQuotes bool, txtEncryptEnabled bool, aesKey []byte
 	log.Debugf("Encrypt the serialized text %#v before returning it.", text)
 	var err error
 	text, err = EncryptText(text, aesKey, encryptionNonce)
-
 	if err != nil {
+		// if encryption failed, the external-dns will crash
 		log.Fatalf("Failed to encrypt the text %#v using the encryption key %#v. Got error %#v.", text, aesKey, err)
 	}
 

endpoint/labels_test.go

@@ -17,9 +17,12 @@ limitations under the License.
 package endpoint
 
 import (
+	"bytes"
+	"crypto/rand"
 	"fmt"
 	"testing"
 
+	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/suite"
 )
 
@@ -79,6 +82,60 @@ func (suite *LabelsSuite) TestEncryptionNonceReUsage() {
 	suite.Equal(serialized, suite.fooAsTextEncrypted, "serialized result should be equal")
 }
 
+func (suite *LabelsSuite) TestEncryptionKeyChanged() {
+	foo, err := NewLabelsFromString(suite.fooAsTextEncrypted, suite.aesKey)
+	suite.NoError(err, "should succeed for valid label text")
+
+	serialised := foo.Serialize(false, true, []byte("passphrasewhichneedstobe32bytes!"))
+	suite.NotEqual(serialised, suite.fooAsTextEncrypted, "serialized result should be equal")
+}
+
+func (suite *LabelsSuite) TestEncryptionFailed() {
+	foo, err := NewLabelsFromString(suite.fooAsTextEncrypted, suite.aesKey)
+	suite.NoError(err, "should succeed for valid label text")
+
+	defer func() { log.StandardLogger().ExitFunc = nil }()
+
+	b := new(bytes.Buffer)
+
+	var fatalCrash bool
+	log.StandardLogger().ExitFunc = func(int) { fatalCrash = true }
+	log.StandardLogger().SetOutput(b)
+
+	_ = foo.Serialize(false, true, []byte("wrong-key"))
+
+	suite.True(fatalCrash, "should fail if encryption key is wrong")
+	suite.Contains(b.String(), "Failed to encrypt the text")
+}
+
+func (suite *LabelsSuite) TestEncryptionFailedFaultyReader() {
+	foo, err := NewLabelsFromString(suite.fooAsTextEncrypted, suite.aesKey)
+	suite.NoError(err, "should succeed for valid label text")
+
+	// remove encryption nonce just for simplicity, so that we could regenerate nonce
+	delete(foo, txtEncryptionNonce)
+
+	originalRandReader := rand.Reader
+	defer func() {
+		log.StandardLogger().ExitFunc = nil
+		rand.Reader = originalRandReader
+	}()
+
+	// Replace rand.Reader with a faulty reader
+	rand.Reader = &faultyReader{}
+
+	b := new(bytes.Buffer)
+
+	var fatalCrash bool
+	log.StandardLogger().ExitFunc = func(int) { fatalCrash = true }
+	log.StandardLogger().SetOutput(b)
+
+	_ = foo.Serialize(false, true, suite.aesKey)
+
+	suite.True(fatalCrash)
+	suite.Contains(b.String(), "Failed to generate cryptographic nonce")
+}
+
 func (suite *LabelsSuite) TestDeserialize() {
 	foo, err := NewLabelsFromStringPlain(suite.fooAsText)
 	suite.NoError(err, "should succeed for valid label text")

registry/dynamodb.go

@@ -18,6 +18,7 @@ package registry
 
 import (
 	"context"
+	b64 "encoding/base64"
 	"errors"
 	"fmt"
 	"strings"
@@ -84,7 +85,10 @@ func NewDynamoDBRegistry(provider provider.Provider, ownerID string, dynamodbAPI
 	if len(txtEncryptAESKey) == 0 {
 		txtEncryptAESKey = nil
 	} else if len(txtEncryptAESKey) != 32 {
-		return nil, errors.New("the AES Encryption key must have a length of 32 bytes")
+		var err error
+		if txtEncryptAESKey, err = b64.StdEncoding.DecodeString(string(txtEncryptAESKey)); err != nil || len(txtEncryptAESKey) != 32 {
+			return nil, errors.New("the AES Encryption key must be 32 bytes long, in either plain text or base64-encoded format")
+		}
 	}
 	if len(txtPrefix) > 0 && len(txtSuffix) > 0 {
 		return nil, errors.New("txt-prefix and txt-suffix are mutually exclusive")

registry/dynamodb_test.go

@@ -61,12 +61,51 @@ func TestDynamoDBRegistryNew(t *testing.T) {
 	require.EqualError(t, err, "table cannot be empty")
 
 	_, err = NewDynamoDBRegistry(p, "test-owner", api, "test-table", "", "", "", []string{}, []string{}, []byte(";k&l)nUC/33:{?d{3)54+,AD?]SX%yh^x"), time.Hour)
-	require.EqualError(t, err, "the AES Encryption key must have a length of 32 bytes")
+	require.EqualError(t, err, "the AES Encryption key must be 32 bytes long, in either plain text or base64-encoded format")
 
 	_, err = NewDynamoDBRegistry(p, "test-owner", api, "test-table", "testPrefix", "testSuffix", "", []string{}, []string{}, []byte(""), time.Hour)
 	require.EqualError(t, err, "txt-prefix and txt-suffix are mutually exclusive")
 }
 
+func TestDynamoDBRegistryNew_EncryptionConfig(t *testing.T) {
+	api, p := newDynamoDBAPIStub(t, nil)
+
+	tests := []struct {
+		encEnabled      bool
+		aesKeyRaw       []byte
+		aesKeySanitized []byte
+		errorExpected   bool
+	}{
+		{
+			encEnabled:      true,
+			aesKeyRaw:       []byte("123456789012345678901234567890asdfasdfasdfasdfa12"),
+			aesKeySanitized: []byte{},
+			errorExpected:   true,
+		},
+		{
+			encEnabled:      true,
+			aesKeyRaw:       []byte("passphrasewhichneedstobe32bytes!"),
+			aesKeySanitized: []byte("passphrasewhichneedstobe32bytes!"),
+			errorExpected:   false,
+		},
+		{
+			encEnabled:      true,
+			aesKeyRaw:       []byte("ZPitL0NGVQBZbTD6DwXJzD8RiStSazzYXQsdUowLURY="),
+			aesKeySanitized: []byte{100, 248, 173, 47, 67, 70, 85, 0, 89, 109, 48, 250, 15, 5, 201, 204, 63, 17, 137, 43, 82, 107, 60, 216, 93, 11, 29, 82, 140, 11, 81, 22},
+			errorExpected:   false,
+		},
+	}
+	for _, test := range tests {
+		actual, err := NewDynamoDBRegistry(p, "test-owner", api, "test-table", "", "", "", []string{}, []string{}, test.aesKeyRaw, time.Hour)
+		if test.errorExpected {
+			require.Error(t, err)
+		} else {
+			require.NoError(t, err)
+			assert.Equal(t, test.aesKeySanitized, actual.txtEncryptAESKey)
+		}
+	}
+}
+
 func TestDynamoDBRegistryRecordsBadTable(t *testing.T) {
 	for _, tc := range []struct {
 		name     string

registry/txt.go

@@ -22,6 +22,8 @@ import (
 	"strings"
 	"time"
 
+	b64 "encoding/base64"
+
 	log "github.com/sirupsen/logrus"
 
 	"sigs.k8s.io/external-dns/endpoint"
@@ -63,11 +65,16 @@ func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID st
 	if ownerID == "" {
 		return nil, errors.New("owner id cannot be empty")
 	}
+
 	if len(txtEncryptAESKey) == 0 {
 		txtEncryptAESKey = nil
 	} else if len(txtEncryptAESKey) != 32 {
-		return nil, errors.New("the AES Encryption key must have a length of 32 bytes")
+		var err error
+		if txtEncryptAESKey, err = b64.StdEncoding.DecodeString(string(txtEncryptAESKey)); err != nil || len(txtEncryptAESKey) != 32 {
+			return nil, errors.New("the AES Encryption key must be 32 bytes long, in either plain text or base64-encoded format")
+		}
 	}
+
 	if txtEncryptEnabled && txtEncryptAESKey == nil {
 		return nil, errors.New("the AES Encryption key must be set when TXT record encryption is enabled")
 	}
@@ -131,7 +138,7 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
 		}
 		// We simply assume that TXT records for the registry will always have only one target.
 		labels, err := endpoint.NewLabelsFromString(record.Targets[0], im.txtEncryptAESKey)
-		if err == endpoint.ErrInvalidHeritage {
+		if errors.Is(err, endpoint.ErrInvalidHeritage) {
 			// if no heritage is found or it is invalid
 			// case when value of txt record cannot be identified
 			// record will not be removed as it will have empty owner
@@ -237,7 +244,6 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo
 		txtNew.ProviderSpecific = r.ProviderSpecific
 		endpoints = append(endpoints, txtNew)
 	}
-
 	return endpoints
 }