Repository navigation
Conversation
Closed
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
fhanik
changed the title
fhanik
force-pushed
the
review/pr3792-fix
branch
2 times, most recently
from
September 24, 2026 13:48
a3235f7 to
6d0bf7a
Compare
fhanik
marked this pull request as ready for review
September 24, 2026 13:48
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical endpoint-path rejection and moderate dotted reserved-claim protection gaps remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds and hardens RFC 8705 mTLS token-endpoint support, including authentication, claim protections, endpoint gating, TLS integration, discovery metadata, documentation, and tests.
Changes:
- Adds certificate-based client authentication,
client_credentialsrestrictions, disabled-endpoint handling, and protected claim mapping. - Adds FIPS BCJSSE/Tomcat support, discovery metadata, configuration/API documentation, and extensive tests.
- Final findings: a critical regular-endpoint rejection and moderate dotted reserved-claim validation gaps remain.
| File | Reviewed change |
|---|---|
uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/OpenIdConnectEndpointsMockMvcZonePathTests.java |
Tests zone-path discovery metadata. |
uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/OpenIdConnectEndpointsMockMvcTests.java |
Tests mTLS discovery metadata. |
uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/OpenIdConnectEndpointDocs.java |
Documents discovery aliases. |
uaa/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenServicesTests.java |
Tests claim protection and overrides. |
uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/token/MtlsDisabledTokenEndpointMockMvcTests.java |
Tests disabled mTLS behavior. |
uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java |
Documents mTLS token requests. |
uaa/slateCustomizations/source/index.html.md.erb |
Adds mTLS API documentation. |
uaa/build.gradle.kts |
Adds certificate test dependencies. |
server/src/test/java/org/cloudfoundry/identity/uaa/web/tomcat/MtlsClientAuthTomcatCustomizerTest.java |
Tests Tomcat mTLS customization. |
server/src/test/java/org/cloudfoundry/identity/uaa/web/tomcat/BCJSSEUtilTest.java |
Tests BCJSSE protocols. |
server/src/test/java/org/cloudfoundry/identity/uaa/web/tomcat/BCJSSESSLContextTest.java |
Tests BCJSSE contexts. |
server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java |
Tests unsupported TLS authentication rejection. |
server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthIdentityProviderConfigValidatorTest.java |
Tests external OAuth validation. |
server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationManagerTest.java |
Tests stale TLS configuration rejection. |
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/token/UaaTokenEndpointTests.java |
Tests mTLS endpoint mappings. |
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/tls/RawPeerCertificateCaptureFilterTest.java |
Tests peer certificate capture. |
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/tls/RawPeerCertificateCaptureFilterRegistrationTest.java |
Tests filter registration and ordering. |
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/tls/ClientCertificateMapperFilterTest.java |
Tests certificate mapper behavior. |
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranterTests.java |
Tests TLS client-credentials grants. |
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/OauthEndpointSecurityConfigurationTests.java |
Tests conditional mTLS security wiring. |
server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminEndpointsTests.java |
Updates validator construction. |
server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapTests.java |
Tests bootstrap mTLS validation. |
server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapMultipleSecretsUpdateTests.java |
Updates bootstrap fixtures. |
server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapMultipleSecretsTest.java |
Updates bootstrap fixtures. |
server/src/test/java/org/cloudfoundry/identity/uaa/authentication/UaaClientAuthenticationProviderTest.java |
Tests TLS client authentication rejection. |
server/src/test/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProviderTests.java |
Tests certificate authentication and required claims. |
server/src/test/java/org/cloudfoundry/identity/uaa/account/OpenIdConnectEndpointsTest.java |
Tests conditional discovery metadata. |
server/src/main/java/org/cloudfoundry/identity/uaa/zone/ZoneEndpointsClientDetailsValidator.java |
Validates zone client mTLS configuration. |
server/src/main/java/org/cloudfoundry/identity/uaa/web/tomcat/NoAcceptedIssuersTrustManager.java |
Provides TLS trust-manager handling. |
server/src/main/java/org/cloudfoundry/identity/uaa/web/tomcat/MtlsClientAuthTomcatCustomizer.java |
Configures mTLS connectors. |
server/src/main/java/org/cloudfoundry/identity/uaa/web/tomcat/BCJSSEUtil.java |
Integrates BCJSSE with Tomcat. |
server/src/main/java/org/cloudfoundry/identity/uaa/web/tomcat/BCJSSESslImplementation.java |
Selects BCJSSE implementation. |
server/src/main/java/org/cloudfoundry/identity/uaa/web/tomcat/BCJSSESSLContext.java |
Wraps BCJSSE SSL contexts. |
server/src/main/java/org/cloudfoundry/identity/uaa/web/FilterChainOrder.java |
Adds mTLS filter ordering. |
server/src/main/java/org/cloudfoundry/identity/uaa/SpringServletXmlFiltersConfiguration.java |
Registers mTLS filters. |
server/src/main/java/org/cloudfoundry/identity/uaa/SpringServletXmlBeansConfiguration.java |
Wires mTLS configuration. |
server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java |
Excludes TLS authentication from external IdPs. |
server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthIdentityProviderConfigValidator.java |
Validates external OAuth methods. |
server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationManager.java |
Rejects unsupported TLS authentication. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenServices.java |
Protects UAA-owned JWT claims. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/UaaTokenEndpoint.java |
Restricts mTLS grants. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/tls/RawPeerCertificateCaptureFilter.java |
Captures genuine peer certificates. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/tls/MtlsPathGuardedFilter.java |
Scopes certificate mapping to mTLS paths. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/tls/MtlsEndpointAvailabilityFilter.java |
Hides the disabled mTLS endpoint. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/tls/MtlsClaimsEnhancer.java |
Adds certificate-derived claims; dotted reserved parents remain unresolved. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranter.java |
Allows TLS client authentication. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/OauthEndpointSecurityConfiguration.java |
Adds the mTLS security chain. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/OauthEndpointBeanConfiguration.java |
Wires TLS authentication. |
server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrap.java |
Validates bootstrapped mTLS clients. |
server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java |
Enforces mTLS authentication; regular /oauth/token handling has a critical rejection issue. |
server/src/main/java/org/cloudfoundry/identity/uaa/account/OpenIdConnectEndpoints.java |
Advertises mTLS discovery metadata. |
server/src/integrationTest/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapProdEncoderTest.java |
Updates bootstrap integration coverage. |
server/build.gradle.kts |
Adds Tomcat and certificate-mapper dependencies. |
model/src/test/resources/org/cloudfoundry/identity/uaa/account/OpenIdConfiguration.json |
Updates discovery fixture. |
model/src/test/java/org/cloudfoundry/identity/uaa/constants/ClientAuthenticationTest.java |
Tests TLS authentication constants. |
model/src/test/java/org/cloudfoundry/identity/uaa/client/UaaClientDetailsTest.java |
Tests TLS configuration persistence. |
model/src/test/java/org/cloudfoundry/identity/uaa/client/TlsClientAuthConfigurationTest.java |
Tests TLS configuration serialization. |
model/src/test/java/org/cloudfoundry/identity/uaa/account/OpenIdConfigurationTests.java |
Tests conditional discovery fields. |
model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java |
Adds the TLS authentication constant. |
model/src/main/java/org/cloudfoundry/identity/uaa/constants/ClientAuthentication.java |
Adds TLS authentication support. |
model/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClientDetails.java |
Stores TLS client configuration. |
model/src/main/java/org/cloudfoundry/identity/uaa/client/TlsClientAuthConfiguration.java |
Defines TLS configuration and protected claims. |
model/src/main/java/org/cloudfoundry/identity/uaa/account/OpenIdConfiguration.java |
Adds mTLS discovery fields. |
gradle/libs.versions.toml |
Adds dependency aliases and versions. |
docs/UAA-Configuration-Reference.md |
Documents the mTLS feature flag. |
docs/UAA-Client-Authentication.md |
Documents RFC 8705 client authentication. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fhanik
force-pushed
the
review/pr3792-fix
branch
from
September 24, 2026 14:39
d0e18c0 to
db71554
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The mapper dependency can auto-register an earlier unguarded filter that defeats genuine peer-certificate capture, and configuration validation/documentation also have unresolved gaps.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (3)
fhanik
force-pushed
the
review/pr3792-fix
branch
from
September 25, 2026 13:23
b33a45a to
28db8c3
Compare
fhanik
added a commit
to fhanik/uaa
that referenced
this pull request
Sep 25, 2026
…, docs Three findings from the latest Copilot review of PR cloudfoundry#4076 (review 5306465064), verified against the code before implementing: 1. HIGH - "Prevent competing certificate mapper from overwriting the peer certificate" (server/build.gradle.kts:44). Verified by decompiling the java-buildpack-client-certificate-mapper-jakarta jar: it ships a @ConditionalOnCloudPlatform(CLOUD_FOUNDRY) @configuration class, ClientCertificateMapperAutoConfiguration, that registers its own ClientCertificateMapper filter at Ordered.HIGHEST_PRECEDENCE -- ahead of RawPeerCertificateCaptureFilter (-300). If ever activated, it would overwrite the servlet X509Certificate attribute with XFCC data before the genuine TLS peer certificate is captured, defeating direct-connection mTLS authentication. The jar carries no spring.factories / AutoConfiguration.imports entry naming it, so nothing triggers this today, but a routine dependency bump could add one with no code-level signal. UaaBootApplication now excludes it by name (excludeName, since the jar is implementation-scoped in the server module and not on uaa's compile classpath) as permanent, version-independent insurance. 2. MEDIUM - "Reject unsupported subject patterns and captureless OU patterns" (ClientAdminEndpointsValidator.java:533). Verified against TlsClientAuthentication.extractClaimMappingValues: pattern is applied only when field == "subject_ou" (the subject_cn/subject_o switch arms ignore mapping.getPattern() entirely), and matchFirstOu only returns a value via m.group(1), guarded by m.groupCount() >= 1. The validator accepted both shapes as syntactically valid, so a client config could be registered that would never produce the claim it declares. validateTlsClientAuthClaimConfig now rejects a pattern on subject_cn/subject_o and a subject_ou pattern with no capturing group. 3. LOW - "Document template length, placeholder, and mapping constraints" (docs/UAA-Client-Authentication.md:180). The tls-client-auth-claim-mappings / -sub-template / -aud-templates rows didn't mention the validator's enforced constraints (256-character cap, at least one {claim} placeholder, placeholder must name a declared claim, plus the pattern constraints from #2). Documented. Two existing tests (validateTlsClientAuthClaimConfig_acceptsValid NativeClaimMappings / _acceptsValidJsonStringClaimMappings) used a pattern on subject_cn -- exactly the now-rejected shape -- and were updated to use subject_ou instead, which still exercises the pattern path meaningfully. Full ./gradlew clean test passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fhanik
added a commit
to fhanik/uaa
that referenced
this pull request
Sep 25, 2026
…, docs Three findings from the latest Copilot review of PR cloudfoundry#4076 (review 5306465064), verified against the code before implementing: 1. HIGH - "Prevent competing certificate mapper from overwriting the peer certificate" (server/build.gradle.kts:44). Verified by decompiling the java-buildpack-client-certificate-mapper-jakarta jar: it ships a @ConditionalOnCloudPlatform(CLOUD_FOUNDRY) @configuration class, ClientCertificateMapperAutoConfiguration, that registers its own ClientCertificateMapper filter at Ordered.HIGHEST_PRECEDENCE -- ahead of RawPeerCertificateCaptureFilter (-300). If ever activated, it would overwrite the servlet X509Certificate attribute with XFCC data before the genuine TLS peer certificate is captured, defeating direct-connection mTLS authentication. The jar carries no spring.factories / AutoConfiguration.imports entry naming it, so nothing triggers this today, but a routine dependency bump could add one with no code-level signal. UaaBootApplication now excludes it by name (excludeName, since the jar is implementation-scoped in the server module and not on uaa's compile classpath) as permanent, version-independent insurance. 2. MEDIUM - "Reject unsupported subject patterns and captureless OU patterns" (ClientAdminEndpointsValidator.java:533). Verified against TlsClientAuthentication.extractClaimMappingValues: pattern is applied only when field == "subject_ou" (the subject_cn/subject_o switch arms ignore mapping.getPattern() entirely), and matchFirstOu only returns a value via m.group(1), guarded by m.groupCount() >= 1. The validator accepted both shapes as syntactically valid, so a client config could be registered that would never produce the claim it declares. validateTlsClientAuthClaimConfig now rejects a pattern on subject_cn/subject_o and a subject_ou pattern with no capturing group. 3. LOW - "Document template length, placeholder, and mapping constraints" (docs/UAA-Client-Authentication.md:180). The tls-client-auth-claim-mappings / -sub-template / -aud-templates rows didn't mention the validator's enforced constraints (256-character cap, at least one {claim} placeholder, placeholder must name a declared claim, plus the pattern constraints from #2). Documented. Two existing tests (validateTlsClientAuthClaimConfig_acceptsValid NativeClaimMappings / _acceptsValidJsonStringClaimMappings) used a pattern on subject_cn -- exactly the now-rejected shape -- and were updated to use subject_ou instead, which still exercises the pattern path meaningfully.
fhanik
force-pushed
the
review/pr3792-fix
branch
2 times, most recently
from
September 25, 2026 16:31
f52ba97 to
dfc6578
Compare
fhanik
added a commit
to fhanik/uaa
that referenced
this pull request
Sep 29, 2026
…other machine Claude Code sessions are local JSONL transcripts keyed to the absolute project path, so this session cannot be resumed on a different computer. This document is the substitute: enough state, topology and hard-won environment detail to pick the work up cold. Records, verified against the repo rather than recalled: - branch/PR state, the remotes to re-add (including rkoster's fork, since refs/pull/3972/head on the cloudfoundry remote is stale and will not update), and a commit-by-commit map of the branch - the load-bearing fact that cloudfoundry#4076 implements RFC 8705 section 2.1.2 subject binding and cloudfoundry#3972 does not, so on that branch any certificate chaining to the configured CA authenticates as the client - that the two branches have diverged (his 14 follow-ups start exactly where our squash ends) and cannot simply be merged - verification commands and the current green state, including that the two remaining integrationTest failures are both SCIM rate-limiter artifacts - environment traps that cost real time: piped gradle output hiding exit codes, ChromeDriver/Chrome major-version coupling and the unmanaged Homebrew binary, gradle daemons pinning a stale PATH, the Jackson 3 TypeReference package, and a local process that rewrites commits and strips trailers - zone-aware MockMvc conventions, especially why servletPath may only be pre-set when no /z/{subdomain} prefix is present - open items here and across all three approaches, plus a document index Also corrects a stale assumption while writing it: origin/review/pr3968 is twelve commits ahead of what was local, and includes two red/green pairs adding JWT-SVID audit events and default-zone-only signing. Those close two findings that pr3968-security-review.md still lists as unaddressed, so the handoff says to take the remote and treat the old local branch as superseded.
fhanik
added a commit
to fhanik/uaa
that referenced
this pull request
Sep 29, 2026
Removes the JWT-SVID/SPIFFE material, which belongs to a separate branch that will not be picked up on the new machine and was only noise for continuing PR cloudfoundry#4076: - drops the section on that branch's divergence from origin entirely - section 10 becomes a two-way comparison of this branch against the Service Accounts RFC, which stays because that design depends on tls_client_auth_san_dns and therefore consumes this branch rather than competing with it - the document index lists only the mTLS-relevant analysis docs - the "verify, do not assert" agreement now cites mTLS examples: the seven stale FINDING labels, the dotted-claim protection we already had, and the CA rotation gap we actually did not - the @ConditionalOnProperty note now describes how the mTLS suites switch the feature on, rather than pointing at the other branch's fixtures
fhanik
force-pushed
the
review/pr3792-fix
branch
from
September 30, 2026 18:18
6f8a31a to
fd53af2
Compare
fhanik
added a commit
to fhanik/uaa
that referenced
this pull request
Sep 30, 2026
…other machine Claude Code sessions are local JSONL transcripts keyed to the absolute project path, so this session cannot be resumed on a different computer. This document is the substitute: enough state, topology and hard-won environment detail to pick the work up cold. Records, verified against the repo rather than recalled: - branch/PR state, the remotes to re-add (including rkoster's fork, since refs/pull/3972/head on the cloudfoundry remote is stale and will not update), and a commit-by-commit map of the branch - the load-bearing fact that cloudfoundry#4076 implements RFC 8705 section 2.1.2 subject binding and cloudfoundry#3972 does not, so on that branch any certificate chaining to the configured CA authenticates as the client - that the two branches have diverged (his 14 follow-ups start exactly where our squash ends) and cannot simply be merged - verification commands and the current green state, including that the two remaining integrationTest failures are both SCIM rate-limiter artifacts - environment traps that cost real time: piped gradle output hiding exit codes, ChromeDriver/Chrome major-version coupling and the unmanaged Homebrew binary, gradle daemons pinning a stale PATH, the Jackson 3 TypeReference package, and a local process that rewrites commits and strips trailers - zone-aware MockMvc conventions, especially why servletPath may only be pre-set when no /z/{subdomain} prefix is present - open items here and across all three approaches, plus a document index Also corrects a stale assumption while writing it: origin/review/pr3968 is twelve commits ahead of what was local, and includes two red/green pairs adding JWT-SVID audit events and default-zone-only signing. Those close two findings that pr3968-security-review.md still lists as unaddressed, so the handoff says to take the remote and treat the old local branch as superseded.
fhanik
added a commit
to fhanik/uaa
that referenced
this pull request
Sep 30, 2026
Removes the JWT-SVID/SPIFFE material, which belongs to a separate branch that will not be picked up on the new machine and was only noise for continuing PR cloudfoundry#4076: - drops the section on that branch's divergence from origin entirely - section 10 becomes a two-way comparison of this branch against the Service Accounts RFC, which stays because that design depends on tls_client_auth_san_dns and therefore consumes this branch rather than competing with it - the document index lists only the mTLS-relevant analysis docs - the "verify, do not assert" agreement now cites mTLS examples: the seven stale FINDING labels, the dotted-claim protection we already had, and the CA rotation gap we actually did not - the @ConditionalOnProperty note now describes how the mTLS suites switch the feature on, rather than pointing at the other branch's fixtures
Require tls-client-auth-trusted-proxy-subject-dn alongside inline or named proxy CA trust. Validate the genuine TLS peer chain, leaf constraints and registered DN before trusting its forwarded workload certificate. Validate the identity during client registration and bootstrap, preserve it through typed/flat configuration and token enrichment, and deny legacy or malformed proxy policy without falling back to direct authentication. Document the required setting and update the client-admin API reference. Validation: 576 model, 316 server and 137 HTTP tests passed; 11 model tests skipped. Markdown lint and git diff --check pass. API docs regenerated successfully with generateDocs. ai-assisted=yes
Startup must stay fast on installations with tens of thousands of zones,
so the trust-bundle validator may only inspect the default zone at
startup, and only when mTLS is effectively enabled there (deployment
switch and zone switch). A disabled default zone or a disabled deployment
leaves its persisted references inert instead of failing startup.
Tenant zones are validated when an update turns mTLS on for that zone:
a client reference that does not resolve against the new configuration
rejects the update, a reference restored by the same update is accepted,
and updates that keep mTLS enabled (bundle removal) or disable it are not
revalidated. Covered both with the default zone disabled and enabled,
through PUT /identity-zones/{id} and at the unit level.
Require the ClientCertificateMapper not to be reflectively loaded when
uaa.mtls.enabled=false; the registration bean still exists, disabled,
around an inert pass-through delegate.
Add the reverse MockMvc zone policy case: disabling a tenant zone leaves
the default zone's mTLS endpoint working (passes already).
The validator gains the deployment flag and a no-op validateOnEnable so
the tests compile. Eleven cases fail at the intended assertions.
ai-assisted=yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…een)
Startup validates only the default zone, and only when mTLS is enabled
for both the deployment and that zone, so startup time no longer grows
with the number of identity zones and a disabled feature leaves stale
references inert instead of failing startup.
PUT /identity-zones/{id} validates a zone's client bundle references
when the update turns mTLS on, answering 422 when a reference does not
resolve against the new configuration. Updates that keep mTLS on or
turn it off are not revalidated; token-time resolution still denies.
Both directions are independent of the default zone's own policy.
The ClientCertificateMapper is no longer reflectively loaded when
uaa.mtls.enabled=false; its disabled registration wraps a pass-through.
ai-assisted=yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Validating trust-bundle references loaded every client of the zone, although almost none of them carry an mTLS configuration. Specify a zone-scoped client query that filters on the additional information key in the database: it returns only clients whose additional information has one of the given keys, ignores clients in other zones and values that merely contain the key text, treats LIKE wildcards in keys literally, and returns nothing without keys. Require the trust-bundle validator to use that query at startup and on zone enable, and never call listClientDetails. A scaffold of the query that still lists every client lets the tests compile. Nine cases fail at the intended assertions. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MultitenantJdbcClientDetailsService filters a zone's clients in SQL on the serialized additional information key, with LIKE wildcards escaped, and confirms the match on the parsed map. The base class keeps an in-memory fallback for non-JDBC implementations. The trust-bundle validator uses this query at startup and on zone enable, so it no longer loads every client of a zone when almost none of them have an mTLS configuration. Also fix the new no-full-scan test, whose tenant stub returned an unresolvable reference and made the enable check throw as designed. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Even a filtered query has to read every client row of a zone, because bundle references live in the additional_information JSON column. Token-time resolution already denies a client whose reference does not resolve, so the early check is not worth a zone-wide scan. Startup may check only the default zone's own bundle definitions and must not touch client details. Enabling mTLS for a zone is accepted even when a persisted client reference does not resolve; that client is denied at token time and works again once the bundle is restored. Both directions of the default zone policy are covered. Three cases fail at the intended assertions. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Startup checks only the default zone's own bundle definitions. Zone updates no longer look up the zone's clients when mTLS is turned on, and the LIKE query over additional_information is removed together with its tests: it could not use an index and still read every client row of the zone. A client reference that does not resolve is denied at token time. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Require config.mtls.enabled to default to false for new zones and for stored zone configuration that omits the switch, and the default zone to stay disabled unless the deployment enables it at bootstrap. A new zone does not expose the mTLS endpoint or advertise it in discovery until it opts in, and discovery for a zone that has not opted in omits mTLS even when the deployment enables it. Tests that exercise mTLS behaviour now enable it explicitly: a JUnit extension installs an mTLS-enabled default zone, and MockMvc zone fixtures opt their zones in. These fixture changes pass against the current default. Also replace the startup-validator MockMvc test left over from the client-scan removal: a removed bundle no longer prevents startup, and removedBundleFailsClosedAtTokenTime covers the token-time denial. Nine cases fail at the intended assertions. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
New zones, and stored zone configuration that omits the switch, now have mTLS disabled. The default zone still follows uaa.mtls.enabled on every restart, and its bootstrap default is false as well. Turning on the deployment flag therefore enables mTLS only for the default zone; each tenant zone opts in through the identity-zone API. Update the configuration reference, client authentication and getting started guides, and the identity-zone API field descriptions, which also still claimed that removed bundle references are rejected at startup. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Excluding granted_scopes from the claims copied onto a refreshed access token is a pre-existing bug for any deployment that registers a token enhancer, independent of mTLS. It now lives on a separate branch based on cloudfoundry/develop, with its own test. This pull request must merge after that one: the mTLS enhancer is always registered, so with mTLS enabled the copy loop runs and would otherwise leak granted_scopes onto narrowed refreshed access tokens. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e (red) Client create, update and uaa.yml bootstrap currently refuse any mTLS configuration while mTLS is off for the deployment or the zone, and never check whether it is well formed. Validation happens when the client is written, so it costs nothing to apply it every time. Require valid configuration (CA plus exactly one subject binding) to be accepted, and malformed configuration (missing subject binding, unparseable trust, bound tokens without trust) to be rejected, with the deployment off, or with the deployment on and the zone off. Covered in the three validators and through POST /oauth/clients. Seventeen cases fail at the intended assertions. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… (green) Client create, update and uaa.yml bootstrap validate mTLS configuration the same way regardless of the deployment and zone switches: valid configuration is stored, malformed configuration is rejected. Whether it is used is still decided per request, so nothing is ever rechecked by scanning stored clients. The deployment flag was only consulted for that refusal, so ClientAdminEndpointsValidator, ClientAdminBootstrap and ZoneEndpointsClientDetailsValidator get their develop constructor signatures back. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… (red) UaaAuthenticationDetails is Serializable, is held by the security context that Spring Session stores, and has no explicit serialVersionUID. The new servletPath field and getter change the computed UID, so during a rolling upgrade a node cannot read a session written by a node on the other version. Pin the previous release's computed UID, and deserialize an instance serialized by cloudfoundry/develop (623f6ea). Both fail: the second with InvalidClassException. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…(green) Pin UaaAuthenticationDetails' serialVersionUID to the value computed for cloudfoundry/develop, so sessions round-trip between nodes on either version during a rolling upgrade. A previous-release instance reads back with a null servletPath, which the mTLS path check treats as not mTLS. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…(red) ClientAuthentication is public API of the model jar and backs external IdP validation. Adding tls_client_auth to UAA_SUPPORTED_METHODS and rewriting the three-argument isValidMethod changed results that have nothing to do with mTLS: private_key_jwt without a key and none with a key stopped validating, and tls_client_auth became a supported method. Pin the previous release's results. Fails at the intended assertion. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…green) Restore ClientAuthentication to its cloudfoundry/develop form apart from the new TLS_CLIENT_AUTH constant. tls_client_auth is accepted only by UAA's own mTLS token endpoint and is not one of UAA_SUPPORTED_METHODS, which governs how UAA authenticates to external identity providers. The external-IdP-only method list and the four-argument isValidMethod and getCalculatedMethod are no longer needed and are removed, along with their tests and the guards in ExternalOAuthAuthenticationManager that only existed because tls_client_auth had become a supported method. The external IdP validator and factory bean are identical to develop again; their tests that reject tls_client_auth still hold. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With mTLS off, /.well-known/openid-configuration now always carries "tls_client_certificate_bound_access_tokens": false, a field the previous release never emitted. Require it to be omitted unless mTLS is enabled for the deployment and the zone, which RFC 8705 section 3.3 defines as false, and document the field as optional. Five cases fail at the intended assertions. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Omit tls_client_certificate_bound_access_tokens from the discovery document unless it is true. The serialized discovery fixture is back to its cloudfoundry/develop form. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every zone's config now serializes an "mtls" block in API responses and
in the stored configuration, and a PUT with "mtls": null or
"enabled": null is refused with 400, where the previous release ignored
the key. Rejecting null made sense while the zone default was true (a
null would have opted in); with the default now false a null means the
disabled default.
Require the block to be omitted while it holds its default, and nulls to
read as the disabled default, through JSON and PUT /identity-zones/{id}.
Five cases fail at the intended assertions.
ai-assisted=yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Omit config.mtls from serialized zone configuration while it equals the default, and read "mtls": null and "enabled": null as the disabled default instead of refusing them. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… test ClientAdminBootstrap regained its develop constructor when client validation stopped depending on the deployment flag; the integration test source set still passed the removed argument. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove statements left over from the earlier rules: registration no longer depends on uaa.mtls.enabled, null zone mtls values read as the disabled default, and malformed default-zone bundles stop startup only while mTLS is enabled there. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng (red) A client with tls-client-auth-trusted-proxy-ca(-ref) and no workload trust can be registered, can never authenticate at /oauth/mtls/token, and, once stored, still authenticates with its secret at /oauth/token, contrary to proxy trust marking a client as mTLS-only. Require registration (client admin API and uaa.yml bootstrap) to reject proxy trust without tls-client-auth-ca(-ref), and a stored proxy-only client, including one with only the proxy subject DN, to be refused at both token endpoints. A client with both kinds of trust stays valid. Also require clearing a client's typed mTLS configuration to remove tls_client_certificate_bound_access_tokens, which otherwise leaves a binding policy that no token can satisfy. Seven cases fail at the intended assertions. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…g (green) Registration rejects tls-client-auth-trusted-proxy-ca(-ref) without tls-client-auth-ca(-ref), after the existing checks so malformed values keep their specific errors. ClientDetailsAuthenticationProvider treats any proxy trust key (TlsClientAuthConfiguration.PROXY_TRUST_KEYS) as mTLS-only and refuses a stored client that has it without workload trust, instead of letting it authenticate with its secret. UaaClientDetails removes tls_client_certificate_bound_access_tokens when the typed mTLS configuration is cleared. Document the requirement for the proxy trust properties. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
PR #4076 review — open findings and next steps (2026-10-15)Pull request: #4076 (RFC 8705 Where things stand
To publish the rebase: Ground rules agreed with the maintainer
Findings from the Copilot-style review of the rebased branchThe Fix before merge1. Enabling mTLS in a zone changes refresh-token access tokens for every client there
3. Bootstrap clients are validated against a stale copy of the default zone
2. The docs about startup bundle validation are wrong (my error earlier the same day)
One rule to end the review cycle (findings 4, 6, 7)4. mTLS clients can be registered with grant types that can never work
6. mTLS-only settings are accepted without workload trust and then silently ignored
7. Malformed stored values return HTTP 500 instead of 400/401
Recommendation for 4, 6 and 7: one shared definition in
The runtime cost on non-mTLS requests is a few map lookups at client authentication. One red/green pair; keep the Docs only5. mTLS clients also lose Basic auth at
|
Tests only; production code is unchanged. Each failing test pins one
finding from the review of the rebased branch, and guard tests pin the
behaviour that must not change, so the fix can be checked against both.
Finding 1, refreshed access tokens in mTLS zones
(UaaTokenServicesTests$WhenRefreshGrant):
- anActiveMtlsEnhancerDoesNotChangeRefreshedAccessTokens fails: an
enabled MtlsClaimsEnhancer makes a refreshed access token gain amr,
acr and the refresh token's external attributes.
- guard: aCustomEnhancerStillReceivesRefreshTokenClaims... passes;
develop's copy for custom enhancers must stay.
Finding 3, bootstrap against a stale zone (ClientAdminBootstrapTests):
- bootstrapClientBundleReferenceIsValidatedAgainstTheSavedDefaultZone
fails with "unknown or empty mTLS trust bundle": the per-thread
IdentityZoneHolder copy predates the saved uaa.mtls.trust_bundles.
Finding 4, grant types (ClientAdminEndpointsValidatorTests):
- rejectsGrantTypesAnMtlsClientCanNeverUse (authorization_code,
password, refresh_token) and rejectsImplicitForAnMtlsClient fail:
a client with workload trust may only use client_credentials.
- guards: the same grants stay valid without mTLS.
Finding 6, mTLS-only settings without workload trust:
- registration (ClientAdminEndpointsValidatorTests): subject bindings,
required claims, claim mappings, sub/aud templates and allowed
resources without tls-client-auth-ca(-ref) are accepted today.
- runtime (MtlsTokenEndpointHardeningMockMvcTests$
StoredConfigurationFailsClosed): a stored client with them gets a
token with its secret (200) instead of 401 invalid_client.
Finding 7, stored values that cannot be read
(StoredConfigurationFailsClosed):
- an unreadable tls_client_certificate_bound_access_tokens or
tls-client-auth-allowed-resources returns 500 server_error instead
of 401 invalid_client.
Constraints for the production change: run new rules after the
existing well-formedness checks so existing error messages are kept;
keep the existing message for bound tokens without trust; no lookups on
non-mTLS requests beyond the client's own additional information.
Findings 2, 5 and 10 are documentation changes and have no failing
test. Twenty-one cases fail, all new; every other test passes.
ai-assisted=yes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The specification commit requires an mTLS client to use only client_credentials, the only grant /oauth/mtls/token issues. Six bootstrap tests built their mTLS client from createClientMap, whose default grant is authorization_code, so they contradict that rule. They test bootstrapping mTLS settings, not grant types; give their clients client_credentials. This includes the red stale-zone test from the specification commit, which would otherwise fail on the grant rule instead of the zone it pins. Without production changes the five existing tests still pass, and the stale-zone test still fails for its original reason. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Makes the tests in 9f4daf8 pass without changing them; every existing message and behaviour outside mTLS clients is kept. Finding 1: MtlsClaimsEnhancer no longer turns on the refresh-claim copy in UaaTokenServices. It only contributes at /oauth/mtls/token, which never returns a refresh token, so other clients' refreshed access tokens in an mTLS zone stay as they are without it. Custom enhancers keep develop's copy. Finding 3: ClientAdminBootstrap validates uaa.yml clients against the default zone read once from identityZoneProvisioning, as the zone bootstrap saved it, instead of a per-thread IdentityZoneHolder copy that may predate uaa.mtls.trust_bundles. It reads only when there are clients to bootstrap, and falls back to the holder without the bean. Findings 4 and 6, registration: validateMtlsClientRules, called after the existing checks on the client admin API, zone client API and bootstrap, rejects certificate-only settings (subject binding, claim mappings, templates, required claims, allowed resources) without tls-client-auth-ca(-ref), and any grant other than client_credentials for a client with workload trust. A null value counts as absent. Findings 6 and 7, runtime: ClientDetailsAuthenticationProvider refuses a stored client with certificate-only settings but no workload trust, and an mTLS client whose tls_client_certificate_bound_access_tokens or tls-client-auth-allowed-resources cannot be read, with 401 invalid_client naming the setting instead of a secret fallback or a 500 at issuance. Non-mTLS clients pay only a few map lookups. Docs: describe the new rules and correct the startup note; malformed uaa.mtls.trust_bundles prevent startup whether or not mTLS is enabled. Model, server and uaa unit and MockMvc tests pass, apart from a timing-sensitive RefreshRotationTest that passes on re-run. Two full clean test integrationTest runs were cut off by system sleep, so the integration tests have not yet run with this change. ai-assisted=yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fhanik
force-pushed
the
review/pr3792-fix
branch
from
October 16, 2026 11:41
bec01f6 to
5bcaa49
Compare
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RFC 8705 mutual-TLS client authentication (
tls_client_auth), hardenedSupersedes #3972. Adds
tls_client_authclient authentication at a dedicatedPOST /oauth/mtls/tokenendpoint, withoptional certificate-bound access tokens (
cnf.x5t#S256), per-client trust configuration, named per-zone trustbundles, Gorouter/XFCC support and optional RFC 8707 resource indicators.
Off by default, opt-in per zone. Nothing changes for a deployment that does not set
uaa.mtls.enabled: true, anda tenant zone only gets mTLS once it sets
config.mtls.enabled: true.Enablement model
uaa.mtls.enabledfalseuaa.yml, restartconfig.mtls.enabledfalseuaa.mtls.enabledon every restart.mTLS is effective in a zone only when both are on, evaluated per request. A zone cannot override a disabled
deployment, and disabling the default zone does not affect tenant zones (and vice versa). All mTLS beans and the
security chain always exist; availability is enforced at runtime (
MtlsFeaturePolicy,MtlsZoneAvailabilityFilter).What the feature does
tls-client-auth-ca(inline PEM) ortls-client-auth-ca-ref(namedbundle) authenticates only at
/oauth/mtls/token, only with a certificate, and is refused at/oauth/token./oauth/mtls/tokenin turn refuses clients without that configuration, so it is not an alias of/oauth/token.tls_client_auth_subject_dn,tls_client_auth_san_dns,…_san_uri,…_san_ip(address literal, compared in binary, never resolved through DNS) or…_san_emailisrequired. Chain validation alone would let any holder of a certificate from a shared CA (Diego instance identity)
authenticate as any client trusting it.
without downtime. Expired leaves and CA certificates used as the leaf are refused. Revocation is not checked.
uaa.mtls.trust_bundlesfor the default zone,config.mtls.trustBundlesfor otherzones. Names and contents are isolated per zone.
GET /oauth/mtls/trust-bundleslists names only.tls-client-auth-trusted-proxy-ca(-ref)makes a client proxy-only. The genuine TLS peer mustchain to that CA and match the mandatory
tls-client-auth-trusted-proxy-subject-dnbefore theX-Forwarded-Client-Certheader is trusted. Without proxy trust the header is never consulted.client_credentials, no refresh token.cnf.x5t#S256only when the client setstls_client_certificate_bound_access_tokens: true(RFC 8705 section 3.4); otherwise an ordinary bearer JWT. Claimmappings from
subject_cn/subject_ou/subject_o(bounded regex on OU only),sub/audtemplates, requiredclaims. Reserved and delegation claims (
amr,acr,cnf,act,may_act, dotted children) cannot be mapped.resourceper request, from the client'stls-client-auth-allowed-resources; anything else isinvalid_target.POSTonly (405+Allow: POSTotherwise); anything below the path is404, which closes the/oauth/mtls/token/oauth/tokenrouting bypass.tls_client_authintoken_endpoint_auth_methods_supported,mtls_endpoint_aliases.token_endpointandtls_client_certificate_bound_access_tokens: true.With mTLS off — no cost, no behaviour change
This was audited path by path for
uaa.mtls.enabled=falsewith no zone configuration.certificate parsing on non-mTLS requests. The inactive token enhancer is skipped, so token issuance takes the same
path as on develop.
ClientCertificateMapperclass is never loaded (its registration is disabled around a pass-through), and the jar registers no
auto-configuration (guarded by a test). The trust-bundle startup validator returns immediately. No Flyway migrations.
enumerates zones or clients, so it does not grow with tens of thousands of zones. Client bundle references are
checked at registration and resolved per request; an unresolvable reference fails closed at token time.
/oauth/mtls/tokenand/oauth/mtls/trust-bundlesanswer404(both are new paths).Discovery is byte-for-byte develop's document (the serialized fixture is unchanged). Zone JSON omits
config.mtlsuntil a zone configures it, and a
nullvalue reads as the default.UaaAuthenticationDetails(stored in Spring Session) gained a field; itsserialVersionUIDis pinned to develop's value, and a test deserializes an instance serialized by develop.
ClientAuthenticationbehaves exactly as on develop apart from the newTLS_CLIENT_AUTHconstant;UAA_SUPPORTED_METHODS, which governs authentication to external IdPs, does not includetls_client_auth.ClientAdminEndpointsValidator,ClientAdminBootstrapandZoneEndpointsClientDetailsValidatorkeep their developconstructors;
ClientDetailsAuthenticationProvideronly gains an overload.Compatibility notes
oauth.clientsbootstrap, whether or not mTLS is on: valid configuration is stored, malformed configuration is a400(or fails bootstrap)tls-client-auth-*/tls_client_*keystls-client-auth-ca(-ref)or proxy trust is mTLS-only: no secret fallback, even with mTLS off; a malformed ornullvalue fails closedtls_client_certificate_bound_access_tokensvalue cannot get a tokenjwt.token.claims.excludecannot removecnf(it would silently turn a bound token into a bearer token)cnf, which UAA never issued beforeUaaTokenEnhancergainsdefaultmethodsgetLateOverrideClaims()(empty set) andisEnabled()(true)ClientAdminBootstrapnow runs after the identity-zone bootstrap, so default-zone bundles exist when clients are validatedOperational notes
uaa.mtls.enabledis connector-wide: every TLS handshake gets aCertificateRequestwith no issuerlist (
certificateVerification=optionalNoCA), and the connector moves to the FIPS BouncyCastle JSSE provider(required for TLS 1.3 post-handshake client authentication). Soak-test interop and performance first.
cnfbut does not enforce it; resource servers must compare it with the certificate they see./introspectand/check_tokenreturncnffor JWT and opaque tokens./oauth/mtls/token,self_signed_tls_client_auth, and certificate binding for public clients.Where to look
ClientDetailsAuthenticationProvider,TlsClientAuthentication,TlsClientAuthSubjectMatcher,OuClaimPattern,UaaAuthenticationDetailsUaaTokenEndpoint,MtlsEndpointAvailabilityFilter,MtlsZoneAvailabilityFilter,RawPeerCertificateCaptureFilter,MtlsPathGuardedFilter,SpringServletXmlFiltersConfiguration,SpringServletXmlSecurityConfiguration,OauthEndpointSecurityConfiguration,FilterChainOrderMtlsClaimsEnhancer,UaaTokenServices,UaaTokenEnhancer,ClientCredentialsTokenGranterMtlsFeaturePolicy,MtlsConfig,IdentityZoneConfiguration,IdentityZoneConfigurationBootstrap,GeneralIdentityZoneConfigurationValidator,MtlsTrustBundles,MtlsTrustBundleEndpoints,MtlsTrustBundleStartupValidator,ClientAdminSecurityConfigurationClientAdminEndpointsValidator,ZoneEndpointsClientDetailsValidator,ClientAdminBootstrap,TlsClientAuthConfiguration,UaaClientDetailsMtlsClientAuthTomcatCustomizer,BCJSSEUtil,BCJSSESSLContext,BCJSSESslImplementation,NoAcceptedIssuersTrustManagerOpenIdConnectEndpoints,OpenIdConfiguration,ClientAuthentication,TokenConstants,OAuth2Exception,InvalidTargetExceptionjava-buildpack-client-certificate-mapper-jakarta2.0.1 andspring-boot-tomcatinlibs.versions.toml/server/build.gradle.kts;bouncyCastlePkixFipsfor testsTests
About 450 new test methods: 24 new test classes (MockMvc suites for hardening, zone paths, flag-off behaviour and flag
spelling, a real embedded-Tomcat TLS integration test, and unit tests for every new class) plus 124 tests added to 25
existing classes. Both directions of the zone policy are covered (default zone off with a tenant on, and the reverse).
No pre-existing test was deleted or disabled. Changes to existing assertions:
UaaClientDetailsTestassertshashCode()is non-zero rather than positive;TokenEndpointDocsruns withuaa.mtls.enabled=truefor its mTLSexample;
OpenIdConnectEndpointDocsdocuments the two optional discovery fields. The rest are constructor and wiringupdates.
./gradlew clean test integrationTestpasses.Documentation
docs/UAA-Client-Authentication.md,docs/UAA-Configuration-Reference.md(uaa.mtls.enabled,uaa.mtls.trust_bundles), the newdocs/UAA-mTLS-Getting-Started.md, the generated API docs (token endpoint,discovery, introspection and check-token
cnf, client registration keys, identity-zoneconfig.mtls, trust-bundlenames) and the slate source.
🤖 Generated with Claude Code