feat: Support for urn:ietf:params:oauth:grant-type:jwt-bearer grant type RFC 7523 #2384

Xopek · 2021-03-05T12:55:44Z

Related issue

Proposed changes

This changes adds Support for JSON Web Token (JWT) Profile for OAuth 2.0 Authorization Grants (RFC7523).
Users of Hydra will be able to grant permission for OAuth 2.0 client to act on behalf of some resource owner using jwt bearer assertions.

Checklist

I have read the contributing 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
security@ory.sh) 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.

Further comments

Xopek · 2021-03-05T13:03:50Z

@aeneasr The only help i need is with e2e test for jwt bearer auth grant. Main problem: don't know how to generate jwk and it is needed to create grant. Maybe you have some ideas?

aeneasr

Awesome thanks! It's quite a big PR so I probably need a bit of time to review it. I will try to merge Go 1.16 first so there might be some merge conflicts along the way...

aeneasr · 2021-03-08T12:02:12Z

.schema/config.schema.json

@@ -201,6 +201,37 @@
      "examples": [
        "1h"
      ]
+    },
+    "grantJWT": {


JSON Schema demands grantJwt here :)

aeneasr · 2021-03-08T12:02:38Z

.schema/config.schema.json

+          "additionalProperties": false,
+          "properties": {
+            "jwt": {
+              "$ref": "#/definitions/grantJWT"


Unless this is being used somewhere else there isn't really a need to have it as a definition so you can just embed it here IMO.

aeneasr · 2021-03-08T12:03:07Z

.schema/config.schema.json

+      "properties": {
+        "client_auth_optional": {
+          "type": "boolean",
+          "description": "If false, client authentication is required to get access token.",


Describe what happens if this is true

aeneasr · 2021-03-08T12:03:33Z

.schema/config.schema.json

+          "description": "If false, client authentication is required to get access token.",
+          "default": false
+        },
+        "id_optional": {


Suggested change

"id_optional": {

"jti_optional": {

aeneasr · 2021-03-08T12:03:40Z

.schema/config.schema.json

+          "description": "If false, JTI claim must be present in JWT assertion.",
+          "default": false
+        },
+        "issued_date_optional": {


Suggested change

"issued_date_optional": {

"iat_optional": {

aeneasr · 2021-03-08T12:04:03Z

.schema/config.schema.json

+          "description": "If false, IAT claim must be present in JWT assertion.",
+          "default": false
+        },
+        "max_duration": {


Suggested change

"max_duration": {

"max_ttl": {

aeneasr · 2021-03-08T12:05:30Z

.schema/config.schema.json

+          "default": false
+        },
+        "max_duration": {
+          "description": "Configures how long JWT assertion is considered valid, since being issued.",


Suggested change

"description": "Configures how long JWT assertion is considered valid, since being issued.",

"description": "Configures what the maximum age of a JWT assertion can be. Overrides the JWT's EXP claim. Useful as a safety measure and recommended to not be set to 720h max.",

Well it doesn't overrides EXP claim, it compares EXP claim to current setting. Made some corrections in description.

aeneasr · 2021-03-11T15:45:55Z

Could you also please rebase on (or merge with) master? That would resolve some of the test issues we're seeing (I hope) :)

Xopek · 2021-03-24T12:09:50Z

Could you also please rebase on (or merge with) master? That would resolve some of the test issues we're seeing (I hope) :)

done

aeneasr · 2021-03-29T09:03:26Z

Thank you! Looks like there is a small conflict with persister_test. I will look into the code with a bit of time this week but from a high-level view it looks really good!

aeneasr · 2021-04-05T11:33:55Z

Ok I resolved the conflicts and brought everything up to speed. Trying to wrap my head around this feature again as it has been some time.

Also, to to make sure - this PR only addresses https://tools.ietf.org/html/rfc7523#section-2.1 not https://tools.ietf.org/html/rfc7523#section-2.2 - correct?

aeneasr · 2021-04-05T11:37:30Z

@Xopek I addressed the merge conflicts locally but can not push on your branch as I am not allowed to. Could you check the box "allow write access for maintainers": https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Also to make sure - this PR only addresses https://tools.ietf.org/html/rfc7523#section-2.1 not https://tools.ietf.org/html/rfc7523#section-2.2 - correct?

Also we should add a guide on how to use this feature in docs/docs/guides/oauth2-grant-type-jwt-bearer-profile.mdx, would you be up to do that?

aeneasr · 2021-04-05T18:01:20Z

Hello, I have made a couple of improvements in my auth-grant-type-jwt-bearer branch. There's a couple more things we need to do:

Documentation - I started adding some stuff but it needs more work with examples (e.g. CURL examples)
Tests for the jwt-bearer handler

Otherwise this looks pretty good to me!

grant/jwtbearer/validator.go

grant/jwtbearer/handler.go

Xopek · 2021-04-06T11:36:05Z

Ok I resolved the conflicts and brought everything up to speed. Trying to wrap my head around this feature again as it has been some time.

Also, to to make sure - this PR only addresses https://tools.ietf.org/html/rfc7523#section-2.1 not https://tools.ietf.org/html/rfc7523#section-2.2 - correct?

Yes, you are correct

aeneasr · 2021-04-06T11:44:05Z

You can find my changes here: Tinkoff#1

Xopek · 2021-04-06T12:06:58Z

@Xopek I addressed the merge conflicts locally but can not push on your branch as I am not allowed to. Could you check the box "allow write access for maintainers": https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

I will try to enable "allow write access for maintainers", however because fork is in organization it can be hard. Alternatively i will cherry your commits in your branch into my branch.

Also to make sure - this PR only addresses https://tools.ietf.org/html/rfc7523#section-2.1 not https://tools.ietf.org/html/rfc7523#section-2.2 - correct?

Correct

Also we should add a guide on how to use this feature in docs/docs/guides/oauth2-grant-type-jwt-bearer-profile.mdx, would you be up to do that?

I think yes

Yes i think i c

Xopek · 2021-04-06T12:29:54Z

You can find my changes here: TinkoffCreditSystems#1

Thanks)

stensonb · 2021-04-19T18:10:44Z

I'd love to help this feature get merged/released. Can I help with documentation here? Anything else that needs work?

Xopek · 2021-04-20T09:15:22Z

I'd love to help this feature get merged/released. Can I help with documentation here? Anything else that needs work?

Will be great! You can make changes in your branch, i will cherry pick them!

Xopek · 2021-04-20T09:15:48Z

Considering this PR - i need some time to get back on it

aeneasr · 2021-04-22T06:31:59Z

@stensonb if you do please consider my PR first: Tinkoff#1

aeneasr · 2021-05-11T09:04:12Z

Marking as draft

jagobagascon · 2021-12-01T11:31:52Z

You are right 😄

I updated Tinkoff#5 with the change. This should make the tests run without a timeout. Thank you.

aeneasr · 2021-12-01T11:47:24Z

Awesome! I think that once the merge conflicts are resolved, the tests should pass :)

feat: remove redundant end-to-end tests

…-bearer

Xopek · 2021-12-05T21:21:42Z

@aeneasr @jagobagascon I fixed merged conflicts. There was also some case-sentitive contributors file hell. There were two versions of this file, one in lowercase, one in uppercase.

Tests are now passing, but we have probmel with security checks now, escpecially with embedded private keys. @jagobagascon can we generate them on the fly maybe? Or what options do we have @aeneasr ?

codecov · 2021-12-05T21:32:27Z

Codecov Report

Merging #2384 (47996e0) into master (e8eeb8e) will increase coverage by 1.07%.
The diff coverage is 92.96%.

@@            Coverage Diff             @@
##           master    #2384      +/-   ##
==========================================
+ Coverage   76.85%   77.93%   +1.07%     
==========================================
  Files         105      110       +5     
  Lines        7146     7636     +490     
==========================================
+ Hits         5492     5951     +459     
- Misses       1248     1267      +19     
- Partials      406      418      +12

Impacted Files	Coverage Δ
x/clean_sql.go	`46.42% <50.00%> (+0.13%)`	⬆️
oauth2/trust/handler.go	`75.86% <75.86%> (ø)`
persistence/sql/persister_grant_jwk.go	`81.08% <81.08%> (ø)`
cmd/cli/handler_janitor.go	`78.88% <100.00%> (+0.98%)`	⬆️
cmd/janitor.go	`100.00% <100.00%> (ø)`
driver/config/provider.go	`89.30% <100.00%> (+0.30%)`	⬆️
driver/registry.go	`80.00% <100.00%> (ø)`
driver/registry_base.go	`91.44% <100.00%> (+2.43%)`	⬆️
driver/registry_sql.go	`85.18% <100.00%> (+0.56%)`	⬆️
oauth2/fosite_store_helpers.go	`100.00% <100.00%> (ø)`
... and 4 more

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 5bad542...47996e0. Read the comment docs.

jagobagascon · 2021-12-09T15:11:38Z

@jagobagascon can we generate them on the fly maybe?

Yes, I added the change here: Tinkoff#6

That change should get rid of all security alerts.

Hard-coded keys were secure because they were only used inside a test. However the CodeQL analyze tool was complaining about it so this change should stop that.

The CodeQL analyze tool was complaining about using a cryptographically insecure random value in a security context. This change should stop that.

jagobagascon · 2021-12-15T09:31:49Z

All failing Code scanning results were fixed with the last change, but for some reason new errors appeared in cmd/token_user.go (This log write receives unsanitized user input from).

I would say that the things it is complaining about were not changed by this PR (they are already in the master branch) so I don't think we should fix them here. WDYT?

Appart from this I would say this is ready to be reviewed by @aeneasr.

…-bearer # Conflicts: # CONTRIBUTORS

aeneasr

Great job everyone, this can now be merged. I'll try to fix the CI now and also fixed one or two things in the SQL schema :)

drwatsno · 2021-12-26T21:16:08Z

This is the best gift for the new year for us! Thank's to everyone! 🎉

vinckr · 2022-01-04T14:04:03Z

Hello @Xopek
Congrats on merging your first PR in Ory 🎉 !
Your contribution is 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 to claim your Ory swag!

PS: You all have been great in the community, big smooch on the forehead for everybody participating in this PR, let me know and we throw in some more for the rest of the team.

pike1212 · 2022-01-11T13:40:38Z

@Xopek thoughts on expanding this to allow passing custom claims in the jwt that get added to the generated token?

Xopek · 2022-01-20T15:53:03Z

@Xopek thoughts on expanding this to allow passing custom claims in the jwt that get added to the generated token?

And what kind of claims you want to add? Because currently claims only needed for hydra to verify, that you have right to get token. After that, your jwt is not needed, you will use token, while it is valid.

pflipp · 2022-01-21T07:21:50Z

@Xopek in my use case it would be something like a group id.

I provide the group id in the access token claims to avoid additional lookup in some of my API's.

but for 95% of my use cases it fits perfectly already, thanks a lot for this great contribution!

pike1212 · 2022-02-15T20:22:51Z

We need a way to do delegation/impersonation, allowing an on_behalf_of claim into the jwt would allow us to do this

Xopek changed the title ~~Support for urn:ietf:params:oauth:grant-type:jwt-bearer grant type RFC 7523~~ feat: Support for urn:ietf:params:oauth:grant-type:jwt-bearer grant type RFC 7523 Mar 5, 2021

aeneasr reviewed Mar 8, 2021

View reviewed changes

Xopek force-pushed the auth-grant-type-jwt-bearer branch from 9dcff3f to 7724517 Compare March 24, 2021 12:09

aeneasr self-assigned this Mar 29, 2021

aeneasr reviewed Apr 5, 2021

View reviewed changes

grant/jwtbearer/validator.go Outdated Show resolved Hide resolved

grant/jwtbearer/handler.go Outdated Show resolved Hide resolved

aeneasr marked this pull request as draft May 11, 2021 09:04

Vladimir Kalugin added 8 commits May 21, 2021 17:52

feat: add auth grant jwt support

e550200

fix: mysql migration for grant jwk and table naming

ac8b3f4

fix: check grant existence on attempt to delete it

7474634

style: imports format

b67fd7f

docs: add swagger docs for grant API

aabb2f0

fix: grant list was not filtered by optional issuer

c0463b8

test(grant): add tests for jwt-bearer grant

b76c70b

test(grant): add public key scopes tests

a5a33d8

drwatsno and others added 4 commits December 2, 2021 00:58

Merge pull request #5 from jagobagascon/auth-grant-type-jwt-bearer

3d9b547

feat: remove redundant end-to-end tests

Merge remote-tracking branch 'origin/master' into auth-grant-type-jwt…

c51a51c

…-bearer

fix: make contributors file

63e7660

style: make format

a0a48df

jagobagascon and others added 3 commits December 13, 2021 14:38

feat: programatically generate testing RSA key pairs

259f23b

Hard-coded keys were secure because they were only used inside a test. However the CodeQL analyze tool was complaining about it so this change should stop that.

feat: replace Math.random with crypto.getRandomValues

dd18daf

The CodeQL analyze tool was complaining about using a cryptographically insecure random value in a security context. This change should stop that.

chore: format

9a59bd3

aeneasr added 5 commits December 26, 2021 17:42

Merge remote-tracking branch 'origin/master' into auth-grant-type-jwt…

8491478

…-bearer # Conflicts: # CONTRIBUTORS

chore: code review

533f8e6

chore: code review

8527fab

chore: format

a8f1a51

chore: code review

ac9f7ec

aeneasr previously approved these changes Dec 26, 2021

View reviewed changes

chore: code review

5e161c8

aeneasr dismissed their stale review via 5e161c8 December 26, 2021 17:20

u

47996e0

aeneasr merged commit 858f2cf into ory:master Dec 26, 2021

aarmam mentioned this pull request Dec 30, 2021

feat: Hardware Security Module support #2625

Merged

6 tasks

	"description": "Configures how long JWT assertion is considered valid, since being issued.",
	"description": "Configures what the maximum age of a JWT assertion can be. Overrides the JWT's EXP claim. Useful as a safety measure and recommended to not be set to 720h max.",

feat: Support for urn:ietf:params:oauth:grant-type:jwt-bearer grant type RFC 7523 #2384

feat: Support for urn:ietf:params:oauth:grant-type:jwt-bearer grant type RFC 7523 #2384

Conversation

Xopek commented Mar 5, 2021 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

Xopek commented Mar 5, 2021

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xopek Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Mar 11, 2021 • edited Loading

Xopek commented Mar 24, 2021

aeneasr commented Mar 29, 2021

aeneasr commented Apr 5, 2021

aeneasr commented Apr 5, 2021

aeneasr commented Apr 5, 2021

Xopek commented Apr 6, 2021

aeneasr commented Apr 6, 2021

Xopek commented Apr 6, 2021

Xopek commented Apr 6, 2021

stensonb commented Apr 19, 2021

Xopek commented Apr 20, 2021

Xopek commented Apr 20, 2021

aeneasr commented Apr 22, 2021

aeneasr commented May 11, 2021

jagobagascon commented Dec 1, 2021

aeneasr commented Dec 1, 2021

Xopek commented Dec 5, 2021

codecov bot commented Dec 5, 2021 • edited Loading

Codecov Report

jagobagascon commented Dec 9, 2021

jagobagascon commented Dec 15, 2021

aeneasr left a comment

Choose a reason for hiding this comment

drwatsno commented Dec 26, 2021

vinckr commented Jan 4, 2022 • edited Loading

pike1212 commented Jan 11, 2022

Xopek commented Jan 20, 2022

pflipp commented Jan 21, 2022

pike1212 commented Feb 15, 2022

Xopek commented Mar 5, 2021 •

edited

Loading

Xopek Mar 24, 2021 •

edited

Loading

aeneasr commented Mar 11, 2021 •

edited

Loading

codecov bot commented Dec 5, 2021 •

edited

Loading

vinckr commented Jan 4, 2022 •

edited

Loading