chore: Migrate to use golang-jwt/jwt v4.2.0 #8136
Conversation
Signed-off-by: Yuan Tang <[email protected]>
|
OSS-fuzz issue: https://oss-fuzz.com/testcase-detail/5460437754314752 |
Signed-off-by: Yuan Tang <[email protected]>
| token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) | ||
| settings, err := mgr.settingsMgr.GetSettings() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| // workaround for https://github.com/argoproj/argo-cd/issues/5217 |
There was a problem hiding this comment.
@alexmt Since you originally added these in #5228. Could you help review this removal? Looks like since we are using the new method jwt.NewNumericDate, the precision will be in seconds.
// TimePrecision sets the precision of times and dates within this library.
// This has an influence on the precision of times when comparing expiry or
// other related time fields. Furthermore, it is also the precision of times
// when serializing.
//
// For backwards compatibility the default precision is set to seconds, so that
// no fractional timestamps are generated.
var TimePrecision = time.Second
// NumericDate represents a JSON numeric date value, as referenced at
// https://datatracker.ietf.org/doc/html/rfc7519#section-2.
type NumericDate struct {
time.Time
}
// NewNumericDate constructs a new *NumericDate from a standard library time.Time struct.
// It will truncate the timestamp according to the precision specified in TimePrecision.
func NewNumericDate(t time.Time) *NumericDate {
return &NumericDate{t.Truncate(TimePrecision)}
}
There was a problem hiding this comment.
I think the workaround that Alex has implemented was due to the fact that our JWTs contain integer timestamps, while the library worked with floats. Now they seem to support multiple precisions and have reverted to integers for the precision we require (seconds), which is nice :) To me, the removal of this workaround looks good.
Signed-off-by: Yuan Tang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #8136 +/- ##
==========================================
- Coverage 41.53% 41.50% -0.03%
==========================================
Files 174 174
Lines 22736 22717 -19
==========================================
- Hits 9443 9429 -14
+ Misses 11949 11945 -4
+ Partials 1344 1343 -1
Continue to review full report at Codecov.
|
| parser := &jwt.Parser{ | ||
| ValidationHelper: jwt.NewValidationHelper(jwt.WithoutClaimsValidation(), jwt.WithoutAudienceValidation()), | ||
| } | ||
| var claims jwt.StandardClaims | ||
| parser := jwt.NewParser(jwt.WithoutClaimsValidation()) | ||
| var claims jwt.RegisteredClaims |
There was a problem hiding this comment.
I was wondering why we previously skipped audience validation, and now validate audience field?
There was a problem hiding this comment.
Oh, thanks for reminding me of this since I meant to double check on this change as well!
This is only because jwt.WithoutAudienceValidation() is no longer available in the new version of JWT client. Although I am not aware of the reason why the audience validation was skipped previously. If you have any suggestions here, please let me know!
There was a problem hiding this comment.
It is important to not validate audience (see #5253 and #5413). Apparently, https://github.com/golang-jwt/jwt does not validate the audience automatically , so it works just as we need right now.
They might re-introduce automatic validate in future golang-jwt/jwt#16 but we are good for now.
|
I have confirmed that this fixes the issue that OSS-Fuzz has complained about. After this PR has been merged, the fuzzer needs to be adapted to import |
I can do that. Edit: Nevermind: cncf/cncf-fuzzing#61 |
The original repo is no longer maintained, see dgrijalva/jwt-go#462. We also received fuzzing reports on this. Upgrading to v4.2.0 might fix some of the issues.
Signed-off-by: Yuan Tang [email protected]
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: