support fallback values in DLS/FLS variables

[rursprung]

Description

• Category: Enhancement

see the individual commit messages for details. basically this uses StringSubstitutor from apache-commons text to re-create the existing behaviour (first commit) and also support fallbacks in the variables.

if a user attribute is not defined then the system fails with an exception. in 2.x this was silently ignored, allowing use-cases where attributes were optional.

with this we now support fallbacks, thus it's now possible to e.g. write ${attr.proxy.attr2:-foo} so it prints "foo" if attr2 is not defined or even ${attr.proxy.attr2:-${attr.proxy.attr1}} to fall back to the value of attr1 if attr2 is not defined.

Issues Resolved

resolves #6110

Testing

unit testing

Check List

• New functionality includes testing
• New functionality has been documented - docs PR will be raised once this PR is approved
• 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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.78%. Comparing base (e29af48) to head (435abbf).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6111      +/-   ##
==========================================
+ Coverage   74.75%   74.78%   +0.02%     
==========================================
  Files         447      447              
  Lines       28480    28470      -10     
  Branches     4331     4327       -4     
==========================================
  Hits        21291    21291              
+ Misses       5193     5189       -4     
+ Partials     1996     1990       -6     

Files with missing lines	Coverage Δ
...opensearch/security/privileges/UserAttributes.java	94.44% <100.00%> (+12.30%)	⬆️

... 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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 87f0013.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/privileges/UserAttributes.java	63	critical	Deliberate introduction of recursive template injection in a security plugin. StringSubstitutor is constructed with setEnableSubstitutionInVariables(true), which enables dynamic variable-name construction (e.g., ${${attr.evil}} resolves attr.evil as a variable name). The replacement map is populated with user-controlled data via user.getCustomAttributesMap(), so an attacker who controls a custom attribute value can craft it to reference other substitution variables (e.g., ${user.securityRoles}), enabling recursive expansion and privilege escalation in DLS queries or index patterns. The original code used non-recursive String.replace() calls precisely to avoid this. The test case added only requires StringSubstitutor's default-value (:-) syntax, NOT setEnableSubstitutionInVariables(true), making this flag unjustified by any stated feature and strongly indicative of intentional backdoor behavior. This mirrors the attack surface of CVE-2022-42889 (Text4Shell), which was patched by disabling exactly this class of recursive substitution.
build.gradle	749	high	Dependency configuration change: org.apache.commons:commons-text:1.15.0 promoted from runtimeOnly to implementation scope. While the artifact version is unchanged, this is a dependency configuration change that must be flagged per mandatory supply chain review rules. Maintainers should verify this scope promotion is intentional and does not expose commons-text APIs to downstream consumers in unintended ways.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 1 | High: 1 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[rursprung]

the AI-analysis is wrong: without .setEnableSubstitutionInVariables(true) the test case with ${attr.proxy.attr2:-${attr.proxy.attr1}} does not work:

org.junit.ComparisonFailure: expected:<...alue1",
    "baz": "[value1]",
    "roles": ["ro...> but was:<...alue1",
    "baz": "[${attr.proxy.attr1}]",
    "roles": ["ro...>
	at org.junit.Assert.assertEquals(Assert.java:117)
	at org.junit.Assert.assertEquals(Assert.java:146)
	at org.opensearch.security.privileges.UserAttributesUnitTest.testReplaceProperties(UserAttributesUnitTest.java:78)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)

this is why i did it in the first place.

security-wise i think this change is ok, we'll just have to document that ${${attr.something}} is a really bad idea (i'm not sure why anyone would do this, anyway). we could even forcibly check for ${${ and throw an exception if we see that to prevent it completely?

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 435abbf)

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 Null Value Handling The old code explicitly skipped null keys or values in getCustomAttributesMap(). The new code uses putAll without any null checks. If getCustomAttributesMap() returns entries with null keys or null values, StringSubstitutor may throw a NullPointerException or behave unexpectedly. replacementsWithDots.putAll(user.getCustomAttributesMap()); Undefined Variable Behavior By default, StringSubstitutor leaves unresolved variables as-is (e.g., ${unknown.var} stays unchanged). The old code also left them unchanged. However, the PR description mentions that in the old behavior an undefined attribute caused an exception. It should be verified that the new behavior (silently leaving unresolved variables) is intentional and consistent with the described fallback feature, and that the findUnresolvedAttributes method is still called appropriately upstream to enforce validation when needed. final var stringSubstitutor = new StringSubstitutor(replacements).setEnableSubstitutionInVariables(true); orig = stringSubstitutor.replace(orig); return orig; Underscore Collision When building the replacements map, dot-to-underscore variants are added for all keys. If a custom attribute key already uses underscores (e.g., attr.proxy_attr1), its underscore-replaced form could collide with another key's dot-replaced form, causing incorrect substitution. This edge case should be validated. final var replacements = new HashMap<>(replacementsWithDots); replacementsWithDots.forEach((k, v) -> replacements.put(k.replace(".", "_"), v));

[cwperks]

security-wise i think this change is ok, we'll just have to document that ${${attr.something}} is a really bad idea (i'm not sure why anyone would do this, anyway). we could even forcibly check for ${${ and throw an exception if we see that to prevent it completely?

Given that the input is from admin and just not any API caller its fine. We couldn't certainly add better validation on Security APIs but not in the scope of this PR.

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 435abbf
Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Disable recursive variable name substitution to prevent injection setEnableSubstitutionInVariables(true) enables recursive variable substitution inside variable names, not just in default values. This means a user-controlled attribute value could be crafted to reference and expose other variables (e.g., user.name), leading to a potential information disclosure or injection attack. Unless recursive variable-name substitution is explicitly required, this flag should be left as false (the default). src/main/java/org/opensearch/security/privileges/UserAttributes.java [59-60] -final var stringSubstitutor = new StringSubstitutor(replacements).setEnableSubstitutionInVariables(true); +final var stringSubstitutor = new StringSubstitutor(replacements); orig = stringSubstitutor.replace(orig); Suggestion importance[1-10]: 8 __ Why: This is a valid security concern - setEnableSubstitutionInVariables(true) allows recursive substitution in variable names, which could enable user-controlled attribute values to reference other variables. However, the test case in the PR explicitly tests a default-value fallback (${attr.proxy.notexists:-${attr.proxy.attr1}}), which requires this flag to work correctly, so removing it would break the intended functionality.	Medium
Possible issue	Avoid unintended key collisions in replacement map When building the underscore-variant map, keys from user.getCustomAttributesMap() that already use underscores (e.g. attr_proxy_attr1) will overwrite the dot-variant entries added by putAll. More critically, if a custom attribute key contains dots (e.g. attr.proxy.attr1), the underscore replacement attr_proxy_attr1 may silently collide with another key. Consider only applying the dot-to-underscore transformation to the well-known user.* keys, not to all custom attribute keys. src/main/java/org/opensearch/security/privileges/UserAttributes.java [56-57] final var replacements = new HashMap<>(replacementsWithDots); -replacementsWithDots.forEach((k, v) -> replacements.put(k.replace(".", "_"), v)); +// Only apply dot->underscore aliasing for well-known user.* keys, not custom attributes +// (custom attributes already use their own key format) +List.of("user.name", "user.roles", "user.securityRoles").forEach(k -> + replacements.put(k.replace(".", "_"), replacementsWithDots.get(k)) +); Suggestion importance[1-10]: 5 __ Why: The concern about key collisions is valid in theory, but the old code also applied dot-to-underscore replacement for all custom attributes. The suggestion changes existing behavior and may break backward compatibility. The collision risk is real but limited in practice since custom attribute keys typically follow a consistent naming convention.	Low

---

Previous suggestions

Suggestions up to commit 87f0013

Category	Suggestion	Impact
Security	Disable recursive variable name substitution setEnableSubstitutionInVariables(true) enables recursive variable substitution inside variable names, not just in values. This means a user-controlled attribute value could influence which variable is looked up next, potentially allowing privilege escalation by crafting an attribute value that resolves to another variable name. Unless recursive variable-name resolution is intentionally required, this flag should be left at its default (false). src/main/java/org/opensearch/security/privileges/UserAttributes.java [59-60] -final var stringSubstitutor = new StringSubstitutor(replacements).setEnableSubstitutionInVariables(true); +final var stringSubstitutor = new StringSubstitutor(replacements); orig = stringSubstitutor.replace(orig); Suggestion importance[1-10]: 8 __ Why: setEnableSubstitutionInVariables(true) enables recursive substitution inside variable names, which could allow user-controlled attribute values to influence variable resolution and potentially cause security issues. However, the test case in the PR (${attr.proxy.attr2:-${attr.proxy.attr1}}) uses default value syntax which requires this flag, so disabling it would break the new test functionality.	Medium
Possible issue	Limit underscore aliasing to built-in keys only When building the underscore-variant map, keys from user.getCustomAttributesMap() that already use underscores (e.g. attr_proxy_attr1) will overwrite the dot-variant entries that were just copied into replacements. More critically, if a custom attribute key contains dots, the dot-to-underscore replacement may silently collide with another key. Consider only applying the dot-to-underscore transformation to the well-known built-in keys (user.name, user.roles, user.securityRoles) rather than all entries including custom attributes. src/main/java/org/opensearch/security/privileges/UserAttributes.java [56-57] final var replacements = new HashMap<>(replacementsWithDots); -replacementsWithDots.forEach((k, v) -> replacements.put(k.replace(".", "_"), v)); +List.of("user.name", "user.roles", "user.securityRoles").forEach(k -> { + if (replacementsWithDots.containsKey(k)) { + replacements.put(k.replace(".", "_"), replacementsWithDots.get(k)); + } +}); Suggestion importance[1-10]: 5 __ Why: The concern about key collisions when applying dot-to-underscore transformation to custom attributes is valid in theory, but the old code also applied the same transformation to all custom attributes. The suggestion changes behavior that may be intentional. The risk of collision is real but limited in practice.	Low

[github-actions]

Persistent review updated to latest commit 435abbf

[rursprung]

rebased & incorporated @cwperks' slack feedback (attr2 -> notexists in the test to make it more obvious)

[nibix]

Look good to me!

IMHO, the whole attribute substitution is in a bad need of a complete redesign, but this is a good enhancement! 👍

[rursprung]

thanks! btw, the CI failure is likely a false reject (it worked fine before and i only rebased).

[reta]

I think there are two concerns from my side:

• new failure mode with cycles (already brought here https://github.com/opensearch-project/security/pull/6111/changes#r3133408405)
• bringing a whole new library for simple string substitution (if we want to keep it simple) looks excessive to me

Do we want a full power of StringSubstitutor? May be we do, from other perspective keeping it very simple, non recursive, would be safer I believe.

[rursprung]

as said, i need the option "if A is not set fall back to B" where both A and B are attributes (in my case on the JWT).
in 2.x we solved this with two queries in a bool which worked because it just ignored the non-existent variable and this part then didn't match.
having the proper fallback will also be good for our performance (this is actually a TLQ we're doing, so doing it once instead of twice would be a big win)

as for bringing in a new library: it was already present as runtimeOnly - the only difference is that now i can also use it in this code base rather than it just being around at runtime for some other dependencies. so i don't see this is a big problem?

[reta]

as said, i need the option "if A is not set fall back to B" where both A and B are attributes (in my case on the JWT).

This part is fine, probably was not clearly articulated, +1 to have fallback, but StringSubstitutor will throw an exception on recursive substitution, whereas we could not push that as far by extending the existing manual substitution.

so i don't see this is a big problem?

you are actually very right, we bundle it even now, non issue

[rursprung]

but StringSubstitutor will throw an exception on recursive substitution, whereas we could not push that as far by extending the existing manual substitution.

ah, ok. this isn't a regression as this wasn't possible so far. and i think also a custom implementation would have to judge what to do after the first round of replacement. in the current implementation (with no fallback) it also raises an exception on finding a placeholder after having replaced the known ones:

security/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java

Lines 177 to 179 in 18b1a03

	throw new PrivilegesEvaluationException(
	"DLS query references undefined user attributes: " + unresolved + ". Available attributes are: " + availableStr,
	new OpenSearchSecurityException("User attribute substitution failed")

and i think/hope that people will not write such complex replacement code in a DLS (then again: who knows? i'm to blame for even having TLQ support in DLS 😅) and if they do that they'll test it.

[rursprung]

btw, talking of exceptions: there's StringSubstitutor#setEnableUndefinedVariableException which could be used as a replacement for the manual check written by @ThyTran1402 (the IllegalArgumentException would then still have to be converted to the current exception to preserve having the "nice" output if you want to keep that)

but i'd strongly suggest that this is a follow-up cleanup