dynamiclistener: add PruneExistingCN to Config (#301)

Add PruneExistingCN bool to Config (alongside the existing FilterCN). When true, FilterCN is also applied to the set of CNs already recorded on the secret via its listener.cattle.io/cn-* annotations at every operation that reads or merges that set: AddCN, Merge, Renew, Regenerate, and certificate generation. Any existing CN that FilterCN would reject is dropped from the certificate the next time it is written. This gives callers a way to keep the stored CN set trimmed to a known-valid subset — for example, pruning IP addresses that are no longer valid endpoints after a rolling restart — without requiring an explicit delete-and-regenerate cycle. false (the default) preserves all existing CNs as before (backwards compatible). No separate filter callback is needed: the same FilterCN that gates new additions also governs what is kept when PruneExistingCN is true. Merge is updated to call generateCert instead of returning an existing cert unchanged when the cert contains CNs that FilterCN would remove, ensuring stale entries are pruned on the next storage sync rather than silently propagated. The pruneAnnotations call inside generateCert is moved to after populateCN so the cert and its annotation set stay in sync. PruneExistingCN is wired through NewListenerWithChain; SANs pre-seeded in Config.SANs are always preserved via allowDefaultSANs regardless of the filter. Add unit tests covering hasStaleCNs, pruneAnnotations, Merge (stale in target, stale in additional, no stale, PruneExistingCN=false, static target), Renew, Regenerate, and AddCN.
2026-07-01 14:54:24 +00:00 · 2026-06-19 15:07:13 -04:00
parent 1c353f118e
commit 02e46989f2
3 changed files with 341 additions and 19 deletions
--- a/factory/gen.go
+++ b/factory/gen.go
@@ -37,10 +37,19 @@ type TLS struct {
 	CAKey               crypto.Signer
 	CN                  string
 	Organization        []string
-	FilterCN            func(...string) []string
+	FilterCN        func(...string) []string
+	// FilterExisting, when true, applies FilterCN to the set of CNs already
+	// recorded on the secret (via cnPrefix annotations) inside Merge, AddCN,
+	// Renew, Regenerate, and cert generation. Any existing CN that FilterCN
+	// would reject is pruned from the cert on the next write. false (default)
+	// preserves all existing CNs as before.
+	FilterExisting bool
 	ExpirationDaysCheck int
 }

+// cns returns the raw CN set recorded on the secret via cnPrefix
+// annotations. No filtering is applied. Used for raw-count checks (e.g.
+// the MaxSANs cap in NeedsUpdate) where the literal stored count matters.
 func cns(secret *v1.Secret) (cns []string) {
 	if secret == nil {
 		return nil
@@ -53,10 +62,19 @@ func cns(secret *v1.Secret) (cns []string) {
 	return
 }

-func collectCNs(secret *v1.Secret) (domains []string, ips []net.IP, err error) {
-	var (
-		cns = cns(secret)
-	)
+// filteredCNs returns the secret's stored CNs, with FilterCN applied to the
+// existing set when FilterExisting is true. Used everywhere the valid CN set
+// is needed for merging or cert generation.
+func (t *TLS) filteredCNs(secret *v1.Secret) []string {
+	out := cns(secret)
+	if t.FilterExisting && t.FilterCN != nil {
+		out = t.FilterCN(out...)
+	}
+	return out
+}
+
+func (t *TLS) collectCNs(secret *v1.Secret) (domains []string, ips []net.IP, err error) {
+	cns := t.filteredCNs(secret)

 	sort.Strings(cns)

@@ -72,6 +90,35 @@ func collectCNs(secret *v1.Secret) (domains []string, ips []net.IP, err error) {
 	return
 }

+// hasStaleCNs reports whether the secret has CN annotations that FilterCN
+// would remove when FilterExisting is true. When true, Merge must call
+// generateCert rather than returning the cert as-is.
+func (t *TLS) hasStaleCNs(secret *v1.Secret) bool {
+	if !t.FilterExisting || t.FilterCN == nil || secret == nil {
+		return false
+	}
+	return len(t.filteredCNs(secret)) < len(cns(secret))
+}
+
+// pruneAnnotations strips cnPrefix annotations whose values FilterCN would
+// reject, when FilterExisting is true. No-op otherwise. Other annotations
+// (Static, Fingerprint, etc.) are preserved.
+func (t *TLS) pruneAnnotations(secret *v1.Secret) *v1.Secret {
+	if !t.FilterExisting || t.FilterCN == nil || secret == nil || secret.Annotations == nil {
+		return secret
+	}
+	keep := make(map[string]bool)
+	for _, cn := range t.filteredCNs(secret) {
+		keep[cn] = true
+	}
+	for k, v := range secret.Annotations {
+		if strings.HasPrefix(k, cnPrefix) && !keep[v] {
+			delete(secret.Annotations, k)
+		}
+	}
+	return secret
+}
+
 // Merge combines the SAN lists from the target and additional Secrets, and
 // returns a potentially modified Secret, along with a bool indicating if the
 // returned Secret is not the same as the target Secret. Secrets with expired
@@ -92,18 +139,27 @@ func (t *TLS) Merge(target, additional *v1.Secret) (*v1.Secret, bool, error) {
 		return target, false, nil
 	}

-	mergedCNs := append(cns(target), cns(additional)...)
+	mergedCNs := append(t.filteredCNs(target), t.filteredCNs(additional)...)

 	// if the additional secret already has all the CNs, use it in preference to the
 	// current one. This behavior is required to allow for renewal or regeneration.
+	// If FilterExisting is active and the cert has stale CNs beyond the filtered
+	// set, fall through to generateCert so pruneAnnotations can remove them.
 	if !NeedsUpdate(0, additional, mergedCNs...) && !t.IsExpired(additional) {
-		return additional, true, nil
+		if !t.hasStaleCNs(additional) {
+			return additional, true, nil
+		}
+		return t.generateCert(additional, mergedCNs...)
 	}

 	// if the target secret already has all the CNs, continue using it. The additional
 	// cert had only a subset of the current CNs, so nothing needs to be added.
+	// Same staleness check as above.
 	if !NeedsUpdate(0, target, mergedCNs...) && !t.IsExpired(target) {
-		return target, false, nil
+		if !t.hasStaleCNs(target) {
+			return target, false, nil
+		}
+		return t.generateCert(target, mergedCNs...)
 	}

 	// neither cert currently has all the necessary CNs or is unexpired; generate a new one.
@@ -117,7 +173,7 @@ func (t *TLS) Renew(secret *v1.Secret) (*v1.Secret, error) {
 	if IsStatic(secret) {
 		return secret, cert.ErrStaticCert
 	}
-	cns := cns(secret)
+	cns := t.filteredCNs(secret)
 	secret = secret.DeepCopy()
 	secret.Annotations = map[string]string{}
 	secret, _, err := t.generateCert(secret, cns...)
@@ -146,7 +202,7 @@ func (t *TLS) AddCN(secret *v1.Secret, cn ...string) (*v1.Secret, bool, error) {
 }

 func (t *TLS) Regenerate(secret *v1.Secret) (*v1.Secret, error) {
-	cns := cns(secret)
+	cns := t.filteredCNs(secret)
 	secret, _, err := t.generateCert(nil, cns...)
 	return secret, err
 }
@@ -163,12 +219,16 @@ func (t *TLS) generateCert(secret *v1.Secret, cn ...string) (*v1.Secret, bool, e

 	secret = populateCN(secret, cn...)

+	// Drop CN annotations that FilterCN would reject — including any
+	// just added by populateCN — so the cert and its annotations stay in sync.
+	secret = t.pruneAnnotations(secret)
+
 	privateKey, err := getPrivateKey(secret)
 	if err != nil {
 		return nil, false, err
 	}

-	domains, ips, err := collectCNs(secret)
+	domains, ips, err := t.collectCNs(secret)
 	if err != nil {
 		return nil, false, err
 	}
--- a/factory/gen_test.go
+++ b/factory/gen_test.go
@@ -7,7 +7,10 @@ import (
 	"testing"

 	"github.com/rancher/dynamiclistener/cert"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

 var hashSuffixRe = regexp.MustCompile(`-[0-9a-f]{6}$`)
@@ -300,18 +303,12 @@ func TestMerge_WildcardCovering(t *testing.T) {
 		t.Fatalf("Merge: %v", err)
 	}

-	// Wildcard must be present.
 	assertCertHasDNSName(t, merged, "*.example.com")
-	// And the specific names from `additional` must NOT have been added —
-	// the wildcard already covers them per RFC 6125. This is what distinguishes
-	// the cover-short-circuit path from a regenerate-fallthrough path.
 	assertCertDoesNotHaveDNSName(t, merged, "foo.example.com")
 	assertCertDoesNotHaveDNSName(t, merged, "bar.example.com")
 }

 func TestAddCN_WildcardAndSpecificCoexist(t *testing.T) {
-	// Realistic shape: admin configures both a wildcard SAN and one or more
-	// specific SANs in a single call. Both must end up in the cert.
 	tlsFactory := newTestTLS(t)
 	secret, _, err := tlsFactory.AddCN(nil, "*.example.com", "other.org")
 	if err != nil {
@@ -320,3 +317,258 @@ func TestAddCN_WildcardAndSpecificCoexist(t *testing.T) {
 	assertCertHasDNSName(t, secret, "*.example.com")
 	assertCertHasDNSName(t, secret, "other.org")
 }
+
+// --- secretWithCNs helper for FilterExisting tests ---
+
+// secretWithCNs returns a dynamiclistener-managed TLS secret pre-populated
+// with the given CNs in its cnPrefix annotations plus real cert data.
+func secretWithCNs(t *testing.T, tls *TLS, cns ...string) *v1.Secret {
+	t.Helper()
+	s := &v1.Secret{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}}
+	result, _, err := tls.generateCert(s, cns...)
+	require.NoError(t, err)
+	return result
+}
+
+// onlyAllow returns a FilterCN that keeps only the listed CNs.
+func onlyAllow(allowed ...string) func(...string) []string {
+	set := make(map[string]bool, len(allowed))
+	for _, cn := range allowed {
+		set[cn] = true
+	}
+	return func(cns ...string) []string {
+		var out []string
+		for _, cn := range cns {
+			if set[cn] {
+				out = append(out, cn)
+			}
+		}
+		return out
+	}
+}
+
+// --- hasStaleCNs ---
+
+func TestHasStaleCNs_NilFilter(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	s := secretWithCNs(t, tls, "live.example.com", "dead.example.com")
+	assert.False(t, tls.hasStaleCNs(s))
+}
+
+func TestHasStaleCNs_NoStale(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+	s := secretWithCNs(t, tls, "live.example.com")
+	assert.False(t, tls.hasStaleCNs(s), "no stale CNs expected")
+}
+
+func TestHasStaleCNs_WithStale(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+	s := secretWithCNs(t, tls, "live.example.com")
+	s.Annotations[getAnnotationKey("dead.example.com")] = "dead.example.com"
+	assert.True(t, tls.hasStaleCNs(s), "dead.example.com should be stale")
+}
+
+// --- pruneAnnotations ---
+
+func TestPruneAnnotations_NilFilter(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	s := secretWithCNs(t, tls, "a.example.com", "b.example.com")
+	before := len(cns(s))
+	tls.pruneAnnotations(s)
+	assert.Equal(t, before, len(cns(s)), "FilterExisting=false must not prune anything")
+}
+
+func TestPruneAnnotations_RemovesStale(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+	s := secretWithCNs(t, tls, "live.example.com")
+	s.Annotations[getAnnotationKey("dead.example.com")] = "dead.example.com"
+
+	tls.pruneAnnotations(s)
+
+	remaining := cns(s)
+	assert.Len(t, remaining, 1)
+	assert.Contains(t, remaining, "live.example.com")
+	assert.NotContains(t, remaining, "dead.example.com")
+}
+
+func TestPruneAnnotations_PreservesNonCNAnnotations(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+	s := secretWithCNs(t, tls, "live.example.com")
+	s.Annotations[getAnnotationKey("dead.example.com")] = "dead.example.com"
+	s.Annotations["custom/annotation"] = "preserved"
+
+	tls.pruneAnnotations(s)
+
+	assert.Equal(t, "preserved", s.Annotations["custom/annotation"],
+		"non-CN annotations must be preserved")
+}
+
+// --- Merge with FilterExisting ---
+
+func TestMerge_StaleInTarget_ForcesRegeneration(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+
+	target := secretWithCNs(t, tls, "live.example.com")
+	target.Annotations[getAnnotationKey("dead.example.com")] = "dead.example.com"
+	additional := secretWithCNs(t, tls, "live.example.com")
+
+	result, _, err := tls.Merge(target, additional)
+	require.NoError(t, err)
+
+	assert.NotContains(t, cns(result), "dead.example.com",
+		"stale CN must be pruned by Merge")
+	assert.Contains(t, cns(result), "live.example.com")
+}
+
+func TestMerge_StaleInAdditional_ForcesRegeneration(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+
+	target := secretWithCNs(t, tls, "live.example.com")
+	additional := secretWithCNs(t, tls, "live.example.com")
+	additional.Annotations[getAnnotationKey("dead.example.com")] = "dead.example.com"
+
+	result, _, err := tls.Merge(target, additional)
+	require.NoError(t, err)
+
+	assert.NotContains(t, cns(result), "dead.example.com")
+	assert.Contains(t, cns(result), "live.example.com")
+}
+
+func TestMerge_NoStale_ReturnsExistingWithoutRegeneration(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("a.example.com", "b.example.com")
+	tls.FilterExisting = true
+
+	target := secretWithCNs(t, tls, "a.example.com", "b.example.com")
+	additional := secretWithCNs(t, tls, "a.example.com")
+
+	result, updated, err := tls.Merge(target, additional)
+	require.NoError(t, err)
+
+	assert.False(t, updated)
+	assert.Equal(t, target.Annotations[Fingerprint], result.Annotations[Fingerprint],
+		"fingerprint should match target (returned without regen)")
+}
+
+func TestMerge_FilterExisting_False_BackwardsCompatible(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	target := secretWithCNs(t, tls, "a.example.com", "b.example.com")
+	additional := secretWithCNs(t, tls, "a.example.com")
+
+	result, updated, err := tls.Merge(target, additional)
+	require.NoError(t, err)
+	assert.False(t, updated)
+	assert.Equal(t, target.Annotations[Fingerprint], result.Annotations[Fingerprint],
+		"target satisfies all merged CNs and has no stale → returned unchanged")
+}
+
+func TestMerge_StaticTarget_NeverModified(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+
+	target := secretWithCNs(t, tls, "live.example.com")
+	target.Annotations[Static] = "true"
+	additional := secretWithCNs(t, tls, "live.example.com")
+
+	result, updated, err := tls.Merge(target, additional)
+	require.NoError(t, err)
+	assert.False(t, updated, "static target must never be replaced")
+	assert.Equal(t, target, result, "static target must be returned unchanged")
+}
+
+// --- Renew prunes stale CNs ---
+
+func TestRenew_PrunesStale(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+
+	s := secretWithCNs(t, tls, "live.example.com")
+	s.Annotations[getAnnotationKey("dead.example.com")] = "dead.example.com"
+	result, err := tls.Renew(s)
+	require.NoError(t, err)
+
+	assert.Contains(t, cns(result), "live.example.com")
+	assert.NotContains(t, cns(result), "dead.example.com")
+}
+
+func TestRenew_NilFilter_KeepsAll(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	s := secretWithCNs(t, tls, "a.example.com", "b.example.com")
+	result, err := tls.Renew(s)
+	require.NoError(t, err)
+	assert.ElementsMatch(t, []string{"a.example.com", "b.example.com"}, cns(result))
+}
+
+// --- Regenerate prunes stale CNs ---
+
+func TestRegenerate_PrunesStale(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com")
+	tls.FilterExisting = true
+
+	s := secretWithCNs(t, tls, "live.example.com")
+	s.Annotations[getAnnotationKey("dead.example.com")] = "dead.example.com"
+	result, err := tls.Regenerate(s)
+	require.NoError(t, err)
+
+	assert.Contains(t, cns(result), "live.example.com")
+	assert.NotContains(t, cns(result), "dead.example.com")
+}
+
+// --- AddCN: stale existing annotation is pruned when generateCert is triggered ---
+
+func TestAddCN_PrunesStaleAnnotationOnGeneration(t *testing.T) {
+	t.Parallel()
+	tls := newTestTLS(t)
+	tls.FilterCN = onlyAllow("live.example.com", "other.example.com")
+	tls.FilterExisting = true
+
+	s := secretWithCNs(t, tls, "live.example.com")
+	// Simulate a stale CN left over from before the filter was set.
+	s.Annotations[getAnnotationKey("dead.example.com")] = "dead.example.com"
+
+	// Adding a new valid CN triggers generateCert, which prunes stale annotations.
+	result, updated, err := tls.AddCN(s, "other.example.com")
+	require.NoError(t, err)
+	require.True(t, updated)
+
+	assert.NotContains(t, cns(result), "dead.example.com",
+		"stale CN must be pruned by pruneAnnotations during generation")
+	assert.Contains(t, cns(result), "live.example.com")
+	assert.Contains(t, cns(result), "other.example.com")
+
+	certs, err := cert.ParseCertsPEM(result.Data[v1.TLSCertKey])
+	require.NoError(t, err)
+	require.NotEmpty(t, certs)
+	for _, san := range certs[0].DNSNames {
+		assert.NotEqual(t, "dead.example.com", san)
+	}
+}
--- a/listener.go
+++ b/listener.go
@@ -66,7 +66,8 @@ func NewListenerWithChain(l net.Listener, storage TLSStorage, caCert []*x509.Cer
 			CAKey:               caKey,
 			CN:                  config.CN,
 			Organization:        config.Organization,
-			FilterCN:            allowDefaultSANs(config.SANs, config.FilterCN),
+			FilterCN:        allowDefaultSANs(config.SANs, config.FilterCN),
+			FilterExisting: config.FilterExisting,
 			ExpirationDaysCheck: config.ExpirationDaysCheck,
 		},
 		Listener:  l,
@@ -151,7 +152,16 @@ type Config struct {
 	ExpirationDaysCheck   int
 	CloseConnOnCertChange bool
 	RegenerateCerts       func() bool
-	FilterCN              func(...string) []string
+	FilterCN func(...string) []string
+	// FilterExisting controls whether FilterCN is also applied to the set of
+	// CNs already recorded on the secret (via listener.cattle.io/cn-*
+	// annotations) during every cert operation (AddCN, Merge, Renew,
+	// Regenerate, cert generation). When true, any existing CN that FilterCN
+	// would reject is pruned from the certificate on the next write — useful
+	// for automatically removing stale dynamically-added CNs (e.g. dead pod
+	// IPs after a rolling restart) without an explicit delete-and-regenerate.
+	// false (the default) preserves all existing CNs as before.
+	FilterExisting bool
 }

 type listener struct {