Replace JWT validation library #5592
Conversation
mantas-sidlauskas
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Pull Request Test Coverage Report for Build 018d3d6c-8b87-42c7-82a5-a6c04a1e85d6
💛 - Coveralls |
go.mod
Outdated
| @@ -66,6 +66,7 @@ require ( | |||
| ) | |||
|
|
|||
| require ( | |||
| github.com/golang-jwt/jwt/v5 v5.2.0 | |||
There was a problem hiding this comment.
we have v5.0.0 of this package in monorepo. Since it doesn't have other dependencies it maybe OK but please confirm @Groxx.
There was a problem hiding this comment.
testing with D12725271
There was a problem hiding this comment.
I think it'll be ok
| return errors.New("IssuedAt is not set") | ||
| } | ||
|
|
||
| // Fill ExpiresAt when TTL is passed |
There was a problem hiding this comment.
I'm a little surprised this is necessary, it's not a problem, but I wouldj have throught that the JWT lib would do this natively
There was a problem hiding this comment.
Our clients (java and go) are not setting ExpiresAt claim. Instead, they are providing TTL claim. This is to support backwards compatibility. Later on, clients needs to be updated to fill in ExpiresAt and we can delegate this check to JWT lib.
There was a problem hiding this comment.
After re-reading your comment, I realized that it was not about TTL check, but for IssuedAt check. And yes, you are right, this is not needed as library will do this validation. Thanks!
There was a problem hiding this comment.
ah, sorry, yeah, i wasn't being super clear and to be honest, i get confused between the different values. I'll don't think it's a blocker, so stamping. Thank you for looking into it.
mantas-sidlauskas
force-pushed
the
jwt_parser
branch
from
a0a5125
to
165923d
Compare
mantas-sidlauskas
force-pushed
the
jwt_parser
branch
from
165923d
to
2e7cada
Compare
| return errors.New("IssuedAt is not set") | ||
| } | ||
|
|
||
| // Fill ExpiresAt when TTL is passed |
There was a problem hiding this comment.
ah, sorry, yeah, i wasn't being super clear and to be honest, i get confused between the different values. I'll don't think it's a blocker, so stamping. Thank you for looking into it.
| } | ||
|
|
||
| timeLeft := exp.Unix() - time.Now().Unix() |
There was a problem hiding this comment.
my comment wasn't great, I actually meant this: I thought this would be something that the library would do natively
davidporter-id-au
merged commit 66897c7
into
cadence-workflow:master
What changed?
Replacing https://github.com/cristalhq/jwt with https://github.com/golang-jwt/jwt.
Why?
To utilize "keyfunc" option for implementing JWKS functionality. It is planed to check token and pick correct public key for token validation. Having this, it is possible to use existing self-signed JWT for internal inter-node communication and 3rd party Identity Provider at the same time.
How did you test it?
Additional unit tests for validate TTL, local tests.
Potential risks
Release notes
Documentation Changes