diff --git a/factory/gen.go b/factory/gen.go index 71bb6bb..edff386 100644 --- 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 } diff --git a/factory/gen_test.go b/factory/gen_test.go index 2c899ad..a38a630 100644 --- 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) + } +} diff --git a/listener.go b/listener.go index cff1533..9b85e05 100644 --- 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 {