-
Notifications
You must be signed in to change notification settings - Fork 1
Implement OpenJDK 25 FIPS patch (for OPENJDK-4184) #40
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
base: fips-25u
Are you sure you want to change the base?
Conversation
fitzsim
left a comment
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.
This looks good.
| String path = "/proc/sys/crypto/fips_enabled"; | ||
| try (java.io.InputStream is = new java.io.FileInputStream(path)) { | ||
| return is.read() == '1'; | ||
| } catch (java.io.IOException ignore) { |
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.
Is java.io.IOException definitely the only type of exception that could be encountered here? Would it be safer to expand this to catch any java.lang.Exception?
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.
Is the only one I would reasonably expect, since it covers file nonexistence and reading errors.
But we can catch java.lang.Throwable for a bulletproof approach, if you agree I will update it tomorrow.
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.
Yes, I think Throwable may be better, but I'm not sure; I can't think of an example issue that this would catch (and that we'd want to catch) that IOException wouldn't. I don't necessarily want to hide really weird error conditions, but if an exception is actually thrown here I think java will fail to run (can you confirm with testing, e.g. just hard-coding a non-existent file name instead of fips_enabled or testing on Ubuntu with its missing fips_enabled? Also try with the new security/redhat/fips.properties I added that avoids the include cycle if __redhat_fips__ fails to expand -- if this means java throws the exception then continues, then just IOException is fine.), because of how we're using __redhat_fips__ early in the configuration. If so, I'm inclined to make this check the strongest possible "best effort".
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.
Yes, I tried hard-coding a nonexistent path during development, I did something like this:
jshell --no-startup -<<'EOF'
/* vvvvvvvvvvvvvvvvvvvvvvvvvvvvv FIPS PATCH vvvvvvvvvvvvvvvvvvvvvvvvvvvvv */
private static final class RedHatFIPS {
// This 'include'-directives-only magic property is an internal
// implementation detail that could (and probably will!) change.
// Red Hat customers should NOT rely on this for their own use.
private static final String MAGIC_PROP = "__redhat_fips__";
private static final String IS_ON = "" + isOn();
private static boolean isOn() {
String path = "/this/does/not/exist";
try (java.io.InputStream is = new java.io.FileInputStream(path)) {
return is.read() == '1';
} catch (java.io.IOException ignore) {
return false;
}
}
static String getProperty(String key) {
return MAGIC_PROP.equals(key) ? IS_ON : System.getProperty(key, "");
}
}
/* ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FIPS PATCH ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ */
System.out.println(RedHatFIPS.getProperty("__redhat_fips__"));
EOFOutput:
false
On one hand, if the FIPS detection logic is broken itself, I would prefer an error (and a customer case) rather than silently disabling the FIPS setup, in cases where the customer may think they are using certified crypto. On the other hand, I'm very confident that handling Throwable instead of IOException doesn't make any difference, so we can do it for assurance.
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 don't think PropertyExpander is the right place for this, a class which is shared by a number of other classes that are never going to encounter this property. If we instead put this into Security (where our previous changes were), we have access to better debug output and it affects less of the JDK.
Also, the idea of grabbing every possible exception doesn't sit well with me, and is even less appealing if you are not producing any debug output.
Also, do you have an example of how this would occur? Neither referenced bug (4184 or 2108) actually discuss it at all, as far as I can see.
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.
@gnu-andrew: it's true, I moved it to Security. In order to avoid touching PropertyExpander at all, the code now temporarily sets the __redhat_fips__ system property during the security properties loading. After that, it uses System::clearProperty (thanks @fitzsim, I think it was your idea!).
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.
NOTE: regarding the logging, I think JDK-8319332 logging should be enough for this case:
$ java -Djava.security.debug=properties -XshowSettings:security:properties 2>&1 | grep __redhat_fips__
properties[0x3|main|Security.java:243|2025-11-17 23:20:33.709]: processing include: '../${__redhat_fips__}/fips.properties' (expanded to '../true/fips.properties')
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 like the revised version. It's a lot simpler and direct now as well. The only thing I'd add is a debug line in the catch block so we can get at that exception. Something like sdebug.println("Failed to read FIPS kernel file: " + exception); As you say, the rest should be handled by the properties logging (wasn't aware of that, nice addition)
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.
Ok, added it because it doesn't hurt. But take in mind this is really unlikely to happen. Also, it should be easy to figure out what's going on by other means, like trying cat /proc/sys/crypto/fips_enabled on a terminal.
NOTE: I'm force-pushing to keep the branch clean in case we want to integrate keeping the two commits separated.
Introduce an 'include'-directives-only __redhat_fips__ magic property that expands as either true or false depending on the System FIPS status, reported by the /proc/sys/crypto/fips_enabled kernel file.
Introduce RedHatFIPSFilter, a lightweight Security Providers Filter that uses an allow-list approach to enable non-cryptographic utilities from the providers that also implement uncertified cryptographic primitives, which should be avoided in a FIPS setup. RedHatFIPSFilter is enabled through the __redhat_fips_filter__ boolean security property.
OPENJDK-4184: Include new FIPS patch in OpenJDK 25 portable build
Hi,
This pull request re-implements a reduced version of the previous releases' FIPS patch-set, adapted to OpenJDK 25 and relying on the JDK-8319332: Security properties files inclusion proposal (introduced in OpenJDK 24).
This patch-set is one of the three pieces of the OPENJDK-2108: Remove crypto-policies and FIPS automation related patches design. The remaining two pieces are the configuration files (to be included in the RPM package) and the nss-native-fips-key-import-export-adapter (to be built in the RPM package).
Changes summary
ae176fa: OPENJDK-2108: Internal __redhat_fips__ property
Introduce an
include-directives-only__redhat_fips__magic property that expands as either true or false depending on the System FIPS status, reported by the/proc/sys/crypto/fips_enabledkernel file.This patch is intended to be temporary, while we explore other alternatives (both upstream and binary-compatible ideas for downstream).
81e2bc0: OPENJDK-2123: Algorithms lockdown
Introduce
RedHatFIPSFilter, a lightweight Security Providers Filter that uses an allow-list approach to enable non-cryptographic utilities from the providers that also implement uncertified cryptographic primitives, which should be avoided in a FIPS setup.RedHatFIPSFilteris enabled through the__redhat_fips_filter__boolean security property.When the JDK-8315487: Security Providers Filter work is ready, this patch can be replaced by a
jdk.security.providers.filtervalue, as planned under OPENJDK-2123.This patch was created in an effort to reduce the pre-existing algorithms-lockdown patch, taking advantage of the lessons learned during the JDK-8315487 development. NOTE: the patch is based on the fact that all the OpenJDK 25 bundled providers use
java.security.Provider::putService()instead of the legacy services registration API (hashtable methods).