Skip to content

Escape/Unescape pipe UserInfo in ThreadContext #801

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 19, 2025
Merged

Escape/Unescape pipe UserInfo in ThreadContext #801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fbdf69
Escape pipe character for injected users
shkhrj Mar 17, 2025
b06dfc5
test updates + Spotless apply
shkhrj Mar 17, 2025
66c1d0a
adding a test case with multiple pipes in username
shkhrj Mar 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/main/java/org/opensearch/commons/InjectSecurity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static org.opensearch.commons.ConfigConstants.INJECTED_USER;
import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_INJECTED_ROLES;
import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_USE_INJECTED_USER_FOR_PLUGINS;
import static org.opensearch.commons.authuser.Utils.escapePipe;

import java.util.List;
import java.util.StringJoiner;
Expand Down Expand Up @@ -143,13 +144,16 @@ public void injectUserInfo(final User user) {
);
return;
}

StringJoiner joiner = new StringJoiner("|");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to know where this serialization logic is used because I thought security was the only place where the user object would be serialized into the ThreadContext. Is this used for interplay between plugins in certain contexts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it may make sense to centralize this logic to prevent duplication, long term I'd like to remove a lot of security logic from common-utils including this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to know where this serialization logic is used because I thought security was the only place where the user object would be serialized into the ThreadContext. Is this used for interplay between plugins in certain contexts?

Yes, this is used for plugins to communicate with one another.

While it may make sense to centralize this logic to prevent duplication, long term I'd like to remove a lot of security logic from common-utils including this.

I agree, this is really needed, but that might require some security features in core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an RFC open in the security repo that outlines 4 security features that need to be built in order for plugins to migrate off of using common-utils: opensearch-project/security#5052

First, the security repo needs to support existing use cases in a model that's opt-in for plugins so that they can adopt the new mechanisms and create migration paths. Particularly, a lot of plugins store a copy of a user in their own system indices and eventually I think we should remove the copies but mechanisms need to be built to support it first.

joiner.add(user.getName());
joiner.add(java.lang.String.join(",", user.getBackendRoles()));
joiner.add(java.lang.String.join(",", user.getRoles()));
// Escape pipe characters in all fields
joiner.add(escapePipe(user.getName()));
joiner.add(escapePipe(java.lang.String.join(",", user.getBackendRoles())));
joiner.add(escapePipe(java.lang.String.join(",", user.getRoles())));

String requestedTenant = user.getRequestedTenant();
if (!Strings.isNullOrEmpty(requestedTenant)) {
joiner.add(requestedTenant);
joiner.add(escapePipe(requestedTenant));
}
threadContext.putTransient(ConfigConstants.OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT, joiner.toString());
}
Expand Down
13 changes: 8 additions & 5 deletions src/main/java/org/opensearch/commons/authuser/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.opensearch.commons.authuser;

import static org.opensearch.commons.authuser.Utils.unescapePipe;
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;

import java.io.IOException;
Expand Down Expand Up @@ -166,24 +167,26 @@ public static User parse(final String userString) {
return null;
}

String[] strs = userString.split("\\|");
// Split on unescaped pipes (negative lookbehind for backslash)
String[] strs = userString.split("(?<!\\\\)\\|");
if ((strs.length == 0) || (Strings.isNullOrEmpty(strs[0]))) {
return null;
}

String userName = strs[0].trim();
// Unescape the values
String userName = unescapePipe(strs[0].trim());
List<String> backendRoles = new ArrayList<>();
List<String> roles = new ArrayList<>();
String requestedTenant = null;

if ((strs.length > 1) && !Strings.isNullOrEmpty(strs[1])) {
backendRoles.addAll(Arrays.asList(strs[1].split(",")));
backendRoles.addAll(Arrays.stream(strs[1].split(",")).map(Utils::unescapePipe).toList());
}
if ((strs.length > 2) && !Strings.isNullOrEmpty(strs[2])) {
roles.addAll(Arrays.asList(strs[2].split(",")));
roles.addAll(Arrays.stream(strs[2].split(",")).map(Utils::unescapePipe).toList());
}
if ((strs.length > 3) && !Strings.isNullOrEmpty(strs[3])) {
requestedTenant = strs[3].trim();
requestedTenant = unescapePipe(strs[3].trim());
}
return new User(userName, backendRoles, roles, Arrays.asList(), requestedTenant);
}
Expand Down
25 changes: 25 additions & 0 deletions src/main/java/org/opensearch/commons/authuser/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.commons.authuser;

public final class Utils {

// Helper method to escape pipe characters
public static String escapePipe(String input) {
if (input == null) {
return "";
}
return input.replace("|", "\\|");
}

// Helper method to un-escape pipe characters
public static String unescapePipe(String input) {
if (input == null) {
return "";
}
return input.replace("\\|", "|");
}
}
36 changes: 36 additions & 0 deletions src/test/java/org/opensearch/commons/InjectSecurityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,42 @@ public void testInjectUserInfo() {
assertNull(threadContext.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT));
}

@Test
public void testInjectUserInfoWithPipeInUserName() {
Settings settings = Settings.builder().build();
Settings headerSettings = Settings.builder().put("request.headers.default", "1").build();
ThreadContext threadContext = new ThreadContext(headerSettings);
threadContext.putHeader("name", "opendistro");
threadContext.putTransient("ctx.name", "plugin");

assertEquals("1", threadContext.getHeader("default"));
assertEquals("opendistro", threadContext.getHeader("name"));
assertEquals("plugin", threadContext.getTransient("ctx.name"));

User user = new User(
"Bob|test-pipe",
List.of("backendRole1", "backendRole2"),
List.of("role1", "role2"),
List.of("attr1", "attr2"),
"tenant1"
);
try (InjectSecurity helper = new InjectSecurity("test-name", null, threadContext)) {
helper.injectUserInfo(user);
assertEquals("1", threadContext.getHeader("default"));
assertEquals("opendistro", threadContext.getHeader("name"));
assertEquals("plugin", threadContext.getTransient("ctx.name"));
assertNotNull(threadContext.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT));
assertEquals(
"Bob\\|test-pipe|backendRole1,backendRole2|role1,role2|tenant1",
threadContext.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT)
);
}
assertEquals("1", threadContext.getHeader("default"));
assertEquals("opendistro", threadContext.getHeader("name"));
assertEquals("plugin", threadContext.getTransient("ctx.name"));
assertNull(threadContext.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT));
}

@Test
public void testInjectProperty() {
Settings settings = Settings.builder().put(OPENSEARCH_SECURITY_USE_INJECTED_USER_FOR_PLUGINS, false).build();
Expand Down
81 changes: 81 additions & 0 deletions src/test/java/org/opensearch/commons/authuser/UserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,87 @@ public void testParseUserStringMalformed() {
assertNull(user);
}

@Test
public void testParseUserStringWithPipeInUserName() {
ThreadContext tc = new ThreadContext(Settings.EMPTY);
tc.putTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT, "myuser\\|test-pipe|bckrole1,bckrol2|role1,role2|myTenant");
String str = tc.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT);
User user = User.parse(str);

assertEquals("myuser|test-pipe", user.getName());
assertEquals(2, user.getBackendRoles().size());
assertEquals(2, user.getRoles().size());
assertTrue(user.getRoles().contains("role1"));
assertTrue(user.getRoles().contains("role2"));
assertEquals("myTenant", user.getRequestedTenant());
}

@Test
public void testParseUserStringWithMultiplePipesInUserName() {
ThreadContext tc = new ThreadContext(Settings.EMPTY);
tc
.putTransient(
OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT,
"myuser\\|test-pipe\\|test-pipe2|bckrole1,bckrol2|role1,role2|myTenant"
);
String str = tc.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT);
User user = User.parse(str);

assertEquals("myuser|test-pipe|test-pipe2", user.getName());
assertEquals(2, user.getBackendRoles().size());
assertEquals(2, user.getRoles().size());
assertTrue(user.getRoles().contains("role1"));
assertTrue(user.getRoles().contains("role2"));
assertEquals("myTenant", user.getRequestedTenant());
}

@Test
public void testParseUserStringWithPipeInBackedRoleName() {
ThreadContext tc = new ThreadContext(Settings.EMPTY);
tc.putTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT, "myuser|bckrole1\\|br1,bckrole2\\|br2|role1,role2|myTenant");
String str = tc.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT);
User user = User.parse(str);

assertEquals("myuser", user.getName());
assertEquals(2, user.getBackendRoles().size());
assertTrue(user.getBackendRoles().contains("bckrole1|br1"));
assertTrue(user.getBackendRoles().contains("bckrole2|br2"));
assertEquals(2, user.getRoles().size());
assertTrue(user.getRoles().contains("role1"));
assertTrue(user.getRoles().contains("role2"));
assertEquals("myTenant", user.getRequestedTenant());
}

@Test
public void testParseUserStringWithPipeInRoleName() {
ThreadContext tc = new ThreadContext(Settings.EMPTY);
tc.putTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT, "myuser|bckrole1,bckrol2|role1\\|r1,role2\\|r2|myTenant");
String str = tc.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT);
User user = User.parse(str);

assertEquals("myuser", user.getName());
assertEquals(2, user.getBackendRoles().size());
assertEquals(2, user.getRoles().size());
assertTrue(user.getRoles().contains("role1|r1"));
assertTrue(user.getRoles().contains("role2|r2"));
assertEquals("myTenant", user.getRequestedTenant());
}

@Test
public void testParseUserStringWithPipeInTenantName() {
ThreadContext tc = new ThreadContext(Settings.EMPTY);
tc.putTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT, "myuser|bckrole1,bckrol2|role1,role2|myTenant\\|t1");
String str = tc.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT);
User user = User.parse(str);

assertEquals("myuser", user.getName());
assertEquals(2, user.getBackendRoles().size());
assertEquals(2, user.getRoles().size());
assertTrue(user.getRoles().contains("role1"));
assertTrue(user.getRoles().contains("role2"));
assertEquals("myTenant|t1", user.getRequestedTenant());
}

@Test
public void testUserIsAdminDnTrue() {
Settings settings = Settings
Expand Down
92 changes: 92 additions & 0 deletions src/test/java/org/opensearch/commons/authuser/UtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.commons.authuser;

import static org.junit.jupiter.api.Assertions.assertEquals;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class UtilsTest {

@Test
public void testEscapePipe_NullInput() {
assertEquals("", Utils.escapePipe(null));
}

@Test
public void testEscapePipe_EmptyString() {
assertEquals("", Utils.escapePipe(""));
}

@Test
public void testEscapePipe_NoPipes() {
assertEquals("normal text", Utils.escapePipe("normal text"));
}

@Test
public void testEscapePipe_SinglePipe() {
assertEquals("before\\|after", Utils.escapePipe("before|after"));
}

@Test
public void testEscapePipe_MultiplePipes() {
assertEquals("a\\|b\\|c", Utils.escapePipe("a|b|c"));
}

@Test
public void testEscapePipe_PipeAtStart() {
assertEquals("\\|text", Utils.escapePipe("|text"));
}

@Test
public void testEscapePipe_PipeAtEnd() {
assertEquals("text\\|", Utils.escapePipe("text|"));
}

@Test
public void testUnescapePipe_NullInput() {
assertEquals("", Utils.unescapePipe(null));
}

@Test
public void testUnescapePipe_EmptyString() {
assertEquals("", Utils.unescapePipe(""));
}

@Test
public void testUnescapePipe_NoEscapedPipes() {
Assertions.assertEquals("normal text", Utils.unescapePipe("normal text"));
}

@Test
public void testUnescapePipe_SingleEscapedPipe() {
assertEquals("before|after", Utils.unescapePipe("before\\|after"));
}

@Test
public void testUnescapePipe_MultipleEscapedPipes() {
assertEquals("a|b|c", Utils.unescapePipe("a\\|b\\|c"));
}

@Test
public void testUnescapePipe_EscapedPipeAtStart() {
assertEquals("|text", Utils.unescapePipe("\\|text"));
}

@Test
public void testUnescapePipe_EscapedPipeAtEnd() {
assertEquals("text|", Utils.unescapePipe("text\\|"));
}

@Test
public void testRoundTrip_ComplexString() {
String original = "user|with|pipes";
String escaped = Utils.escapePipe(original);
String unescaped = Utils.unescapePipe(escaped);
assertEquals(original, unescaped);
}
}
Loading