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

Support multiple audience for jwt authentication #4359

Conversation

donggyu04
Copy link
Contributor

@donggyu04 donggyu04 commented May 21, 2024

Description

Related: #3723

Currently OpenSearch doesn't support multiple audience for JWT authentication.
I want to pass the audience claims if aud value in JWT is at least one of the required audiences.

For example, If I set my OpenSearch security configuration for JWT like below.
I want to pass verification if there is either project1 or admin audience in the JWT token.

This may be useful for some cases, like if someone need to access opensearch for all projects(project1, project2, ....) via admin aud.

required_audience: project1,admin

Please confirm this concept positively.
Thanks in advance.

Testing

I added unit tests.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

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.

@willyborankin
Copy link
Collaborator

willyborankin commented May 21, 2024

Hi @donggyu04, thank you. Could you please:

@donggyu04 donggyu04 force-pushed the support-multiple-audience-for-jwt-auth branch from bf80f66 to f08ceb3 Compare May 22, 2024 02:30
@donggyu04
Copy link
Contributor Author

@willyborankin

Yes, I fixed that you mentioned.

Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 65.46%. Comparing base (382bc5f) to head (a912c75).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4359   +/-   ##
=======================================
  Coverage   65.46%   65.46%           
=======================================
  Files         310      310           
  Lines       21986    21992    +6     
  Branches     3552     3554    +2     
=======================================
+ Hits        14393    14397    +4     
  Misses       5824     5824           
- Partials     1769     1771    +2     
Files Coverage Δ
...ic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java 60.43% <100.00%> (ø)
...azon/dlic/auth/http/jwt/keybyoidc/JwtVerifier.java 80.00% <80.00%> (+0.37%) ⬆️
...mazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java 73.58% <75.00%> (-0.68%) ⬇️

... and 2 files with indirect coverage changes

@willyborankin
Copy link
Collaborator

Could you please fix spotless scan failure:
https://github.com/opensearch-project/security/actions/runs/9184452058/job/25272008401?pr=4359

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @donggyu04 ! The change looks good to me, but I had a general question.

When a user configures multiple values for required audience, it will behave as an OR operation where as long as a token has one of the required audiences its considered valid. Would there ever be a valid use-case where an admin would want to enforce that a jwt has all required audiences?

Can you please also submit a PR to the documentation-website to accompany this change?

@donggyu04 donggyu04 force-pushed the support-multiple-audience-for-jwt-auth branch 2 times, most recently from fae7c7f to dae5eaf Compare May 22, 2024 15:43
@donggyu04
Copy link
Contributor Author

@cwperks

Thank you for the contribution @donggyu04 ! The change looks good to me, but I had a general question.

When a user configures multiple values for required audience, it will behave as an OR operation where as long as a token has one of the required audiences its considered valid. Would there ever be a valid use-case where an admin would want to enforce that a jwt has all required audiences?

Can you please also submit a PR to the documentation-website to accompany this change?

Yes, I wanted to make sure that the JWT token was valid if it had any of the required audiences.

I can't think of any cases that force all required audience to be required. If I need to meet all required audience, I think I will use only one required audience as before.

I will make PR to documentation-website about this changes. thanks for your guidance

@donggyu04 donggyu04 force-pushed the support-multiple-audience-for-jwt-auth branch from dae5eaf to a912c75 Compare May 22, 2024 16:01
@donggyu04
Copy link
Contributor Author

Could you please fix spotless scan failure: https://github.com/opensearch-project/security/actions/runs/9184452058/job/25272008401?pr=4359

@willyborankin

I fixed and check it again in my local machine. thanks

image

@donggyu04
Copy link
Contributor Author

donggyu04 commented May 23, 2024

@willyborankin

test (citest, ubuntu-latest, 11) test was failed.
But looking at the error logs, it probably seems like a momentary failure.
I think it will pass if it tries again

Caused by: java.net.ConnectException: Connection refused (Connection refused)

...

java.net.BindException: Address already in use (Bind failed)

@willyborankin
Copy link
Collaborator

@willyborankin

test (citest, ubuntu-latest, 11) test was failed. But looking at the error logs, it probably seems like a momentary failure. I think it will pass if it tries again

Caused by: java.net.ConnectException: Connection refused (Connection refused)

...

java.net.BindException: Address already in use (Bind failed)

Yes it is a well known issue for the old test framework we use for testing. I will re-run jobs

@donggyu04
Copy link
Contributor Author

@willyborankin

Test is failed again but, I don't know what the problem is.
It only has logs below about the error.

Process 'Gradle Test Executor 2' finished with non-zero exit value 255
This problem might be caused by incorrect test process configuration.
For more on test execution, please refer to https://docs.gradle.org/8.5/userguide/java_testing.html#sec:test_execution in the Gradle documentation.

@willyborankin
Copy link
Collaborator

willyborankin commented May 23, 2024

@willyborankin

Test is failed again but, I don't know what the problem is. It only has logs below about the error.

Process 'Gradle Test Executor 2' finished with non-zero exit value 255
This problem might be caused by incorrect test process configuration.
For more on test execution, please refer to https://docs.gradle.org/8.5/userguide/java_testing.html#sec:test_execution in the Gradle documentation.

Yes I saw it as well. So far I can't answer the question why it happened.

@cwperks cwperks added the backport 2.x backport to 2.x branch label May 24, 2024
@cwperks cwperks merged commit f71d2e6 into opensearch-project:main May 24, 2024
83 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 24, 2024
Signed-off-by: leedonggyu <[email protected]>
(cherry picked from commit f71d2e6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@donggyu04
Copy link
Contributor Author

@willyborankin @cwperks Thanks for reviews. Is this going to be included in what month's release?

@donggyu04
Copy link
Contributor Author

@willyborankin @cwperks

Hi, Is this going to be included in what month's release?
Because our company is planning to use this feature.

Thanks in advance

@willyborankin
Copy link
Collaborator

Hi @donggyu04, yes it should be part of the upcoming release.

@donggyu04
Copy link
Contributor Author

@willyborankin

Sorry for continuing to ask questions. When are you planning the next release? june or july?
Our deadline is July 15th, so I'm wondering if it will happen before that.

@willyborankin
Copy link
Collaborator

@donggyu04, here is the official release scheduler: https://opensearch.org/releases.html

@donggyu04
Copy link
Contributor Author

@willyborankin Oh Thanks so much 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants