Username validation for special characters by rutuja-amazon · Pull Request #2277 · opensearch-project/security

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


+        // 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

            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

            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 aws-security@amazon.com. 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 <rutuja@amazon.com>

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 <petern@amazon.com>

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 <petern@amazon.com>

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

* 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) Co-authored-by: rutuja-amazon <110013621+rutuja-amazon@users.noreply.github.com>

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

) * 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) * Fix compliation issue Signed-off-by: Peter Nied <petern@amazon.com> Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: rutuja-amazon <110013621+rutuja-amazon@users.noreply.github.com>

…pensearch-project#2317) * 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) Co-authored-by: rutuja-amazon <110013621+rutuja-amazon@users.noreply.github.com>

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 suggested changes Nov 28, 2022

View reviewed changes

username validation for special chars

2958f40

Signed-off-by: Rutuja Surve <rutuja@amazon.com>

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 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 <petern@amazon.com>

cwperks reviewed Dec 9, 2022

View reviewed changes

Fix format violations

3e552a9

Signed-off-by: Peter Nied <petern@amazon.com>

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

Conversation

rutuja-amazon commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shikharj05 commented Nov 25, 2022

Uh oh!

rutuja-amazon commented Nov 25, 2022

Uh oh!

cwperks commented Nov 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Reporting a Vulnerability

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rutuja-amazon commented Dec 1, 2022

Uh oh!

DarshitChanpura commented Dec 1, 2022

Uh oh!

peternied commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Repro

Additional details

Uh oh!

peternied commented Dec 2, 2022

Uh oh!

rutuja-amazon commented Dec 6, 2022

Uh oh!

peternied commented Dec 7, 2022

Uh oh!

rutuja-amazon commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwperks left a comment

Choose a reason for hiding this comment

Uh oh!

peternied commented Dec 9, 2022

Uh oh!

peternied commented Dec 9, 2022

Uh oh!

peternied commented Dec 9, 2022

Uh oh!

opensearch-trigger-bot bot commented Dec 9, 2022

Uh oh!

opensearch-trigger-bot bot commented Dec 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

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