Import all PEM certificates with BCFIPS by Josh-Tracy · Pull Request #53613 · keycloak/keycloak

Josh-Tracy · 2026-10-06T13:33:48Z

Summary

With BCFIPS, CertificateFactory.generateCertificate() can consume an entire multi-certificate PEM file when given a non-markable FileInputStream. TruststoreBuilder then imports only the first certificate without reporting an error.

This change wraps the input in BufferedInputStream, allowing the existing import loop to read each certificate. It also adds a regression test that loads two distinct certificates from one PEM file and verifies both appear in the generated BCFKS truststore in BCFIPS approved-only mode.

Testing

TruststoreBuilderTest: 12 passed on JDK 21.
FIPS1402KeystoreTypesTest: new regression test ran and passed with BCFIPS approved-only mode on JDK 21; two tests for non-approved mode were skipped as expected.
spotless:check passed.

The BCFIPS test ran on Ubuntu with FIPS settings for the test JVM, not on an OS-level FIPS-certified host. End-to-end client-certificate authentication was not tested.

Deployment validation

We also tested the change in a controlled comparison on a running deployment using a downstream Keycloak 26.7.3-based FIPS container image on kubernetes running in HA mode. Both replicas used the same mounted multi-certificate PEM bundle and BCFIPS provider; only one replica had the buffered-stream change documented here.

A provider-level probe imported 1 of 177 certificate blocks with FileInputStream and all 177 with BufferedInputStream. The original replica's generated BCFKS truststore omitted 29 distinct certificates from the bundle; the patched replica omitted none and retained all entries present in the original truststore. Both replicas became Ready.

Documentation

No documentation change: this fixes certificate import for the existing --truststore-paths behavior.

AI assistance

An AI coding agent assisted with the implementation and regression test. I reviewed the change and am responsible for addressing review feedback.

Copilot

Copilot review overview

🟢 Approval recommended

The focused implementation addresses the provider-specific stream behavior and includes appropriate regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes multi-certificate PEM imports when using BCFIPS.

Changes:

Wraps PEM file streams in BufferedInputStream.
Adds an approved-only BCFIPS regression test importing two certificates.

File	Description
`services/src/main/java/org/keycloak/truststore/TruststoreBuilder.java`	Enables BCFIPS to process each PEM certificate.
`crypto/fips1402/src/test/java/org/keycloak/crypto/fips/test/FIPS1402KeystoreTypesTest.java`	Verifies both certificates reach a BCFKS truststore.

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

Copilot

Copilot review overview

🟢 Approval recommended

The focused implementation addresses the provider-specific behavior and includes appropriate regression coverage.

Review effort: Balanced
Findings: None

slaskawi · 2026-10-07T05:42:45Z

Hey @shawkins 👋🏻

Could I kindly ask you to have a look at this one?

rmartinc · 2026-10-08T06:42:19Z

I detected this independently and created a draft PR in #53706. WDYT about using generateCertificates instead of generateCertificate and the loop? Besides in my PR I changed to force the BC provider instead of the default java one, this way BC is always managed for this and reduce the difference between FIPS and non-FIPS.

slaskawi · 2026-10-08T07:41:11Z

Thanks for checking this one @rmartinc! We highly appreciate your time!

Besides in my PR I changed to force the BC provider instead of the default java one, this way BC is always managed for this and reduce the difference between FIPS and non-FIPS.

I'm not sure this is necessary. A while back I've implemented using BCFKS for Strict FIPS mode (see #50149). Using PKCS12 for non-strict should probably be allowed as it's not strictly forbidden by the FIPS spec. In order to guarantee that nothing slips in in FIPS mode, my change uses BCFKS.

With the above in mind, I think we should never end up in this codepath when FIPS Strict mode is enabled (unless I missed something).

rmartinc · 2026-10-08T08:10:59Z

@slaskawi My change was only to use X.509 implementation in the BC (not the java one). But whatever you prefer, and what about using generateCertificates instead of the loop of generateCertificate? Do you think that it's better?

slaskawi · 2026-10-08T11:18:02Z

My change was only to use X.509 implementation in the BC (not the java one). But whatever you prefer

@rmartinc I've been experimenting on the side with alternative Crypto Providers (GlaSSLess in #51082 and Project Brisbane in #53711). This is being coordinated with @stianst and we're looking at how complicated it is to support FIPS without Bouncy Castle.

If any of this experiments take off, we might be interested in lowering Keycloak dependency on Bouncy Castle. But to be fair - we're quite far away from that point.

With the above in mind, my personal preference (but feel free to ignore me) is to limit code dependencies to BC if possible. I think in this particular piece of code, we might get away with just sticking to the Java implementation.

But let me also ask you - what benefits does it bring if we switched to CertificateFactory certFactory = CertificateFactory.getInstance("X509", CryptoIntegration.getProvider().getBouncyCastleProvider());? Maybe there's a good argument for doing that?

, and what about using generateCertificates instead of the loop of generateCertificate? Do you think that it's better?

Switching to generateCertificates make perfect sense to me. It better describes the intention I think.

Josh-Tracy · 2026-10-08T13:24:30Z

Switching to generateCertificates make perfect sense to me. It better describes the intention I think.

I agree.

rmartinc · 2026-10-08T13:52:08Z

@Josh-Tracy Then just change to use generateCertificates, we will keep using the default X509 factory (which is default java in non-fips and BCFIPS in fips mode). Regarding the test, don't know what to say, there is no easy way of testing this except a direct test, but I suppose now generateCertifiactes should work OK (if you want just skip the specific test for fips mode).

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

rmartinc

Thanks @Josh-Tracy! LGTM! Just please add the signature to the second commit (git commit --amend --signoff) to pass the DCO check. Later we can run the CI with this.

slaskawi

Looks good from my side as well!

Josh-Tracy · 2026-10-09T13:11:30Z

Thanks @Josh-Tracy! LGTM! Just please add the signature to the second commit (git commit --amend --signoff) to pass the DCO check. Later we can run the CI with this.

Signed :)

slaskawi

Approved from my side

rmartinc

Thanks @Josh-Tracy! Approved! I have started the CI.

@shawkins Do you want to also take a look?

shawkins

LGTM, thanks @Josh-Tracy and @rmartinc

rmartinc · 2026-10-13T14:38:52Z

@Josh-Tracy I'm going to rebase, the CI issue is because #53856. So it should be fixed with the rebase.

Closes keycloak#53612 Signed-off-by: josh-tracy <josh.tracy@defenseunicorns.com>

Signed-off-by: josh-tracy <josh.tracy@defenseunicorns.com>

Copilot

🟢 Approval recommended

The focused implementation correctly addresses multi-certificate parsing and includes appropriate regression coverage.

0 open findings

🧠 Review effort: Balanced

* Import all PEM certificates with BCFIPS Closes keycloak#53612 Signed-off-by: josh-tracy <josh.tracy@defenseunicorns.com> * Import all certificates from PEM bundles Signed-off-by: josh-tracy <josh.tracy@defenseunicorns.com> --------- Signed-off-by: josh-tracy <josh.tracy@defenseunicorns.com>

Josh-Tracy requested a review from a team as a code owner October 6, 2026 13:33

Copilot AI balanced review requested due to automatic review settings October 6, 2026 13:33

Copilot started reviewing on behalf of Josh-Tracy October 6, 2026 13:34

View session

Josh-Tracy force-pushed the fix/bcfips-multi-pem-truststore branch from c707e2a to 038076b Compare October 6, 2026 13:34

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 6, 2026 13:35

Copilot started reviewing on behalf of Josh-Tracy October 6, 2026 13:36

View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Josh-Tracy mentioned this pull request Oct 6, 2026

BCFIPS imports only the first certificate from a multi-certificate PEM trust bundle #53612

Closed

1 of 2 tasks

slaskawi approved these changes Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 8, 2026 14:55

Copilot AI reviewed Oct 8, 2026

rmartinc requested changes Oct 9, 2026

View reviewed changes

slaskawi approved these changes Oct 9, 2026

View reviewed changes

Josh-Tracy force-pushed the fix/bcfips-multi-pem-truststore branch from 2a0dd96 to 2087b30 Compare October 9, 2026 13:11

Josh-Tracy requested a review from rmartinc October 9, 2026 13:11

slaskawi approved these changes Oct 12, 2026

View reviewed changes

rmartinc approved these changes Oct 13, 2026

View reviewed changes

rmartinc requested a review from shawkins October 13, 2026 11:20

shawkins approved these changes Oct 13, 2026

View reviewed changes

Import all PEM certificates with BCFIPS

aea7833

Closes keycloak#53612 Signed-off-by: josh-tracy <josh.tracy@defenseunicorns.com>

Import all certificates from PEM bundles

e903054

Signed-off-by: josh-tracy <josh.tracy@defenseunicorns.com>

Copilot AI balanced review requested due to automatic review settings October 13, 2026 14:39

rmartinc force-pushed the fix/bcfips-multi-pem-truststore branch from 2087b30 to e903054 Compare October 13, 2026 14:39

Copilot started reviewing on behalf of Josh-Tracy October 13, 2026 14:40

View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

rmartinc merged commit 46b9e88 into keycloak:main Oct 13, 2026
97 of 168 checks passed

rmartinc mentioned this pull request Oct 14, 2026

Import all PEM certificates with BCFIPS (26.8) #53942

Merged

Conversation

Josh-Tracy commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Deployment validation

Documentation

AI assistance

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

slaskawi commented Oct 7, 2026

Uh oh!

rmartinc commented Oct 8, 2026

Uh oh!

slaskawi commented Oct 8, 2026

Uh oh!

rmartinc commented Oct 8, 2026

Uh oh!

slaskawi commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Josh-Tracy commented Oct 8, 2026

Uh oh!

rmartinc commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

rmartinc left a comment

Choose a reason for hiding this comment

Uh oh!

slaskawi left a comment

Choose a reason for hiding this comment

Uh oh!

Josh-Tracy commented Oct 9, 2026

Uh oh!

slaskawi left a comment

Choose a reason for hiding this comment

Uh oh!

rmartinc left a comment

Choose a reason for hiding this comment

Uh oh!

shawkins left a comment

Choose a reason for hiding this comment

Uh oh!

rmartinc commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Josh-Tracy commented Oct 6, 2026 •

edited

Loading

slaskawi commented Oct 8, 2026 •

edited

Loading

rmartinc commented Oct 8, 2026 •

edited

Loading

rmartinc commented Oct 13, 2026 •

edited

Loading