Adjustments for FIPS 140 testing #49319
Conversation
- Don't install ingest-attachment and don't run the related docs tests, since ingest-attachement is not supported in FIPS 140 JVMs - Move copying extra jars and extra config files earlier on in the node configuration so that elasticsearch-keystore and elasticsearch-plugin that run before the node starts have all files (policy, properties, jars) available.
|
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
mark-vieira
left a comment
mark-vieira
left a comment
There was a problem hiding this comment.
LGTM.
As per my comment, if we are simply toggling FIPS tests based on a system property we can simplify some things around BuildParams which currently assumes this information isn't available until task execution time. Feel free to merge as is though and I'll submit a follow up PR to refactor this.
| forbiddenApisTest.enabled = false | ||
| jarHell.enabled = false | ||
| thirdPartyAudit.enabled = false | ||
| if (Boolean.parseBoolean(System.getProperty("tests.fips.enabled"))){ |
There was a problem hiding this comment.
Looking at this again, are we determining whether we are in a fips JVM only by looking at this system property? If so, we should refactor how we set the corresponding BuildParams property and use that instead. As I remember, we used to involve a script command using the runtime java home to make this determination but at some point that was removed?
There was a problem hiding this comment.
You're absolutely right. Before #48378 we would check the SecurityProvider that was in use in runtime and determine if we're running in a FIPS 140 JVM. Nowadays, we explicitly control this with the system property so there is no need to defer to runtime and we can handle everything on configuration. Thus, I followed your advice and removed the BuildParams.inFipsJvm and the globalInfo.ready() blocks where applicable. Tagging you for another review based on that
…trustedcerty entry, it's not enough for it to be in privatekeyentry for it to be trusted
jkakavas
dismissed
mark-vieira’s stale review
further changes require a new look
mark-vieira
left a comment
mark-vieira
left a comment
There was a problem hiding this comment.
I may have misspoken, we still want to use BuildParams.inFipsJvm everywhere rather than the system property. We can then set the value of the build param in a single location based on the system property in GlobalBuildInfoPlugin as we do for other things.
There are many reasons to do this, such as not hard-coding tests.fips.enabled in a million places, having a static, type-safe method for returning what is a boolean, and providing better error reporting for scenarios where this may have not gotten set for some reason.
| private static JavaVersion gradleJavaVersion; | ||
| private static JavaVersion compilerJavaVersion; | ||
| private static JavaVersion runtimeJavaVersion; | ||
| private static Boolean inFipsJvm; |
There was a problem hiding this comment.
We don't want to remove this. We want this property to live here, rather than us checking for a system property all over the place. We simply want to initialize this at configuration time in GlobalBuildInfoPlugin. Then use BuildParams.inFipsJvm everywhere else.
There was a problem hiding this comment.
Ack, I will revert and adjust
mark-vieira
left a comment
mark-vieira
left a comment
There was a problem hiding this comment.
Only one minor comment, otherwise changes LGTM 👍
| writer.write(" Compiler java.home : " + compilerJavaHome + "\n"); | ||
| writer.write(" Runtime JDK Version : " + runtimeJavaVersionEnum + " (" + runtimeJavaVersionDetails + ")"); | ||
| writer.write(inFipsJvm ? " (in FIPS 140 mode)\n":"\n"); | ||
| writer.write(inFipsJvm ? " (in FIPS 140 mode)\n" : "\n"); |
There was a problem hiding this comment.
Let's move this to the PrintGlobalBuildInfoTask since it's more appropriate.
There was a problem hiding this comment.
I think whether or not we're in a FIPS 140 mode is a property of the JVM and were printing the current runtime version here, so it feels natural to add this here. WDYT ?
There was a problem hiding this comment.
That is true, but we aren't actually making this determination based on the JVM. If we were, I'd agree, but since we're just toggling based on a system property, it doesn't make sense to include here. In fact, this will result in potential errors, because this generate task is cached, and we aren't tracking that system property as an input. So we expose ourselves to a scenario where we run a build, change the system property, and still get the old output.
There was a problem hiding this comment.
Gotcha, thanks for elaborating
|
Changes look good @jkakavas. Feel free to merge, and let's ensure this get's backported to 7.x as well. |
- Don't install ingest-attachment and don't run the related docs tests, since ingest-attachment is not supported in FIPS 140 JVMs - Move copying extra jars and extra config files earlier on in the node configuration so that elasticsearch-keystore and elasticsearch-plugin that run before the node starts have all files (policy, properties, jars) available. - BCJSSE needs a certificate to be explicitly added in a keystore as a trustedcerty entry, it's not enough for it to be in privatekeyentry for it to be trusted - Set the value for BuildParams.inFipsJvm configuration time
4 participants
This is a follow up to #48378 after running the build locally for a few
times in FIPS mode to identify potential test issues. Changes
include:
and don't run the related docs tests, since ingest-attachement
plugin is not supported in FIPS 140 JVMs
node configuration so that elasticsearch-keystore and
elasticsearch-plugin that run before the node starts have all files
(policy, properties, jars) available.