Make the nonce stable when generating delete records

Author: Sewci0

endpoint/crypto.go

@@ -29,27 +29,32 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
+const standardGcmNonceSize = 12
+
+// GenerateNonce creates a random nonce of a fixed size
+func GenerateNonce() ([]byte, error) {
+	nonce := make([]byte, standardGcmNonceSize)
+	if _, err := io.ReadFull(rand.Reader, nonce); err != nil {
+		return nil, err
+	}
+	return []byte(base64.StdEncoding.EncodeToString(nonce)), nil
+}
+
 // EncryptText gzip input data and encrypts it using the supplied AES key
 func EncryptText(text string, aesKey []byte, nonceEncoded []byte) (string, error) {
 	block, err := aes.NewCipher(aesKey)
 	if err != nil {
 		return "", err
 	}
 
-	gcm, err := cipher.NewGCM(block)
+	gcm, err := cipher.NewGCMWithNonceSize(block, standardGcmNonceSize)
 	if err != nil {
 		return "", err
 	}
 
-	nonce := make([]byte, gcm.NonceSize())
-	if nonceEncoded == nil {
-		if _, err = io.ReadFull(rand.Reader, nonce); err != nil {
-			return "", err
-		}
-	} else {
-		if _, err = base64.StdEncoding.Decode(nonce, nonceEncoded); err != nil {
-			return "", err
-		}
+	nonce := make([]byte, standardGcmNonceSize)
+	if _, err = base64.StdEncoding.Decode(nonce, nonceEncoded); err != nil {
+		return "", err
 	}
 
 	data, err := compressData([]byte(text))

endpoint/labels.go

@@ -119,6 +119,9 @@ func (l Labels) SerializePlain(withQuotes bool) string {
 	sort.Strings(keys) // sort for consistency
 
 	for _, key := range keys {
+		if key == txtEncryptionNonce {
+			continue
+		}
 		tokens = append(tokens, fmt.Sprintf("%s/%s=%s", heritage, key, l[key]))
 	}
 	if withQuotes {
@@ -133,10 +136,16 @@ func (l Labels) Serialize(withQuotes bool, txtEncryptEnabled bool, aesKey []byte
 		return l.SerializePlain(withQuotes)
 	}
 
-	var encryptionNonce []byte = nil
+	var encryptionNonce []byte
 	if extractedNonce, nonceExists := l[txtEncryptionNonce]; nonceExists {
 		encryptionNonce = []byte(extractedNonce)
-		delete(l, txtEncryptionNonce)
+	} else {
+		var err error
+		encryptionNonce, err = GenerateNonce()
+		if err != nil {
+			log.Fatalf("Failed to generate cryptographic nonce %#v.", err)
+		}
+		l[txtEncryptionNonce] = string(encryptionNonce)
 	}
 
 	text := l.SerializePlain(false)

registry/txt.go

@@ -220,7 +220,7 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo
 
 	endpoints := make([]*endpoint.Endpoint, 0)
 
-	if !im.mapper.recordTypeInAffix() && r.RecordType != endpoint.RecordTypeAAAA {
+	if !im.txtEncryptEnabled && !im.mapper.recordTypeInAffix() && r.RecordType != endpoint.RecordTypeAAAA {
 		// old TXT record format
 		txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), endpoint.RecordTypeTXT, r.Labels.Serialize(true, im.txtEncryptEnabled, im.txtEncryptAESKey))
 		if txt != nil {

registry/txt_test.go

@@ -1527,6 +1527,47 @@ func TestFailGenerateTXT(t *testing.T) {
 	assert.Equal(t, expectedTXT, gotTXT)
 }
 
+func TestTXTRegistryApplyChangesEncrypt(t *testing.T) {
+	p := inmemory.NewInMemoryProvider()
+	p.CreateZone(testZone)
+	ctxEndpoints := []*endpoint.Endpoint{}
+	ctx := context.WithValue(context.Background(), provider.RecordsContextKey, ctxEndpoints)
+
+	p.ApplyChanges(ctx, &plan.Changes{
+		Create: []*endpoint.Endpoint{
+			newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, ""),
+			newEndpointWithOwnerAndOwnedRecord("txt.cname-foobar.test-zone.example.org", "\"h8UQ6jelUFUsEIn7SbFktc2MYXPx/q8lySqI4VwfVtVaIbb2nkHWV/88KKbuLtu7fJNzMir8ELVeVnRSY01KdiIuj7ledqZe5ailEjQaU5Z6uEKd5pgs6sH8\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"),
+		},
+	})
+
+	r, _ := NewTXTRegistry(p, "txt.", "", "owner", time.Hour, "", []string{}, []string{}, true, []byte("12345678901234567890123456789012"))
+	records, _ := r.Records(ctx)
+	changes := &plan.Changes{
+		Delete: records,
+	}
+
+	// ensure that encryption nonce gets reused when deleting records
+	expected := &plan.Changes{
+		Delete: []*endpoint.Endpoint{
+			newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"),
+			newEndpointWithOwnerAndOwnedRecord("txt.cname-foobar.test-zone.example.org", "\"h8UQ6jelUFUsEIn7SbFktc2MYXPx/q8lySqI4VwfVtVaIbb2nkHWV/88KKbuLtu7fJNzMir8ELVeVnRSY01KdiIuj7ledqZe5ailEjQaU5Z6uEKd5pgs6sH8\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"),
+		},
+	}
+
+	p.OnApplyChanges = func(ctx context.Context, got *plan.Changes) {
+		mExpected := map[string][]*endpoint.Endpoint{
+			"Delete": expected.Delete,
+		}
+		mGot := map[string][]*endpoint.Endpoint{
+			"Delete": got.Delete,
+		}
+		assert.True(t, testutils.SamePlanChanges(mGot, mExpected))
+		assert.Equal(t, nil, ctx.Value(provider.RecordsContextKey))
+	}
+	err := r.ApplyChanges(ctx, changes)
+	require.NoError(t, err)
+}
+
 // TestMultiClusterDifferentRecordTypeOwnership validates the registry handles environments where the same zone is managed by
 // external-dns in different clusters and the ingress record type is different. For example one uses A records and the other
 // uses CNAME. In this environment the first cluster that establishes the owner record should maintain ownership even