fix: detect and warn on inconsistent SSL host/certificate configuration - #2810
fix: detect and warn on inconsistent SSL host/certificate configuration#2810shreemaan-abhishek wants to merge 5 commits into
Conversation
Two correctness gaps in the SSL conflict detector let colliding TLS
configurations be admitted for the same GatewayProxy:
1. Hosts were compared by exact string equality, so a covering wildcard host
and an exact host were never compared against each other ("*.example.com"
and "app.example.com" are indexed under different keys). Because APISIX
resolves an exact SNI ahead of a covering wildcard, two objects whose hosts
overlap only through a wildcard could both be admitted and then collide at
the data plane. Add sslutil.HostsOverlap / ParentWildcard (single-label
wildcard semantics); for an exact host also look up its covering wildcard,
for a wildcard host enumerate TLS resources and filter by overlap (the
exact-key index can't answer a suffix query), and compare mappings with
HostsOverlap instead of string equality.
2. The conflict key used only the server certificate hash, so two objects for
the same host and server cert but different mTLS client config (spec.client)
were treated as non-conflicting, leaving client-verification behavior for
that SNI nondeterministic. Add ClientConfigHash to HostCertMapping (digest
of the CA secret reference, depth and skip_mtls_uri_regex; empty when no
mTLS) and treat a differing client config as a conflict too.
Adds unit tests for the overlap helpers and detector-level tests for both
wildcard/exact overlap and differing mTLS config, with false-positive guards.
shreemaan-abhishek
changed the title
There was a problem hiding this comment.
Pull request overview
This PR hardens TLS/SNI correctness in the APISIX Ingress Controller by improving SSL conflict detection (including wildcard/exact overlaps and mTLS client config differences) and by validating that declared SNI hosts are covered by the certificate’s DNS SANs during translation (to avoid silently programming invalid cert/host combos to the data plane).
Changes:
- Extend SSL conflict detection to treat wildcard/exact host overlaps as conflicting, and include mTLS client verification config in the conflict key.
- Add SSL host utility helpers (
HostsOverlap,ParentWildcard,HostCoveredBy) and unit tests for wildcard semantics + SAN coverage. - Add translator-side SAN coverage validation for
ApisixTlsand tests to ensure mismatched cert/host declarations are rejected.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/webhook/v1/ssl/conflict_detector.go | Adds mTLS client-config hashing and wildcard-aware candidate lookup for conflict detection. |
| internal/webhook/v1/ssl/conflict_detector_test.go | Adds detector-level tests covering wildcard/exact overlaps and mTLS client-config conflicts. |
| internal/ssl/util.go | Introduces wildcard-aware host overlap + SAN coverage helpers. |
| internal/ssl/util_test.go | Adds unit tests for overlap/coverage/wildcard-parent helpers. |
| internal/adc/translator/apisixtls.go | Adds SAN coverage validation before programming SSL objects for ApisixTls. |
| internal/adc/translator/apisixtls_test.go | Adds tests for SAN coverage validation behavior (match/wildcard/mismatch/no-SAN lenient). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // First, check for conflicts within the new resource itself. | ||
| seen := make(map[string]string, len(newMappings)) | ||
| seen := make(map[string]HostCertMapping, len(newMappings)) | ||
| for _, mapping := range newMappings { | ||
| if mapping.Host == "" || mapping.CertificateHash == "" { | ||
| continue |
There was a problem hiding this comment.
This remains valid on the current head: seen[mapping.Host] still compares exact strings, while the external path uses HostsOverlap. A single object can therefore admit *.example.com and app.example.com with different certificate or client config. Please make the intra-resource check overlap-aware and add coverage for both Ingress TLS blocks and Gateway listeners.
| // Same server cert AND same mTLS client config: no conflict. A | ||
| // differing client config is still a conflict even when the server | ||
| // cert matches. | ||
| newMapping := newMappings[host] | ||
| if mapping.CertificateHash == newMapping.CertificateHash && |
There was a problem hiding this comment.
This also remains on the current head. ClientConfigHash can be the only differing field, but SSLConflict carries no cause and FormatConflicts always reports a “different certificate”. Please report a generic differing TLS configuration, or carry the differing field into the diagnostic.
There was a problem hiding this comment.
The skip-regex hash collision is fixed, but this diagnostic is still unchanged: FormatConflicts says “different certificate” when only ClientConfigHash differs. Please make the warning generic or report which TLS component differs.
TranslateApisixTls copied spec.hosts verbatim into the SSL object's SNIs and only checked that the referenced Secret existed and yielded a keypair. A certificate whose SANs don't cover a declared host (e.g. an internal cert bound to a public hostname) was programmed with no signal, so external clients would receive a certificate invalid for the requested host. APISIX serves whatever certificate is configured for an SNI regardless of its SANs (the client validates), so this is advisory rather than fatal: log a warning listing the uncovered hosts and the certificate SANs, using the wildcard-aware sslutil.HostCoveredBy (a wildcard SAN covers single-label subdomains, an exact SAN covers only itself). No warning when the cert declares no DNS SANs or can't be parsed.
3f2c0a2 to
e3a5e6a
Compare
| // concrete hostname. It understands single-label wildcards ("*.example.com" | ||
| // matches "app.example.com" but not "a.b.example.com"). Two distinct wildcards | ||
| // never share a concrete host. | ||
| func HostsOverlap(a, b string) bool { |
There was a problem hiding this comment.
An exact host and a covering wildcard are not ambiguous, so I do not think this should be treated as a conflict.
Gateway API prescribes the opposite handling. Listener.TLS: "The GatewayClass MUST use the longest matching SNI out of all available certificates for any TLS handshake." And v1.3 added the OverlappingTLSConfig listener condition, whose own example is foo.example.com vs *.example.com: controllers MUST detect that overlap and set a condition on both listeners — the resources stay admitted and both keep working.
APISIX already behaves that way: a reversed exact SNI is a fully static path, so match_route finds it in hash_path before it ever walks the tree holding the wildcard prefixes. app.example.com gets its own certificate and other.example.com gets the wildcard one, every time.
The cost of calling it a conflict is concrete: "wildcard cert plus a dedicated cert for one subdomain" stops being expressible, and since ValidateUpdate re-runs the full detection over the whole object, existing Ingresses in that shape can no longer be edited at all after an upgrade — an unrelated annotation change gets rejected, and the only way out is deleting the other resource.
The case worth keeping is two objects carrying the same SNI: those land in one hash_path[sni_rev] array and sort_route ties on both priority and path_org length, so the winner is insertion order. Exact-key matching already covers it, which is why I would leave that part as it is and drop the overlap handling.
| } | ||
| // Wildcard-aware: an exact host and a covering wildcard overlap even | ||
| // though their index keys differ ("app.example.com" vs "*.example.com"). | ||
| if sslutil.HostsOverlap(mapping.Host, host) { |
There was a problem hiding this comment.
First overlap wins here, so the outcome depends on mapping order: an exact host using the very same certificate gets reported as a conflict.
Repro — existing Ingress with tls[0]={hosts:[*.example.com], secret:cert-a}, tls[1]={hosts:[app.example.com], secret:cert-b}, then an ApisixTls for app.example.com with cert-b. The wildcard entry is hit first, cert-a != cert-b, conflict.
If overlap matching stays, match an exact mapping.Host == host first and only fall back to overlap.
| // listAllTLSResources enumerates every TLS-bearing Gateway, Ingress and | ||
| // ApisixTls. Used when the incoming host is a wildcard, whose covered exact | ||
| // hosts can't be resolved through the exact-key host index. | ||
| func (d *ConflictDetector) listAllTLSResources(ctx context.Context) ([]client.Object, error) { |
There was a problem hiding this comment.
This lists every Gateway, Ingress and ApisixTls in the cluster and then runs resolveGatewayProxy (→ FindMatchingIngressClass) per candidate, on every admission whose host starts with *.. Wildcard hosts are the common case for TLS, so this is the common path, and it runs inside the admission timeout.
If overlap detection is kept, index for it instead: have ssl_host.go emit ParentWildcard(host) as an extra index key for exact hosts, and a *.example.com lookup resolves through the index. This function and the extra exact-side List then both go away.
|
|
||
| // First, check for conflicts within the new resource itself. | ||
| seen := make(map[string]string, len(newMappings)) | ||
| seen := make(map[string]HostCertMapping, len(newMappings)) |
There was a problem hiding this comment.
The self-conflict check still compares exact mapping.Host, so it misses what the external path now catches. A single Ingress with tls[0]={hosts:[*.example.com], secret:a} and tls[1]={hosts:[app.example.com], secret:b} passes. Whichever matching rule you settle on, both loops should use it.
|
|
||
| // APISIX serves the cert regardless of SAN, so this is advisory: warn when a | ||
| // declared host isn't covered by the cert SANs (clients may reject it). | ||
| if uncovered, sans := uncoveredSNIHosts(cert, tls.Spec.Hosts); len(uncovered) > 0 { |
There was a problem hiding this comment.
Move this to the ApisixTls webhook as an admission.Warnings. APISIX never looks at cert SANs when selecting an SSL object, so this is purely advice for the user, and a log line in the translator is not something they will see — ValidateCreate/ValidateUpdate already return warnings that show up on kubectl apply. It also avoids re-parsing the certificate on every reconcile.
The PR description says translation is rejected when a host is not covered; the code only logs. Worth updating.
There was a problem hiding this comment.
We are after the same thing: no host should quietly get served a certificate the operator did not intend. My concern is that admission rejection cannot deliver that, and that it costs existing users something real.
Rejecting at admission does not make the data plane deterministic. When two SSL objects carry the same SNI they land in one hash_path[sni_rev] array, and sort_route ties on both priority and path_org length, so the served certificate is decided by insertion order. Admission blocks one way in; it does not change what happens once a conflict exists.
Conflicts arrive without an admission event. A certificate rotated inside the Secret makes two previously-agreeing resources diverge. A GatewayProxy or IngressClass change moves a resource into a different group. The webhook can be disabled, and anything created before it was installed never passed through it. ValidateCreate / ValidateUpdate see none of these.
The gate has a cost on existing clusters. ValidateUpdate runs the full detection over the whole object, not just the changed fields. After an upgrade, an Ingress that has always coexisted with a wildcard resource can no longer be edited at all — an unrelated annotation change gets rejected — and the only way out is deleting the other resource. That breaks configuration that was never wrong.
Gateway API defines this case and prescribes the opposite response. Listener.TLS: "The GatewayClass MUST use the longest matching SNI out of all available certificates for any TLS handshake." v1.3 then added the OverlappingTLSConfig listener condition — "Controllers MUST detect the presence of overlapping hostnames... This condition MUST be set on all Listeners with overlapping TLS config" — and the spec's own example is foo.example.com vs *.example.com. Tie-breaking is specified too: oldest creationTimestamp, then first alphabetically by namespace/name. We do not set that condition anywhere today.
| this PR | report + resolve | |
|---|---|---|
*.example.com and app.example.com |
rejected at admission | admitted; longest SNI wins, as the spec requires |
| same SNI on two objects | rejected when admission can see it | admitted; oldest creationTimestamp wins |
| conflict from cert rotation / proxy change / pre-existing object | invisible | found on the next reconcile |
| how the operator learns about it | one error at apply time | condition on both objects, plus a webhook warning at apply time |
| what the data plane serves | unchanged, insertion order | one certificate per SNI, by a stated rule |
The last row is the one worth weighing: this PR adds a gate and leaves what the data plane serves exactly as it was.
Can we agree on the direction before iterating further on the implementation?
The SSL conflict detector rejected overlapping SSL objects at admission. That gate only sees object create/update, so it misses conflicts introduced by a certificate rotating inside its Secret, a GatewayProxy or IngressClass regrouping, a disabled webhook, or objects that predate the webhook. It also blocks edits to configurations that already coexist. APISIX admits overlapping SSL objects and resolves the served certificate by SNI specificity, so a conflict is now surfaced as an admission warning on create and update.
shreemaan-abhishek
changed the title
|
Agreed on the direction — thanks for the detailed writeup. You're right on the two points that matter: admission rejection is incomplete (it can't see a conflict introduced by a certificate rotating inside its Secret, a GatewayProxy/IngressClass regrouping, a disabled webhook, or an object created before the webhook existed), and it's a backward-compatibility break (an unrelated edit to an object that already coexists with an overlapping one gets rejected). And overlapping SNIs are what Gateway API says to admit-and-report, not deny. I've pushed a change so the detector now emits an admission warning instead of denying, on both The fuller treatment you describe — detect on reconcile, set an One clarification for context: reject-at-admission predates this PR. This change extended what the detector notices (wildcard overlap + mTLS config) and now moves the response from deny to warn. |
The ssl_conflict webhook suite asserted overlapping SSL config was rejected at admission. Now that the detector reports overlap as an admission warning, each case expects the resource to be admitted and the kubectl output to contain the conflict warning instead.
Admission warnings are delivered as HTTP Warning headers, and Kubernetes drops a warning that contains newlines. FormatConflicts built a multi-line message, so the conflict warning never reached the client. Join the per-conflict details on one line instead.
| // Overlapping SSL config is advisory: APISIX serves overlapping objects and | ||
| // resolves by SNI, so warn instead of denying. | ||
| skipADCValidation := v.initErr != nil || len(warnings) > 0 | ||
| if conflicts := detector.DetectConflicts(ctx, tls); len(conflicts) > 0 { |
There was a problem hiding this comment.
[P1] Apply the warning behavior to ValidateUpdate too. This hunk changes only create; the update path still returns a hard error for the same conflict. An existing ApisixTls in an overlapping setup therefore still cannot be edited, which is one of the cases this PR says the warning conversion fixes. Please mirror this flow in ValidateUpdate and add an update regression test.
Description
Improves how the controller detects and reports inconsistent TLS host/certificate configuration. The SSL conflict detector now warns on overlap instead of denying it at admission (see the note below), and its detection is made wildcard- and mTLS-aware so the warnings are accurate.
1. Wildcard vs. exact host matching
Hosts were compared by exact string equality, so a covering wildcard and an exact host were never compared against each other (
*.example.comandapp.example.comare indexed under different keys). APISIX resolves an exact SNI ahead of a covering wildcard, so two objects whose hosts overlap only through a wildcard collide at the data plane.sslutil.HostsOverlap/ParentWildcard(single-label wildcard semantics)HostsOverlapinstead of string equality2. mTLS client config in the conflict key
The conflict key used only the server certificate hash, so two objects for the same host and server certificate but different mTLS client config (
spec.client) were not flagged as overlapping.ClientConfigHashtoHostCertMapping(CA secret reference, depth,skip_mtls_uri_regex; empty when no mTLS)3. Overlap is reported as a warning, not a denial
The detector previously rejected overlapping SSL objects at admission. That gate only sees object create/update, so it misses conflicts introduced by a certificate rotating inside its Secret, a
GatewayProxy/IngressClassregrouping, a disabled webhook, or objects that predate the webhook; and it blocks edits to configurations that already coexist. APISIX admits overlapping SSL objects and resolves by SNI specificity, so overlap is now surfaced as an admission warning on create and update.A reconcile-time treatment (detect on reconcile + an
OverlappingTLSConfig-style status condition with a documented tie-break) is planned as a follow-up rather than expanding this PR.4. Certificate SAN coverage warning (translator)
TranslateApisixTlsprogrammed the SSL object without any signal when the certificate SANs don't cover a declared host. APISIX serves the cert regardless (the client validates), so this logs a warning listing the uncovered hosts, using the wildcard-awaresslutil.HostCoveredBy.Tests
HostsOverlap/ParentWildcard/HostCoveredByChecklist