fix!: NumericDate parsing conformance

[dunglas]

The current implementation of the standard claims parser is invalid. It doesn't allow NumericDate value to be floats, while it's explicitly allowed by the RFC:

NumericDate
A JSON numeric value representing the number of seconds from
1970-01-01T00:00:00Z UTC until the specified UTC date/time,
ignoring leap seconds. This is equivalent to the IEEE Std 1003.1,
2013 Edition [POSIX.1] definition "Seconds Since the Epoch", in
which each day is accounted for by exactly 86400 seconds, other
than that non-integer values can be represented. See RFC 3339
[RFC3339] for details regarding date/times in general and UTC in
particular.

(Emphasis mine).

This is annoying because popular libraries generate tokens containing floats in the exp, iat and nbf fields. For instance, it's the case of lcobucci/jwt, one of the most popular JWT library written in PHP.

This PR fixes this, and also allows comparing fractions of seconds.

This is a BC break, so it should be merged in the 4.0 version.

Also, as this library is a core dependency of my Mercure project, if you need help for the maintenance of this library, I'll be glad to participate!

Closes dunglas/mercure#404 and form3tech-oss/jwt-go#11.

[oxisto]

Looking at the CI... this is interesting, it seems that this fails for very old Go versions (which might be ok). I think we still have not quite 100% decided on which minimum Go version we want to support. Do you know what the minimum version for your patch would be?

[lggomez]

@oxisto I guess we'll see how does the change affect this. From the github actions we already know it works on newer versions but just for the sake of verifying I added those versions to the travis build

[mfridman]

Thank you for submitting a PR.

I believe one of the goals here is to maintain backwards compatibility with the upstream repo. I.e., make it a drop-in replacement.

Instead of changing the StandardClaims struct, would it suffice to add a new struct with an explanation and/or a deprecation notice?

New users would be pushed towards the "correct struct", while existing users are unaffected but have the option to update their implementations?

[lggomez]

This would be one way to go. I'm feeling very urged to fix the current one but as we want to maintain the drop-in contract in order to ease the transition from the already deprecated and stale package we'll probably have to work around it this way until a new release is made in this one

[dunglas]

As you wish, but the current implementation is not valid according to the spec and fails hard in the wild (basically when interacting with any PHP project, because Lcobucci's lib is the most popular in this ecosystem).

[mfridman]

I'd love to see this fixed as well. Although I think it's an easier migration story for users:

golang-jwt/jwt v1.x.x is backwards-compatible with dgrijalva/jwt-go (despite the incorrect implementation you pointed out)

and then we could consider further improvements for those already on /v1 that breaks compatibility with a /v2 release

[oxisto]

I'd love to see this fixed as well. Although I think it's an easier migration story for users:

golang-jwt/jwt v1.x.x is backwards-compatible with dgrijalva/jwt-go (despite the incorrect implementation you pointed out)

and then we could consider further improvements for those already on /v1 that breaks compatibility with a /v2 release

I fear, that this won't be completely possible anyway, because the cleanest fix to #6 (which we agreed on fixing before our first "release") is to adjust the claims struct, changing Audience from string to []string. We might as well correct the float/int issue as well before releasing v1.x.x.

[mfridman]

Hehe, these are the trickiest tradeoffs. The breaking change is justified from a security and spec perspective, but keeping StandardClaims as-is maintains backwards-compatibility. The latter is something most Go folks have come to appreciate. Just want to make sure if we do break StandardClaims its worth it.

What are your thoughts w.r.t adding a new struct RegisteredClaims while keeping StandardClaims unchanged with a deprecation notice and comment suggesting against its use?

[oxisto]

I will play around with this idea over the weekend. With that in mind I would then also probably choose to make the audience field directly a time Field instead of an into or float and have appropriate MarshalJSON functions that take care of the conversion. This feels more naturally and idiomatic to me than assigning the expiry in a float value.

[lggomez]

This got weird quick

So, here's a complete test matrix and it turns out it breaks the tests as late as go 1.12 with the same errors:

# github.com/golang-jwt/jwt_test [github.com/golang-jwt/jwt.test]
./http_example_test.go:154:4: cannot use float64(time.Now().Add(time.Minute * 1).Unix()) (type float64) as type int64 in field value
./parser_test.go:142:4: cannot use float64(time.Now().Add(time.Second * 10).Unix()) (type float64) as type int64 in field value
./rsa_pss_test.go:136:3: cannot use float64(time.Now().Unix()) (type float64) as type int64 in field value
FAIL	github.com/golang-jwt/jwt [build failed]

But there are no noteworthy changes in 1.13 that may seem related to this: https://golang.org/doc/go1.13#time

[lggomez]

Aside from this thread, this PR would push the minimum go requirement to 1.13. Not that I disagree since the project is already temporarily (?) defined as a 1.14 module but it is something to take into account in regards to backwards compatibility

If I get some spare time I'll try to dig deeper into this issue to see if I can find the cause

[oxisto]

While we are at: Changing this reference to https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.4 would be appropriate as well. That is btw the source of the "wrong" implementation. When the library was initially started, RFC7519 did not exist - only its drafts. The referenced draft actually stated that iat, exp and so on must be a IntDate, which did not include the float part. It got then later changed to a NumericDate format in draft version 26, which - as you correctly pointed out - included also float representations.

[oxisto]

@dunglas As you might have seen, I have prepared a PR (#15) that addresses several issues with regards to the RFC conformance. Would you mind taking a look there, to check if this would suit your particular needs?

My current plan is to include this in a v3.3.0 release (see https://github.com/golang-jwt/jwt/milestone/3) with a new struct called RegisteredClaims, which well then eventually replace the old StandardClaims.

[dunglas]

Closing in favor of #15.