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.

Sign up for GitHub

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

Jump to bottom

Support for mutil-value audience claims in JWT token #13490

Merged
merged 3 commits into from
Sep 21, 2022
Merged

Support for mutil-value audience claims in JWT token #13490

merged 3 commits into from
Sep 21, 2022

Conversation

rstyp
Copy link
Contributor

@rstyp rstyp commented Aug 4, 2022

Description

requiredAudience validation fails if aud claim contains multiple audiences in JWT token.

  • Remove jwtParser.requireAudience(config.getRequiredAudience()); to not to validate aud during parseClaimsJws.
  • Add validateAudience method and call it from createIdentity

Is this change a fix, improvement, new feature, refactoring, or other?

a fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

JWT Authentication

How would you describe this change to a non-technical end user or system administrator?

Support for mutil-value audience claims in JWT token

Related issues, pull requests, and links

#13442

Documentation

( *) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# JWT Authentication
* Support for mutil-value audience claims in JWT token. ({issue}`13442 `)

@cla-bot
Copy link

cla-bot bot commented Aug 4, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: rstyp.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Aug 4, 2022

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

lukasz-walkiewicz
lukasz-walkiewicz reviewed Aug 5, 2022
View reviewed changes
Copy link
Member

@lukasz-walkiewicz lukasz-walkiewicz left a comment

Choose a reason for hiding this comment

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

Looks like a good start but we need to add tests for this change.
Please take a look at io.trino.server.security.TestResourceSecurity. There are a few test for JWT authentication.

core/trino-main/src/main/java/io/trino/server/security/jwt/JwtAuthenticator.java Outdated

public class JwtAuthenticator
extends AbstractBearerAuthenticator
{
private final JwtParser jwtParser;
private final String principalField;
private final UserMapping userMapping;
private final String requiredAudience;
Copy link
Member

Choose a reason for hiding this comment

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

Typically we use Optional for values that can have null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukasz-walkiewicz changed it to Optional

@cla-bot
Copy link

cla-bot bot commented Aug 6, 2022

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@rstyp rstyp requested a review from lukasz-walkiewicz August 8, 2022 17:37
@cla-bot
Copy link

cla-bot bot commented Aug 11, 2022

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@cla-bot
Copy link

cla-bot bot commented Aug 12, 2022

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@lukasz-walkiewicz
Copy link
Member

@rstyp Please sign CLA. See instructions from cla-bot.

@cla-bot
Copy link

cla-bot bot commented Aug 12, 2022

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@cla-bot cla-bot bot added the cla-signed label Aug 22, 2022
@rstyp
Copy link
Contributor Author

rstyp commented Aug 22, 2022

@lukasz-walkiewicz CLA is signed now.

lukasz-walkiewicz
lukasz-walkiewicz reviewed Sep 15, 2022
View reviewed changes
Copy link
Member

@lukasz-walkiewicz lukasz-walkiewicz left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @rstyp. Looks good, just a couple of minor comments.

core/trino-main/src/main/java/io/trino/server/security/jwt/JwtAuthenticator.java Outdated
@@ -71,6 +75,32 @@ protected Optional<Identity> createIdentity(String token)
.build());
}

private void validateAudience(Claims claims)
{
Object tokenAudience = claims.get(AUDIENCE);
Copy link
Member

Choose a reason for hiding this comment

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

change order, if requiredAudience is empty then getting audience from claims is redundant

core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java
@@ -538,6 +552,34 @@ public void testJwtWithJwkAuthenticator()
}
}

@Test
public void testJwtAuthenticatorWithInvalidAudience()
Copy link
Member

Choose a reason for hiding this comment

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

I think we're missing a test with empty audience and without http-server.authentication.jwt.required-audience specified.

@kokosing kokosing requested review from s2lomon and 11xor6 September 15, 2022 12:02
lukasz-walkiewicz
lukasz-walkiewicz approved these changes Sep 20, 2022
View reviewed changes
Copy link
Member

@lukasz-walkiewicz lukasz-walkiewicz 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 @rstyp. LGTM 👍
@kokosing could you please take a look as well and merge it?

@rstyp
Copy link
Contributor Author

rstyp commented Sep 21, 2022

Thank you @lukasz-walkiewicz

@kokosing
Copy link
Member

CI hit: #12818

@kokosing
Copy link
Member

CI hit: #13288

@kokosing kokosing merged commit ccfcb9d into trinodb:master Sep 21, 2022
@kokosing
Copy link
Member

Merged, thanks

@github-actions github-actions bot added this to the 397 milestone Sep 21, 2022
@colebow colebow mentioned this pull request Sep 21, 2022
Merged
@jhbigler jhbigler mentioned this pull request Feb 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasz-walkiewicz lukasz-walkiewicz lukasz-walkiewicz approved these changes

@kokosing kokosing Awaiting requested review from kokosing

@s2lomon s2lomon Awaiting requested review from s2lomon

@11xor6 11xor6 Awaiting requested review from 11xor6

Assignees
No one assigned
Labels
cla-signed
Milestone
397
Development

Successfully merging this pull request may close these issues.
3 participants
@rstyp @lukasz-walkiewicz @kokosing