AlinsRan commented on code in PR #2810:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2810#discussion_r3662348827
##########
internal/ssl/util.go:
##########
@@ -180,6 +180,77 @@ func NormalizeHosts(hosts []string) []string {
return normalized
}
+// HostsOverlap reports whether two SNI host patterns can both match a common
+// 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 {
Review Comment:
An exact host and a covering wildcard are not a conflict — please drop this
part.
APISIX reverses each SNI and uses it as a radixtree path. An exact SNI is a
fully static path, so it goes into `hash_path`, and `match_route` checks
`hash_path` before walking the tree that holds the wildcard prefixes.
`app.example.com` deterministically gets its own SSL object and
`other.example.com` gets the wildcard one. "Wildcard cert plus a dedicated cert
for one subdomain" is a standard setup, and this change rejects it at admission.
The nondeterminism this detector exists to catch is two SSL objects carrying
the *same* SNI: they land in the same `hash_path[path]` array and `sort_route`
ties on both priority and path length, so the winner is insertion order.
Exact-key matching already covers that.
##########
internal/webhook/v1/ssl/conflict_detector.go:
##########
@@ -478,7 +572,12 @@ func (d *ConflictDetector) mappingForHostWithCache(ctx
context.Context, obj clie
}
for _, mapping := range mappings {
- if mapping.Host == host {
+ if mapping.Host == "" {
+ continue
+ }
+ // 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) {
Review 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.
##########
internal/webhook/v1/ssl/conflict_detector.go:
##########
@@ -410,6 +471,39 @@ func (d *ConflictDetector) findExternalConflicts(ctx
context.Context, obj client
return results, nil
}
+// 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) {
Review 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.
##########
internal/webhook/v1/ssl/conflict_detector.go:
##########
@@ -251,17 +258,37 @@ func (d *ConflictDetector) BuildApisixTlsMappings(ctx
context.Context, tls *apiv
// if len(hosts) == 0 {
// hosts = info.hosts
// }
+ clientHash := clientConfigHash(tls.Spec.Client)
for _, host := range hosts {
mappings = append(mappings, HostCertMapping{
- Host: host,
- CertificateHash: info.hash,
- ResourceRef: fmt.Sprintf("%s/%s/%s",
internaltypes.KindApisixTls, tls.Namespace, tls.Name),
+ Host: host,
+ CertificateHash: info.hash,
+ ClientConfigHash: clientHash,
+ ResourceRef: fmt.Sprintf("%s/%s/%s",
internaltypes.KindApisixTls, tls.Namespace, tls.Name),
})
}
return mappings
}
+// clientConfigHash digests an ApisixTls mTLS client-verification config into a
+// stable key. Returns "" when no mTLS is configured, so a resource that
enforces
+// mTLS and one that doesn't produce different keys for the same host+cert. It
+// keys on the CA secret reference (namespace/name), which uniquely identifies
+// the trust anchor, plus depth and the skip regexes.
+func clientConfigHash(client *apiv2.ApisixMutualTlsClientConfig) string {
Review Comment:
Gateway listeners are missing here. `translateFrontendValidation` writes the
same `ssl.client` field from `listener.tls.frontendValidation`, but
`BuildGatewayMappings` leaves `ClientConfigHash` empty. Two Gateways on the
same host and cert with different client CAs still pass, and any Gateway
compared against an ApisixTls looks like "no mTLS".
Related: `FormatConflicts` always reports "is already configured with a
different certificate", which is misleading when the certificates are identical
and only the mTLS config differs.
##########
internal/webhook/v1/ssl/conflict_detector.go:
##########
@@ -92,22 +98,23 @@ func (d *ConflictDetector) DetectConflicts(ctx
context.Context, obj client.Objec
conflicts := make([]SSLConflict, 0)
// First, check for conflicts within the new resource itself.
- seen := make(map[string]string, len(newMappings))
+ seen := make(map[string]HostCertMapping, len(newMappings))
Review 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.
##########
internal/adc/translator/apisixtls.go:
##########
@@ -50,6 +50,14 @@ func (t *Translator) TranslateApisixTls(tctx
*provider.TranslateContext, tls *ap
return nil, err
}
+ // 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 {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
