Use new SecureRandom() to avoid blocking

[andrross]

This class was recently changed to use SecureRandom.getInstanceStrong() instead of new SecureRandom(). This results in a blocking call on certain systems if the entropy is exhausted. This commit reverts back to the previous implementation in the non-FIPS case to avoid problems with blocking.

Related Issues

Resolves #18729

Check List

• Functionality includes testing.

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.

[github-actions]

❌ Gradle check result for b8d73a6: 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?

[github-actions]

❌ Gradle check result for f51e9c8: 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?

[github-actions]

❌ Gradle check result for 3f47eb3: 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?

[github-actions]

✅ Gradle check result for 3f47eb3: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.85%. Comparing base (8f69dcf) to head (3f47eb3).
⚠️ Report is 97 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18758      +/-   ##
============================================
+ Coverage     72.79%   72.85%   +0.05%     
- Complexity    68525    68529       +4     
============================================
  Files          5574     5563      -11     
  Lines        314807   314437     -370     
  Branches      45675    45627      -48     
============================================
- Hits         229178   229071     -107     
+ Misses        67046    66830     -216     
+ Partials      18583    18536      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[reta]

@beanuwave FYI, I think this change is a step back towards FIPS compliance (but addresses the issue with hanging service)

[peterzhuamazon]

Hi @andrross thanks for this PR.
Assume we dont need to backport this to 2.19 (assume FIPS only merged in main).

Thanks.

[andrross]

@beanuwave FYI, I think this change is a step back towards FIPS compliance (but addresses the issue with hanging service)

@reta I honestly don't fully understand the compliance requirements, but in FIPS mode won't we hit this case?

OpenSearch/server/src/main/java/org/opensearch/common/Randomness.java

Line 169 in 3f47eb3

return (SecureRandom) drbgInstance;

[andrross]

@peterzhuamazon Correct, no backport to 2.19 needed

[reta]

@reta I honestly don't fully understand the compliance requirements, but in FIPS mode won't we hit this case?

The fallback [1] is still using SecureRandom.getInstanceStrong(), ...

OpenSearch/server/src/main/java/org/opensearch/common/Randomness.java

Line 175 in 52b102e

return SecureRandom.getInstanceStrong();

[YeonghyeonKO]

@andrross Thanks for fixing the issue quickly!
Will this commit be released as a fix starting from version 3.2, or will it be backported to version 3.0?
(ex. 3.0.1 & 3.1.2 ?)

[andrross]

@YeonghyeonKO This fix will be available in 3.2, scheduled to be released next month.

[andrross]

@reta I honestly don't fully understand the compliance requirements, but in FIPS mode won't we hit this case?

The fallback [1] is still using SecureRandom.getInstanceStrong(), ...

OpenSearch/server/src/main/java/org/opensearch/common/Randomness.java

Line 175 in 52b102e

return SecureRandom.getInstanceStrong();

@reta @cwperks @beanuwave What's the right thing to do here? Should that fallback also be doing new SecureRandom()?

[opensearch-trigger-bot]

The backport to 3.2 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-3.2 3.2
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-3.2
# Create a new branch
git switch --create backport/backport-18758-to-3.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 52b102e36d7dc3f309864e8e4d5e023375a93830
# Push it to GitHub
git push --set-upstream origin backport/backport-18758-to-3.2
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-3.2

Then, create a pull request where the base branch is 3.2 and the compare/head branch is backport/backport-18758-to-3.2.

[andrross]

@cwperks This was merged 3 weeks ago, way before the 3.2 branch was created. Why did you add the backport label?

[cwperks]

@cwperks This was merged 3 weeks ago, way before the 3.2 branch was created. Why did you add the backport label?

I meant to add the v3.2.0 label >.<