Username validation for special characters #2277

rutuja-amazon · 2022-11-25T10:11:34Z

Add username validation:

Username should not include special characters

shikharj05 · 2022-11-25T10:23:31Z

Thank you for the change. Can you add tests as well?

rutuja-amazon · 2022-11-25T10:49:53Z

Added unit tests.

cwperks · 2022-11-25T14:20:18Z

@rutuja-amazon Should I add any backport labels to this PR?

cliu123 · 2022-11-28T06:04:15Z

src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java

@@ -133,6 +133,10 @@ public void testUserApi() throws Exception {
        response = rh.executePutRequest(ENDPOINT + "/internalusers/", "{\"hash\": \"123\"}", new Header[0]);
        Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode());

+        // username has special characters
+        response = rh.executePutRequest(ENDPOINT + "/internalusers/n@ag:ilum", "{\"hash\": \"123\"}", new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode());


Is this test case specifically related to this fix? If yes, why isn't it also HttpStatus.SC_BAD_REQUEST.

Yes, we need to updated all test cases that use special characters in username to now fail at creation

Agreed. Then shouldn't this PUT request with special characters in username get HttpStatus.SC_BAD_REQUEST, which would be consistent with the fix?

Sounds good, updated to HttpStatus.SC_BAD_REQUEST.

shikharj05 · 2022-11-28T06:48:50Z

src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java

@@ -93,6 +94,12 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
            return;
        }

+        Pattern usernamePattern = Pattern.compile("[$&+,:;=\\\\?@#|/'<>.^*()%!-]");


I think . should be allowed

Permitted .. Do we also allow @ in username?

@ shouldn't be allowed and only one . should be allowed. Thoughts? @cwperks @peternied @davidlago

Do we also allow -, _ and @ along with .? Please suggest.
In the current fix I have permitted the four special characters above.

codecov-commenter · 2022-11-28T12:42:41Z

Codecov Report

Merging #2277 (3e552a9) into main (7cad5e4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2277      +/-   ##
============================================
+ Coverage     61.05%   61.07%   +0.01%     
  Complexity     3270     3270              
============================================
  Files           259      260       +1     
  Lines         18337    18369      +32     
  Branches       3248     3251       +3     
============================================
+ Hits          11196    11219      +23     
- Misses         5555     5563       +8     
- Partials       1586     1587       +1

Impacted Files	Coverage Δ
...security/dlic/rest/api/InternalUsersApiAction.java	`83.78% <100.00%> (+1.43%)`	⬆️
...ecurity/configuration/ConfigurationRepository.java	`72.13% <0.00%> (-2.19%)`	⬇️
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	`57.46% <0.00%> (-1.50%)`	⬇️
...iance/ComplianceIndexingOperationListenerImpl.java	`61.76% <0.00%> (-1.48%)`	⬇️
...arch/security/ssl/OpenSearchSecuritySSLPlugin.java	`78.82% <0.00%> (-1.29%)`	⬇️
...ensearch/security/ssl/DefaultSecurityKeyStore.java	`67.28% <0.00%> (-0.55%)`	⬇️
...amazon/dlic/util/SettingsBasedSSLConfigurator.java	`62.30% <0.00%> (ø)`
...opensearch/security/auditlog/sink/WebhookSink.java	`76.74% <0.00%> (ø)`
...opensearch/security/ssl/util/SSLRequestHelper.java	`65.71% <0.00%> (ø)`
...ensearch/security/ssl/util/SSLConfigConstants.java	`77.77% <0.00%> (ø)`
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

peternied

This change impacts forward combability as username might have been viable in older versions of OpenSearch that will no longer work.

peternied · 2022-11-28T22:42:53Z

src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java

@@ -93,6 +94,12 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
            return;
        }

+        Pattern usernamePattern = Pattern.compile("[$&+,:;=\\\\?@#|/'<>^*()%!-]");


Username should not include special characters

Why not, are there bugs associated with these characters?

permitting special characters results in security risk & vulnerabilities.

Thanks for looking out for issues of this nature, could you be more specific as to the risks associated with individual characters? I am reticent to limit these usernames without cause.

If there is an issue please follow our reporting process [1] also quoted below.

Reporting a Vulnerability

If you discover a potential security issue in this project we ask that you notify AWS/Amazon Security via our vulnerability reporting page or directly via email to [email protected]. Please do not create a public GitHub issue.

[1] https://github.com/opensearch-project/security/security/policy

Added more details on the main thread #2277 (comment)

Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon · 2022-12-01T06:01:17Z

Requesting addition of backport labels @cwperks

DarshitChanpura · 2022-12-01T14:50:35Z

@rutuja-amazon which branches would you like this change to be backported to?

peternied · 2022-12-01T16:03:07Z

@rutuja-amazon Since this is change is based on a LOW security finding we should discuss everything in the public on this pull request. Pulling in the context from our internal threads:

Bug

Repro

Create two users through the dashboards create internal user:

username: hello password: world:a_great_day

username: hello:world password: a_great_day

Attempt to logging to login via basic auth into both accounts
curl -k -s https://hello:world:a_great_day@localhost:9200/_plugins/_security/api/account | jq
curl -k -s https://hello:world:a_great_day@localhost:9200/_plugins/_security/api/account | jq (Both are the same!)

You cannot login as the user hello:world.

Additional details

Note; this is expected as the username cannot contain a colon [1].

As per CVSS rating (3.1), it's been categorized as LOW risk.

[1] https://stackoverflow.com/questions/11612854/http-https-basic-authentication-colon-in-username

For the moment, lets block only the colon character.

Signed-off-by: Peter Nied <[email protected]>

peternied · 2022-12-02T20:35:28Z

@rutuja-amazon I've created rutuja-amazon#1, if you merge that pull request into this change we can get this issue fixed, what do you think?

rutuja-amazon · 2022-12-06T01:50:19Z

@rutuja-amazon I've created rutuja-amazon#1, if you merge that pull request into this change we can get this issue fixed, what do you think?

Issue is with other special characters as well, as discussed offline

peternied · 2022-12-07T22:23:06Z

I've created an issue to track the set of special characters that should be included or not, and at this time the only exclusion that seems justified is colon :. Please feel free to discuss on the issue, but as this pull request stands its too restrictive without justification for me approve and merge.

Block user create with certain characters/strings #2309

rutuja-amazon · 2022-12-09T16:55:31Z

Updated PR to only allow : for this use-case. Requesting merging this PR with backported labels.

cwperks

The changes look good to me, once the code hygiene check is fixed I will approve this PR.

peternied · 2022-12-09T19:56:16Z

The changes look good to me, once the code hygiene check is fixed I will approve this PR.

I'll take care of this to help get this merged more quickly...

Signed-off-by: Peter Nied <[email protected]>

peternied · 2022-12-09T20:59:48Z

Note; CI / build-artifact-names (pull_request) will fail because this hasn't merged the latest from main, and I'm am attempting to avoid rebasing these changes.

peternied · 2022-12-09T21:11:06Z

Failing CI check is known fixed in main, merging!

opensearch-trigger-bot · 2022-12-09T21:12:01Z

The backport to 1.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-2277-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 efbc48b405ff6ff372c4821aa15c53fc50a409a3
# Push it to GitHub
git push --set-upstream origin backport/backport-2277-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

Then, create a pull request where the base branch is 1.x and the compare/head branch is backport/backport-2277-to-1.x.

opensearch-trigger-bot · 2022-12-09T21:12:02Z

The backport to 1.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-2277-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 efbc48b405ff6ff372c4821aa15c53fc50a409a3
# Push it to GitHub
git push --set-upstream origin backport/backport-2277-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3

Then, create a pull request where the base branch is 1.3 and the compare/head branch is backport/backport-2277-to-1.3.

* 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)

* 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) Co-authored-by: rutuja-amazon <[email protected]>

) * 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)

) * 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) * Fix compliation issue Signed-off-by: Peter Nied <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: rutuja-amazon <[email protected]>

…pensearch-project#2317) * 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) Co-authored-by: rutuja-amazon <[email protected]>

rutuja-amazon requested a review from a team November 25, 2022 10:11

rutuja-amazon force-pushed the username-validation branch from 39b2627 to d895275 Compare November 25, 2022 10:16

rutuja-amazon force-pushed the username-validation branch from d895275 to ba09e9d Compare November 25, 2022 10:49

rutuja-amazon force-pushed the username-validation branch from ba09e9d to 3d5f259 Compare November 25, 2022 11:30

rutuja-amazon force-pushed the username-validation branch from 3d5f259 to c7f7ee1 Compare November 28, 2022 05:56

cliu123 reviewed Nov 28, 2022

View reviewed changes

rutuja-amazon force-pushed the username-validation branch from c7f7ee1 to d207161 Compare November 28, 2022 06:07

shikharj05 reviewed Nov 28, 2022

View reviewed changes

rutuja-amazon force-pushed the username-validation branch 4 times, most recently from a7d4d64 to ad27f3e Compare November 28, 2022 11:43

peternied requested changes Nov 28, 2022

View reviewed changes

username validation for special chars

2958f40

Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon force-pushed the username-validation branch from ad27f3e to 2958f40 Compare November 29, 2022 09:30

peternied added backport 1.x backport to 1.x branch backport 1.3 backport to 1.3 branch backport 2.x backport to 2.x branch labels Dec 1, 2022

peternied mentioned this pull request Dec 1, 2022

[BUG] Users can be created the violate the policy defined below the text field opensearch-project/security-dashboards-plugin#1234

Open

Only prevent user creation on colon characters, seperate out tests

3ed610b

Signed-off-by: Peter Nied <[email protected]>

cwperks reviewed Dec 9, 2022

View reviewed changes

Fix format violations

3e552a9

Signed-off-by: Peter Nied <[email protected]>

peternied dismissed DarshitChanpura’s stale review via 3e552a9 December 9, 2022 19:59

cwperks approved these changes Dec 9, 2022

View reviewed changes

peternied approved these changes Dec 9, 2022

View reviewed changes

peternied merged commit efbc48b into opensearch-project:main Dec 9, 2022

opensearch-trigger-bot bot mentioned this pull request Dec 9, 2022

[Backport 2.x] Username validation for special characters #2317

Merged

This was referenced Dec 9, 2022

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

Merged

[Backport 1.x] Username validation for special characters (#2277) #2320

Merged

peternied added the backport 2.4 label Dec 9, 2022

opensearch-trigger-bot bot mentioned this pull request Dec 9, 2022

[Backport 2.4] Username validation for special characters #2322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Username validation for special characters #2277

Username validation for special characters #2277

rutuja-amazon commented Nov 25, 2022 •

edited

Loading

shikharj05 commented Nov 25, 2022

rutuja-amazon commented Nov 25, 2022

cwperks commented Nov 25, 2022

cliu123 Nov 28, 2022

rutuja-amazon Nov 28, 2022

cliu123 Nov 28, 2022

rutuja-amazon Nov 28, 2022

shikharj05 Nov 28, 2022

rutuja-amazon Nov 28, 2022

DarshitChanpura Nov 28, 2022

rutuja-amazon Nov 29, 2022

codecov-commenter commented Nov 28, 2022 •

edited

Loading

peternied left a comment

peternied Nov 28, 2022

rutuja-amazon Nov 29, 2022

peternied Nov 29, 2022

peternied Dec 1, 2022

rutuja-amazon commented Dec 1, 2022

DarshitChanpura commented Dec 1, 2022

peternied commented Dec 1, 2022 •

edited

Loading

Bug

Repro

Additional details

peternied commented Dec 2, 2022

rutuja-amazon commented Dec 6, 2022

peternied commented Dec 7, 2022

rutuja-amazon commented Dec 9, 2022 •

edited

Loading

cwperks left a comment

peternied commented Dec 9, 2022

peternied commented Dec 9, 2022

peternied commented Dec 9, 2022

opensearch-trigger-bot bot commented Dec 9, 2022

opensearch-trigger-bot bot commented Dec 9, 2022

Username validation for special characters #2277

Username validation for special characters #2277

Conversation

rutuja-amazon commented Nov 25, 2022 • edited Loading

shikharj05 commented Nov 25, 2022

rutuja-amazon commented Nov 25, 2022

cwperks commented Nov 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 28, 2022 • edited Loading

Codecov Report

peternied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reporting a Vulnerability

Choose a reason for hiding this comment

rutuja-amazon commented Dec 1, 2022

DarshitChanpura commented Dec 1, 2022

peternied commented Dec 1, 2022 • edited Loading

Bug

Repro

Additional details

peternied commented Dec 2, 2022

rutuja-amazon commented Dec 6, 2022

peternied commented Dec 7, 2022

rutuja-amazon commented Dec 9, 2022 • edited Loading

cwperks left a comment

Choose a reason for hiding this comment

peternied commented Dec 9, 2022

peternied commented Dec 9, 2022

peternied commented Dec 9, 2022

opensearch-trigger-bot bot commented Dec 9, 2022

opensearch-trigger-bot bot commented Dec 9, 2022

rutuja-amazon commented Nov 25, 2022 •

edited

Loading

codecov-commenter commented Nov 28, 2022 •

edited

Loading

peternied commented Dec 1, 2022 •

edited

Loading

rutuja-amazon commented Dec 9, 2022 •

edited

Loading