[Backport 1.3] Username validation for special characters (#2277) (#2319

) * Username validation for special characters (#2277) * Only prevent user creation on colon characters, separate out tests Signed-off-by: Rutuja Surve <rutuja@amazon.com> Signed-off-by: Rutuja Surve <110013621+rutuja-amazon@users.noreply.github.com> Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com> (cherry picked from commit efbc48b)
opensearch-project · Dec 15, 2022 · f48fcdc · f48fcdc
1 parent aafce5c
commit f48fcdc
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 1 deletion.
diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
@@ -46,12 +46,18 @@
 import java.io.IOException;
 import java.nio.file.Path;
 import java.util.List;
+import java.util.stream.Collectors;
+
 import com.google.common.collect.ImmutableList;
 
 import static org.opensearch.security.dlic.rest.support.Utils.hash;
 import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
 
 public class InternalUsersApiAction extends PatchableResourceApiAction {
+    static final List<String> RESTRICTED_FROM_USERNAME = ImmutableList.of(
+        ":" // Not allowed in basic auth, see https://stackoverflow.com/a/33391003/533057
+    );
+
     private static final List<Route> routes = addRoutesPrefix(ImmutableList.of(
             new Route(Method.GET, "/user/{name}"),
             new Route(Method.GET, "/user/"),
@@ -96,6 +102,13 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
             return;
         }
 
+        final List<String> foundRestrictedContents = RESTRICTED_FROM_USERNAME.stream().filter(username::contains).collect(Collectors.toList());
+        if (!foundRestrictedContents.isEmpty()) {
+            final String restrictedContents = foundRestrictedContents.stream().map(s -> "'" + s + "'").collect(Collectors.joining(","));
+            badRequestResponse(channel, "Username has restricted characters " + restrictedContents + " that are not permitted.");
+            return;
+        }
+
         // TODO it might be sensible to consolidate this with the overridden method in
         // order to minimize duplicated logic
 

diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java
@@ -35,7 +35,12 @@
 import java.util.List;
 
 import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
 import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX;
+import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.RESTRICTED_FROM_USERNAME;
+
 
 @RunWith(Parameterized.class)
 public class UserApiTest extends AbstractRestApiUnitTest {
@@ -482,7 +487,7 @@ public void testPasswordRules() throws Exception {
         addUserWithPassword("$1aAAAAAAAac", "$1aAAAAAAAAC", HttpStatus.SC_BAD_REQUEST);
         addUserWithPassword(URLEncoder.encode("$1aAAAAAAAac%", "UTF-8"), "$1aAAAAAAAAC%", HttpStatus.SC_BAD_REQUEST);
         addUserWithPassword(URLEncoder.encode("$1aAAAAAAAac%!=\"/\\;:test&~@^", "UTF-8").replace("+", "%2B"), "$1aAAAAAAAac%!=\\\"/\\\\;:test&~@^", HttpStatus.SC_BAD_REQUEST);
-        addUserWithPassword(URLEncoder.encode("$1aAAAAAAAac%!=\"/\\;: test&", "UTF-8"), "$1aAAAAAAAac%!=\\\"/\\\\;: test&123", HttpStatus.SC_CREATED);
+        addUserWithPassword(URLEncoder.encode("$1aAAAAAAAac%!=\"/\\;: test&", "UTF-8"), "$1aAAAAAAAac%!=\\\"/\\\\;: test&123", HttpStatus.SC_BAD_REQUEST);
 
         response = rh.executeGetRequest(PLUGINS_PREFIX + "/api/internalusers/nothinghthere?pretty", new Header[0]);
         Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
@@ -638,7 +643,29 @@ public void testUserApiForNonSuperAdmin() throws Exception {
         // Patch multiple hidden users
         response = rh.executePatchRequest(ENDPOINT + "/internalusers", "[{ \"op\": \"add\", \"path\": \"/hide/description\", \"value\": \"foo\" }]", new Header[0]);
         Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
+    }
+
+    @Test
+    public void restrictedUsernameContents() throws Exception {
+        setup();
 
+        rh.keystore = "restapi/kirk-keystore.jks";
+        rh.sendAdminCertificate = true;
+
+        RESTRICTED_FROM_USERNAME.stream().forEach(restrictedTerm -> {
+            final String username = "nag" + restrictedTerm + "ilum";
+            final String url = ENDPOINT + "/internalusers/" + username;
+            final String bodyWithDefaultPasswordHash = "{\"hash\": \"456\"}";
+            HttpResponse response = null;
+            try {
+                response = rh.executePutRequest(url, bodyWithDefaultPasswordHash);
+            } catch (final Exception e) {
+                Assert.fail("Unexpected exception " + e);
+            }
+            Assert.assertNotNull("Should have a non-null response object", response);
+            assertThat("Expected " + username + " to be rejected", response.getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST));
+            assertThat(response.getBody(), containsString(restrictedTerm));
+        });
     }
 
     @Test