Log default compliance salt warning once at startup

[MdTanwer]

Summary

• Logs the default compliance salt warning once at plugin startup via Salt.warnIfDefaultComplianceSalt(), instead of on every Salt construction (which produced duplicate WARN lines on startup).
• Keeps the warning unconditional when the default salt is in use, per maintainer feedback — even if no roles define masked_fields at bootstrap.
• Removes the warning from the Salt constructor so repeated Salt.from() calls stay silent.
• Leaves 4.0 bootstrap enforcement (Salt.validateSaltSettings()) as a TODO, gated on plugins.security.allow_unsafe_democertificates.

Fixes #6105 (duplicate/noisy startup warnings; does not suppress the warning entirely when FLS is unused).

Test plan

• ./gradlew test --tests "org.opensearch.security.configuration.SaltTest"
• ./gradlew spotlessJavaCheck
• Start a cluster without plugins.security.compliance.salt — single WARN at startup (not repeated)
• Configure a custom compliance salt — no WARN at startup

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 37a34ca)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [550]: Uncomment for 4.0 - enforce that the default compliance salt is not used outside of demo configuration

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 37a34ca

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Ensure warning runs unconditionally at startup The warnIfDefaultComplianceSalt call is placed inside the demo-certificates branch (else { throw new RuntimeException("Unable to look for demo certificates"); } block) area — verify it's actually outside that conditional. If it is inside a conditional guarded by demo/dev checks, the warning will not be emitted in production nodes where it matters most. Ensure the call executes unconditionally during plugin startup. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [548] +} + Salt.warnIfDefaultComplianceSalt(settings); // TODO: Uncomment for 4.0 - enforce that the default compliance salt is not used outside of demo configuration // Salt.validateSaltSettings(settings); Suggestion importance[1-10]: 2 __ Why: Based on the diff, the Salt.warnIfDefaultComplianceSalt(settings) call appears to be placed after the closing brace of the demo-certificates block, so it should already execute unconditionally. The suggestion is speculative and asks the user to verify existing behavior.	Low

---

Previous suggestions

Suggestions up to commit fecf36a

Category	Suggestion	Impact
General	Ensure warning runs on every startup The warning is placed inside a conditional block (after the demo certificates check) which may prevent it from being executed in all startup paths. Move Salt.warnIfDefaultComplianceSalt(settings) outside of the conditional block so it always runs once during plugin startup, regardless of demo-certificate configuration. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [548] +} Salt.warnIfDefaultComplianceSalt(settings); Suggestion importance[1-10]: 2 __ Why: Based on the diff, Salt.warnIfDefaultComplianceSalt(settings) already appears to be placed after the closing brace of the demo certificate conditional block (line 546 shows } followed by a blank line, then the call at line 548). The suggestion seems to misread the diff context, making it largely inaccurate.	Low

Suggestions up to commit 60bbd2c

Category	Suggestion	Impact
Possible issue	Avoid breaking config reload on validation Throwing an OpenSearchException from validateSaltSettings during configuration reload will propagate up through notifyConfigurationListeners and may break the reload flow or destabilize the node when an admin pushes a roles config with masked_fields and a default salt. Consider catching/logging the validation failure here (or at least logging instead of throwing post-startup), so a misconfigured role update doesn't crash the configuration listener pipeline. src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java [561-568] private void validateComplianceSaltIfNeeded(ConfigurationMap configuration) { if (!configuration.containsKey(CType.ROLES)) { return; } final SecurityDynamicConfiguration<RoleV7> rolesConfig = configuration.get(CType.ROLES); - Salt.validateSaltSettings(settings, Salt.isFieldMaskingConfigured(rolesConfig)); + try { + Salt.validateSaltSettings(settings, Salt.isFieldMaskingConfigured(rolesConfig)); + } catch (OpenSearchException e) { + LOGGER.error("Compliance salt validation failed during configuration reload: {}", e.getMessage()); + } } Suggestion importance[1-10]: 6 __ Why: Throwing during configuration reload could destabilize the listener pipeline when admins push role updates. Catching and logging is a reasonable defensive measure, though it changes the intended enforcement semantics.	Low
	Guard against null collections in roles getIndex_permissions() or getMasked_fields() can return null for partially populated role configurations, causing a NullPointerException during reload. Guard against null collections before streaming/inspecting them. src/main/java/org/opensearch/security/configuration/Salt.java [93-97] -return rolesConfig.getCEntries() - .values() - .stream() - .flatMap(role -> role.getIndex_permissions().stream()) - .anyMatch(index -> !index.getMasked_fields().isEmpty()); +return rolesConfig.getCEntries().values().stream().filter(role -> role.getIndex_permissions() != null).flatMap(role -> role.getIndex_permissions().stream()).anyMatch(index -> index.getMasked_fields() != null && !index.getMasked_fields().isEmpty()); Suggestion importance[1-10]: 5 __ Why: Guarding against null getIndex_permissions() or getMasked_fields() adds defensive safety, though these getters typically return empty collections in the RoleV7 model. The suggestion is reasonable but of moderate impact.	Low

Suggestions up to commit f2c09bb

Category	Suggestion	Impact
Possible issue	Null-guard role permission traversal The accessors getIndex_permissions() and getMasked_fields() may return null for roles where these fields are not configured (common for YAML-deserialized objects). Add null-safety to avoid NullPointerException during role configuration loading, which could prevent the cluster from starting or reloading roles. src/main/java/org/opensearch/security/configuration/Salt.java [88-98] public static boolean isFieldMaskingConfigured(final SecurityDynamicConfiguration<RoleV7> rolesConfig) { if (rolesConfig == null) { return false; } return rolesConfig.getCEntries() .values() .stream() + .filter(role -> role != null && role.getIndex_permissions() != null) .flatMap(role -> role.getIndex_permissions().stream()) - .anyMatch(index -> !index.getMasked_fields().isEmpty()); + .anyMatch(index -> index != null && index.getMasked_fields() != null && !index.getMasked_fields().isEmpty()); } Suggestion importance[1-10]: 6 __ Why: Adding null-safety to getIndex_permissions() and getMasked_fields() is a reasonable defensive measure for YAML-deserialized objects, though the existing code may already handle this via default initialization. Moderately useful for robustness.	Low
General	Avoid breaking config reload on validation validateSaltSettings throws OpenSearchException when masked_fields are configured with the default salt. Since this is invoked from notifyConfigurationListeners during every roles config reload, throwing here will propagate up and could prevent legitimate configuration updates from being applied. Consider catching and logging the error (or only enforcing on initial load) so that subsequent listeners still run. src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java [561-568] private void validateComplianceSaltIfNeeded(ConfigurationMap configuration) { if (!configuration.containsKey(CType.ROLES)) { return; } final SecurityDynamicConfiguration<RoleV7> rolesConfig = configuration.get(CType.ROLES); - Salt.validateSaltSettings(settings, Salt.isFieldMaskingConfigured(rolesConfig)); + try { + Salt.validateSaltSettings(settings, Salt.isFieldMaskingConfigured(rolesConfig)); + } catch (OpenSearchException e) { + LOGGER.error("Compliance salt validation failed during roles configuration reload", e); + } } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern that throwing during config reload could prevent listeners from running, but the intended behavior (strict enforcement) may be desirable. The recommendation is a design tradeoff rather than a clear bug fix.	Low

[github-actions]

Persistent review updated to latest commit 60bbd2c

[cwperks]

@MdTanwer IMO This warning should always be logged if this salt is unconfigured even if any roles at bootstrap don't have masking. In fact, in the next major version I'd like to fail bootstrap if its unconfigured unless plugins.security.allow_unsafe_democertificates is set to true. This is the setting that explicitly allows a cluster to run with demo security settings.

[MdTanwer]

@cwperks Thanks for the feedback — I've revised the PR accordingly.

• The default compliance salt warning is now logged unconditionally at plugin startup (via Salt.warnIfDefaultComplianceSalt()), even when no roles define masked_fields.
• The warning was removed from the Salt constructor to fix the duplicate 3× WARNING.

This keeps the always-warn behavior . Please let me know if this looks good.

[github-actions]

Persistent review updated to latest commit fecf36a

[github-actions]

Persistent review updated to latest commit 37a34ca

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.09%. Comparing base (69c5180) to head (37a34ca).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6253      +/-   ##
==========================================
+ Coverage   75.02%   75.09%   +0.06%     
==========================================
  Files         451      451              
  Lines       29275    29279       +4     
  Branches     4416     4417       +1     
==========================================
+ Hits        21964    21986      +22     
+ Misses       5274     5257      -17     
+ Partials     2037     2036       -1     

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	85.19% <100.00%> (+0.01%)	⬆️
...va/org/opensearch/security/configuration/Salt.java	96.42% <100.00%> (+0.27%)	⬆️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.