Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: NumericDate parsing conformance #9

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,13 @@ go:
- 1.5
- 1.6
- 1.7
- 1.8
- 1.9
- 1.10
- 1.11
- 1.12
- 1.13
- 1.14
- 1.15
- 1.16
- tip
64 changes: 38 additions & 26 deletions claims.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package jwt
import (
"crypto/subtle"
"fmt"
"math"
"time"
)

Expand All @@ -16,13 +17,13 @@ type Claims interface {
// https://tools.ietf.org/html/rfc7519#section-4.1
// See examples for how to use this with your own claim types
type StandardClaims struct {
Audience string `json:"aud,omitempty"`
ExpiresAt int64 `json:"exp,omitempty"`
Id string `json:"jti,omitempty"`
IssuedAt int64 `json:"iat,omitempty"`
Issuer string `json:"iss,omitempty"`
NotBefore int64 `json:"nbf,omitempty"`
Subject string `json:"sub,omitempty"`
Audience string `json:"aud,omitempty"`
ExpiresAt float64 `json:"exp,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dunglas dunglas May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@oxisto oxisto May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Id string `json:"jti,omitempty"`
IssuedAt float64 `json:"iat,omitempty"`
Issuer string `json:"iss,omitempty"`
NotBefore float64 `json:"nbf,omitempty"`
Subject string `json:"sub,omitempty"`
}

// Validates time based claims "exp, iat, nbf".
Expand All @@ -31,12 +32,12 @@ type StandardClaims struct {
// be considered a valid claim.
func (c StandardClaims) Valid() error {
vErr := new(ValidationError)
now := TimeFunc().Unix()
now := TimeFunc()

// The claims below are optional, by default, so if they are set to the
// default value in Go, let's not fail the verification for them.
if c.VerifyExpiresAt(now, false) == false {
delta := time.Unix(now, 0).Sub(time.Unix(c.ExpiresAt, 0))
delta := now.Sub(parseUnixFloat(c.ExpiresAt))
vErr.Inner = fmt.Errorf("token is expired by %v", delta)
vErr.Errors |= ValidationErrorExpired
}
Expand Down Expand Up @@ -66,13 +67,13 @@ func (c *StandardClaims) VerifyAudience(cmp string, req bool) bool {

// Compares the exp claim against cmp.
// If required is false, this method will return true if the value matches or is unset
func (c *StandardClaims) VerifyExpiresAt(cmp int64, req bool) bool {
func (c *StandardClaims) VerifyExpiresAt(cmp time.Time, req bool) bool {
return verifyExp(c.ExpiresAt, cmp, req)
}

// Compares the iat claim against cmp.
// If required is false, this method will return true if the value matches or is unset
func (c *StandardClaims) VerifyIssuedAt(cmp int64, req bool) bool {
func (c *StandardClaims) VerifyIssuedAt(cmp time.Time, req bool) bool {
return verifyIat(c.IssuedAt, cmp, req)
}

Expand All @@ -84,7 +85,7 @@ func (c *StandardClaims) VerifyIssuer(cmp string, req bool) bool {

// Compares the nbf claim against cmp.
// If required is false, this method will return true if the value matches or is unset
func (c *StandardClaims) VerifyNotBefore(cmp int64, req bool) bool {
func (c *StandardClaims) VerifyNotBefore(cmp time.Time, req bool) bool {
return verifyNbf(c.NotBefore, cmp, req)
}

Expand All @@ -101,34 +102,45 @@ func verifyAud(aud string, cmp string, required bool) bool {
}
}

func verifyExp(exp int64, now int64, required bool) bool {
if exp == 0 {
func verifyExp(exp float64, now time.Time, required bool) bool {
if exp == 0. {
return !required
}
return now <= exp

pexp := parseUnixFloat(exp)

return pexp.Equal(now) || now.Before(pexp)
}

func verifyIat(iat int64, now int64, required bool) bool {
if iat == 0 {
func verifyIat(iat float64, now time.Time, required bool) bool {
if iat == 0. {
return !required
}
return now >= iat

piat := parseUnixFloat(iat)

return piat.Equal(now) || now.After(piat)
}

func verifyIss(iss string, cmp string, required bool) bool {
if iss == "" {
return !required
}
if subtle.ConstantTimeCompare([]byte(iss), []byte(cmp)) != 0 {
return true
} else {
return false
}

return subtle.ConstantTimeCompare([]byte(iss), []byte(cmp)) != 0
}

func verifyNbf(nbf int64, now int64, required bool) bool {
if nbf == 0 {
func verifyNbf(nbf float64, now time.Time, required bool) bool {
if nbf == 0. {
return !required
}
return now >= nbf

pnbf := parseUnixFloat(nbf)

return pnbf.Equal(now) || now.After(pnbf)
}

func parseUnixFloat(ts float64) time.Time {
int, frac := math.Modf(ts)
return time.Unix(int64(int), int64(frac*(1e9)))
}
81 changes: 81 additions & 0 deletions claims_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package jwt

import (
"testing"
"time"
)

func Test_StandardClaims_VerifyExpiresAt_empty(t *testing.T) {
c := StandardClaims{}
if !c.VerifyExpiresAt(time.Now(), false) {
t.Fatalf("Failed to verify exp claim, wanted: %v got %v", true, false)
}
}

func Test_StandardClaims_VerifyExpiresAt_expired(t *testing.T) {
c := StandardClaims{
ExpiresAt: float64(time.Now().Add(-1*time.Hour).Unix()) + 0.123,
}
if c.VerifyExpiresAt(time.Now(), true) {
t.Fatalf("Failed to verify exp claim, wanted: %v got %v", false, true)
}
}

func Test_StandardClaims_VerifyExpiresAt_not_expired(t *testing.T) {
c := StandardClaims{
ExpiresAt: float64(time.Now().Add(1*time.Hour).Unix()) + 0.123,
}
if !c.VerifyExpiresAt(time.Now(), true) {
t.Fatalf("Failed to verify exp claim, wanted: %v got %v", true, false)
}
}

func Test_StandardClaims_VerifyIssuedAt_empty(t *testing.T) {
c := StandardClaims{}
if !c.VerifyIssuedAt(time.Now(), false) {
t.Fatalf("Failed to verify iat claim, wanted: %v got %v", true, false)
}
}

func Test_StandardClaims_VerifyIssuedAt_expired(t *testing.T) {
c := StandardClaims{
IssuedAt: float64(time.Now().Add(1*time.Hour).Unix()) + 0.123,
}
if c.VerifyIssuedAt(time.Now(), true) {
t.Fatalf("Failed to verify iat claim, wanted: %v got %v", false, true)
}
}

func Test_StandardClaims_VerifyIssuedAt_past(t *testing.T) {
c := StandardClaims{
IssuedAt: float64(time.Now().Add(-1*time.Hour).Unix()) + 0.123,
}
if !c.VerifyIssuedAt(time.Now(), true) {
t.Fatalf("Failed to verify iat claim, wanted: %v got %v", true, false)
}
}

func Test_StandardClaims_VerifyNotBefore_empty(t *testing.T) {
c := StandardClaims{}
if !c.VerifyNotBefore(time.Now(), false) {
t.Fatalf("Failed to verify nbf claim, wanted: %v got %v", true, false)
}
}

func Test_StandardClaims_VerifyNotBefore_expired(t *testing.T) {
c := StandardClaims{
NotBefore: float64(time.Now().Add(1*time.Hour).Unix()) + 0.123,
}
if c.VerifyNotBefore(time.Now(), true) {
t.Fatalf("Failed to verify nbf claim, wanted: %v got %v", false, true)
}
}

func Test_StandardClaims_VerifyNotBefore_passed(t *testing.T) {
c := StandardClaims{
NotBefore: float64(time.Now().Add(-1*time.Hour).Unix()) + 0.123,
}
if !c.VerifyNotBefore(time.Now(), true) {
t.Fatalf("Failed to verify nbf claim, wanted: %v got %v", true, false)
}
}
7 changes: 4 additions & 3 deletions http_example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import (
"bytes"
"crypto/rsa"
"fmt"
"github.com/dgrijalva/jwt-go"
"github.com/dgrijalva/jwt-go/request"
"io"
"io/ioutil"
"log"
Expand All @@ -17,6 +15,9 @@ import (
"net/url"
"strings"
"time"

"github.com/dgrijalva/jwt-go"
"github.com/dgrijalva/jwt-go/request"
)

// location of the files used for signing and verification
Expand Down Expand Up @@ -150,7 +151,7 @@ func createToken(user string) (string, error) {
&jwt.StandardClaims{
// set the expire time
// see http://tools.ietf.org/html/draft-ietf-oauth-json-web-token-20#section-4.1.4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ExpiresAt: time.Now().Add(time.Minute * 1).Unix(),
ExpiresAt: float64(time.Now().Add(time.Minute * 1).Unix()),
},
"level1",
CustomerInfo{user, "human"},
Expand Down
21 changes: 11 additions & 10 deletions map_claims.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package jwt
import (
"encoding/json"
"errors"
"time"
// "fmt"
)

Expand All @@ -19,25 +20,25 @@ func (m MapClaims) VerifyAudience(cmp string, req bool) bool {

// Compares the exp claim against cmp.
// If required is false, this method will return true if the value matches or is unset
func (m MapClaims) VerifyExpiresAt(cmp int64, req bool) bool {
func (m MapClaims) VerifyExpiresAt(cmp time.Time, req bool) bool {
switch exp := m["exp"].(type) {
case float64:
return verifyExp(int64(exp), cmp, req)
return verifyExp(exp, cmp, req)
case json.Number:
v, _ := exp.Int64()
v, _ := exp.Float64()
return verifyExp(v, cmp, req)
}
return req == false
}

// Compares the iat claim against cmp.
// If required is false, this method will return true if the value matches or is unset
func (m MapClaims) VerifyIssuedAt(cmp int64, req bool) bool {
func (m MapClaims) VerifyIssuedAt(cmp time.Time, req bool) bool {
switch iat := m["iat"].(type) {
case float64:
return verifyIat(int64(iat), cmp, req)
return verifyIat(iat, cmp, req)
case json.Number:
v, _ := iat.Int64()
v, _ := iat.Float64()
return verifyIat(v, cmp, req)
}
return req == false
Expand All @@ -52,12 +53,12 @@ func (m MapClaims) VerifyIssuer(cmp string, req bool) bool {

// Compares the nbf claim against cmp.
// If required is false, this method will return true if the value matches or is unset
func (m MapClaims) VerifyNotBefore(cmp int64, req bool) bool {
func (m MapClaims) VerifyNotBefore(cmp time.Time, req bool) bool {
switch nbf := m["nbf"].(type) {
case float64:
return verifyNbf(int64(nbf), cmp, req)
return verifyNbf(nbf, cmp, req)
case json.Number:
v, _ := nbf.Int64()
v, _ := nbf.Float64()
return verifyNbf(v, cmp, req)
}
return req == false
Expand All @@ -69,7 +70,7 @@ func (m MapClaims) VerifyNotBefore(cmp int64, req bool) bool {
// be considered a valid claim.
func (m MapClaims) Valid() error {
vErr := new(ValidationError)
now := TimeFunc().Unix()
now := TimeFunc()

if m.VerifyExpiresAt(now, false) == false {
vErr.Inner = errors.New("Token is expired")
Expand Down
2 changes: 1 addition & 1 deletion parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ var jwtTestData = []struct {
"",
defaultKeyFunc,
&jwt.StandardClaims{
ExpiresAt: time.Now().Add(time.Second * 10).Unix(),
ExpiresAt: float64(time.Now().Add(time.Second * 10).Unix()),
},
true,
0,
Expand Down
2 changes: 1 addition & 1 deletion rsa_pss_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func TestRSAPSSSaltLengthCompatibility(t *testing.T) {
func makeToken(method jwt.SigningMethod) string {
token := jwt.NewWithClaims(method, jwt.StandardClaims{
Issuer: "example",
IssuedAt: time.Now().Unix(),
IssuedAt: float64(time.Now().Unix()),
})
privateKey := test.LoadRSAPrivateKeyFromDisk("test/sample_key")
signed, err := token.SignedString(privateKey)
Expand Down