Add wildcard certificate fallback for expired/unknown exact certs (#216)

Author: aa1ex

internal/grpcapi/utils.go

@@ -5,7 +5,6 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
-	"strings"
 	"time"
 
 	"connectrpc.com/connect"
@@ -15,7 +14,6 @@ import (
 	"github.com/kaasops/envoy-xds-controller/pkg/api/grpc/util/v1/utilv1connect"
 	virtual_service_templatev1 "github.com/kaasops/envoy-xds-controller/pkg/api/grpc/virtual_service_template/v1"
 	"google.golang.org/protobuf/types/known/timestamppb"
-	corev1 "k8s.io/api/core/v1"
 )
 
 type UtilsService struct {
@@ -28,44 +26,38 @@ func NewUtilsService(s store.Store) *UtilsService {
 }
 
 // VerifyDomains checks if valid TLS certificates exist for the given domains.
-// Note: This intentionally uses MapDomainSecrets() without namespace preference
-// because verification checks domain-level certificate availability, not
-// VirtualService-specific secret binding. For VirtualService-specific binding,
-// see MapDomainSecretsForNamespace() which considers namespace affinity.
+// Uses the same wildcard fallback logic as the secrets builder: if exact cert is
+// expired/unparseable, falls back to a valid wildcard certificate.
+//
+// Wildcard matching: Only immediate parent wildcard is checked.
+// For example, "api.example.com" will match "*.example.com" but
+// "sub.api.example.com" will only match "*.api.example.com" (not "*.example.com").
 func (s *UtilsService) VerifyDomains(_ context.Context, req *connect.Request[v1.VerifyDomainsRequest]) (*connect.Response[v1.VerifyDomainsResponse], error) {
 	results := make([]*v1.DomainVerificationResult, 0, len(req.Msg.Domains))
 	for _, domain := range req.Msg.Domains {
-		res := verifyDomainFromSecrets(domain, s.store.MapDomainSecrets())
+		res := s.verifyDomainWithFallback(domain)
 		results = append(results, res)
 	}
 	return connect.NewResponse(&v1.VerifyDomainsResponse{Results: results}), nil
 }
 
-func verifyDomainFromSecrets(domain string, secrets map[string]*corev1.Secret) *v1.DomainVerificationResult {
+// verifyDomainWithFallback checks certificate for a domain using the same
+// wildcard fallback logic as the secrets builder
+func (s *UtilsService) verifyDomainWithFallback(domain string) *v1.DomainVerificationResult {
 	result := &v1.DomainVerificationResult{Domain: domain}
-	var matchedSecret *corev1.Secret
-	var matchedByWildcard bool
-
-	if secret, ok := secrets[domain]; ok {
-		matchedSecret = secret
-	} else {
-		parts := strings.Split(domain, ".")
-		for i := 1; i < len(parts)-1; i++ {
-			wildcard := "*." + strings.Join(parts[i:], ".")
-			if secret, ok := secrets[wildcard]; ok {
-				matchedSecret = secret
-				matchedByWildcard = true
-				break
-			}
-		}
-	}
 
-	if matchedSecret == nil {
+	// Use store's wildcard fallback logic - empty namespace for domain-level check
+	lookupResult := s.store.GetDomainSecretWithWildcardFallbackInfo(domain, "")
+
+	if lookupResult.Secret == nil {
 		result.Error = "no matching certificate found"
 		return result
 	}
 
-	crtBytes, ok := matchedSecret.Data["tls.crt"]
+	result.MatchedByWildcard = lookupResult.UsedWildcard
+
+	// Parse and validate the matched certificate
+	crtBytes, ok := lookupResult.Secret.Data["tls.crt"]
 	if !ok {
 		result.Error = "missing tls.crt in secret"
 		return result
@@ -85,7 +77,10 @@ func verifyDomainFromSecrets(domain string, secrets map[string]*corev1.Secret) *
 
 	result.Issuer = cert.Issuer.CommonName
 	result.ExpiresAt = timestamppb.New(cert.NotAfter)
-	result.MatchedByWildcard = matchedByWildcard
+
+	// Note: lookupResult also contains FallbackReason and ExactValidity
+	// for debugging purposes, but the proto doesn't support these fields yet.
+	// When the proto is updated, this info can be exposed in the API response.
 
 	if time.Now().After(cert.NotAfter) {
 		result.Error = "certificate expired"

internal/store/domain_secrets.go

@@ -20,13 +20,67 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"sort"
+	"strings"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/kaasops/envoy-xds-controller/internal/helpers"
 )
 
+// ValidateDomainPattern checks if a domain pattern is valid.
+// Returns an error message if invalid, empty string if valid.
+//
+// Valid patterns:
+//   - example.com (exact domain)
+//   - *.example.com (wildcard - asterisk followed by dot)
+//
+// Invalid patterns:
+//   - *example.com (asterisk without dot)
+//   - **.example.com (double asterisk)
+//   - example.*.com (asterisk not at start)
+//   - * (standalone asterisk)
+func ValidateDomainPattern(domain string) string {
+	if domain == "" {
+		return "empty domain"
+	}
+
+	// Check for asterisk in the domain
+	asteriskIdx := strings.Index(domain, "*")
+	if asteriskIdx == -1 {
+		// No wildcard, valid exact domain
+		return ""
+	}
+
+	// Wildcard validation
+	if asteriskIdx != 0 {
+		return "wildcard (*) must be at the start of domain"
+	}
+
+	if len(domain) < 3 {
+		// Need at least "*.x"
+		return "wildcard domain too short"
+	}
+
+	if domain[1] != '.' {
+		return "wildcard must be followed by dot (e.g., *.example.com, not *example.com)"
+	}
+
+	// Check for multiple asterisks
+	if strings.Count(domain, "*") > 1 {
+		return "multiple wildcards not allowed"
+	}
+
+	// Check that there's something after "*."
+	rest := domain[2:]
+	if rest == "" || rest == "." {
+		return "wildcard domain must have at least one label after *."
+	}
+
+	return ""
+}
+
 // SecretDomainEntry holds information about a secret for domain lookup
 type SecretDomainEntry struct {
 	NamespacedName helpers.NamespacedName
@@ -68,108 +122,136 @@ func (idx DomainSecretsIndex) Remove(domain string, nn helpers.NamespacedName) {
 type validityPriority int
 
 const (
-	validityExpired validityPriority = iota // Known expired certificate - lowest priority
-	validityUnknown                         // Could not parse certificate - medium priority (better than nothing)
-	validityValid                           // Known valid (non-expired) certificate - highest priority
+	validityNotFound validityPriority = iota // Domain not found in index - used only for "not found" returns
+	validityExpired                          // Known expired certificate - lowest priority for actual certs
+	validityUnknown                          // Could not parse certificate - medium priority (better than nothing)
+	validityValid                            // Known valid (non-expired) certificate - highest priority
 )
 
-// GetBestSecret returns the best secret for a domain with preference for the given namespace.
-// Selection logic:
-// 1. If only one secret exists - return it (if it exists in secrets map)
-// 2. Priority by validity: valid > unknown > expired
-// 3. Among same validity: prefer same namespace
-// 4. Final tie-breaker: alphabetically by namespace/name
-//
-// Note: Secrets with unparseable certificates (validityUnknown) are ranked below valid
-// certificates but above expired ones. This ensures we prefer known-good certificates
-// while still providing fallback behavior when certificate parsing fails.
-func (idx DomainSecretsIndex) GetBestSecret(
+// SecretLookupResult contains the result of a secret lookup with additional metadata.
+// This provides complete diagnostic information about the secret selection process.
+type SecretLookupResult struct {
+	Secret             *corev1.Secret
+	UsedWildcard       bool   // true if wildcard secret was used instead of exact
+	FallbackReason     string // reason for fallback: "expired", "unknown", or empty if no fallback
+	ExactSecretName    string // name of the exact secret if it existed (format: "namespace/name")
+	ExactValidity      string // validity of exact secret: "valid", "expired", "unknown", "not_found"
+	WildcardSecretName string // name of the wildcard secret if it was considered (format: "namespace/name")
+	WildcardValidity   string // validity of wildcard secret: "valid", "expired", "unknown", "not_found"
+}
+
+// GetBestSecretWithValidity returns the best secret for a domain along with its validity status.
+// This is an optimized version that does a single traversal instead of calling
+// GetBestSecret and GetSecretValidity separately.
+// Returns (nil, validityNotFound) if domain not found in index.
+func (idx DomainSecretsIndex) GetBestSecretWithValidity(
 	domain string,
 	preferredNamespace string,
 	secrets map[helpers.NamespacedName]*corev1.Secret,
-) *corev1.Secret {
+) (*corev1.Secret, validityPriority) {
 	entries, exists := idx[domain]
 	if !exists || len(entries) == 0 {
-		return nil
+		return nil, validityNotFound
 	}
 
+	// Capture time once for consistent validity checks throughout this call
+	now := time.Now()
+
 	// Fast path: single secret
+	// Note: using range to get the single element from a map is a Go idiom
 	if len(entries) == 1 {
-		for nn := range entries {
-			// Defensive nil check for consistency between index and secrets map
-			if secret := secrets[nn]; secret != nil {
-				return secret
+		for nn, entry := range entries {
+			secret := secrets[nn]
+			if secret == nil {
+				return nil, validityNotFound // indexed but missing from secrets map
 			}
-			return nil
+			validity := getValidityFromEntry(entry, now)
+			return secret, validity
 		}
 	}
 
-	now := time.Now()
-
-	// Collect entries into a slice for sorting
+	// Collect and sort candidates
 	type candidateEntry struct {
 		nn       helpers.NamespacedName
-		notAfter time.Time
 		validity validityPriority
 		isSameNs bool
 	}
 
 	candidates := make([]candidateEntry, 0, len(entries))
 	for nn, entry := range entries {
-		// Skip entries that don't exist in the secrets map (defensive check)
 		if secrets[nn] == nil {
 			continue
 		}
 
 		var validity validityPriority
 		switch {
 		case entry.NotAfter.IsZero():
-			// Certificate parsing failed - treat as unknown validity
 			validity = validityUnknown
 		case entry.NotAfter.After(now):
-			// Certificate is valid (not expired)
 			validity = validityValid
 		default:
-			// Certificate is expired
 			validity = validityExpired
 		}
 
 		candidates = append(candidates, candidateEntry{
 			nn:       nn,
-			notAfter: entry.NotAfter,
 			validity: validity,
 			isSameNs: nn.Namespace == preferredNamespace,
 		})
 	}
 
-	// No valid candidates found
 	if len(candidates) == 0 {
-		return nil
+		return nil, validityNotFound // all indexed secrets missing from secrets map
 	}
 
-	// Sort candidates by priority:
-	// 1. Higher validity priority first (valid > unknown > expired)
-	// 2. Same namespace first
-	// 3. Alphabetically by namespace/name
+	// Sort: validity desc, same namespace first, then alphabetically
 	sort.Slice(candidates, func(i, j int) bool {
-		// Higher validity priority first
 		if candidates[i].validity != candidates[j].validity {
 			return candidates[i].validity > candidates[j].validity
 		}
-		// Same namespace first
 		if candidates[i].isSameNs != candidates[j].isSameNs {
 			return candidates[i].isSameNs
 		}
-		// Alphabetically by namespace
 		if candidates[i].nn.Namespace != candidates[j].nn.Namespace {
 			return candidates[i].nn.Namespace < candidates[j].nn.Namespace
 		}
-		// Alphabetically by name
 		return candidates[i].nn.Name < candidates[j].nn.Name
 	})
 
-	// Return the best candidate
-	return secrets[candidates[0].nn]
+	best := candidates[0]
+	return secrets[best.nn], best.validity
+}
+
+// getValidityFromEntry determines validity from a single entry.
+// The now parameter ensures consistent time comparison across all validity checks.
+func getValidityFromEntry(entry SecretDomainEntry, now time.Time) validityPriority {
+	switch {
+	case entry.NotAfter.IsZero():
+		return validityUnknown
+	case entry.NotAfter.After(now):
+		return validityValid
+	default:
+		return validityExpired
+	}
+}
+
+// GetBestSecret returns the best secret for a domain with preference for the given namespace.
+// Selection logic:
+// 1. If only one secret exists - return it (if it exists in secrets map)
+// 2. Priority by validity: valid > unknown > expired
+// 3. Among same validity: prefer same namespace
+// 4. Final tie-breaker: alphabetically by namespace/name
+//
+// Note: Secrets with unparseable certificates (validityUnknown) are ranked below valid
+// certificates but above expired ones. This ensures we prefer known-good certificates
+// while still providing fallback behavior when certificate parsing fails.
+func (idx DomainSecretsIndex) GetBestSecret(
+	domain string,
+	preferredNamespace string,
+	secrets map[helpers.NamespacedName]*corev1.Secret,
+) *corev1.Secret {
+	secret, _ := idx.GetBestSecretWithValidity(domain, preferredNamespace, secrets)
+	return secret
 }
 
 // GetAnySecret returns any valid secret for a domain (for backward compatibility)
@@ -186,18 +268,27 @@ func (idx DomainSecretsIndex) GetAnySecret(
 // expiration time to ensure we consider the most restrictive validity period.
 // This handles cases where the end-entity certificate expires before intermediate/root CAs.
 // Returns zero time if no valid certificates could be parsed.
+// Logs warnings for parsing errors to aid debugging in production.
 func ParseCertificateNotAfter(secret *corev1.Secret) time.Time {
+	logger := log.Log.WithName("certificate-parser")
+
 	if secret == nil {
 		return time.Time{}
 	}
 
+	secretKey := secret.Namespace + "/" + secret.Name
+
 	certData, ok := secret.Data[corev1.TLSCertKey]
 	if !ok || len(certData) == 0 {
+		logger.V(1).Info("Secret missing tls.crt data",
+			"secret", secretKey)
 		return time.Time{}
 	}
 
 	var minNotAfter time.Time
 	rest := certData
+	blockIndex := 0
+	parseErrors := 0
 
 	// Parse all PEM blocks in the certificate chain
 	for {
@@ -206,6 +297,7 @@ func ParseCertificateNotAfter(secret *corev1.Secret) time.Time {
 			break
 		}
 		rest = remaining
+		blockIndex++
 
 		// Skip non-certificate blocks (e.g., private keys that might be included)
 		if block.Type != "CERTIFICATE" {
@@ -214,6 +306,11 @@ func ParseCertificateNotAfter(secret *corev1.Secret) time.Time {
 
 		cert, err := x509.ParseCertificate(block.Bytes)
 		if err != nil {
+			parseErrors++
+			logger.V(1).Info("Failed to parse certificate in chain",
+				"secret", secretKey,
+				"blockIndex", blockIndex,
+				"error", err.Error())
 			continue
 		}
 
@@ -223,5 +320,13 @@ func ParseCertificateNotAfter(secret *corev1.Secret) time.Time {
 		}
 	}
 
+	// Log warning if all certificates failed to parse
+	if minNotAfter.IsZero() && blockIndex > 0 {
+		logger.Info("Failed to parse any certificates from secret",
+			"secret", secretKey,
+			"totalBlocks", blockIndex,
+			"parseErrors", parseErrors)
+	}
+
 	return minNotAfter
 }