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

Customizable JWT audiences #16330

Merged
merged 19 commits into from
Feb 27, 2023
Merged

Conversation

skisel-da
Copy link
Contributor

No description provided.

@skisel-da skisel-da changed the title Customizable JWT audiences (another approach) Customizable JWT audiences Feb 17, 2023
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Nice work! This looks good to me in terms of functionality & UX, I think testing needs to be a bit more thorough around IDP support. I'll leave detailed review of the implementation to your team.

skisel-da and others added 3 commits February 17, 2023 13:23
…ity_provider_config_service.proto

Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
todo removal
…ity_provider_config_service.proto

Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>

it should "deny calls with user token for 'unknown_user' without expiration" taggedAs adminSecurityAsset
.setAttack(
attackPermissionDenied(threat = "Present a user JWT for 'unknown_user' without expiration")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the deciding factor for this threat to be permission_denied and for others to be unauthenticated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow this is what was there in the impl before. Depending on which layer authorization fails - it gives either permission denied or unauthenticated. Permission denied usually is thrown when something has been correctly provided. "Aud" claim check happens during parsing of the token, so it fails very early which is why it throws "unauthenticated"

Copy link
Contributor

@mziolekda mziolekda left a comment

Choose a reason for hiding this comment

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

Left a few comments and questions

@skisel-da skisel-da marked this pull request as ready for review February 27, 2023 08:23
@skisel-da skisel-da requested review from a team as code owners February 27, 2023 08:23
Copy link
Contributor

@mziolekda mziolekda left a comment

Choose a reason for hiding this comment

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

Looks good, please merge it in

@skisel-da skisel-da merged commit b33d635 into main Feb 27, 2023
@skisel-da skisel-da deleted the skisel-da/jwt-audience-customizable-part2 branch February 27, 2023 15:05
dasormeter pushed a commit that referenced this pull request Mar 30, 2023
dasormeter added a commit that referenced this pull request Mar 30, 2023
…6.x (#16631)

* Add identity provider config to ledger client (#16625)

* IdentityProviderConfigService through ledger client: create config

* IdentityProviderConfigService through ledger client: get config

* IdentityProviderConfigService through ledger client: list configs

* IdentityProviderConfigService through ledger client: delete config

* Fix updating of the audience field in IDP (#16630)

* Customizable JWT audiences (#16330)

---------

Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>
Co-authored-by: Sergey Kisel <98825453+skisel-da@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants