RFC0055 Identity-Aware Routing#535
Conversation
cf-foundation-community-automation
Bot
moved this to Inbox
in Application Runtime Platform Working Group
rkoster
force-pushed
the
feature/app-to-app-mtls-routing
branch
from
46b4007 to
23b4a0c
Compare
Latest Update: RFC-Compliant Post-Selection AuthorizationImplemented breaking change to replace pre-selection authorization with strict post-selection enforcement per RFC lines 475-517. Key Changes (commit cbf0695)Architecture:
Implementation:
Test Coverage:
RFC Compliance✅ Intermittent 403s - Expected for shared routes across scope boundaries (RFC-compliant) Breaking ChangeDeprecated:
Integration Test ResultsAll integration tests compile successfully. Shared route scenarios validate:
Ready for full integration test run and review. |
Refactoring: AuthError for Future ExtensibilityCommit: 4ff64b9 Renamed Changes
Benefits
No functional changes - pure refactoring for extensibility. |
rkoster
force-pushed
the
feature/app-to-app-mtls-routing
branch
3 times, most recently
from
1f9b804 to
79271b7
Compare
rkoster
force-pushed
the
feature/app-to-app-mtls-routing
branch
from
5cc4170 to
b875867
Compare
HTTP clients that include explicit ports in URLs (e.g., https://app.example.com:443/) result in Go's http.Request.Host containing the port (app.example.com:443). Previously, GetMtlsDomainConfig() did not strip the port before matching against configured mTLS domains (e.g., *.apps.identity), causing: - Domain matching to fail for requests with explicit ports - No XFCC header added (fell back to default behavior) - Identity extraction failure in CallerIdentity - Pre-auth handler denying requests with 403 and reason "identity-extraction-failed" This particularly affected Java Spring Boot HTTP clients which construct URLs with explicit ports by default. Fix: Use net.SplitHostPort() to strip port before domain matching, ensuring consistent behavior regardless of whether clients include explicit ports. Added comprehensive unit tests covering: - Wildcard domain matching with/without ports - Exact domain matching with/without ports - IsMtlsDomain() function with/without ports - Negative test cases for non-mTLS domains
The test was incorrectly expecting 403 Forbidden when a route is registered on an mTLS domain without route policy enforcement enabled. The correct behavior is to allow the request through (200 OK) and let the backend handle authorization. Route policy enforcement is controlled by Cloud Controller via the RoutePolicyScope field. When RoutePolicyScope is empty (enforcement disabled), GoRouter allows authenticated requests through. Default-deny only applies when enforcement IS enabled but no policies are configured.
- Fix domainMatches wildcard to only match single DNS label (security) - Improve SNI/Host mismatch security checks to prevent domain confusion attacks - Add AuthError.ClientMessage() to prevent information leakage in error responses - Add nil CallerIdentity checks in post-selection auth handlers (defense in depth) - Set AuthResult.Outcome in all auth success paths for proper observability - Add proper error handling for response writes in proxy error handler - Remove unnecessary blank type assertion - Add comprehensive unit tests for mtls_pre_auth handler
- Fix scope=any to populate AuthResult for access log consistency - Add AuthResult assertions to scope=any test case - Add test coverage for unknown RoutePolicyScope default case - Add empty GUID guard to cf:app: policy for consistency with cf:space:/cf:org:
- Fix wildcard domain matching inconsistency between GetMtlsDomainConfig and domainMatches to only match single-level subdomains - Add bounds check for empty Subject field in XFCC header parsing - Change RoutePool nil handling from silent bypass to explicit denial with error logging to prevent authorization bypass - Improve error messages by including malformed DN in error output - Add comprehensive test coverage for edge cases including multi-level subdomains, empty Subject strings, and nil RoutePolicies - Update existing tests to validate new security-focused behavior All tests pass (163 config, 390 handlers) and code passes go vet, gofmt, and staticcheck linting.
…line - Fix dead-code bug: skip internal error handler for *AuthError in proxy_round_tripper so ReverseProxy.ErrorHandler can write the 403 - Fix error leak: replace err.Error() with generic status text in fallback error handler to avoid exposing internal details - Extract handleReverseProxyError() as testable package-level function - Add unit tests for handleReverseProxyError (proxy_error_handler_test.go) - Add post-selection pipeline tests in proxy_round_tripper_test.go - Add Layer 0 security branch test in mtls_pre_auth_test.go
Add ERB template validation that raises a deployment error when xfcc_format is configured alongside forwarded_client_cert: always_forward on an mTLS domain. In always_forward mode the XFCC header is passed through untouched, so xfcc_format has no effect and the combination indicates operator misconfiguration. Add rspec coverage for the new validation and surrounding valid combinations (sanitize_set+envoy, always_forward alone, xfcc_format without explicit forwarded_client_cert).
Previously this combination was only rejected by the BOSH template at deploy time. With gorouter now used outside of BOSH (cf-on-kind), the Go config must also enforce this constraint. Also removes dead code in GetMtlsDomainConfig wildcard matching where the strings.Contains check was redundant due to SplitN guarantees.
…ort/readyreader to routing-api BOSH package spec
- Rename caller_app/space/org → caller_cf_app/space/org for clarity - Remove auth, auth_rule, auth_denied_reason fields (not needed) - Always emit tls_sni and caller_cf_* fields with "-" when empty - Removes conditional emission that caused inconsistent log output
Per-request denial log statements (mtls-route-policies-denied, mtls-pre-auth-denied, mtls-scope-auth-denied, post-selection-auth-denied) now log at DEBUG level to avoid log volume amplification in production. The access log already captures all denial information via caller_cf_* fields and HTTP status codes. These DEBUG logs remain available for local debugging when operators enable debug-level logging.
…ation tests - Update router.client_cert_validation description to note that router.domains enforce mTLS independently - Update router.domains description to clarify relationship with router.client_cert_validation - Add rspec tests for all ERB template validation branches: non-array input, non-hash entry, missing/empty name, missing/empty ca_certs, invalid forwarded_client_cert mode, and invalid xfcc_format value Addresses PR #535 review threads 1-8.
- Rename identityHandler to cfIdentityHandler / NewCfIdentity to clarify it is specific to CF app instance identity certificates (thread 9) - Guard identity extraction: only run when (1) TLS was used and (2) the host is a configured mTLS domain, preventing XFCC header spoofing on non-mTLS routes (thread 10) - Move MtlsPreAuth handler above ClientCert in the proxy chain so a 421 response skips unnecessary certificate processing (thread 11) - Use configured xfcc_format from domain config instead of auto-detecting format at runtime; reject if format doesn't match (thread 12) All 386 handler tests and 179 proxy tests passing.
Split MtlsPreAuth into MtlsSniCheck (early 421) and MtlsPreAuth (post- CfIdentity 403) to fix the handler ordering regression from ac2e87e where moving MtlsPreAuth above ClientCert/CfIdentity caused CallerIdentity to always be nil, denying all mTLS app-to-app requests with 403. Handler chain order is now: Lookup → MtlsSniCheck → ClientCert → CfIdentity → MtlsPreAuth Additional PR #535 review feedback addressed: - Add route_policy field to access logs (renamed from auth_rule, always emitted with '-' when empty) - Remove per-request denial/granted log statements entirely (they duplicate access log information) - Move routePolicies/routePolicyScope from endpoint to pool-level fields to avoid stale data and reduce mutex contention All handler, access log, route, and proxy tests passing.
Cover the behavior introduced when moving route policy fields from endpoint to pool level: initial state, Put updates, re-Put updates, persistence after Remove, and default-deny (empty policies with scope).
DNS hostnames are case-insensitive per RFC 1035 (https://www.ietf.org/rfc/rfc1035.txt), but IsMtlsDomain() and GetMtlsDomainConfig() used case-sensitive map lookups. This caused mTLS domain matching to fail when clients sent uppercase or mixed-case hostnames in the Host header or SNI field. Fix by normalizing domain names to lowercase both when storing in mtlsDomainMap (in processMtlsDomains) and when looking up in GetMtlsDomainConfig. Added unit tests covering: - Wildcard domain matching with uppercase host - Exact domain matching with mixed case host - Matching with uppercase host and port - IsMtlsDomain with various case combinations
rkoster
force-pushed
the
feature/app-to-app-mtls-routing
branch
from
82e9fa1 to
846c45b
Compare
The route policies auth handler was using pool-level policies instead of endpoint-level policies. This caused authorization failures when multiple endpoints on the same route have different route policies (e.g., backend-1 allows app-1, backend-2 allows app-2). Now uses the selected endpoint's RoutePolicies field which is already passed to the Check method, enabling per-endpoint authorization decisions. Fixes CI test: allows only the specified app and denies others (per-endpoint rules)
| // domainMatches checks if a hostname matches a domain pattern (supports wildcard domains). | ||
| // Wildcard patterns (*.domain) only match a single DNS label, not multiple levels. | ||
| func domainMatches(hostname, domainPattern string) bool { | ||
| if hostname == domainPattern { |
There was a problem hiding this comment.
I think there is still a case sensitivity issue here because there is nothing that lowercases the hostname.
There was a problem hiding this comment.
Fixed in d3f4135 — domainMatches() now normalizes both hostname and domainPattern to lowercase before comparison, per RFC 1035. Added tests for uppercase hostnames, mixed-case suffixes, and mixed-case domain patterns.
| } | ||
|
|
||
| // Store with lowercase key for case-insensitive matching (RFC 1035) | ||
| c.mtlsDomainMap[strings.ToLower(domain.Domain)] = domain |
There was a problem hiding this comment.
There is still a casing issue here. The map key is lowercased. But domain.Domain is not.
I think it should be consistent and lowercased everywhere.
There was a problem hiding this comment.
Fixed in d3f4135 — domain.Domain is now normalized to lowercase when storing, not just the map key. This ensures downstream code (like domainMatches) receives consistent lowercase values. Added tests verifying cfg.Domain returns lowercase even when configured with mixed case.
|
I found more per-request logs, which goes against the logging standards for gorouter. mtls_sni_check.go:52 - Non-mTLS domain with cert required All of these should already result in access logs (I think). Thus these logs are duplicative and should be deleted. |
| n.Use(handlers.NewMtlsSniCheck(cfg, logger)) | ||
| n.Use(handlers.NewClientCert( | ||
| SkipSanitize(routeServiceHandler.(*handlers.RouteService)), | ||
| ForceDeleteXFCCHeader(routeServiceHandler.(*handlers.RouteService), cfg.ForwardedClientCert, logger), | ||
| cfg.ForwardedClientCert, | ||
| cfg, | ||
| logger, | ||
| errorWriter, | ||
| )) | ||
| n.Use(handlers.NewCfIdentity(cfg)) | ||
| n.Use(handlers.NewMtlsPreAuth(cfg, logger)) |
There was a problem hiding this comment.
Can you add ifs so that these handlers aren't always run? I suggest something like...
shouldRunMtlsHandlers := len(config.Domains) > 0 || config.GlobalMtlsEnabled
There was a problem hiding this comment.
actually DOES this code work when config.GlobalMtlsEnabled is true? if that is true, should all domains automatically work with route policies? Or do you want to force people to configure the domains in that case?
There was a problem hiding this comment.
Done, I went with the no-op handler pattern instead of adding conditionals in proxy.go.
When I tried to write tests for conditional wrapping in proxy.go, I ran into a code smell: the tests needed intimate knowledge of handler internals to verify observable behavior differences. That coupling suggested the abstraction was in the wrong place.
So instead, the handler constructors now return a no-op handler when len(cfg.Domains) == 0. This keeps the decision inside the handler package and is straightforward to test. See commit a4c165a.
Regarding GlobalMtlsEnabled: I think requiring explicit router.domains config makes sense for identity-aware routing. The feature is strongest when a dedicated CA is configured for the identity domain. With global client_cert_validation: require, operators would need to add off-platform client CAs to the trust store to allow external traffic — but then those external certs could potentially spoof CF identity OUs (app:, space:, org:), weakening the trust model. A dedicated domain with only the Diego CA keeps the identity guarantees tight.
Address PR #535 review threads 14, 15, 16: - Normalize domain.Domain to lowercase when storing in mtlsDomainMap, not just the map key (thread 16) - Make domainMatches() case-insensitive by lowercasing both hostname and pattern before comparison (thread 15) Added 9 new tests covering mixed-case domain configuration and hostname matching. All 569 tests passing (175 config + 394 handlers).
…g standards Remove duplicative per-request logs that violate gorouter logging standards. Access logs already capture all necessary information via status codes and the caller_cf_* fields. Removed log statements: - clientcert.go: using-mtls-domain-xfcc-config (Debug) - mtls_sni_check.go: mtls-enforcement-mismatch (Warn) x2 - mtls_scope_auth.go: mtls-scope-auth-no-route-pool (Error) - mtls_route_policies_auth.go: mtls-route-policies-auth-no-route-pool (Error) - router.go: mtls-domain-detected (Debug) Addresses PR #535 comment: #535 (comment)
Removed all 5 in f100947. Also found and removed one additional per-request log: |
mTLS handler constructors now return NoopHandler/NoopPostSelectionHandler when len(cfg.Domains) == 0, avoiding unnecessary handler instantiation. This keeps the conditional logic encapsulated in the handler package rather than coupling proxy setup to handler internals. Handlers affected: - NewMtlsSniCheck -> NoopHandler - NewCfIdentity -> NoopHandler - NewMtlsPreAuth -> NoopHandler - NewMtlsScopeAuth -> NoopPostSelectionHandler - NewMtlsRoutePoliciesAuth -> NoopPostSelectionHandler Tests added to each handler's test file verifying constructor behavior.
2 participants
RFC0055: Identity-Aware mTLS Routing
Implements Phase 1 (1a + 1b) of RFC0055: App-to-App mTLS Routing.
Tracking: cloudfoundry/community#1481
Acceptance Testing Guide: https://gist.github.com/rkoster/5b252b0edca606f10be2dbdcb81a796f
What This Does
Enables GoRouter to enforce mutual TLS and identity-based authorization on a per-domain basis. Apps calling routes on configured mTLS domains must present a valid Diego instance identity certificate. GoRouter extracts the caller's app/space/org identity and checks it against route policies before forwarding the request.
Phase 1a: mTLS Infrastructure
GetConfigForClientcallback (SNI-based)raw: base64-encoded full certificate (~1.5KB)envoy: compactHash=...;Subject="..."format (~250 bytes)router.domainsPhase 1b: Authorization
scopeandallowed_sources) against selected endpointKey Design Decisions
router.domainsis configured in the BOSH manifest and a shared domain with--enforce-access-rulesis created.Testing
go fmt,go vet,staticcheck, ginkgo with--raceConfiguration Example
Related PRs
Merge Ordering
All PRs are independently safe to merge — the feature is dormant without the ops-file and domain creation. No strict ordering required. Recommend merging around the same time once all are approved.
AI Disclosure
This PR was developed with AI assistance. All code has been read and verified manually. Each error path, branch, and edge case has corresponding test coverage.