-
Notifications
You must be signed in to change notification settings - Fork 6.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue: 26568 - bcfips version bump and fixes #29670
base: main
Are you sure you want to change the base?
Conversation
2e0eb3d
to
1eab216
Compare
84ea0df
to
f005a93
Compare
* bump BCFIPS to 1.0.2.5 * fix bc-fips related test error * remove unused imports Closes: keycloak#26568 Signed-off-by: Andre F de M <trixpan@users.noreply.github.com>
Signed-off-by: Andre F de M <trixpan@users.noreply.github.com>
Signed-off-by: Andre F de M <trixpan@users.noreply.github.com>
Signed-off-by: Andre F de M <trixpan@users.noreply.github.com>
Signed-off-by: Andre F de M <trixpan@users.noreply.github.com>
Signed-off-by: Andre F de M <trixpan@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trixpan Thanks! Added some comments inline. Also the testsuite needs to be investigated and fixed as I see there are tests failing now.
@@ -11,10 +11,6 @@ org.keycloak.testsuite.x509.** | |||
MutualTLSClientTest | |||
FAPI1Test | |||
FAPICIBATest | |||
KcRegTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot remove tests this way. If tests don't work, they need to be fixed rather than removed from the testsuite.
@@ -12,3 +12,6 @@ | |||
# 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.7=XMLDSig | |||
|
|||
# TODO: Comment/remove this once https://issues.redhat.com/browse/RHEL-3478 is fixed. | |||
securerandom.strongAlgorithms=PKCS11:SunPKCS11-NSS-FIPS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why we explicitly need to set this in the testsuite.
Ideal is, if this is configured automatically as otherwise, we would need to document it that people must set this as well in their environments. The current configuration is done in FIPS1402Provider.checkSecureRandom
. You can doublecheck if there is some update needed here, so we don't need to add any additional stuff to the documentation or to the the kc.java.security
file in our testsuite.
@@ -143,6 +143,8 @@ requirement as they are longer than 14 characters. | |||
|
|||
* RSA keys of 1024 bits do not work (2048 is the minimum). This applies for keys used by the {project_name} realm itself (Realm keys from the `Keys` tab in the admin console), but also client keys and IDP keys | |||
|
|||
* Since version 1.0.2.4, the Bouncy Castle FIPS library now requires a flag to allow the use of the RSA PKCS1.5 algorithm used by RS256. `-Dorg.bouncycastle.rsa.allow_pkcs15_enc=true` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we need this note as the property is set by default in Fips1402StrictCryptoProvider
constructor?
@@ -258,6 +260,8 @@ In addition to the preceding requirements, be sure to doublecheck this before sw | |||
|
|||
* Make sure that clients relying on `Signed JWT with Client Secret` use at least 14 characters long secrets (ideally generated secrets) | |||
|
|||
* Avoid using "RS256" OIDC algorithm. If this is required, ensure you properly configure the environment as documented above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note as above
# The following property can be used to override the default behavior: | ||
# org.bouncycastle.rsa.allow_pkcs15_enc (allow use of PKCS1.5) | ||
# This is required by crypto/fips1402/src/test/java/org/keycloak/crypto/fips/test/FIPS1402JWETest.java | ||
./mvnw test -nsu -B -pl crypto/default,crypto/fips1402 -Dcom.redhat.fips=true -Dorg.bouncycastle.fips.approved_only=true -Dorg.bouncycastle.rsa.allow_pkcs15_enc=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope we don't need this change as long as property is set as you did in Fips1402StrictCryptoProvider
constructor?
JAVA_SECURITY_FILE="/etc/alternatives/java_sdk_17/conf/security/java.security" | ||
|
||
echo "fips.provider.7=XMLDSig" >> "${JAVA_SECURITY_FILE}" | ||
sed -i 's/securerandom.strongAlgorithms=NativePRNGBlocking:SUN,DRBG:SUN/securerandom.strongAlgorithms=PKCS11:SunPKCS11-NSS-FIPS/g' "${JAVA_SECURITY_FILE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as in other place. I hope we can update initialization rather than require to manually set additional property as that would mean that we will need to update documentation as well.
Closes: #26568