-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add build-tooling to run in the FIPS environment #18921
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?
Add build-tooling to run in the FIPS environment #18921
Conversation
❌ Gradle check result for f656bd4: 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? |
f656bd4
to
20a5611
Compare
❌ Gradle check result for 20a5611: 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? |
20a5611
to
2241009
Compare
❌ Gradle check result for 2241009: 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? |
2241009
to
1829731
Compare
❕ Gradle check result for 1829731: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #18921 +/- ##
============================================
- Coverage 73.11% 73.07% -0.04%
- Complexity 70661 70692 +31
============================================
Files 5724 5741 +17
Lines 323498 323915 +417
Branches 46852 46900 +48
============================================
+ Hits 236518 236706 +188
- Misses 67846 68108 +262
+ Partials 19134 19101 -33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Patch coverage is showing as 0% because the new tests are guarded with
The actual patch coverage is much higher, but the gradle check of this repo does not run with FIPS enabled. |
server/src/main/java/org/opensearch/bootstrap/MultiProviderTrustStoreHandler.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/bootstrap/MultiProviderTrustStoreHandler.java
Outdated
Show resolved
Hide resolved
❌ Gradle check result for 974cec3: 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? |
server/src/main/java/org/opensearch/bootstrap/MultiProviderTrustStoreHandler.java
Outdated
Show resolved
Hide resolved
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]>
…erts file; add bc-jsse provider 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]>
…tegy for default trust-store 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]>
974cec3
to
b08999b
Compare
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 69d46e3: 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? |
client/test/src/main/java/org/opensearch/client/BouncyCastleThreadFilter.java
Show resolved
Hide resolved
...demo-installer-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/CommonOptions.java
Show resolved
Hide resolved
buildSrc/src/main/groovy/org/opensearch/gradle/test/StandaloneRestTestPlugin.groovy
Outdated
Show resolved
Hide resolved
if (inFipsJvm()) { | ||
SecurityProviderManager.removeNonCompliantFipsProviders(); | ||
} else { | ||
addSunJceProvider(); |
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 we need addSunJceProvider()
here? It is already executed for each test case https://github.com/opensearch-project/OpenSearch/pull/18921/files#diff-2cf06f036bcc6d4f14a98424445a8fa2d5c6db18edfb88ee6f046a7fcc4754a3R38, right?
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.
To undo the JCE provider removal, we need to add it back both before each test and after the test class completes. This ensures that other test units, which run in random order after SecurityProviderManagerTests
and rely on JCE won't fail.
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 should find a way to run more of the gradle check against a FIPS compliant cluster in the future (perhaps the tests in the Rest High-Level client?) which could help to find issues early.
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.
To undo the JCE provider removal, we need to add it back both before each test and after the test class completes.
If we do exactly that - why do we need after class hook? The test order will not have any impact since before / after will be called for each test every time.
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 fixes a race condition I encountered locally after committing this test class. NoSuchProviderException
was thrown in unexpected places, suggesting that some test class isn't properly cleaning up after execution. I don't mind withdraw this change and do a proper bug-fix commit if this is more suitable, or the issue simply does not exist any longer.
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 think we are missing @After
(not @AfterClass
) to restore the original providers
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 already have @Before
method to restore the original providers for us. This setup/teardown is probably not what you expect:
@Before
@Override
public void setUp() throws Exception {
super.setUp();
addSunJceProvider();
}
@After
public void tearDown() throws Exception {
if (!inFipsJvm()) {
addSunJceProvider();
}
}
@AfterClass
// restore the same state as before running the tests.
public static void afterClass() throws Exception {
if (inFipsJvm()) {
SecurityProviderManager.removeNonCompliantFipsProviders();
}
}
...ller-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/ConfigurationProperties.java
Outdated
Show resolved
Hide resolved
...ller-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/ConfigurationProperties.java
Outdated
Show resolved
Hide resolved
…erride and simplify ConfigurationProperties#toString 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]>
3473a65
to
a5b935f
Compare
…l unit tests 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]>
- cluster.routing.allocation.disk.watermark.high=1b | ||
- cluster.routing.allocation.disk.watermark.flood_stage=1b | ||
- node.store.allow_mmap=false | ||
- "KEYSTORE_PASSWORD=${KEYSTORE_PASSWORD}" |
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.
outside of running in fips compliance mode, these will be blank?
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 that's the purpose. Docker warns you that this parameter is not set when running w/o FIPS. E.g.
WARN[0000] The "FIPS_GENERATE_TRUSTSTORE" variable is not set. Defaulting to a blank string.
Inside docker-entrypoint
the check ensures the value is true
otherwise no action is taken.
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.
@beanuwave thank you for updating this to a CLI and removing from the bootstrap flow. Overall this looks like the right direction for this change and having the separate CLI can log out to a cluster administrator what its doing to prepare the cluster to run in FIPS approved mode. I don't have any concerns adding picocli and actually think that would be a good replacement for other CLIs in this repo in the future as picocli is very widely used in the Java community.
❕ Gradle check result for 4368d76: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
...staller-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/ConfigurationService.java
Show resolved
Hide resolved
...-installer-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/TrustStoreService.java
Show resolved
Hide resolved
...ller-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/SystemTrustStoreCommand.java
Show resolved
Hide resolved
...staller-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/CreateFipsTrustStore.java
Show resolved
Hide resolved
public class CreateFipsTrustStore { | ||
|
||
private static final String JAVAX_NET_SSL_TRUST_STORE_PASSWORD = "javax.net.ssl.trustStorePassword"; | ||
private static final String TRUST_STORE_PASSWORD = Security.getProperty(JAVAX_NET_SSL_TRUST_STORE_PASSWORD); |
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 the TRUST_STORE_PASSWORD
be passed as an argument or entered from the command line?
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.
Atm it's auto-generated or entered via command line - but I think we can and should extend the CLI with an option for password.
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 updated CLI with new --password
option, please have a look.
...staller-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/CreateFipsTrustStore.java
Outdated
Show resolved
Hide resolved
...staller-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/ConfigurationService.java
Show resolved
Hide resolved
...installer-cli/src/main/java/org/opensearch/tools/cli/fips/truststore/SecureRandomHolder.java
Outdated
Show resolved
Hide resolved
Thanks @beanuwave for picking it up, did first pass, definitely missed something (the change is large) but we are getting there, thank you. |
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 f826d03: 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? |
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 7cfeb72: 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? |
Description
Provides additional build tooling to support builds in FIPS env, including a CLI trust-store installer to override
$JAVA_HOME/lib/security/cacerts
-- migrate JVM's default SSL trust store to a BCFKS-formatted one
-- use an existing PKCS#11 trust store
-- display installed 'KeyStore' providers
-- show help
-- execute above commands interactively or in script mode
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.