Username validation for special characters (#2277)

* Only prevent user creation on colon characters, separate out tests Signed-off-by: Rutuja Surve <[email protected]> Signed-off-by: Rutuja Surve <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> (cherry picked from commit efbc48b)
opensearch-project · Dec 9, 2022 · c4c17ed · c4c17ed
1 parent ad3656c
commit c4c17ed
Show file tree

Hide file tree

Showing 2 changed files with 35 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
@@ -14,6 +14,7 @@
 import java.io.IOException;
 import java.nio.file.Path;
 import java.util.List;
+import java.util.stream.Collectors;
 
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.node.ObjectNode;
@@ -49,6 +50,10 @@
 import static org.opensearch.security.dlic.rest.support.Utils.hash;
 
 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/"),
@@ -93,6 +98,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
@@ -28,7 +28,12 @@
 import org.opensearch.security.test.helper.file.FileHelper;
 import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse;
 
+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;
+
 
 public class UserApiTest extends AbstractRestApiUnitTest {
     private final String ENDPOINT; 
@@ -468,7 +473,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());
@@ -624,7 +629,24 @@ 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\"}";
+            final HttpResponse response = rh.executePutRequest(url, bodyWithDefaultPasswordHash);
 
+            assertThat("Expected " + username + " to be rejected", response.getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST));
+            assertThat(response.getBody(), containsString(restrictedTerm));
+        });
     }
 
     @Test