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.