feat: transit from jwt-go to go-jose #593

narg95 · 2021-05-06T22:07:14Z

Related issue

#514

Proposed changes

This pull request migrates from jwt-go to go-jose. The following are the key considerations for this transition:

go-jose does not support alg:none therefore tests trying to check this functionality where removed
JWTStrategy interface was kept almost identical to provide an easy migration, except for the following changes:
- jwt.Token struct type from jwt-go was replaced by a local and simplified type definition Token that covers all fosite use cases
- jwt.MapClaims was replaced by a local copy of that data type
- jwt.Claims interface dependency to jwt-go was replaced by the local MapClaims struct type and not by another interface because it simplified the adaptation and fosite always type casted jwt.Claims to *jwt.MapClaims. Source code performing this type cast was therefore not needed and removed
jwt.ValidationErrors from jwt-go was replaced by a local copy in order to keep backwards compatibility with jwt validation errors
jwt.SigningMethod{Family} from jwt-go has not equivalence to go-jose signing methods, therefore it was updated by using concrete signing method names e.g. jwt.SigningMethodRSA -> jose.RS256, jose.RS384, jose.RS512
RS256JWTStrategy and ES256JWTStrategy types were kept to stay backwards compatible, but they are just the same implementation calling under the hood the same helper functions with some minor difference in parameters. This is so because go-jose makes no distinction on signing methods, this is hidden from callers and it is inferred based on the private/public key provided.

Checklist

I have read the contributing guidelines
and signed the CLA.
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 necessary documentation within the code base (if
appropriate).

Further comments

As mentioned in the issue, this is a first step in a full migration to go-jose. This step aims to remove jwt-go but keeping best-effort-backwards-compatible methods and behaviors using underneaths go-jose. This way simplifies the transition process. It is known that go-jose offer other benefits, but those need to be addressed in further pull requests.

narg95 · 2021-05-07T19:03:32Z

I do not understand why the conformity OpenID Connect Conformity Tests / oidc-conformity (pull_request) is failing. It is also happening in main branch

aeneasr · 2021-05-08T08:35:16Z

Thank you! I'll look into this next week - also for the failing conformity tests

aeneasr · 2021-05-08T08:38:58Z

The problem here is I think one of forks - on master I have to double check. Basically we are doing a module rewrite with the latest PR commit hash in go.mod of Ory Hydra for fosite. This fails apparently for forks. Also GitHub recently changed the way how SHA is computed for PRs as they use a virtual merge commit to do the diff or something. Long story short - probably not related to your changes but we need to fix it before merge. I thought i fixed it but apparently not so for forks

aeneasr · 2021-05-10T07:03:00Z

I've created ory/hydra#2525 - it shows how to upgrade fosite to point to your branch. To ensure that OIDC works correctly and there are no serious regressions could you please get Ory Hydra tests to pass? If they're green there we can merge this one here right away! Thank you :)

narg95 · 2021-05-10T14:23:46Z

@aeneasr sure, I am on it, so far it compiles, linter is happy, I am taking a look to the tests. I had to upgrade my go to 1.6 to use import "embed". make docker works. So it looks promising :)

narg95 · 2021-05-10T16:09:00Z

Hi @aeneasr, I finished to massage hydra to work with this PR.
It looks all green, you can see it in hyra's PR #2526

As you can see the migration is smooth, mostly changing imports.

I added a test case here in fosite in order to comply with the implicit expectation that the jwt.Parse function with an invalid but Parse-able JWT token should return the parsed token with validation errors, I was previously just returning a nil token on validation errors.

aeneasr

I noticed that none was removed from the test assertions - it's unfortunately needed to pass OIDC certification iirc.

I started a test run for OIDC certification here: https://github.com/ory/hydra/pull/2526/checks?check_run_id=2554759989

Let's see if it passes :)

authorize_request_handler_oidc_request_test.go

aeneasr · 2021-05-11T11:53:42Z

As suspected, the OIDC tests fail with the none alg missing: https://github.com/ory/hydra/pull/2526/checks?check_run_id=2554759989#step:5:114

narg95 · 2021-05-11T15:10:50Z

I added support for alg:none, it should work now. I added a unit test to check it.
I hope 🤞 this time the OpenID conformity test do not complain.

It has just failed because wrong expected commit. Running those test is actually confusing, they need some how sync between hydra and fosite PRs?

anyway I updated Hydra PR pointing to latest commit here so that should trigger them again, if not please do it manually

aeneasr · 2021-05-13T12:39:04Z

Thank you! I approved the run just now. It's really annoying that GitHub requires us to hit "Approve Run" over and over again :( Especially if the task takes an hour or more to finish...

aeneasr

Awesome, thank you for the great work! It looks like the CI in Ory Hydra is passing so this is a ✅ 😃

I think we need a couple more tests in here but otherwise this looks really solid! I'll go over the jwt token validation strategy once again when tests have been added.

All in all thank you for the great contribution!

authorize_request_handler_oidc_request_test.go

token/jwt/map_claims.go

token/jwt/token_test.go

token/jwt/token.go

aeneasr · 2021-05-14T13:42:52Z

I've updated the CI config so it now runs the conformity tests directly here!

narg95 · 2021-05-17T12:10:12Z

I hope those tests provide the confidence required to merge it 🤞
This PR is becoming bigger ;)

.github/workflows/oidc-conformity.yml

narg95 · 2021-05-19T13:11:32Z

hi @aeneasr, perhaps you did not noticed it :) but I already addressed your comments and conformity tests are green, so from my point of view this is ready to be merged. Could you take a look?

Btw. before or after merging the .github/workflows/oidc-conformity.yml file must be reverted to point to ory/hydra repo and master branch, as I explained above, this was needed to run conformity tests with the correct hydra's go-jose_migration branch.

aeneasr

Thank you! This looks much better already :) Sorry for the slow process in this PR. My workload is so intense at the moment and these type of PRs (that touch cryptography) are always very difficult to review!

I'm leaving for holiday on saturday for ~10 days but I think we can get it merged before :)

aeneasr · 2021-05-19T19:26:12Z

token/jwt/map_claims.go

+	if !ok {
+		strAud, ok := m["aud"].(string)
+		if !ok {
+			return false


If I read this correctly, this would fail when aud is not set and req is false which is contrary to what If required is false, this method will return true if the value matches or is unset says.

I think a test for this would be good!

Good catch, agree, actually the error is in the forked https://github.com/form3tech-oss/jwt-go/blob/v3.2.1/map_claims.go and not in https://github.com/dgrijalva/jwt-go/blob/v3.2.0/map_claims.go. I will double check and update the PR with the fix and some tests with a new commit so you do not have to review all again.

I updated with latest map_claims.go from form3tech-oss/jwt-go which accepts aud as an array. I noted that repo provides map_claims_test.go, I copy them and added a test to check for the inconsistency that you found.

aeneasr · 2021-05-19T19:41:32Z

token/jwt/token_test.go

+			errors:      ValidationErrorMalformed,
+		},
+		{
+			name:        "wrong key, expected RSA got ECDSA",


This appears to be

Suggested change

name: "wrong key, expected RSA got ECDSA",

name: "wrong key, expected ECDSA got RSA",

Good catch, fixed, I structured a bit better the tests to make it more explicit what is given and what is expected, I copied tests from dgrijalva/jwt-go were a bit confusing.

token/jwt/token_test.go

narg95 · 2021-05-19T20:08:58Z

Thank you! This looks much better already :) Sorry for the slow process in this PR. My workload is so intense at the moment and these type of PRs (that touch cryptography) are always very difficult to review!

I'm leaving for holiday on saturday for ~10 days but I think we can get it merged before :)

It is understandable, it touches a core component. I think we can manage a merge before Saturday, I think I can get the changes by tomorrow

aarmam · 2021-05-21T06:32:33Z

Thank you! This looks much better already :) Sorry for the slow process in this PR. My workload is so intense at the moment and these type of PRs (that touch cryptography) are always very difficult to review!

I'm leaving for holiday on saturday for ~10 days but I think we can get it merged before :)

That would be great! I'm looking to add HSM support and this PR would be greatly appreciated!

.github/workflows/oidc-conformity.yml

aeneasr

Awesome, good job! Will release this as the next minor version :)

Closes ory#514 Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

mitar · 2021-06-02T20:53:09Z

It is worth noting that now JWT tokens do not unclude typ anymore in the header: square/go-jose#126

aeneasr · 2021-06-03T08:02:40Z

#606

mitar · 2021-09-13T11:52:10Z

token/jwt/map_claims.go

+
+	if !m.VerifyExpiresAt(now, false) {
+		vErr.Inner = errors.New("Token is expired")
+		vErr.Errors |= ValidationErrorExpired


@narg95 One late question about this change. So I was looking into this codebase while debugging on our side and I noticed that Errors is a bitset value, where multiple errors can be set. I like that. But, everywhere else code is using == to test for various types of errors like in the switch/case case, where it is case jwt.ValidationErrorExpired for example, that switch/case would reach the end if there are two errors set at any point, no?

The ve.Errors == jwt.ValidationErrorExpired check which is there is in fact OK, because that case should be handled only if only ValidationErrorExpired is set.

@mitar sorry for the late answer. I was disconnected… Actually I thing you are right, I found switch cases and == comparisons for jwt.ValidationErrors. Because an non-nil error is anyway returned it is still safe, but it is buggy in case of asserting a concrete error. I’ll take a deeper look and submit a patch. Good catch ;) !

narg95 changed the title ~~transit from jwt-go to go-jose~~ feat: transit from jwt-go to go-jose May 6, 2021

narg95 force-pushed the go-jose_migration branch 3 times, most recently from b8ef805 to d69f4a1 Compare May 7, 2021 18:57

narg95 force-pushed the go-jose_migration branch from d69f4a1 to 4f09eb7 Compare May 7, 2021 19:06

narg95 mentioned this pull request May 10, 2021

chore: integrate with fosite v0.40 (go-jose migration) ory/hydra#2526

Merged

aeneasr reviewed May 11, 2021

View reviewed changes

authorize_request_handler_oidc_request_test.go Outdated Show resolved Hide resolved

narg95 force-pushed the go-jose_migration branch from 69c95db to ca2ed20 Compare May 11, 2021 14:57

aeneasr reviewed May 13, 2021

View reviewed changes

authorize_request_handler_oidc_request_test.go Show resolved Hide resolved

token/jwt/map_claims.go Show resolved Hide resolved

token/jwt/token_test.go Show resolved Hide resolved

token/jwt/token.go Show resolved Hide resolved

narg95 force-pushed the go-jose_migration branch from 427d5f8 to 45a8e10 Compare May 17, 2021 11:52

narg95 force-pushed the go-jose_migration branch 2 times, most recently from 05ae184 to bff8d5d Compare May 17, 2021 12:23

narg95 commented May 17, 2021

View reviewed changes

.github/workflows/oidc-conformity.yml Outdated Show resolved Hide resolved

aeneasr reviewed May 19, 2021

View reviewed changes

narg95 added 3 commits May 20, 2021 01:21

transit from jwt-go to go-jose

b136d9d

return parsed token on invalid claims

6c1e2b2

add support for alg:none

1e3ae90

narg95 added 2 commits May 20, 2021 01:21

add better coverage for token generation and validation

fc4f4c8

point temporaly to go-jose_migration hydra branch for conformity tests

7fcf4be

narg95 force-pushed the go-jose_migration branch from bff8d5d to 7fcf4be Compare May 19, 2021 23:21

fix mapClaims, add and refactor tests

38b21d7

aeneasr reviewed May 21, 2021

View reviewed changes

.github/workflows/oidc-conformity.yml Outdated Show resolved Hide resolved

Update .github/workflows/oidc-conformity.yml

509c4a9

aeneasr approved these changes May 21, 2021

View reviewed changes

aeneasr merged commit d022bbc into ory:master May 21, 2021

narg95 added a commit to narg95/fosite that referenced this pull request May 22, 2021

feat: transit from jwt-go to go-jose (ory#593)

03b22f6

Closes ory#514 Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>

mitar reviewed Sep 13, 2021

View reviewed changes

narg95 mentioned this pull request Oct 22, 2021

fix: use bitwise comparison for jwt validation errors #633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: transit from jwt-go to go-jose #593

feat: transit from jwt-go to go-jose #593

narg95 commented May 6, 2021 •

edited

Loading

narg95 commented May 7, 2021

aeneasr commented May 8, 2021

aeneasr commented May 8, 2021

aeneasr commented May 10, 2021

narg95 commented May 10, 2021

narg95 commented May 10, 2021 •

edited

Loading

aeneasr left a comment

aeneasr commented May 11, 2021

narg95 commented May 11, 2021 •

edited

Loading

aeneasr commented May 13, 2021

aeneasr left a comment

aeneasr commented May 14, 2021

narg95 commented May 17, 2021

narg95 commented May 19, 2021 •

edited

Loading

aeneasr left a comment

aeneasr May 19, 2021

narg95 May 19, 2021

narg95 May 20, 2021

aeneasr May 19, 2021

narg95 May 20, 2021

narg95 commented May 19, 2021

aarmam commented May 21, 2021

aeneasr left a comment

mitar commented Jun 2, 2021

aeneasr commented Jun 3, 2021

mitar Sep 13, 2021 •

edited

Loading

narg95 Oct 16, 2021

	name: "wrong key, expected RSA got ECDSA",
	name: "wrong key, expected ECDSA got RSA",

feat: transit from jwt-go to go-jose #593

feat: transit from jwt-go to go-jose #593

Conversation

narg95 commented May 6, 2021 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

narg95 commented May 7, 2021

aeneasr commented May 8, 2021

aeneasr commented May 8, 2021

aeneasr commented May 10, 2021

narg95 commented May 10, 2021

narg95 commented May 10, 2021 • edited Loading

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented May 11, 2021

narg95 commented May 11, 2021 • edited Loading

aeneasr commented May 13, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented May 14, 2021

narg95 commented May 17, 2021

narg95 commented May 19, 2021 • edited Loading

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr May 19, 2021

Choose a reason for hiding this comment

narg95 May 19, 2021

Choose a reason for hiding this comment

narg95 May 20, 2021

Choose a reason for hiding this comment

aeneasr May 19, 2021

Choose a reason for hiding this comment

narg95 May 20, 2021

Choose a reason for hiding this comment

narg95 commented May 19, 2021

aarmam commented May 21, 2021

aeneasr left a comment

Choose a reason for hiding this comment

mitar commented Jun 2, 2021

aeneasr commented Jun 3, 2021

mitar Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

narg95 Oct 16, 2021

Choose a reason for hiding this comment

narg95 commented May 6, 2021 •

edited

Loading

narg95 commented May 10, 2021 •

edited

Loading

narg95 commented May 11, 2021 •

edited

Loading

narg95 commented May 19, 2021 •

edited

Loading

mitar Sep 13, 2021 •

edited

Loading