Truststore consolidation broke keycloak fips mode

danweller18 commented 5 months ago

Before reporting an issue

[X] I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.

Area

authentication

Describe the bug

The new truststore in keycloak v24 hardcodes a PKCS12 type for the truststore. It appears to try to merge any supplied truststores with the hardcoded PKCS12 type truststore. This causes an error because you cannot merge a type bcfks fips compatible truststore with a type pkcs12 truststore.

The instructions for using keycloak in fips mode require setting --features=fips --fips-mode=strict to be fips compliant

The next section says "strict mode may not work with pkcs12 keystore. It is required to use another keystore (like bcfks) as mentioned earlier. Also jks and pkcs12 keystores are not supported in Keycloak when using strict mode."

It is very clear that fips mode is not going to work as is. Keycloak should not be hardcoding a type PKCS12 truststore

Version

24.0.2

Regression

[ ] The issue is a regression

Expected behavior

I would expect the consolidated single truststore from the migration of v23 -> v24 to take in a configured truststore, truststore password and truststore type as arguments. I would also expect keycloak to support different types of truststores- at minimum PKCS12 & BCFKS (in fips mode)

Actual behavior

When I try to start up keycloak, it fails to startup and throws the following error.

Error log

ERROR: Unexpected error when starting the server in (production) mode
Error details:
java.lang.RuntimeException: Failed to start quarkus
    at io.quarkus.runner.ApplicationImpl.<clinit>(Unknown Source)
    at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
    at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
    at io.quarkus.runtime.Quarkus.run(Quarkus.java:70)
    at org.keycloak.quarkus.runtime.KeycloakMain.start(KeycloakMain.java:117)
    at org.keycloak.quarkus.runtime.cli.command.AbstractStartCommand.run(AbstractStartCommand.java:33)
    at picocli.CommandLine.executeUserObject(CommandLine.java:2026)
    at picocli.CommandLine.access$1500(CommandLine.java:148)
    at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2461)
    at picocli.CommandLine$RunLast.handle(CommandLine.java:2453)
    at picocli.CommandLine$RunLast.handle(CommandLine.java:2415)
    at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2273)
    at picocli.CommandLine$RunLast.execute(CommandLine.java:2417)
    at picocli.CommandLine.execute(CommandLine.java:2170)
    at org.keycloak.quarkus.runtime.cli.Picocli.parseAndRun(Picocli.java:125)
    at org.keycloak.quarkus.runtime.KeycloakMain.main(KeycloakMain.java:107)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at io.quarkus.bootstrap.runner.QuarkusEntryPoint.doRun(QuarkusEntryPoint.java:62)
    at io.quarkus.bootstrap.runner.QuarkusEntryPoint.main(QuarkusEntryPoint.java:33)
Caused by: java.lang.RuntimeException: Failed to initialize truststore, could not merge: /opt/keycloak/conf/truststores/truststore.bcfks
    at org.keycloak.truststore.TruststoreBuilder.mergePemFile(TruststoreBuilder.java:226)
    at org.keycloak.truststore.TruststoreBuilder.mergeFiles(TruststoreBuilder.java:110)
    at org.keycloak.truststore.TruststoreBuilder.createMergedTruststore(TruststoreBuilder.java:90)
    at org.keycloak.truststore.TruststoreBuilder.setSystemTruststore(TruststoreBuilder.java:52)
    at org.keycloak.quarkus.runtime.KeycloakRecorder.configureTruststore(KeycloakRecorder.java:90)
    at io.quarkus.deployment.steps.KeycloakProcessor$configureTruststore2112467525.deploy_0(Unknown Source)
    at io.quarkus.deployment.steps.KeycloakProcessor$configureTruststore2112467525.deploy(Unknown Source)
    ... 25 more
Caused by: java.security.cert.CertificateException: sequence wrong size for a certificate
    at org.bouncycastle.jcajce.provider.CertificateFactory.readCertificate(Unknown Source)
    at org.bouncycastle.jcajce.provider.CertificateFactory.engineGenerateCertificate(Unknown Source)
    at java.base/java.security.cert.CertificateFactory.generateCertificate(CertificateFactory.java:355)
    at org.keycloak.truststore.TruststoreBuilder.mergePemFile(TruststoreBuilder.java:202)
    ... 31 more
Caused by: java.lang.IllegalArgumentException: sequence wrong size for a certificate
    at org.bouncycastle.asn1.x509.Certificate.<init>(Unknown Source)
    at org.bouncycastle.asn1.x509.Certificate.getInstance(Unknown Source)
    at org.bouncycastle.jcajce.provider.CertificateFactory.readDERCertificate(Unknown Source)
    ... 35 more

I also tried setting a combination of the following with similar error results -> all erroring trying to merge the supplied truststore type of bcfks into the default pkcs12 type

 --truststore-paths=/opt/keycloak/conf/truststores/truststore.bcfks
or
setting a combo of
-Djavax.net.ssl.trustStore=/opt/keycloak/conf/truststores/truststore.bcfks
-Djavax.net.ssl.trustStorePassword=<password>
-Djavax.net.ssl.trustStoreType=bcfks

How to Reproduce?

Create a bcfks keystore & truststore following the fips guide
Build keycloak using the --features=fips --fips-mode=strict setting during the kc.sh build cmd
Try to startup a keycloak v24+ using the generated bcfks type keystore and truststore. You may have to set the truststore you are trying to use to be --truststore-paths=/opt/keycloak/conf/truststores/truststore.bcfks

Anything else?

In keycloak v23, fips was working with the following kc.sh start cmd arguments

  --https-trust-store-file=/opt/keycloak/truststore.bcfks
  --https-trust-store-type=bcfks
  --spi-truststore-file-file=/opt/keycloak/truststore.bcfks
  --spi-truststore-file-password=<password>
  --spi-truststore-file-hostname-verification-policy=<filled_in>

mabartos commented 5 months ago

@shawkins I think it's related to changes around truststore you've made, right?

shawkins commented 5 months ago

In keycloak v23, fips was working with the following kc.sh start cmd arguments

To make sure, is this what you are referring to as a regession? Is the set full set of properties you were using - a password for the spi-truststore-file, but not for the same truststore as the https-trust-store-file? And that it no longer works on KC24?

This causes an error because you cannot merge a type bcfks fips compatible truststore with a type pkcs12 truststore.

All the merging is doing is moving certs from one truststore to another. As long as you can load / save, the truststores can be merged.

It is very clear that fips mode is not going to work as is. Keycloak should not be hardcoding a type PKCS12 truststore

That language in the docs unfortunately should specify keys - it does not apply to certs.

Here's our integration test showing using a password-less / unencrypted pkcs12 truststore in fips strict mode: https://github.com/keycloak/keycloak/blob/3fffc5182ebf93225b3da40fce5d945e04155cdf/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/FipsDistTest.java#L95

Try to startup a keycloak v24+ using the generated bcfks type keystore and truststore. You may have to set the truststore you are trying to use to be --truststore-paths=/opt/keycloak/conf/truststores/truststore.bcfks

That is not expected to work. Truststore-paths works with pem encoded files or unencrypted pkcs12 files.

danweller18 commented 5 months ago

To make sure, is this what you are referring to as a regession? Is the set full set of properties you were using - a password for the spi-truststore-file, but not for the same truststore as the https-trust-store-file? And that it no longer works on KC24?

I marked the regression box because keycloak in fips mode with truststore type bcfks worked in keycloak v23 however it does not work in keycloak v24 according to the keycloak documentation & documentation on how to use keycloak with fips mode. My understanding is the 2 truststores now are a single truststore- as described in the release/migration notes. So I was saying the not working as the docs describe part is a regression because it was working in previous major release versions.

All the merging is doing is moving certs from one truststore to another. As long as you can load / save, the truststores can be merged.

Awesome, this sounds like it should work as is then. However I can't figure out how to configure keycloak to use my custom bcfks file. Feels like at minimum we need to update the documentation. It does not sound like I can use --truststore-paths, because you said "Truststore-paths works with pem encoded files or unencrypted pkcs12 files". If it does work right now, how would you suggest to configure the bcfks truststore in fips-mode=strict?

Should I use --https-trust-store-file=<file>.bcfks just like the test you linked? Even thought the docs say using that argument is deprecated?

shawkins commented 5 months ago

However I can't figure out how to configure keycloak to use my custom bcfks file. Feels like at minimum we need to update the documentation.

The docs and help both mention the file types accepted by --truststore-paths. Your bcfks is not expected to work with this option.

Should I use --https-trust-store-file=.bcfks just like the test you linked? Even thought the docs say using that argument is deprecated?

You may continue to use the deprecated options for at least another major release.

The alternative is to use just pem files or a password-less pkcs12 - see also this discussion #28007

rmartinc commented 5 months ago

My feeling is that the idea of the truststore not having a password is incompatible with using a bcfks truststore. Besides it is documented that only pkcs12 and pem file are expected. You can use pem files as @shawkins commented. I don't think this is a regression because it affects the new truststore that cannot be used before.

danweller18 commented 4 months ago

@shawkins I was able to use the same deprecated truststore options in keycloak v24. To be clear, is the plan to have a solution in place for fips bcfks truststores before removing the deprecated truststore options in future major releases?

From reading the documentation it was not clear that the new truststore options were never intended to work with fips bcfks. The docs found here were written as if the new truststore should support all of the features of the truststore in keycloak v23. So I opened a ticket when reality did not match the documentation.

Part of the confusion here I think is that a feature was deprecated before we have a suitable replacement for it. @rmartinc That is why I checked the regression box. In the ideal world where the software works as expected and the docs are correct, you are not supposed to use deprecated features. Keycloak did not perform as the documentation suggested when I attempted to use the new features. The regression is that the new way to use a truststore does not work for all truststore implementations.

shawkins commented 4 months ago

The docs found here were written as if the new truststore should support all of the features of the truststore in keycloak v23

We try not to make the upgrade guide read like the full documentation and instead there is a link to see more details in the main docs.

The regression is that the new way to use a truststore does not work for all truststore implementations.

That is expected and by design. You can feel free to continue to use the deprecated approach until it removed.

shawkins commented 4 months ago

Removed the regression check box.

~priority-low

keycloak-github-bot[bot] commented 4 months ago

Due to the amount of issues reported by the community we are not able to prioritise resolving this issue at the moment.

If you are affected by this issue, upvote it by adding a :thumbsup: to the description. We would also welcome a contribution to fix the issue.

Dahu-Taquin commented 3 weeks ago

Deprecation of https-trust-store-* options also causes a regression in case of mTLS/X509 browser authentication : the trusted CAs for client authentication are not the same. When adding the CAs certificates in conf/truststores, trusted client certificates can be signed by these CAs AS WELL AS all CAs in the Java default truststore, which is not the expected behaviour. Trusted CAs can be checked with following command: _openssl sclient -connect host:8443 -prexit under "Acceptable client certificate CA names".

shawkins commented 3 weeks ago

@Dahu-Taquin that is an expected behavioral change, you should convert this comment to a discussion or a new issue if you want additional follow-up.

keycloak / keycloak