From fbc9177f275be7e4f87572e8d52a4d6e22bd3bcf Mon Sep 17 00:00:00 2001 From: rmartinc Date: Tue, 14 Feb 2023 15:14:25 +0100 Subject: [PATCH] Doublecheck if we need to override properties in java.security Closes https://github.com/keycloak/keycloak/issues/16702 --- .github/scripts/run-fips-it.sh | 1 + .../crypto/fips/FIPS1402Provider.java | 43 ++++++++++++++++++ .../fips/KeycloakFipsSecurityProvider.java | 2 +- .../admin-cli/src/main/bin/kcadm.bat | 2 +- .../admin-cli/src/main/bin/kcadm.sh | 2 +- .../src/main/bin/kcreg.bat | 2 +- .../src/main/bin/kcreg.sh | 2 +- quarkus/dist/src/main/content/bin/kc.bat | 2 +- quarkus/dist/src/main/content/bin/kc.sh | 2 +- .../auth-server/common/fips/kc.java.security | 45 ------------------- 10 files changed, 51 insertions(+), 52 deletions(-) diff --git a/.github/scripts/run-fips-it.sh b/.github/scripts/run-fips-it.sh index 462ebf4bef9..b3d32135321 100755 --- a/.github/scripts/run-fips-it.sh +++ b/.github/scripts/run-fips-it.sh @@ -14,4 +14,5 @@ echo "STRICT_OPTIONS: $STRICT_OPTIONS" TESTS=`testsuite/integration-arquillian/tests/base/testsuites/suite.sh fips` echo "Tests: $TESTS" export JAVA_HOME=/etc/alternatives/java_sdk_17 +set -o pipefail ./mvnw test -Dsurefire.rerunFailingTestsCount=$SUREFIRE_RERUN_FAILING_COUNT -nsu -B -Pauth-server-quarkus,auth-server-fips140-2 -Dcom.redhat.fips=false $STRICT_OPTIONS -Dtest=$TESTS -pl testsuite/integration-arquillian/tests/base | misc/log/trimmer.sh diff --git a/crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java b/crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java index cb0582ecd36..356efe8194e 100644 --- a/crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java +++ b/crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java @@ -26,15 +26,18 @@ import java.security.cert.CollectionCertStoreParameters; import java.util.Arrays; import java.util.Collections; import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import javax.crypto.Cipher; import javax.crypto.NoSuchPaddingException; import javax.crypto.SecretKeyFactory; +import javax.net.ssl.KeyManagerFactory; import javax.net.ssl.SNIHostName; import javax.net.ssl.SSLParameters; import javax.net.ssl.SSLSocket; import javax.net.ssl.SSLSocketFactory; +import javax.net.ssl.TrustManagerFactory; import org.bouncycastle.asn1.x9.ECNamedCurveTable; import org.bouncycastle.asn1.x9.X9ECParameters; @@ -87,6 +90,8 @@ public class FIPS1402Provider implements CryptoProvider { checkSecureRandom(() -> Security.insertProviderAt(this.bcFipsProvider, 2)); Provider bcJsseProvider = new BouncyCastleJsseProvider("fips:BCFIPS"); Security.insertProviderAt(bcJsseProvider, 3); + // force the key and trust manager factories if default values not present in BCJSSE + modifyKeyTrustManagerSecurityProperties(bcJsseProvider); log.debugf("Inserted security providers: %s", Arrays.asList(this.bcFipsProvider.getName(),bcJsseProvider.getName())); } else { log.debugf("Security provider %s already loaded", existingBcFipsProvider.getName()); @@ -289,4 +294,42 @@ public class FIPS1402Provider implements CryptoProvider { } } } + + /** + * BCJSSE manages X.509, X509 and PKIX for KeyManagerFactory and + * TrustManagerFactory (names or aliases) while JSSE manages SunX509, + * NewSunX509 and PKIX for KeyManagerFactory and SunX509, PKIX, SunPKIX, + * X509 and X.509 for the TrustManagerFactory. As BCJSSE is used when + * fips enabled, the default implementations are changed to the ones + * provided by BC if selected ones are not present in the BCJSSE. + * + * @param bcJsseProvider The BCJSSE provider + */ + private static void modifyKeyTrustManagerSecurityProperties(Provider bcJsseProvider) { + boolean setKey = bcJsseProvider.getService(KeyManagerFactory.class.getSimpleName(), KeyManagerFactory.getDefaultAlgorithm()) == null; + boolean setTrust = bcJsseProvider.getService(TrustManagerFactory.class.getSimpleName(), TrustManagerFactory.getDefaultAlgorithm()) == null; + if (!setKey && !setTrust) { + return; + } + Set services = bcJsseProvider.getServices(); + if (services != null) { + for (Provider.Service service : services) { + if (setKey && KeyManagerFactory.class.getSimpleName().equals(service.getType())) { + Security.setProperty("ssl.KeyManagerFactory.algorithm", service.getAlgorithm()); + setKey = false; + if (!setTrust) { + return; + } + } else if (setTrust && TrustManagerFactory.class.getSimpleName().equals(service.getType())) { + Security.setProperty("ssl.TrustManagerFactory.algorithm", service.getAlgorithm()); + setTrust = false; + if (!setKey) { + return; + } + } + } + } + throw new IllegalStateException("Provider " + bcJsseProvider.getName() + + " does not provide KeyManagerFactory or TrustManagerFactory algorithms for TLS"); + } } diff --git a/crypto/fips1402/src/main/java/org/keycloak/crypto/fips/KeycloakFipsSecurityProvider.java b/crypto/fips1402/src/main/java/org/keycloak/crypto/fips/KeycloakFipsSecurityProvider.java index a588820f358..e39706d860e 100644 --- a/crypto/fips1402/src/main/java/org/keycloak/crypto/fips/KeycloakFipsSecurityProvider.java +++ b/crypto/fips1402/src/main/java/org/keycloak/crypto/fips/KeycloakFipsSecurityProvider.java @@ -48,7 +48,7 @@ public class KeycloakFipsSecurityProvider extends Provider { isSystemFipsEnabled.setAccessible(true); return (boolean) isSystemFipsEnabled.invoke(null); } catch (Throwable ignore) { - logger.warn("Could not detect if FIPS is enabled from the host"); + logger.debug("Could not detect if FIPS is enabled from the host"); } finally { if (isSystemFipsEnabled != null) { isSystemFipsEnabled.setAccessible(false); diff --git a/integration/client-cli/admin-cli/src/main/bin/kcadm.bat b/integration/client-cli/admin-cli/src/main/bin/kcadm.bat index b8ea360774b..1dfdbf10679 100644 --- a/integration/client-cli/admin-cli/src/main/bin/kcadm.bat +++ b/integration/client-cli/admin-cli/src/main/bin/kcadm.bat @@ -5,4 +5,4 @@ if "%OS%" == "Windows_NT" ( ) else ( set DIRNAME=.\ ) -java %KC_OPTS% -cp "%DIRNAME%\client\keycloak-admin-cli-${project.version}.jar" -Dkc.lib.dir="%DIRNAME%\client\lib" org.keycloak.client.admin.cli.KcAdmMain %* +java %KC_OPTS% -cp "%DIRNAME%\client\keycloak-admin-cli-${project.version}.jar" --add-opens=java.base/java.security=ALL-UNNAMED -Dkc.lib.dir="%DIRNAME%\client\lib" org.keycloak.client.admin.cli.KcAdmMain %* diff --git a/integration/client-cli/admin-cli/src/main/bin/kcadm.sh b/integration/client-cli/admin-cli/src/main/bin/kcadm.sh index 49db302a189..49e82355c03 100755 --- a/integration/client-cli/admin-cli/src/main/bin/kcadm.sh +++ b/integration/client-cli/admin-cli/src/main/bin/kcadm.sh @@ -29,4 +29,4 @@ if [ "x$JAVA" = "x" ]; then fi fi -"$JAVA" $KC_OPTS -cp $DIRNAME/client/keycloak-admin-cli-${project.version}.jar -Dkc.lib.dir=$DIRNAME/client/lib org.keycloak.client.admin.cli.KcAdmMain "$@" +"$JAVA" $KC_OPTS -cp $DIRNAME/client/keycloak-admin-cli-${project.version}.jar --add-opens=java.base/java.security=ALL-UNNAMED -Dkc.lib.dir=$DIRNAME/client/lib org.keycloak.client.admin.cli.KcAdmMain "$@" diff --git a/integration/client-cli/client-registration-cli/src/main/bin/kcreg.bat b/integration/client-cli/client-registration-cli/src/main/bin/kcreg.bat index 35bda5919c8..f8252f214b3 100644 --- a/integration/client-cli/client-registration-cli/src/main/bin/kcreg.bat +++ b/integration/client-cli/client-registration-cli/src/main/bin/kcreg.bat @@ -5,4 +5,4 @@ if "%OS%" == "Windows_NT" ( ) else ( set DIRNAME=.\ ) -java %KC_OPTS% -cp "%DIRNAME%\client\keycloak-client-registration-cli-${project.version}.jar" -Dkc.lib.dir="%DIRNAME%\client\lib" org.keycloak.client.registration.cli.KcRegMain %* +java %KC_OPTS% -cp "%DIRNAME%\client\keycloak-client-registration-cli-${project.version}.jar" --add-opens=java.base/java.security=ALL-UNNAMED -Dkc.lib.dir="%DIRNAME%\client\lib" org.keycloak.client.registration.cli.KcRegMain %* diff --git a/integration/client-cli/client-registration-cli/src/main/bin/kcreg.sh b/integration/client-cli/client-registration-cli/src/main/bin/kcreg.sh index ada45800062..7f9e44aef7c 100755 --- a/integration/client-cli/client-registration-cli/src/main/bin/kcreg.sh +++ b/integration/client-cli/client-registration-cli/src/main/bin/kcreg.sh @@ -28,4 +28,4 @@ if [ "x$JAVA" = "x" ]; then fi DIRNAME=`dirname "$RESOLVED_NAME"` -"$JAVA" $KC_OPTS -cp $DIRNAME/client/keycloak-client-registration-cli-${project.version}.jar -Dkc.lib.dir=$DIRNAME/client/lib org.keycloak.client.registration.cli.KcRegMain "$@" +"$JAVA" $KC_OPTS -cp $DIRNAME/client/keycloak-client-registration-cli-${project.version}.jar --add-opens=java.base/java.security=ALL-UNNAMED -Dkc.lib.dir=$DIRNAME/client/lib org.keycloak.client.registration.cli.KcRegMain "$@" diff --git a/quarkus/dist/src/main/content/bin/kc.bat b/quarkus/dist/src/main/content/bin/kc.bat index 1ac91b81d72..3c6c5d3d1fd 100644 --- a/quarkus/dist/src/main/content/bin/kc.bat +++ b/quarkus/dist/src/main/content/bin/kc.bat @@ -81,7 +81,7 @@ if not "x%JAVA_OPTS%" == "x" ( if not "x%JAVA_ADD_OPENS%" == "x" ( echo "JAVA_ADD_OPENS already set in environment; overriding default settings with values: %JAVA_ADD_OPENS%" ) else ( - set "JAVA_ADD_OPENS=--add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED" + set "JAVA_ADD_OPENS=--add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.security=ALL-UNNAMED" ) set "JAVA_OPTS=%JAVA_OPTS% %JAVA_ADD_OPENS%" diff --git a/quarkus/dist/src/main/content/bin/kc.sh b/quarkus/dist/src/main/content/bin/kc.sh index fd4b4fe9d6d..846e5de3666 100644 --- a/quarkus/dist/src/main/content/bin/kc.sh +++ b/quarkus/dist/src/main/content/bin/kc.sh @@ -92,7 +92,7 @@ fi # See also https://github.com/wildfly/wildfly-core/blob/7e5624cf92ebe4b64a4793a8c0b2a340c0d6d363/core-feature-pack/common/src/main/resources/content/bin/common.sh#L57-L60 if [ "x$JAVA_ADD_OPENS" = "x" ]; then - JAVA_ADD_OPENS="--add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED" + JAVA_ADD_OPENS="--add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.security=ALL-UNNAMED" else echo "JAVA_ADD_OPENS already set in environment; overriding default settings with values: $JAVA_ADD_OPENS" fi diff --git a/testsuite/integration-arquillian/servers/auth-server/common/fips/kc.java.security b/testsuite/integration-arquillian/servers/auth-server/common/fips/kc.java.security index 7b96cb4108b..bebd796ca6a 100644 --- a/testsuite/integration-arquillian/servers/auth-server/common/fips/kc.java.security +++ b/testsuite/integration-arquillian/servers/auth-server/common/fips/kc.java.security @@ -4,14 +4,6 @@ # NOTE: Each property is specified 2 times. This is so the same file can be used on both FIPS based RHEL host (which uses "fips" prefixed properties by default) # and the non-fips based (EG. when running the tests on GH actions) -# -# List of providers and their preference orders (see above). Used on the host without FIPS (EG. when running the tests on GH actions) -# Uses only BouncyCastle FIPS providers to make sure to use only FIPS compliant cryptography. -# -#security.provider.1=org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider -#security.provider.2=org.bouncycastle.jsse.provider.BouncyCastleJsseProvider fips:BCFIPS -#security.provider.3= - # # Security providers used when global crypto-policies are set to FIPS (Usually it is used when FIPS enabled on system/JVM level) # @@ -19,41 +11,4 @@ # However once it is present, there won't be a need to override this and this part can be fully commented/removed. # TODO: Comment/remove this once https://bugzilla.redhat.com/show_bug.cgi?id=1940064 is fixed and OpenJDK 17 updated to corresponding version where XMLDSig is available by default # -fips.provider.1=SunPKCS11 ${java.home}/conf/security/nss.fips.cfg -fips.provider.2=SUN -fips.provider.3=SunEC -fips.provider.4=SunJSSE -fips.provider.5=SunJCE -fips.provider.6=SunRsaSign fips.provider.7=XMLDSig - -# Commented this provider for now (and also other providers) as it uses lots of non-FIPS services. -# See https://access.redhat.com/documentation/en-us/openjdk/11/html-single/configuring_openjdk_11_on_rhel_with_fips/index#ref_openjdk-default-fips-configuration_openjdk -# fips.provider.2=SUN - -# -# Default keystore type. -# -keystore.type=PKCS12 -fips.keystore.type=PKCS12 - -# This is needed especially if we cannot add security provider "com.sun.net.ssl.internal.ssl.Provider BCFIPS" as a security provider. -# OpenJDK has "SunX509" as default algorithm, but that one is not supported by BCJSSE. So adding the Sun provider delegating to BCFIPS is needed (as above) -# or changing default algorithm as described here -ssl.KeyManagerFactory.algorithm=PKIX -fips.ssl.KeyManagerFactory.algorithm=PKIX - -ssl.TrustManagerFactory.algorithm=PKIX -fips.ssl.TrustManagerFactory.algorithm=PKIX - -# -# Controls compatibility mode for JKS and PKCS12 keystore types. -# -# When set to 'true', both JKS and PKCS12 keystore types support loading -# keystore files in either JKS or PKCS12 format. When set to 'false' the -# JKS keystore type supports loading only JKS keystore files and the PKCS12 -# keystore type supports loading only PKCS12 keystore files. -# -# This is set to true as when set to false on OpenJDK 17 and PKCS12 is default keystore type, loading of default truststore (from java cacerts) fails. -#keystore.type.compat=false -#fips.keystore.type.compat=false