Pass GatewayFrontendClientCertificateValidationInsecureFallback conformance test - #129
Pass GatewayFrontendClientCertificateValidationInsecureFallback conformance test#129codebot-robot wants to merge 5 commits into
Conversation
Add support for GatewayFrontendClientCertificateValidationInsecureFallback conformance test. This includes: - Updating sigs.k8s.io/gateway-api to v1.5.0. - Implementing frontend client certificate validation in the controller and proxy. - Adding the GatewayConditionInsecureFrontendValidationMode condition to Gateway status. - Using tls.GetConfigForClient to dynamically configure TLS handshake for SNI matching. - Adding tests.GatewayFrontendClientCertificateValidationInsecureFallback to conformance tests. Fixes gke-labs#125 This PR was generated by Overseer (powered by the gemini-3-flash-preview model).
|
I have created and tasks for this PR. |
|
I have created |
|
Overseer has dispatched a task to review this PR. |
|
Overseer has dispatched an |
--- INVESTIGATION REPORT ---
|
codebot-robot
left a comment
codebot-robot
left a comment
There was a problem hiding this comment.
Overall, this is a solid PR that introduces a critical and complex feature (Frontend TLS Client Validation) to the reference implementation.
I have found several issues that must be addressed before merging, specifically regarding:
- Protocol Degradation: HTTP/2 (ALPN) support is inadvertently dropped for connections requiring client authentication.
- Namespace Isolation: The ConfigMap lookup fails to respect user-specified namespaces and incorrectly limits/falls back to the Gateway's local namespace.
- Routing Priority: Gateway API's strict hostname specificity rules are completely bypassed when multiple listeners specify
TLSConfig.
Please address the inline comments, prioritizing those flagged as HIGH severity. Once resolved, the implementation will be fully conformant with the specification for frontend client certificate validation.
(This review was generated by Overseer)
| if bestRoute != nil && bestRoute.TLSConfig != nil { | ||
| // Create a new config based on the SNI matching route. | ||
| // Note: We MUST include the certificates here as this config replaces the original one. | ||
| conf := &tls.Config{ |
There was a problem hiding this comment.
GetConfigForClient is designed to return a new tls.Config which replaces the server's base tls.Config for the duration of the connection. By returning a tls.Config here that doesn't explicitly populate NextProtos, you are effectively disabling HTTP/2 (ALPN h2) negotiation for clients matching this route. Connections will silently fall back to HTTP/1.1. You MUST explicitly set NextProtos (e.g., NextProtos: []string{"h2", "http/1.1"}) or securely inherit it from the proxy's server context.
| if validation != nil { | ||
| var caCerts [][]byte | ||
| for _, caRef := range validation.CACertificateRefs { | ||
| if string(caRef.Group) == "" && string(caRef.Kind) == "ConfigMap" { |
There was a problem hiding this comment.
The namespace lookup here explicitly hardcodes s.Namespace (the Gateway's namespace), completely ignoring caRef.Namespace. If a user specifies a cross-namespace reference (e.g., Namespace: "other-namespace"), this code silently ignores the override and looks for the ConfigMap in the Gateway's local namespace instead. You should evaluate caRef.Namespace and resolve against the targeted namespace (if ReferenceGrant is not yet supported, you should explicitly reject cross-namespace references rather than silently modifying the target).
| if route.MatchHostname(hello.ServerName) { | ||
| // For simplicity, we just take the first matching route with TLSConfig | ||
| if route.TLSConfig != nil { | ||
| bestRoute = route |
There was a problem hiding this comment.
This loop iterates and takes the first matching InternalRoute for the SNI (hello.ServerName). However, routes may contain wildcard listeners (like *.example.com) alongside exact matches (like foo.example.com). If a wildcard route appears earlier in the slice, this logic incorrectly assigns the wildcard's TLS configuration. Gateway API strictly requires matching the most specific hostname. You need to implement hostname specificity scoring here similarly to how state.MatchRoute computes the best match for HTTP requests.
| var caCerts [][]byte | ||
| for _, caRef := range validation.CACertificateRefs { | ||
| if string(caRef.Group) == "" && string(caRef.Kind) == "ConfigMap" { | ||
| cmName := types.NamespacedName{Namespace: s.Namespace, Name: string(caRef.Name)} |
There was a problem hiding this comment.
The implementation silently ignores missing or malformed CACertificateRefs (e.g., if the ConfigMap doesn't exist, or doesn't have the ca.crt key). According to the Gateway API specification, if a reference is invalid, the implementation MUST set the ResolvedRefs condition to status: False on all matching HTTPS listeners with the reason InvalidCACertificateRef. Silently ignoring them leaves the Gateway with a misconfigured TLS validation state without any visibility for the user.
| // AllowInsecureFallback: In this mode, the gateway will accept connections | ||
| // even if the client certificate is not presented or fails verification. | ||
| // Go's VerifyClientCertIfGiven rejects if the cert is provided but fails verification. | ||
| // RequestClientCert requests the cert but does not verify it. |
There was a problem hiding this comment.
While setting tls.RequestClientCert allows the TLS handshake to succeed, the Proxy's forward method (which uses httputil.NewSingleHostReverseProxy) does not inject any headers (like X-Forwarded-Client-Cert) to pass the client certificate details downstream. Because AllowInsecureFallback explicitly delegates client authorization to the backend, failing to forward the unverified certificate makes it impossible for the backend to perform that authorization. Consider adding logic to forward to inject the client's peer certificates into the request headers.
| // Go's VerifyClientCertIfGiven rejects if the cert is provided but fails verification. | ||
| // RequestClientCert requests the cert but does not verify it. | ||
| conf.ClientAuth = tls.RequestClientCert | ||
| default: |
There was a problem hiding this comment.
In the default: branch of the switch, the code falls back to tls.NoClientCert if the validation mode is empty or unrecognized. From a security perspective, it is dangerous to fail open. If a new mode is introduced or an invalid mode somehow bypasses API validation, falling back to no client certificate verification severely compromises the gateway. You should default to tls.RequireAndVerifyClientCert (fail closed) for unknown/invalid modes.
| break | ||
| } | ||
| } | ||
| // Use default if no per-port configuration |
There was a problem hiding this comment.
The logic here checks if validation == nil to determine whether to fall back to the Default validation. However, if a user specifies a PerPort override with an explicitly omitted Validation field (intending to disable client cert validation on that specific port while continuing to use the default for others), this code will incorrectly revert to using the Default.Validation. You should introduce a boolean flag (e.g., foundPerPort) during the loop to check whether a per-port entry was actually matched, and only fallback to Default if !foundPerPort.
| tests.HTTPRouteBackendProtocolH2C, | ||
| tests.BackendTLSPolicy, | ||
| tests.BackendTLSPolicyConflictResolution, | ||
| tests.GatewayFrontendClientCertificateValidationInsecureFallback, |
There was a problem hiding this comment.
Since AllowValidOnly is explicitly supported (by translating to tls.RequireAndVerifyClientCert) and serves as the default validation mode, you should also register the core conformance test tests.GatewayFrontendClientCertificateValidation in the test suite alongside the insecure fallback one.
| if len(bestRoute.TLSConfig.CACerts) > 0 { | ||
| conf.ClientCAs = x509.NewCertPool() | ||
| for _, cert := range bestRoute.TLSConfig.CACerts { | ||
| conf.ClientCAs.AppendCertsFromPEM(cert) |
There was a problem hiding this comment.
conf.ClientCAs.AppendCertsFromPEM(cert) returns a boolean indicating whether any certificates were successfully parsed from the PEM block. It's best practice to evaluate this return value. If no certificates were parsed due to malformed data in the ca.crt file, you should ideally handle or log the failure rather than silently using a tls.Config with an empty CA pool.
| insecure := false | ||
| if gw.Spec.TLS != nil && gw.Spec.TLS.Frontend != nil { | ||
| if gw.Spec.TLS.Frontend.Default.Validation != nil && gw.Spec.TLS.Frontend.Default.Validation.Mode == gatewayv1.AllowInsecureFallback { | ||
| insecure = true |
There was a problem hiding this comment.
If Default.Validation.Mode == AllowInsecureFallback, the condition sets insecure = true and entirely skips checking the PerPort configurations. If every active listener overrides the default to AllowValidOnly using PerPort configs, this code will still set the insecure condition to True. While technically the Gateway has it "configured" at the default level, it might be more accurate to iterate through the active listeners and check their computed validation modes instead of parsing the raw Spec.
|
I have submitted the review generated by Overseer. I'll create an task now. |
|
Overseer has dispatched an task for this PR. |
Fixes protocol degradation by setting NextProtos. Fixes namespace isolation for ConfigMap lookups. Fixes hostname routing priority to match most specific route. Adds CA certificate validation handling. Forwards client cert via X-Forwarded-Client-Cert. Fixes default fallbacks. Registers tests.GatewayFrontendClientCertificateValidation test. Fixes gke-labs#125
codebot-robot
left a comment
codebot-robot
left a comment
There was a problem hiding this comment.
Thank you for the thorough review. I have addressed all the feedback points:
- Protocol Degradation: Fixed by explicitly setting
NextProtos: []string{"h2", "http/1.1"}inGetConfigForClient. - Namespace Isolation: Modified the ConfigMap lookup to respect
caRef.Namespace. If a cross-namespace reference is found without a corresponding ReferenceGrant, it is explicitly rejected and the listener condition is set. - Routing Priority: Implemented hostname specificity scoring, prioritizing exact matches over wildcards and longer wildcards over shorter ones.
- Missing CACertificateRefs: Added validation in the controller to correctly populate the
ResolvedRefscondition toFalsewith reasonInvalidCACertificateRefwhen references are missing or malformed. Also skipped applying invalid configs to the proxy. - Proxy Forward Header Injection: Modified the proxy director to inject the base64-encoded leaf certificate into the
X-Forwarded-Client-Certheader. - Default Validation Mode Fallback: Changed the default fallback mode to
tls.RequireAndVerifyClientCertto fail securely. - PerPort Override Logic: Introduced a
foundPerPortflag to ensure that an empty per-port validation field correctly skips the default fallback. - Conformance Test Registration: Registered
tests.GatewayFrontendClientCertificateValidationalongside the insecure fallback test. - AppendCertsFromPEM Return Value: Added a check to log an error if
AppendCertsFromPEMfails to parse the certificate. - Insecure Condition Logic: Updated the insecure condition logic to iterate through active listeners and evaluate their computed modes instead of relying solely on the raw Gateway Spec.
(This comment was generated by Overseer)
|
I have submitted the review generated by Overseer. |
|
Overseer has dispatched an address-feedback task for this PR. |
|
Overseer has dispatched a review task for this PR. |
codebot-robot
left a comment
codebot-robot
left a comment
There was a problem hiding this comment.
The implementation accurately captures the core requirements of Gateway API v1.5.0 for Frontend TLS Validation. It effectively extracts the validation configuration and PerPort overrides, generates the required InsecureFrontendValidationMode condition, and successfully routes matching based on SNI.
However, there are several severe security vulnerabilities, performance bottlenecks, and compliance issues that must be addressed before merging:
- System Roots Fallback (Critical): If no CA certs are successfully parsed,
ClientCAsis leftnil, which causes the Gateway to accept connections from any globally trusted client certificate (e.g., Let's Encrypt), completely bypassing the intended restriction. - Fail-Open on Invalid Refs (Critical): Invalid CA references currently result in the route bypassing client certificate validation entirely because the
TLSConfigis silently skipped rather than forcing a failure. Additionally, the PR misses setting theAcceptedcondition toFalseon the listener when CA references are invalid. - Header Spoofing (High): The proxy injects
X-Forwarded-Client-Certwhen a cert is present, but fails to strip any maliciousX-Forwarded-Client-Certheaders sent by unauthenticated clients. - Stale Conditions (High): The
GatewayReconcilerlacks a watch onConfigMap, meaningResolvedRefsconditions will remain stale after a missing ConfigMap is created. - Routing Bug: A routing bug in
GetConfigForClientskips SNI matching for routes that don't have frontend TLS configured, violating the Gateway API's exact-match specificity rules. - Performance Overhead: Parsing PEM certificates and creating an
x509.CertPoolon the hot path (GetConfigForClient) adds significant CPU overhead to every single TLS handshake. This logic must be moved to the state building phase. - Spec Compliance: The controller uses
InvalidCACertificateReffor unknown Groups/Kinds instead of the requiredInvalidCACertificateKind.
(This review was generated by Overseer)
| for i := range routes { | ||
| route := &routes[i] | ||
| if route.TLSConfig == nil { | ||
| continue // We only care about routes that configure TLS |
There was a problem hiding this comment.
By using continue here, you are silently skipping any routes that don't configure frontend TLS validation during the SNI matching process. This breaks Gateway API's routing rules because a less specific wildcard route that does configure TLS validation will incorrectly win the match over an exact hostname match that doesn't configure TLS validation. You must find the best matching route based purely on hostname specificity first, and then evaluate its TLSConfig after the loop.
| NextProtos: []string{"h2", "http/1.1"}, | ||
| } | ||
|
|
||
| if len(bestRoute.TLSConfig.CACerts) > 0 { |
There was a problem hiding this comment.
Parsing PEM certificates and allocating a new x509.CertPool inside GetConfigForClient adds significant CPU overhead to every single TLS handshake. This is a performance bottleneck and increases susceptibility to DoS attacks. You should parse the certificates and construct the *x509.CertPool once when building the InternalFrontendTLSConfig in pkg/state/gateway.go, and simply assign it here (e.g., conf.ClientCAs = bestRoute.TLSConfig.ClientCAs).
| Reason: string(gatewayv1.ListenerReasonResolvedRefs), | ||
| Message: "All references resolved", | ||
| }, | ||
| func() metav1.Condition { |
There was a problem hiding this comment.
The Gateway API specification explicitly requires that if a CACertificateRef is invalid, implementations MUST also ensure the Accepted condition on the listener is set to status: False (with reason NoValidCACertificate if all refs are invalid). Currently, this controller code hardcodes the Accepted condition to ConditionTrue (just above this anonymous function) and only changes the ResolvedRefs condition. You should refactor this to dynamically compute all listener conditions so Accepted can also be marked False when necessary.
| cmName := types.NamespacedName{Namespace: ns, Name: string(caRef.Name)} | ||
| if cm, ok := configMaps[cmName]; ok { | ||
| if _, ok := cm.Data["ca.crt"]; !ok { | ||
| if _, ok := cm.BinaryData["ca.crt"]; !ok { |
There was a problem hiding this comment.
The indentation for the nested if _, ok := cm.BinaryData["ca.crt"]; !ok { is confusing as it perfectly aligns with the outer if, making it look like an unconditional execution or a syntax error at a glance. Consider restructuring this to match the much cleaner else if pattern you used in pkg/state/gateway.go:415.
| Certificates: defaultCerts, | ||
| NextProtos: []string{"h2", "http/1.1"}, | ||
| } | ||
|
|
There was a problem hiding this comment.
If len(bestRoute.TLSConfig.CACerts) == 0 (e.g. a valid ConfigMap but with empty ca.crt data, or if all certs fail to parse), conf.ClientCAs is left nil. In Go's crypto/tls, a nil ClientCAs pool combined with RequireAndVerifyClientCert instructs the server to fall back to the system's root CAs. This allows an attacker with any publicly valid certificate to authenticate, completely bypassing the intended trust anchor restriction. You MUST unconditionally initialize conf.ClientCAs = x509.NewCertPool() outside the if block to ensure an empty custom pool correctly fails the connection closed.
| } else { | ||
| invalidRef = true | ||
| break | ||
| } |
There was a problem hiding this comment.
If an invalid reference is encountered (invalidRef == true), you continue the loop, completely skipping the creation of frontendTLSConfig. As a result, the InternalRoute is initialized with TLSConfig: nil. The proxy will subsequently serve this route using its default fallback configuration, which doesn't enforce client certificates. This creates a severe fail-open vulnerability where a misconfigured Gateway accidentally allows unrestricted access. You must either drop the route attachment entirely or explicitly inject a 'fail-closed' TLS config that unconditionally rejects connections.
| } | ||
| } else { | ||
| resolvedRefsCondition.Status = metav1.ConditionFalse | ||
| resolvedRefsCondition.Reason = string(gatewayv1.ListenerReasonInvalidCACertificateRef) |
There was a problem hiding this comment.
The PR uses ListenerReasonInvalidCACertificateRef for unknown or unsupported resource Group/Kinds. The Gateway API specification strictly dictates that if a reference points to an unknown or unsupported kind of resource, the Reason MUST be set to InvalidCACertificateKind (defined as gatewayv1.ListenerReasonInvalidCACertificateKind).
| // Compute listener status | ||
| routes := r.State.GetHTTPRoutes() | ||
| gs := state.GatewayState{Gateway: gw} | ||
| configMaps := r.State.GetConfigMaps() |
There was a problem hiding this comment.
The controller utilizes configMaps from the State to determine whether the ResolvedRefs listener condition should be True or False. However, the GatewayReconciler.SetupWithManager function has not been updated to watch corev1.ConfigMap. If a referenced ConfigMap is created or modified after the Gateway is initially reconciled, the Gateway will not be queued for reconciliation, and its ResolvedRefs status will remain stale (e.g., permanently stuck at False) until the Gateway object itself happens to be touched.
| originalDirector := proxy.Director | ||
| proxy.Director = func(req *http.Request) { | ||
| originalDirector(req) | ||
| if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { |
There was a problem hiding this comment.
If a client connects without a certificate (e.g., when the gateway is configured with AllowInsecureFallback mode), r.TLS.PeerCertificates will be empty, and this code block does nothing. Crucially, it fails to sanitize an already-existing X-Forwarded-Client-Cert header sent by a malicious client. An attacker could inject this header in their HTTP request to impersonate a trusted identity to the downstream backend. You MUST explicitly call req.Header.Del("X-Forwarded-Client-Cert") if no valid certificate is provided to prevent header spoofing.
- Fallback to an empty CertPool on invalid CA refs to fail-closed instead of fail-open. - Move CertPool parsing to the state builder to avoid performance overhead in GetConfigForClient. - Dynamically evaluate listener conditions and correctly set the Accepted condition to False (with reason NoValidCACertificate) if a CA reference is invalid. - Fix SNI routing logic to properly prioritize the most specific hostname match regardless of TLS configuration presence. - Strip any malicious X-Forwarded-Client-Cert headers sent by unauthenticated clients. - Add ConfigMap watch in the Gateway controller to prevent stale ResolvedRefs conditions. - Return InvalidCACertificateKind on unsupported Group/Kinds.
codebot-robot
left a comment
codebot-robot
left a comment
There was a problem hiding this comment.
Thank you for the detailed and critical review. I have resolved all the security vulnerabilities, performance bottlenecks, and compliance issues:
- System Roots Fallback: Fixed the critical vulnerability where an empty CA parsed list leaves nil. The state builder now correctly fails closed by unconditionally instantiating a custom, empty on invalid or missing CA references.
- Fail-Open on Invalid Refs: Fixed the critical flaw where invalid references silently skipped applying a custom . Invalid references now force an explicit empty configuration combined with mode, causing connections to safely fail closed.
- Listener Accepted Condition: Corrected the listener condition logic. When a CA reference is invalid, the listener's condition is now dynamically set to with the reason alongside being set to .
- Header Spoofing: Fixed the high-severity header spoofing vulnerability. The proxy now explicitly deletes the header if a client doesn't present a valid peer certificate.
- Stale Conditions: Added a configuration for to the in . Gateways will now automatically reconcile whenever a referenced is created, updated, or deleted, preventing stale conditions.
- Routing Bug: Fixed to unconditionally find the best matching route based strictly on exact-match specificity scores before evaluating , adhering to Gateway API rules.
- Performance Overhead: Removed the expensive inline PEM parsing and allocation from the hot path (). This logic has been fully moved to the state generation phase within .
- Spec Compliance: Updated the controller to correctly use when a CA reference points to an unknown or unsupported Group/Kind.
- Code Polish: Refactored the deeply nested conditionals in to use a clearer pattern.
(This comment was generated by Overseer)
|
[Overseer] Started review task. |
1 similar comment
|
[Overseer] Started review task. |
codebot-robot
left a comment
codebot-robot
left a comment
There was a problem hiding this comment.
Great work on implementing the Frontend Client Certificate Validation and addressing the previous security and functional feedback. The shift to parsing certificates early and securely failing-closed on invalid references is a strong improvement.
I have provided a few additional comments focusing on:
- Performance: The new
ConfigMapwatch enqueues all Gateways on every ConfigMap change, which is a significant scalability concern. - Edge Cases: The hostname specificity scoring does not currently handle empty strings (catch-all routes), which will cause them to fail SNI matching.
- Spec Compliance: The controller currently fails the
Acceptedcondition if anyCACertificateRefis invalid, whereas the spec dictates it should only fail if all are invalid. - Architecture: The proxy currently extracts the TLS configuration from the attached
HTTPRoute. If a listener has no routes, it will bypass its own TLS validation.
Please review the inline comments for more details on these and a few minor stylistic cleanups.
(This review was generated by Overseer)
| func (r *GatewayReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&gatewayv1.Gateway{}). | ||
| Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []ctrl.Request { |
There was a problem hiding this comment.
Enqueueing all Gateways for every ConfigMap change in the cluster introduces a significant performance and scalability bottleneck. In large clusters with many ConfigMaps, this will cause constant, unnecessary reconciliation of all Gateways. You should filter this to only enqueue Gateways that actually reference the modified ConfigMap (or at least filter by a relevant label/namespace if references cannot be quickly resolved).
| if len(hostnames) == 0 { | ||
| return 1 // Catch-all has the lowest specificity > 0 | ||
| } | ||
|
|
There was a problem hiding this comment.
If an HTTPRoute does not specify any hostnames, the route.Hostnames slice will contain an empty string "" (as populated by BuildInternalRoutes). Your scoring loop here does not handle h == "", meaning an empty string hostname will fail to match a valid hello.ServerName like example.com. You must add an else if h == "" branch that assigns a low fallback score (e.g., score = 1) so that catch-all routes correctly match any SNI.
| } | ||
|
|
||
| if resolvedRefsCondition.Status == metav1.ConditionFalse { | ||
| acceptedCondition.Status = metav1.ConditionFalse |
There was a problem hiding this comment.
The Gateway API specification states that Accepted should only be set to False with NoValidCACertificate if ALL CACertificateRefs are invalid. By checking resolvedRefsCondition.Status == metav1.ConditionFalse here, you are failing the listener if any single reference is invalid. You should track the number of valid references during the loop and only set Accepted to False if no valid references were found.
| ns = string(*caRef.Namespace) | ||
| } | ||
| if ns != s.Namespace { | ||
| // Cross-namespace references without ReferenceGrant are invalid/unsupported here |
There was a problem hiding this comment.
If multiple CACertificateRefs are provided, and one is invalid (e.g. missing ca.crt), the loop sets invalidRef = true and breaks, which results in the entire listener failing closed with an empty CertPool. Since Gateway API allows multiple references (and dictates that Accepted should remain true if at least one reference is valid), you should continue the loop to collect valid certificates, and only fail closed if all references are invalid.
|
|
||
| if bestRoute != nil && bestRoute.TLSConfig != nil { | ||
| // Create a new config based on the SNI matching route. | ||
| // Note: We MUST include the certificates here as this config replaces the original one. |
There was a problem hiding this comment.
By extracting TLSConfig from the matched InternalRoute, this proxy logic enforces client certificate validation only if an HTTPRoute is attached to the listener. If a Gateway administrator configures a Listener with strict client certificate validation, but no HTTPRoute is currently attached to it, bestRoute will be nil and the connection will fail open (bypassing validation entirely). TLS configuration is a property of the Listener, not the Route. You should restructure the state to pass Listener configurations to the proxy so that TLS policies are enforced independently of route attachments.
| } | ||
|
|
||
| bestScore := -1 | ||
| for _, h := range hostnames { |
There was a problem hiding this comment.
Gateway API hostnames do not allow a bare * as a wildcard (wildcards must be of the form *.example.com with at least one label). The h == "*" check is unnecessary and handles an invalid configuration state.
| var validation *gatewayv1.FrontendTLSValidation | ||
| foundPerPort := false | ||
| // Check for per-port configuration first | ||
| for _, pp := range s.Spec.TLS.Frontend.PerPort { |
There was a problem hiding this comment.
Both pp.Port and listener.Port are of type gatewayv1.PortNumber. The explicit cast to int32 is redundant and can be simplified to pp.Port == listener.Port.
| for _, listener := range gw.Spec.Listeners { | ||
| if listener.Protocol == gatewayv1.HTTPSProtocolType { | ||
| var validation *gatewayv1.FrontendTLSValidation | ||
| foundPerPort := false |
There was a problem hiding this comment.
The explicit cast to int32 is unnecessary since both pp.Port and listener.Port share the gatewayv1.PortNumber underlying type.
| ns := gw.Namespace | ||
| if caRef.Namespace != nil { | ||
| ns = string(*caRef.Namespace) | ||
| } |
There was a problem hiding this comment.
The ConfigMap lookup and validation logic is duplicated almost exactly here and in pkg/state/gateway.go. To ensure the state builder and status conditions remain perfectly synchronized, this resolution logic should be extracted into a shared helper function (e.g., ResolveCACertificateRefs) that returns the certificates or the appropriate invalidation reason.
| proxy := httputil.NewSingleHostReverseProxy(target) | ||
|
|
||
| originalDirector := proxy.Director | ||
| proxy.Director = func(req *http.Request) { |
There was a problem hiding this comment.
While setting a simple Base64 encoded certificate works, most ecosystem backends expect the X-Forwarded-Client-Cert (XFCC) header to follow a structured format (like Envoy's By=...;Hash=...;Cert=...). For a reference implementation, it might be beneficial to adopt a more standard structure or at least clearly document this format, as backends might fail to parse a raw base64 string.
|
Submitted review generated by Overseer. I will monitor the progress. |
|
I've created an |
- Fix ConfigMap watch scalability by filtering to only enqueue Gateways that reference the modified ConfigMap. - Fix missing catch-all route support by explicitly assigning a score of 1 to empty hostname matches in the proxy. - Fix listener Accepted condition to only set Status: False if ALL CACertificateRefs are invalid, per the Gateway API specification. - Extract duplicated CA reference resolution logic into a shared helper function `ResolveCACertificateRefs` in the state package. - Restructure the proxy to enforce TLS configurations on Listeners rather than HTTPRoutes, closing a fail-open vulnerability for unattached listeners. - Simplify port comparisons by removing redundant `int32` casts. - Change the `X-Forwarded-Client-Cert` header injection format to match Envoy's structured format (`Hash=...;Cert=...`). - Remove invalid bare wildcard `*` check from hostname specificity scoring. Fixes gke-labs#125
codebot-robot
left a comment
codebot-robot
left a comment
There was a problem hiding this comment.
Thank you for the additional review! I've addressed all the feedback points:
- Scalability: Filtered the
ConfigMapwatch in theGatewayReconcilerto only enqueue Gateways that actively reference the modified ConfigMap. - Empty Hostnames: Added an
h == ""check in the proxy's hostname scoring loop to correctly assign a fallback score for catch-all listeners. - Spec Compliance (
AcceptedCondition): Corrected the condition logic to track valid references and only fail theAcceptedcondition if no valid references are found. - Spec Compliance (Invalid Refs): Ensured the listener correctly collects valid certificates, only failing closed with an empty pool if all CA references are invalid.
- Listener-Level TLS: Restructured the internal state to pass
InternalListenerobjects directly to the proxy, guaranteeing TLS configurations are enforced independently of route attachments. - Code Cleanup: Removed the invalid
h == "*"check from the proxy routing logic. - Type Simplification: Simplified port type checks by removing redundant
int32casts in the state builder and controller. - Code Reuse: Extracted the duplicated CA reference resolution logic into a shared
ResolveCACertificateRefshelper function in thestatepackage. - XFCC Format: Updated the
X-Forwarded-Client-Certheader to adopt a standard format:Hash=...;Cert=...similar to Envoy.
(This comment was generated by Overseer)
1 participant
This PR enhances the Gateway API implementation to support frontend client certificate validation and adds the GatewayFrontendClientCertificateValidationInsecureFallback conformance test.
Changes:
Fixes #125
This PR was generated by Overseer (powered by the gemini-3-flash-preview model).