From a8254e0df089164695d182adbf097544651171ab Mon Sep 17 00:00:00 2001
From: Rutuja Surve <rutuja@amazon.com>
Date: Tue, 13 Dec 2022 12:33:55 +0530
Subject: [PATCH] fix username validation for internal user api

Signed-off-by: Rutuja Surve <rutuja@amazon.com>
---
 .../dlic/rest/api/InternalUsersApiAction.java | 12 ++++++++
 .../security/dlic/rest/api/UserApiTest.java   | 29 ++++++++++++++++++-
 2 files changed, 40 insertions(+), 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
index 3a849c1277..541420b6e7 100644
--- 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,17 @@
 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 +101,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
index 051b71478e..ebf824979b 100644
--- 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,11 @@
 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 +486,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());
@@ -641,6 +645,29 @@ public void testUserApiForNonSuperAdmin() throws Exception {
 
     }
 
+    @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
     public void checkNullElementsInArray() throws Exception{
         setup();