Skip to content

Allow per-IdP caCertificates entries to hold a full certificate chain - #4064

Open
duanemay wants to merge 3 commits into
developfrom
fix/ca-certificate-chain-parsing
Open

Allow per-IdP caCertificates entries to hold a full certificate chain#4064
duanemay wants to merge 3 commits into
developfrom
fix/ca-certificate-chain-parsing

Conversation

@duanemay

@duanemay duanemay commented Sep 3, 2026

Copy link
Copy Markdown
Member

Summary

  • caCertificates list entries (SAML, LDAP, and OAuth2/OIDC identity providers) were previously parsed as exactly one certificate each; pasting a full concatenated chain (leaf, intermediate, root) into a single entry silently dropped everything after the first certificate.
  • PemCertificateParser now extracts every certificate found in an entry, so an entry may hold a single certificate or a full chain, while separate list entries still work for supplying independent trust anchors (e.g. during a CA rotation).
  • IdpOutboundTrustCache.buildMergedTrustManager now wraps a malformed-certificate IllegalArgumentException into IllegalStateException, consistent with its other failure modes, so a stored config that fails to parse degrades cleanly instead of an unhandled exception reaching request-handling code.
  • Updated the REST API docs and UAA-Configuration-Reference.md (OAuth/OIDC, SAML, and ldap.ssl.caCertificates) to describe the new behavior.

Test plan

  • PemCertificateParserTest — single cert, concatenated chain, malformed chain, mixed single/chain list entries
  • ExternalOAuthIdentityProviderConfigValidatorTest, LdapIdentityProviderConfigValidatorTest, IdentityProviderConfigValidationDelegatorTest — chain-in-one-entry accepted
  • IdpOutboundTrustCacheTest — malformed certificate now throws IllegalStateException

PemCertificateParser previously read only the first PEM object from
each caCertificates string, silently discarding the rest if a caller
concatenated a full chain (leaf, intermediate, root) into one entry.
Parse every certificate found in a string instead, so an entry may
hold a single certificate or a full chain while still supporting
multiple independent trust anchors across separate list entries (e.g.
during a CA rotation). Applies uniformly to SAML, LDAP, and OAuth2/OIDC
identity providers since they share the same parsing/validation path.
Document why parseCertificateChain rethrows IllegalArgumentException as-is
instead of falling through to the generic wrapping catch. Also have
buildMergedTrustManager catch IllegalArgumentException from the parser and
wrap it the same way as its other build failures, so a pre-existing stored
caCertificates config that fails to parse degrades to a clear
IllegalStateException instead of an unhandled exception reaching
request-handling code.
Copilot AI lite review requested due to automatic review settings September 3, 2026 18:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Only a minor test-name mismatch was identified; functional changes, docs, and test coverage appear consistent with the stated intent.

Pull request overview

This PR updates UAA’s handling of per-IdP caCertificates so that a single list entry can contain either one PEM certificate or multiple concatenated PEM certificates (a chain), avoiding silent truncation and improving resilience when stored configs are malformed.

Changes:

  • Enhanced PemCertificateParser to parse and return all certificates found in a single PEM string, and to flatten multi-cert entries when parsing List<String>.
  • Updated outbound TLS trust construction to wrap malformed certificate parsing failures in IllegalStateException for consistent degradation.
  • Updated REST Docs and configuration reference documentation, plus added/expanded tests covering concatenated chains and malformed inputs.
File summaries
File Description
uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointDocs.java Updates REST-doc field description for config.caCertificates to document chain-in-one-entry behavior.
server/src/test/java/org/cloudfoundry/identity/uaa/util/PemCertificateParserTest.java Adds tests for parseCertificateChain and mixed single/chain list entry parsing.
server/src/test/java/org/cloudfoundry/identity/uaa/security/IdpOutboundTrustCacheTest.java Adds assertion that malformed cert input now results in IllegalStateException.
server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthIdentityProviderConfigValidatorTest.java Verifies OAuth IdP validator accepts concatenated PEM chains in one entry.
server/src/test/java/org/cloudfoundry/identity/uaa/provider/ldap/LdapIdentityProviderConfigValidatorTest.java Verifies LDAP IdP validator accepts concatenated PEM chains and simplifies validate calls.
server/src/test/java/org/cloudfoundry/identity/uaa/provider/IdentityProviderConfigValidationDelegatorTest.java Verifies SAML IdP config validation accepts concatenated PEM chains.
server/src/main/java/org/cloudfoundry/identity/uaa/util/PemCertificateParser.java Implements chain parsing and list-entry flattening behavior.
server/src/main/java/org/cloudfoundry/identity/uaa/security/IdpOutboundTrustCache.java Wraps malformed-certificate IllegalArgumentException as IllegalStateException to match other failure modes.
docs/UAA-Configuration-Reference.md Documents that each caCertificates entry may contain a single cert or concatenated chain (OAuth/OIDC, SAML, LDAP).
Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-project-automation github-project-automation Bot moved this from Inbox to Pending Merge | Prioritized in Foundational Infrastructure Working Group Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending Merge | Prioritized

Development

Successfully merging this pull request may close these issues.

3 participants