RFC0055 Identity-Aware Routing#535
Conversation
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. |
1f9b804 to
79271b7
Compare
5cc4170 to
b875867
Compare
Fix struct field alignment in test files to pass CI gofmt validation.
Complete the terminology rebrand by updating integration test helper functions to use RoutePolicyScope and RoutePolicySources instead of the old AccessScope and AccessRules field names. Fixes go vet error: unknown field AccessScope in struct literal
This commit fixes a critical bug where route policies were incorrectly enforced on ALL routes (including public routes and mTLS routes without --enforce-route-policies flag), causing 403 Forbidden errors. Root Cause: - MtlsRoutePoliciesAuth.Check() only checked CallerIdentity == nil to decide whether to skip enforcement - It did NOT check routePolicyScope (which indicates if enforcement is actually enabled for the domain) - This caused enforcement on routes where it should be skipped Fix: - Add routePolicyScope check at the beginning of Check() method - Only enforce route policies when routePolicyScope != "" (enforcement enabled via --enforce-route-policies flag on domain creation) - This mirrors the pattern already used in MtlsScopeAuth handler Impact: - Public routes (non-mTLS domains): No longer incorrectly rejected - mTLS routes WITHOUT --enforce-route-policies: No longer rejected - mTLS routes WITH --enforce-route-policies: Still correctly enforced Test Coverage: - Added regression test for the specific bug scenario: RoutePolicyScope empty + CallerIdentity present - This test would have caught the bug if it existed originally - All 18 MtlsRoutePoliciesAuth tests now pass - Test suite now covers all skip scenario combinations Files Changed: - src/code.cloudfoundry.org/gorouter/handlers/mtls_route_policies_auth.go - src/code.cloudfoundry.org/gorouter/handlers/mtls_route_policies_auth_test.go
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
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)
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)
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.
Move tls_sni, caller_cf_app, caller_cf_space, caller_cf_org, and route_policy from always-present to the router.logging.extra_access_log_fields opt-in mechanism. Foundations not using the mTLS feature will no longer see these fields (always '-') in every access log line, preventing breakage of existing log parsers. Update the spec description to list all five new available field names. Add three test cases covering populated values, empty (dash) values, and absence from output when not listed in ExtraFields. Addresses PR #535 review thread 18.
…in ERB Add test cases for all five previously untested error paths in processMtlsDomains (threads 19-23): - invalid forwarded_client_cert value - invalid xfcc_format value - ca_certs containing invalid PEM data - ca_certs empty/missing - domain name empty Normalize forwarded_client_cert and xfcc_format values to lowercase in the ERB template before validation, so mixed-case input from operators (e.g. 'Sanitize_Set') is accepted consistently. Also remove debug annotation from GetMtlsDomainConfig test context string (thread 24). Addresses PR #535 review threads 19-24.
Add tests for the three new fields populated in the access log handler from RequestInfo (thread 25): - CallerIdentity -> CallerCFApp, CallerCFSpace, CallerCFOrg - AuthResult -> RoutePolicy - TlsSNI -> TlsSNI Each field is tested with a value present and with the source nil/empty to verify correct zero-value behaviour. Addresses PR #535 review thread 25.
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.