feat: add support for trust grants that can issue tokens for any subject #3012

jagobagascon · 2022-02-25T10:56:43Z

Previously, a trust relationship had to be setup for every subject
before the issuer could sign a JWT token for it. This change will allow
setting up token services that can issue tokens with any value in the
subject field.

Related issue(s)

#2930

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Implementation details

REST api

This PR adds a new field to the trust creation request called allow_any_subject, and as long as this flag is set to false (or unset), the trust relationship creation service will behave just like it did before this change: the subject field will be required.

{
  "issuer": "https://jwt-idp.example.com",
  "subject": "[email protected]",
  "...": "..."
}

When allow_any_subject is set to true, then the issuer will be allowed to sign tokens for any subject. Also, as a safety measure, the subject field MUST be empty (or unset), otherwise the validation will fail. This is done to avoid creating an issuer that can sign tokens for any subject by mistake.

{
  "issuer": "https://jwt-idp.example.com",
  "allow_any_subject": true,
  "...": "..."
}

Database

When allow_any_subject is set to true, then the subject field is stored with an empty string value in the database. This allows us to keep (and benefit from) the UNIQUE constraint that the hydra_oauth2_trusted_jwt_bearer_issuer table has, requires no changes to the database schema and makes the database query used to find an issuer's public key really simple: subject = ? or subject = ''.

jagobagascon · 2022-02-25T10:57:28Z

I've seen that the documentation is now in another repository, and I'm not sure if I should update the docs now or wait for this PR to be merged in case there's any changes.

codecov · 2022-02-25T11:32:33Z

Codecov Report

Merging #3012 (4257ff6) into master (00100a1) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3012      +/-   ##
==========================================
+ Coverage   79.40%   79.60%   +0.19%     
==========================================
  Files         112      112              
  Lines        7890     7957      +67     
==========================================
+ Hits         6265     6334      +69     
+ Misses       1223     1222       -1     
+ Partials      402      401       -1

Impacted Files	Coverage Δ
oauth2/trust/manager.go	`100.00% <ø> (ø)`
oauth2/fosite_store_helpers.go	`100.00% <100.00%> (ø)`
oauth2/trust/handler.go	`76.27% <100.00%> (+0.40%)`	⬆️
oauth2/trust/validator.go	`100.00% <100.00%> (ø)`
persistence/sql/persister_grant_jwk.go	`81.41% <100.00%> (+0.33%)`	⬆️
oauth2/handler.go	`68.27% <0.00%> (-0.09%)`	⬇️
persistence/sql/persister_oauth2.go	`81.85% <0.00%> (+0.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00100a1...4257ff6. Read the comment docs.

jagobagascon · 2022-02-25T12:19:34Z

Hi @aeneasr, I'm not sure why e2e tests are failing. I just ran them locally and everything seems to be working fine. The error it's throwing is

"error": "missing_required_parameter",
"error_description": "One of the required parameters is missing. Check your request parameters. Field 'subject' is required."

But that string has been deleted from the code, it's almost as if it was running on an older binary. Could it be related to the actions/cache@v2 GitHub action used in the end-to-end tests setup? I'm a bit confused right now.

aeneasr

Thank you! Sorry for the late review! This looks really good. The one thing I would like to change is that we store this "any" relationship explicitly in the database. An empty string could get into the DB with a number of things (e.g. broken migrations, rollbacks, ...) and we really need to prevent that this feature is enabled "on accident". Therefore, please add a SQL column to store this state :)

persistence/sql/persister_grant_jwk.go

glerchundi · 2022-03-31T22:13:41Z

Thanks @jagobagascon for your time & effort 👏 👏

aeneasr

Thank you so much! This looks great!

persistence/sql/persister_grant_jwk.go

aeneasr · 2022-04-11T22:23:17Z

@grantzvolsky please take note that this PR introduces a schema change. We'll squash merge it so that it's just one commit which hopefully will make it easier to rebase.

Previously, a trust relationship had to be setup for every subject before the issuer could sign a JWT token for it. This change will allow setting up token services that can issue tokens with any value in the subject field. Closes #2930

jagobagascon · 2022-04-12T08:30:04Z

I added the tests as requested.

aeneasr

Thank you so much for your great work @jagobagascon ! I added a small test, and now this is ready to be merged! But, one last ask - could you please update the document https://github.com/ory/docs/edit/master/docs/hydra/guides/oauth2-grant-type-jwt-bearer.mdx to include your changes so that others know about this feature? Thank you so much!

jagobagascon · 2022-04-17T09:54:22Z

Thank you @aeneasr! I updated the docs as requested: ory/docs#764

Added documentation describing how to setup trust relationships that allow signing tokens for any subject. This is a complement to ory/hydra#3012 and should be merged after linked PR.

aeneasr · 2022-04-17T16:20:23Z

🥳

vinckr · 2022-04-19T08:22:03Z

Hello @jagobagascon
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

jagobagascon · 2022-04-20T08:30:11Z

Thank you @vinckr but I already received my gift for helping on a previous PR 😊.
Also thanks to @aeneasr for all the help and effort put on this. I can't wait to add this feature to our project!

If it's not too much to ask, are there any plans for the next Hydra release anytime soon?

vinckr · 2022-04-20T14:31:55Z

Oh thanks for being so honest @jagobagascon, for some reason this showed up as your first PR 😅.
hackerman is currently on vacation, but I am sure we will do a Ory Hydra release very soon / once he is back 🙏

glerchundi · 2022-04-20T17:51:36Z

Great job to all!! Cannot wait to see this working in prod! 🎉 🎉

jagobagascon requested a review from aeneasr as a code owner February 25, 2022 10:56

aeneasr self-assigned this Feb 28, 2022

aeneasr requested a review from tomekpapiernik March 7, 2022 09:35

aeneasr assigned tomekpapiernik Mar 7, 2022

aeneasr removed the request for review from tomekpapiernik March 7, 2022 09:35

aeneasr unassigned tomekpapiernik Mar 7, 2022

aeneasr requested changes Mar 25, 2022

View reviewed changes

persistence/sql/persister_grant_jwk.go Outdated Show resolved Hide resolved

aeneasr approved these changes Apr 11, 2022

View reviewed changes

aeneasr requested changes Apr 11, 2022

View reviewed changes

persistence/sql/persister_grant_jwk.go Show resolved Hide resolved

persistence/sql/persister_grant_jwk.go Show resolved Hide resolved

persistence/sql/persister_grant_jwk.go Show resolved Hide resolved

jagobagascon requested a review from aeneasr April 12, 2022 09:05

feat: add test for success of any subject

4257ff6

aeneasr reviewed Apr 16, 2022

View reviewed changes

jagobagascon mentioned this pull request Apr 17, 2022

docs: add docs for trust grants with any subject ory/docs#764

Merged

6 tasks

aeneasr merged commit a3c4304 into ory:master Apr 17, 2022

jagobagascon deleted the feature/allow-any-subject-on-jwt-assertion-grant branch April 17, 2022 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for trust grants that can issue tokens for any subject #3012

feat: add support for trust grants that can issue tokens for any subject #3012

jagobagascon commented Feb 25, 2022 •

edited

Loading

jagobagascon commented Feb 25, 2022

codecov bot commented Feb 25, 2022 •

edited

Loading

jagobagascon commented Feb 25, 2022

aeneasr left a comment

glerchundi commented Mar 31, 2022

aeneasr left a comment

aeneasr commented Apr 11, 2022

jagobagascon commented Apr 12, 2022

aeneasr left a comment

jagobagascon commented Apr 17, 2022

aeneasr commented Apr 17, 2022

vinckr commented Apr 19, 2022

jagobagascon commented Apr 20, 2022

vinckr commented Apr 20, 2022

glerchundi commented Apr 20, 2022

feat: add support for trust grants that can issue tokens for any subject #3012

feat: add support for trust grants that can issue tokens for any subject #3012

Conversation

jagobagascon commented Feb 25, 2022 • edited Loading

Related issue(s)

Checklist

Implementation details

REST api

Database

jagobagascon commented Feb 25, 2022

codecov bot commented Feb 25, 2022 • edited Loading

Codecov Report

jagobagascon commented Feb 25, 2022

aeneasr left a comment

Choose a reason for hiding this comment

glerchundi commented Mar 31, 2022

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented Apr 11, 2022

jagobagascon commented Apr 12, 2022

aeneasr left a comment

Choose a reason for hiding this comment

jagobagascon commented Apr 17, 2022

aeneasr commented Apr 17, 2022

vinckr commented Apr 19, 2022

jagobagascon commented Apr 20, 2022

vinckr commented Apr 20, 2022

glerchundi commented Apr 20, 2022

jagobagascon commented Feb 25, 2022 •

edited

Loading

codecov bot commented Feb 25, 2022 •

edited

Loading