Review of 3972

[fhanik]

Review notes: RFC 8705 mTLS client authentication

I spent some time on this branch and wrote a set of adversarial MockMvc tests against /oauth/mtls/token to check the boundaries rather than reason about them. Everything below is something the tests actually produced, unless marked (from reading).

The trust model is carefully built — the XFCC-spoofing defenses in particular are better than I expected, and the RawPeerCertificateCaptureFilter / mapper-parse-failure guard is a real subtlety that's handled correctly. The findings below are mostly about the blast radius of the new endpoint rather than the certificate validation itself.

What I verified as working

Worth stating up front, because these are the parts under the most load:

• A certificate that doesn't chain to the client's tls-client-auth-ca is rejected cleanly: 401 invalid_client, "tls_client_auth: certificate chain validation failed: Path does not chain with any of the trust anchors". Same for expired certs and a malformed CA PEM.
• Claim mappings cannot forge scope, client_id or zid — NON_ADDITIONAL_ROOT_CLAIMS in UaaTokenServices does its job.
• tls-client-auth-required-claims correctly rejects a valid certificate from the same CA in the wrong space. The shared-CA mitigation the docs describe holds.
• An mTLS-configured client can't fall back to its client_secret, can't mix cert+secret on one request, and a CA certificate presented as the leaf is rejected by validateEndEntityConstraints.

1. The mTLS endpoint serves every grant type, including password and refresh_token

POST /oauth/mtls/token with grant_type=password, authenticating with a valid instance-identity certificate, returns 200. Decoded access token:

{ "client_auth_method": "tls_client_auth",
  "cnf": { "x5t#S256": "YMmciXQkCQXoo0RJtoqsVM6pH39epZebkh6YnfNVHhk" },
  "app_id": "b1-app",
  "grant_type": "password",
  "user_id": "d78bfe26-78ca-4400-b44f-15243c9b2e3d",
  "user_name": "...", "email": "...", "origin": "uaa",
  "scope": ["uaa.user"] }

The refresh token it returns is then usable at the same endpoint (200, "revocable": true, amr: ["pwd"]).

The concern isn't that the client had password in its authorized_grant_types — it's what the resulting token claims. cnf.x5t#S256 is RFC 8705 §3 sender-constraint: a resource server reading it concludes the presenter holds that certificate. The same token also carries a full user identity. So a Diego app instance certificate ends up certificate-binding a user's token, and app_id (certificate-derived) sits beside user_id (password-derived) with nothing marking their provenance.

mtlsTokenEndpointSecurity installs BackwardsCompatibleTokenEndpointAuthenticationFilter exactly as tokenEndpointSecurity does, so every grant is reachable. Was that deliberate? If the endpoint is meant for workload identity, restricting it to client_credentials (and maybe jwt-bearer) would match the feature's purpose, and would keep cnf meaning what RFC 8705 says it means.

2. The same validation failure returns 500 with a stack trace, depending on which client-auth filter runs

Authorization: Basic base64("clientId:") — empty secret, which still satisfies ObjectUtils.isEmpty(authentication.getCredentials()) in ClientDetailsAuthenticationProvider. ClientParametersAuthenticationFilter.wrapClientCredentialLogin skips whenever an Authorization header is present, so this is handled by ClientBasicAuthenticationFilter, which catches only AuthenticationException:

"http.path":"/oauth/mtls/token","http.status_code":"500"

Uncaught Exception:
error="invalid_client", error_description="tls_client_auth: certificate chain validation failed: ..."
	at TlsClientAuthentication.validateClientCert(TlsClientAuthentication.java:343)
	at ClientDetailsAuthenticationProvider.validateTlsClientAuth(ClientDetailsAuthenticationProvider.java:217)
	at ClientDetailsAuthenticationProvider.additionalAuthenticationChecks(ClientDetailsAuthenticationProvider.java:95)

Identical credentials and certificate as the 401 case above — only the filter differs. The clean 401 comes from AbstractClientParametersAuthenticationFilter.performClientAuthentication's catch (Exception e) -> BadCredentialsException, which is incidental rather than by design.

InvalidClientDetailsException is a UaaException → OAuth2Exception → RuntimeException, not a Spring AuthenticationException, so it escapes the security chain to SecurityFilterChainPostProcessor's UaaLoggingFilter, which logs ERROR "Uncaught Exception:" and sends 500. That's an unauthenticated endpoint writing a stack trace per attempt.

Smallest fix is in validateTlsClientAuth: catch and convert rather than depending on a downstream filter's catch-all.

3. /oauth/mtls/token is an unrestricted alias of /oauth/token

A client with no tls-client-auth-ca at all, authenticating with client_secret_basic and presenting no certificate, gets a normal token from /oauth/mtls/token (200). Since mtls_endpoint_aliases.token_endpoint now advertises this path in discovery, it's advertising an endpoint that accepts non-mTLS authentication.

4. tls-client-auth-sub-template with no placeholders forges an arbitrary sub

MtlsClaimsEnhancer.renderTemplate returns a template containing no {placeholder} verbatim, and UaaTokenServices.createJWTAccessToken re-applies sub after its own defaults. ClientAdminEndpointsValidator.validateTemplatePlaceholders only checks that placeholders that are present are declared — it never requires one.

Configuring tls-client-auth-sub-template: "00000000-0000-0000-0000-000000000000" produced a token with exactly that sub. It needs clients.write, so it's client-admin → subject-spoofing rather than an anonymous hole, but sub is the canonical identity claim for every downstream resource server, and user_id/cid are protected while sub deliberately is not. Requiring at least one placeholder would close it.

5. Claim mappings can set amr and acr

"amr": "mfa", "acr": "urn:example:high"

NON_ADDITIONAL_ROOT_CLAIMS protects UAA's own claims but not the authentication-context claims that downstream policy engines read, and the validator applies no allowlist to claim names. An allowlist (or a reserved-name denylist) at config time would be better than relying on the token-builder denylist.

6. With uaa.mtls-enabled=false, the endpoint still resolves

The security chain is @ConditionalOnProperty, but UaaTokenEndpoint's @RequestMapping lists /oauth/mtls/token unconditionally and both new servlet filters are registered unconditionally. With the feature off, a POST returns:

{"error":"Could not verify the provided CSRF token because no token was found to compare."}

That's the uiSecurity catch-all's CSRF filter — so the request reaches the browser login chain rather than 404ing. It fails closed here, but by accident. A disabled feature's endpoint should not resolve.

Related (from reading): clientCertificateMapperFilter() reflectively instantiates a package-private third-party class at startup unconditionally and throws IllegalStateException if it's absent — a hard startup failure for a feature that's off.

7. token-endpoint-auth-method rejection breaks unrelated clients

ClientAdminEndpointsValidator.checkMtlsClientConfigAllowed throws on the mere presence of that key in additionalInformation, for every client create/update, mTLS or not. Creating a plain client carrying it returns 400. That string appears nowhere else in UAA, but hyphenated unknown keys from a BOSH oauth.clients manifest land in additionalInformation, so deployments may already have clients carrying it — previously ignored, now uncreatable and unupdatable. Is this a leftover from an earlier iteration?

Smaller things, from reading rather than testing

• MtlsClientAuthTomcatCustomizer.customize iterates all connectors and all SSLHostConfigs with no port scoping, so enabling the feature makes UAA send a CertificateRequest on every TLS connection including the login page — browsers holding a client cert will prompt end users — and moves the whole connector to BCJSSE in FIPS mode. Worth scoping to a dedicated connector, or documenting loudly.
• ClientDetailsAuthenticationProvider.getTlsClientAuthConfiguration and MtlsClaimsEnhancer.loadTlsConfig are ~60 lines of identical parsing in different modules. One decides authentication, the other decides what identity claims go in the token; if they drift, those two disagree. A single static factory on TlsClientAuthConfiguration would be safer.
• parsePemCertificate does one parser.readObject(), so an operator pasting a root+intermediate bundle into tls-client-auth-ca silently gets only the first certificate as trust anchor.
• setRevocationEnabled(false) is reasonable for short-lived instance certs but deserves a line in the docs, since the same config accepts arbitrary long-lived certificates.
• The new endpoint inherits allowQueryStringForTokens (default true), so GET /oauth/mtls/token?grant_type=... works.
• ClientCredentialsTokenGranter.isAllowedAuthMethod is public static and called only from a test.

Offer

I have the MockMvc tests that produced all of the above — roughly 17 tests covering credential confusion, grant types at the endpoint, validation-failure semantics, and claim-mapping reach, using real BouncyCastle-generated certificates rather than mocks. Happy to push them to the branch if useful; several of them are worth keeping as regression coverage regardless of what you decide about the findings, particularly the wrong-CA case and the protected-claim guard, which aren't currently covered.

[fhanik]

Example fixes here #4076

At this point, I'm not comfortable merging this because it wasn't a pure TDD pattern. Tests were changed along with production code, making it more difficult to review.

[fhanik]

Superseded by #4076

[rkoster]

Pushed 14 follow-up commits to #3972, from 471ad57 through a80e1c9, addressing this review and the example fixes in #4076.

The mTLS endpoint serves every grant type, including password and refresh_token

38357eb restricts the endpoint to client_credentials. This is the workload-identity endpoint policy; RFC 8705 itself permits certificate-bound user tokens.

The same validation failure returns 500 with a stack trace, depending on which client-auth filter runs

433728d converts certificate failures to authentication exceptions, yielding consistent 401 / invalid_client responses for Basic and parameter authentication.

/oauth/mtls/token is an unrestricted alias of /oauth/token

e4ca968 requires mTLS client configuration at the dedicated endpoint, including descendants and zone-prefixed paths.

tls-client-auth-sub-template with no placeholders forges an arbitrary sub

343cf61 requires a declared placeholder in nonblank subject templates. Invalid constant/oversized subjects reaching token generation fail issuance. Literal audiences remain supported for federation. Placeholder presence does not guarantee subject uniqueness.

Claim mappings can set amr and acr

8741802 protects reserved root claims at configuration and enhancement time, including dotted mappings such as acr.level.

With uaa.mtls-enabled=false, the endpoint still resolves

6f3988e explicitly returns 404 before browser security. cb5fbd8 avoids loading or registering the mapper when disabled, verified with the mapper class unavailable.

token-endpoint-auth-method rejection breaks unrelated clients

b3c51f6 restores acceptance as inert metadata; it cannot select or bypass mTLS authentication.

Smaller things, from reading rather than testing

• 09f1992 consolidates configuration parsing into one factory.
• ede683e parses every certificate in CA trust bundles, supporting old/new overlap rotation for workload and proxy CAs. Malformed PEM entries reject the entire bundle; every entry is an anchor.
• 1cc3ed4 documents disabled CRL/OCSP checks.
• aa075f5 makes mTLS POST-only, returning 405 with Allow: POST for authenticated GET requests.
• 067fac0 removes the test-only helper and verifies actual granter behavior.
• a80e1c9 distinguishes missing certificates from required-claims failures without exposing claim values.
• Connector-wide TLS effects are explicitly documented in both references.

Tests were changed along with production code, making it more difficult to review.

The follow-ups are split by finding. Behavioral fixes were exercised with failing regressions before implementation; refactors used tests run before and after extraction/removal. Tests and production changes are committed together, so history is not a separate red/green sequence. Existing test changes reflect the explicit policies above and new diagnostic messages; positive issuance, ordinary-endpoint, literal-audience, and rotation coverage remains.

Targeted suites passed at each step; the latest run passed 17 provider tests, 59 HTTP regressions, and the mTLS REST Docs test. Affected API snippets were regenerated. Full repository/deployment testing was not performed in this round. Full HTML generation remains blocked locally by missing Ruby/gem; pre-existing Markdownlint findings remain.