Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape/Unescape pipe UserInfo in ThreadContext #801

Merged
merged 3 commits into from
Mar 19, 2025

Conversation

shikharj05
Copy link
Contributor

Description

Escape pipe while setting UserInfo in ThreadContext, Unescape pipe while reading UserInfo from ThreadContext

Related Issues

opensearch-project/security#2756

Check List

  • [] New functionality includes testing.
  • [NA] New functionality has been documented.
  • [NA] API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • [NA] Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@@ -143,13 +144,16 @@ public void injectUserInfo(final User user) {
);
return;
}

StringJoiner joiner = new StringJoiner("|");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to know where this serialization logic is used because I thought security was the only place where the user object would be serialized into the ThreadContext. Is this used for interplay between plugins in certain contexts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it may make sense to centralize this logic to prevent duplication, long term I'd like to remove a lot of security logic from common-utils including this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to know where this serialization logic is used because I thought security was the only place where the user object would be serialized into the ThreadContext. Is this used for interplay between plugins in certain contexts?

Yes, this is used for plugins to communicate with one another.

While it may make sense to centralize this logic to prevent duplication, long term I'd like to remove a lot of security logic from common-utils including this.

I agree, this is really needed, but that might require some security features in core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an RFC open in the security repo that outlines 4 security features that need to be built in order for plugins to migrate off of using common-utils: opensearch-project/security#5052

First, the security repo needs to support existing use cases in a model that's opt-in for plugins so that they can adopt the new mechanisms and create migration paths. Particularly, a lot of plugins store a copy of a user in their own system indices and eventually I think we should remove the copies but mechanisms need to be built to support it first.

lezzago
lezzago previously approved these changes Mar 18, 2025
cwperks
cwperks previously approved these changes Mar 18, 2025
@shikharj05 shikharj05 dismissed stale reviews from cwperks and lezzago via 332655a March 18, 2025 17:55
@lezzago lezzago merged commit ef830db into opensearch-project:main Mar 19, 2025
9 checks passed
@shikharj05 shikharj05 deleted the escape-pipe branch March 19, 2025 17:38
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 19, 2025
* Escape pipe character for injected users

Signed-off-by: Shikhar Jain <[email protected]>

* test updates + Spotless apply

Signed-off-by: Shikhar Jain <[email protected]>

* adding a test case with multiple pipes in username

Signed-off-by: Shikhar Jain <[email protected]>

---------

Signed-off-by: Shikhar Jain <[email protected]>
Co-authored-by: Shikhar Jain <[email protected]>
(cherry picked from commit ef830db)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 19, 2025
* Escape pipe character for injected users

Signed-off-by: Shikhar Jain <[email protected]>

* test updates + Spotless apply

Signed-off-by: Shikhar Jain <[email protected]>

* adding a test case with multiple pipes in username

Signed-off-by: Shikhar Jain <[email protected]>

---------

Signed-off-by: Shikhar Jain <[email protected]>
Co-authored-by: Shikhar Jain <[email protected]>
(cherry picked from commit ef830db)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
shikharj05 added a commit to shikharj05/common-utils that referenced this pull request Mar 20, 2025
shikharj05 added a commit to shikharj05/common-utils that referenced this pull request Mar 20, 2025
lezzago pushed a commit that referenced this pull request Mar 20, 2025
* Escape/Unescape pipe UserInfo in ThreadContext (#801)

Signed-off-by: Shikhar Jain <[email protected]>

* spotless

Signed-off-by: Shikhar Jain <[email protected]>

---------

Signed-off-by: Shikhar Jain <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 20, 2025
* Escape/Unescape pipe UserInfo in ThreadContext (#801)

Signed-off-by: Shikhar Jain <[email protected]>

* spotless

Signed-off-by: Shikhar Jain <[email protected]>

---------

Signed-off-by: Shikhar Jain <[email protected]>
(cherry picked from commit 12d4357)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
bowenlan-amzn pushed a commit that referenced this pull request Mar 21, 2025
* Escape/Unescape pipe UserInfo in ThreadContext (#801)



* spotless



---------


(cherry picked from commit 12d4357)

Signed-off-by: Shikhar Jain <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants