Validate password hashing algorithm for FIPS

[terryquigleysas]

Description

• Category (Enhancement)
• Only PBKDF2 is FIPS-compliant

Issues Resolved

#3420

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Longer-term more rationalization of the FIPS mode flags may be required.
See opensearch-project/OpenSearch#20738

Testing

Tests added

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

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]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Environment Variable The FIPS mode check relies solely on the OPENSEARCH_FIPS_MODE environment variable via System.getenv(). If OpenSearch has other mechanisms to enable FIPS mode (e.g., JVM properties, settings flags, or the opensearch.fips_mode system property), those are not checked here, potentially allowing non-FIPS-compliant algorithms to be used when FIPS is enabled through alternative means. validateFipsMode(System.getenv("OPENSEARCH_FIPS_MODE"), settings); Startup Failure The validation throws an IllegalStateException during plugin construction, which will prevent the node from starting. While this is intentional, there is no warning or graceful degradation path. If an operator has existing passwords hashed with bcrypt and enables FIPS mode, the node will fail to start with no migration path other than rehashing all passwords before enabling FIPS. Consider whether a warning log (in addition to or instead of a hard failure) would be more operationally friendly, or at least ensure the error message is surfaced clearly in startup logs. static void validateFipsMode(final String fipsModeEnvValue, final Settings settings) { if ("true".equalsIgnoreCase(fipsModeEnvValue)) { String hashingAlgorithm = settings.get( ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT ); if (!ConfigConstants.PBKDF2.equalsIgnoreCase(hashingAlgorithm)) { throw new IllegalStateException( "FIPS mode is enabled (OPENSEARCH_FIPS_MODE=true) but password hashing algorithm is set to '" + hashingAlgorithm + "'. Only PBKDF2 is allowed in FIPS mode. Set '" + ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM + "' to 'pbkdf2'. Note: changing the hashing algorithm requires all existing passwords to be rehashed." ); } } }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Guard test against default algorithm changes The test comment says "Default algorithm is bcrypt" but this assumption is hardcoded and could silently pass or fail if SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT changes to PBKDF2 in the future. The test should explicitly assert that the default is not PBKDF2, or the test name/comment should be updated to reflect that it tests whatever the current default is, making the test more robust and self-documenting. src/test/java/org/opensearch/security/OpenSearchSecurityPluginFIPSValidationTest.java [25-33] @Test public void testFipsModeWithDefaultAlgorithmThrows() { - // Default algorithm is bcrypt, which is not FIPS-compliant + // Assumes the default algorithm is not PBKDF2 (e.g., bcrypt) + // If the default changes to PBKDF2, this test should be updated accordingly + org.junit.Assume.assumeFalse( + "Test only valid when default is not PBKDF2", + ConfigConstants.PBKDF2.equalsIgnoreCase(ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT) + ); Settings settings = Settings.builder().build(); IllegalStateException ex = assertThrows( IllegalStateException.class, () -> OpenSearchSecurityPlugin.validateFipsMode("true", settings) ); Suggestion importance[1-10]: 4 __ Why: The suggestion to use Assume.assumeFalse makes the test more robust against future changes to SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT, preventing false passes if the default changes to PBKDF2. However, this is a minor maintainability improvement and the improved_code accurately reflects the change.	Low
Possible issue	Guard against null hashing algorithm value The hashingAlgorithm value retrieved from settings could potentially be null if SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT itself is null, causing a NullPointerException when ConfigConstants.PBKDF2.equalsIgnoreCase(hashingAlgorithm) is called. You should add a null check or use ConfigConstants.PBKDF2.equalsIgnoreCase(hashingAlgorithm) only after ensuring hashingAlgorithm is non-null, or use !ConfigConstants.PBKDF2.equalsIgnoreCase(hashingAlgorithm) with a fallback. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [360-366] static void validateFipsMode(final String fipsModeEnvValue, final Settings settings) { if ("true".equalsIgnoreCase(fipsModeEnvValue)) { String hashingAlgorithm = settings.get( ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM, ConfigConstants.SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT ); - if (!ConfigConstants.PBKDF2.equalsIgnoreCase(hashingAlgorithm)) { + if (hashingAlgorithm == null || !ConfigConstants.PBKDF2.equalsIgnoreCase(hashingAlgorithm)) { Suggestion importance[1-10]: 3 __ Why: The concern about null is largely theoretical since settings.get() with a default value will return the default if the key is absent. However, if SECURITY_PASSWORD_HASHING_ALGORITHM_DEFAULT itself is null, a null check would be warranted. The equalsIgnoreCase is called on ConfigConstants.PBKDF2 (not hashingAlgorithm), so a NPE would only occur if PBKDF2 constant is null, which is unlikely. The suggestion has limited practical impact.	Low

[cwperks]

Thank you @terryquigleysas ! The changes LGTM. I'll look into the test flakiness, those don't appear to be related to the changes in this PR.