From 5fbdf6925f21c156eef7ac633d5a42bbcb59d408 Mon Sep 17 00:00:00 2001 From: Shikhar Jain Date: Mon, 17 Mar 2025 11:44:56 +0530 Subject: [PATCH 1/3] Escape pipe character for injected users Signed-off-by: Shikhar Jain --- .../opensearch/commons/InjectSecurity.java | 12 ++- .../org/opensearch/commons/authuser/User.java | 18 +++- .../opensearch/commons/authuser/Utils.java | 25 +++++ .../commons/InjectSecurityTest.java | 36 +++++++ .../opensearch/commons/authuser/UserTest.java | 62 +++++++++++ .../commons/authuser/UtilsTest.java | 102 ++++++++++++++++++ 6 files changed, 246 insertions(+), 9 deletions(-) create mode 100644 src/main/java/org/opensearch/commons/authuser/Utils.java create mode 100644 src/test/java/org/opensearch/commons/authuser/UtilsTest.java diff --git a/src/main/java/org/opensearch/commons/InjectSecurity.java b/src/main/java/org/opensearch/commons/InjectSecurity.java index e6b283d9..b4a354ad 100644 --- a/src/main/java/org/opensearch/commons/InjectSecurity.java +++ b/src/main/java/org/opensearch/commons/InjectSecurity.java @@ -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; @@ -143,13 +144,16 @@ public void injectUserInfo(final User user) { ); return; } + StringJoiner joiner = new StringJoiner("|"); - 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()); } diff --git a/src/main/java/org/opensearch/commons/authuser/User.java b/src/main/java/org/opensearch/commons/authuser/User.java index 3e507857..1a9d41a2 100644 --- a/src/main/java/org/opensearch/commons/authuser/User.java +++ b/src/main/java/org/opensearch/commons/authuser/User.java @@ -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; @@ -166,28 +167,35 @@ 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("(? backendRoles = new ArrayList<>(); List 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); } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder diff --git a/src/main/java/org/opensearch/commons/authuser/Utils.java b/src/main/java/org/opensearch/commons/authuser/Utils.java new file mode 100644 index 00000000..ad04390e --- /dev/null +++ b/src/main/java/org/opensearch/commons/authuser/Utils.java @@ -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("\\|", "|"); + } +} diff --git a/src/test/java/org/opensearch/commons/InjectSecurityTest.java b/src/test/java/org/opensearch/commons/InjectSecurityTest.java index b2dea7f3..6e20ca73 100644 --- a/src/test/java/org/opensearch/commons/InjectSecurityTest.java +++ b/src/test/java/org/opensearch/commons/InjectSecurityTest.java @@ -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(); diff --git a/src/test/java/org/opensearch/commons/authuser/UserTest.java b/src/test/java/org/opensearch/commons/authuser/UserTest.java index df4e6602..94f30fee 100644 --- a/src/test/java/org/opensearch/commons/authuser/UserTest.java +++ b/src/test/java/org/opensearch/commons/authuser/UserTest.java @@ -205,6 +205,68 @@ 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 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 diff --git a/src/test/java/org/opensearch/commons/authuser/UtilsTest.java b/src/test/java/org/opensearch/commons/authuser/UtilsTest.java new file mode 100644 index 00000000..65888695 --- /dev/null +++ b/src/test/java/org/opensearch/commons/authuser/UtilsTest.java @@ -0,0 +1,102 @@ +/* + * 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); + } + + @Test + public void testRoundTrip_AlreadyEscapedString() { + String original = "user\\|with\\|escaped\\|pipes"; + String escaped = Utils.escapePipe(original); + String unescaped = Utils.unescapePipe(escaped); + // Note: This might need adjustment based on your intended behavior + // Currently it will double-escape and then unescape once + assertEquals("user\\|with\\|escaped\\|pipes", unescaped); + } +} From b06dfc561d45b0d9106c652e50a5866b708a0813 Mon Sep 17 00:00:00 2001 From: Shikhar Jain Date: Mon, 17 Mar 2025 12:01:14 +0530 Subject: [PATCH 2/3] test updates + Spotless apply Signed-off-by: Shikhar Jain --- .../java/org/opensearch/commons/authuser/User.java | 9 ++------- .../org/opensearch/commons/InjectSecurityTest.java | 14 +++++++------- .../org/opensearch/commons/authuser/UtilsTest.java | 10 ---------- 3 files changed, 9 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/opensearch/commons/authuser/User.java b/src/main/java/org/opensearch/commons/authuser/User.java index 1a9d41a2..82db0747 100644 --- a/src/main/java/org/opensearch/commons/authuser/User.java +++ b/src/main/java/org/opensearch/commons/authuser/User.java @@ -180,14 +180,10 @@ public static User parse(final String userString) { String requestedTenant = null; if ((strs.length > 1) && !Strings.isNullOrEmpty(strs[1])) { - backendRoles.addAll(Arrays.stream(strs[1].split(",")) - .map(Utils::unescapePipe) - .toList()); + backendRoles.addAll(Arrays.stream(strs[1].split(",")).map(Utils::unescapePipe).toList()); } if ((strs.length > 2) && !Strings.isNullOrEmpty(strs[2])) { - roles.addAll(Arrays.stream(strs[2].split(",")) - .map(Utils::unescapePipe) - .toList()); + roles.addAll(Arrays.stream(strs[2].split(",")).map(Utils::unescapePipe).toList()); } if ((strs.length > 3) && !Strings.isNullOrEmpty(strs[3])) { requestedTenant = unescapePipe(strs[3].trim()); @@ -195,7 +191,6 @@ public static User parse(final String userString) { return new User(userName, backendRoles, roles, Arrays.asList(), requestedTenant); } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder diff --git a/src/test/java/org/opensearch/commons/InjectSecurityTest.java b/src/test/java/org/opensearch/commons/InjectSecurityTest.java index 6e20ca73..19cc365c 100644 --- a/src/test/java/org/opensearch/commons/InjectSecurityTest.java +++ b/src/test/java/org/opensearch/commons/InjectSecurityTest.java @@ -137,11 +137,11 @@ public void testInjectUserInfoWithPipeInUserName() { 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" + "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); @@ -150,8 +150,8 @@ public void testInjectUserInfoWithPipeInUserName() { 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) + "Bob\\|test-pipe|backendRole1,backendRole2|role1,role2|tenant1", + threadContext.getTransient(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT) ); } assertEquals("1", threadContext.getHeader("default")); diff --git a/src/test/java/org/opensearch/commons/authuser/UtilsTest.java b/src/test/java/org/opensearch/commons/authuser/UtilsTest.java index 65888695..7c1ff2ed 100644 --- a/src/test/java/org/opensearch/commons/authuser/UtilsTest.java +++ b/src/test/java/org/opensearch/commons/authuser/UtilsTest.java @@ -89,14 +89,4 @@ public void testRoundTrip_ComplexString() { String unescaped = Utils.unescapePipe(escaped); assertEquals(original, unescaped); } - - @Test - public void testRoundTrip_AlreadyEscapedString() { - String original = "user\\|with\\|escaped\\|pipes"; - String escaped = Utils.escapePipe(original); - String unescaped = Utils.unescapePipe(escaped); - // Note: This might need adjustment based on your intended behavior - // Currently it will double-escape and then unescape once - assertEquals("user\\|with\\|escaped\\|pipes", unescaped); - } } From 66c1d0a2dc32e1497799035497ca834f8c86852d Mon Sep 17 00:00:00 2001 From: Shikhar Jain Date: Tue, 18 Mar 2025 23:25:05 +0530 Subject: [PATCH 3/3] adding a test case with multiple pipes in username Signed-off-by: Shikhar Jain --- .../opensearch/commons/authuser/UserTest.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/test/java/org/opensearch/commons/authuser/UserTest.java b/src/test/java/org/opensearch/commons/authuser/UserTest.java index 94f30fee..1e0db06c 100644 --- a/src/test/java/org/opensearch/commons/authuser/UserTest.java +++ b/src/test/java/org/opensearch/commons/authuser/UserTest.java @@ -220,6 +220,25 @@ public void testParseUserStringWithPipeInUserName() { 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);