From 3f98b4a4b85478c680c75a26a9a7f9468489a91a Mon Sep 17 00:00:00 2001
From: rutuja-amazon <110013621+rutuja-amazon@users.noreply.github.com>
Date: Sat, 10 Dec 2022 02:41:42 +0530
Subject: [PATCH] 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 efbc48b405ff6ff372c4821aa15c53fc50a409a3)
---
 .../dlic/rest/api/InternalUsersApiAction.java | 13 ++++++++++
 .../security/dlic/rest/api/UserApiTest.java   | 24 ++++++++++++++++++-
 2 files changed, 36 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 f1754bbfde..07cef99fd7 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,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
index 13ea2fe359..cde3bbd635 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,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,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