-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Make test-suite runnable under FIPS JVM #18491
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: main
Are you sure you want to change the base?
Make test-suite runnable under FIPS JVM #18491
Conversation
|
❌ Gradle check result for 9b5da5c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 986dce7: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
I'd like to raise a general point here to keep in mind with this development, especially as instructions will be required for Java setups other than the bundled version. The Red Hat JDK 21, for example, has a default of fips.keystore.type: PKCS12 - see https://docs.redhat.com/en/documentation/red_hat_build_of_openjdk/21/html/configuring_red_hat_build_of_openjdk_21_on_rhel_with_fips/fips_settings#fips_settings . We'd like to ensure that code checks aren't so stringent as to prevent this setup from working. |
|
❌ Gradle check result for 939e6b5: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
939e6b5 to
11da667
Compare
|
❌ Gradle check result for 11da667: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
11da667 to
9a387a4
Compare
|
❌ Gradle check result for 9a387a4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
9a387a4 to
4e0af75
Compare
|
❌ Gradle check result for 4e0af75: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
4e0af75 to
9efd838
Compare
|
❌ Gradle check result for 9efd838: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
9efd838 to
4fc6b40
Compare
|
❌ Gradle check result for 4fc6b40: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
4fc6b40 to
0139eaa
Compare
|
❌ Gradle check result for 0139eaa: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
0139eaa to
f52e720
Compare
|
❌ Gradle check result for f52e720: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
@terryquigleysas Thank you for pointing out those limitations.
We rely on SunPKCS12 provider to load the JVM's default truststore. In case of OpenJKD the default type is the same as RHEL's - so nothing changes for us. |
Good news. Thank you for the reply. Much appreciated! |
f52e720 to
cb7949d
Compare
|
❌ Gradle check result for cb7949d: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
cb7949d to
0680de8
Compare
|
❌ Gradle check result for 0680de8: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
0680de8 to
732e412
Compare
|
❌ Gradle check result for 732e412: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
732e412 to
6164088
Compare
| (PrivilegedAction<Version>) () -> Version.parse(System.getProperty("java.version")) | ||
| ); | ||
| if (full.compareTo(Version.parse("12.0.1")) < 0) { | ||
| if (!inFipsJvm()) { |
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.
Not related, but our baseline is JDK-21, we could clean up this code
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've reverted my changes to this plugin. Azure classic is generally not worth trying to make it FIPS-compliant. We should keep it as is - unsupported.
| if (Security.getProvider("BCFIPS") != null) { | ||
| certTrustStore = KeyStore.getInstance("BCFKS"); | ||
| InputStream keyStoreStream = getClass().getResourceAsStream("/google.bcfks"); | ||
| SecurityUtils.loadKeyStore(certTrustStore, keyStoreStream, "notasecret"); |
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 believe we definitely should not hardcode password here, please have a setting for it
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.
Let's make a setting for SSL trust as well as it's passphrase w/o a fallback and use this google.bcfks store only for tests.
Thanks @beanuwave , did a first pass, a few minor comments but I have two major concerns:
Thank you. |
I believe @cwperks suggested creating a new GitHub workflow that supports both scheduled runs and PR comments. |
…or TYPE_TO_EXTENSION_MAP; revert changes to SecureSettingsHelpers.getSecureSettingsProvider; revert changes to AzureClassic Signed-off-by: Igonin <[email protected]> Co-authored-by: Benny Goerzig <[email protected]> Co-authored-by: Karsten Schnitter <[email protected]> Co-authored-by: Kai Sternad <[email protected]>
Signed-off-by: Igonin <[email protected]> Co-authored-by: Benny Goerzig <[email protected]> Co-authored-by: Karsten Schnitter <[email protected]> Co-authored-by: Kai Sternad <[email protected]>
Sorry, a bit late here, I think this is in general viable option, but:
|
Signed-off-by: Igonin <[email protected]> Co-authored-by: Benny Goerzig <[email protected]> Co-authored-by: Karsten Schnitter <[email protected]> Co-authored-by: Kai Sternad <[email protected]>
… google.p12 in non-FIPS mode and an exception in FIPS mode when no truststore is set up. Signed-off-by: Igonin <[email protected]> Co-authored-by: Benny Goerzig <[email protected]> Co-authored-by: Karsten Schnitter <[email protected]> Co-authored-by: Kai Sternad <[email protected]>
|
❌ Gradle check result for 07c6358: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
What I was thinking was something similar to benchmark-pull-request where the benchmarks are run ad hoc
@peterzhuamazon @rishabh6788 Would you be able to help us with setup for such a workflow? |
Signed-off-by: Igonin <[email protected]> Co-authored-by: Benny Goerzig <[email protected]> Co-authored-by: Karsten Schnitter <[email protected]> Co-authored-by: Kai Sternad <[email protected]>
Signed-off-by: Igonin <[email protected]> Co-authored-by: Benny Goerzig <[email protected]> Co-authored-by: Karsten Schnitter <[email protected]> Co-authored-by: Kai Sternad <[email protected]>
Signed-off-by: Igonin <[email protected]> Co-authored-by: Benny Goerzig <[email protected]> Co-authored-by: Karsten Schnitter <[email protected]> Co-authored-by: Kai Sternad <[email protected]>
…oreUtils Signed-off-by: Igonin <[email protected]> Co-authored-by: Benny Goerzig <[email protected]> Co-authored-by: Karsten Schnitter <[email protected]> Co-authored-by: Kai Sternad <[email protected]>
cwperks
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.
Thank you for the diligence on this PR @beanuwave! With these changes it greatly expands the test coverage when running in FIPS approved mode.
@peterzhuamazon what would be required to add another gradle check to run with the FIPS approved setting? i.e. ./gradlew check -Pcrypto.standard=FIPS-140-3
| final KeyStore keyStore = KeyStore.getInstance(keyStoreType, jcaProvider); | ||
|
|
||
| keyStore.load( | ||
| SecureNetty4HttpServerTransportTests.class.getResourceAsStream("/netty4-server-keystore" + fileExtension), |
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.
try (InputStream in = SecureNetty4HttpServerTransportTests.class.getResourceAsStream("/netty4-server-keystore" + fileExtension) {
keyStore.load(in, PASSWORD);
}
|
|
||
| // Cached factory to avoid repeated keystore loading (especially slow in FIPS mode with BCFKS). | ||
| // TODO: Replace with @BeforeAll when migrating to JUnit 5. | ||
| private static volatile KeyManagerFactory cachedKeyManagerFactory; |
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.
Why we cannot use @BeforeClass (JUnit 4) here?
|
|
||
| // Cached factories to avoid repeated keystore loading (especially slow in FIPS mode) | ||
| // TODO: Replace with @BeforeAll when migrating to JUnit 5. | ||
| private static volatile KeyManagerFactory cachedServerKeyManagerFactory; |
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 question please, @BeforeClass should be sufficient
|
|
||
| SSLEngine engine = SslContextBuilder.forServer(keyManagerFactory) | ||
| return Optional.of( | ||
| SslContextBuilder.forServer(getServerKeyManagerFactory()) |
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.
Don't we need to provide trustManager here as well?
| import static org.opensearch.repositories.gcs.GoogleCloudStorageClientSettings.TRUSTSTORE_PATH_SETTING; | ||
| import static org.opensearch.repositories.gcs.GoogleCloudStorageClientSettings.TRUSTSTORE_TYPE_SETTING; | ||
|
|
||
| @SuppressForbidden(reason = "use a http server") |
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.
Sorry, why do we need @SuppressForbidden here?
|
|
||
| // Cached keystore to avoid expensive cert generation on every test (especially slow in FIPS mode) | ||
| // TODO: Replace with @BeforeAll when migrating to JUnit 5. | ||
| private static volatile KeyStore cachedServerKeyStore; |
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.
Please use @BeforeClass
| testTask.systemProperty 'javax.net.ssl.trustStoreProvider', 'BCFIPS' | ||
| testTask.systemProperty 'javax.net.ssl.trustStorePassword', 'changeit' | ||
|
|
||
| // Exclude base test classes that have corresponding FipsTests or FipsIT |
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.
Should we just rename these tests to:
XxxNoFipsTests.java
XxxNoFipsIT.java
?
| */ | ||
| final class KeyStoreUtil { | ||
|
|
||
| public static final Supplier<Boolean> inFipsMode = () -> { |
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.
Why do need Supplier here? The value of the FIPS mode will not change, we need to capture it only once:
public static final boolean IN_FIPS_MODE = isInFipsMode();
private static boolean isInFipsMode() {
try {
// Equivalent to: boolean approvedOnly = CryptoServicesRegistrar.isInApprovedOnlyMode()
var registrarClass = Class.forName("org.bouncycastle.crypto.CryptoServicesRegistrar");
var isApprovedOnlyMethod = registrarClass.getMethod("isInApprovedOnlyMode");
return (Boolean) isApprovedOnlyMethod.invoke(null);
} catch (ReflectiveOperationException e) {
return false;
}
}| } | ||
| try { | ||
| KeyStore keyStore = KeyStore.getInstance(type); | ||
| if (CryptoServicesRegistrar.isInApprovedOnlyMode() && !FIPS_COMPLIANT_KEYSTORE_TYPES.contains(type)) { |
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.
So why we are calling CryptoServicesRegistrar directly here but using reflection above (inFipsMode)? If we do not require CryptoServicesRegistrar to be present on the classpath, we should use IN_FIPS_MODE , otherwise we should just use CryptoServicesRegistrar.isInApprovedOnlyMode()
Thanks a lot @beanuwave , it looks great, few quite minor comments but awesome work by all means! Thank you! |
Description
Makes the required changes to build and test under FIPS-140-3 compliance support. FIPS mode can be activated by adding the
-Pcrypto.standard=FIPS-140-3Gradle parameter.NOTE:
Due to the netty FIPS reflection bug in v4.1.125.Final and v4.1.126.Final, theNot relevant after update to netty 4.2.7:plugins:transport-reactor-netty4:testtask will fail. To run properly, netty needs to be updated to a newer version, such as v4.1.127.Final.Related Issues
Resolves RFC
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.