Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

Builder serializes integers as floats, which subsequently fail to Decode. #142

Closed
jacobstr opened this issue Mar 23, 2017 · 7 comments
Closed
Assignees
Labels

Comments

@jacobstr
Copy link

Assuming I'm doing this all using the blessed path. I've included a snippet at the bottom to demonstrate.

Relevant output produced:

serialized to eyJhbGciOiJIUzUxMiJ9.eyJudW1iZXIiOjFlKzA2fQ.oFRp6CQOyNEicaiXDZASuGZpAX00fWnCoMRd89ueIFqo2t48WUo4C3Ld0EkrSViLYk5FxoxmyxXRJaq6BYKWuA
parsing signed
parsing claims
json: cannot unmarshal number 1e+06 into Go value of type int64

If you base64 decode the 2nd component of the compact JWT you'll see "number": 1e+06.

I suspect the culprit is here: https://github.com/square/go-jose/blob/v2/jwt/builder.go#L132 - the Marshal/Unmarshal loses the type information that would otherwise cause go's marshaler to encode Number as an integer.

package main

import (
	"fmt"

	jose "gopkg.in/square/go-jose.v2"
	jwt "gopkg.in/square/go-jose.v2/jwt"
)

const SecretKey = "WoW"

type Claims struct {
	Number int64 `json:"number"`
}

func main() {
	fmt.Println("building signer")
	signer, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.HS512, Key: []byte(SecretKey)}, nil)
	if err != nil {
		fmt.Println(err)
		return
	}

	fmt.Println("building builder")
	token := jwt.Signed(signer)
	token = token.Claims(Claims{Number: 1000000})

	fmt.Println("serializing")
	enc, err := token.CompactSerialize()
	if err != nil {
		fmt.Println(err)
		return
	}
	fmt.Printf("serialized to %s\n", enc)

	fmt.Println("parsing signed")
	tok, err := jwt.ParseSigned(enc)
	if err != nil {
		fmt.Println(err)
		return
	}

	fmt.Println("parsing claims")
	claims := Claims{}
	if err := tok.Claims([]byte(SecretKey), &claims); err != nil {
		fmt.Println(err)
		return
	}
	fmt.Printf("%#v\n", claims)
}
@csstaub
Copy link
Collaborator

csstaub commented Mar 24, 2017

Yep, that looks like a bug. I'll have to think about how to best fix this.

@csstaub csstaub added the bug label Mar 24, 2017
@csstaub
Copy link
Collaborator

csstaub commented Mar 24, 2017

Seems like this should fix it:

func normalizeNew(i interface{}) (map[string]interface{}, error) {
	m := make(map[string]interface{})

	raw, err := json.Marshal(i)
	if err != nil {
		return nil, err
	}

	d := json.NewDecoder(bytes.NewReader(raw))
	d.UseNumber()

	if err := d.Decode(&m); err != nil {
		return nil, err
	}

	return m, nil
}

Example: https://play.golang.org/p/2irRfWlxgN

original: {1234567890 12345.6789}
normalized (old): map[int:1.23456789e+09 float:12345.6789]
normalized (new): map[float:12345.6789 int:1234567890]

So that should preserve ints as ints, and floats as floats. I'll make a PR shortly.

csstaub added a commit that referenced this issue Mar 24, 2017
We normalize claims for tokens given to us in the builder by marshaling
and then unmarshaling as a JSON object. However, because JSON officially
only supports floating-point numbers, this loses precision for integers
embedded in structs. We switch to using json.Decoder (with UseNumber
option) to decode numbers into json.Number, which preserves the precise
value for both integers and floats.
csstaub added a commit that referenced this issue Mar 24, 2017
We normalize claims for tokens given to us in the builder by marshaling
and then unmarshaling as a JSON object. However, because JSON officially
only supports floating-point numbers, this loses precision for integers
embedded in structs. We switch to using json.Decoder (with UseNumber
option) to decode numbers into json.Number, which preserves the precise
value for both integers and floats.
@csstaub csstaub self-assigned this Mar 24, 2017
csstaub added a commit that referenced this issue Mar 24, 2017
Preserve integers when normalizing JWT claims (fixes #142)
@bwplotka
Copy link

bwplotka commented May 3, 2017

Thanks a lot for fixing it! I got caught with the same problem. I spent like 3 days to figure out why AWS AssumeRoleWithWebIdentity did not understand my ID token's exp claim... Every other client can parse float just fine (e.g https://jwt.io/ - javascript), but on AWS I was getting unreasonable error.. Token expired: current date/time 1493802627 must be before the expiration date/time0...

Any reason why go-jose.v2 serializes int as float? When I swapped to encode/json I did not need any fix in normalize function. I understand go-jose is keeping it's own json fork, but can we fix just that serialization issue? Especially that JWT is used heavily with OpenID numeric value (int!) concept.

Furthermore, not sure why, but when you do go get gopkg.in/square/go-jose.v2 you don't get fixed version. I needed to manually checkout to origin/v2, can you update upstream as well?

Otherwise, I think we can close this issue. (:

@bwplotka
Copy link

bwplotka commented May 3, 2017

You can ignore me on that:

Any reason why go-jose.v2 serializes int as float? When I swapped to encode/json I did not need any fix in normalize function. I understand go-jose is keeping it's own json fork, but can we fix just that serialization issue? Especially that JWT is used heavily with OpenID numeric value (int!) concept.

AWS STS server seems to be just broken if it does not accept fractional part with exponential E notation in JSON as number..

@csstaub
Copy link
Collaborator

csstaub commented May 3, 2017

Sorry, I haven't tagged a release with the fix yet. That's why go get via gopkg.in still gives you the broken version. I can tag a release today. @Bplotka do you have an example code snippet for the behavior that you're talking about? i.e. where in go-jose is it serializing as float?

@bwplotka
Copy link

bwplotka commented May 3, 2017

So any unmarshaller (jose and encode/json) parses normally any number to float64 type:

package main

import (
	enc_json "encoding/json"
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	jose_json "gopkg.in/square/go-jose.v2/json"
)

func TestJSONUnmarshallers(t *testing.T) {
	inputJSON := `{"exp": 1493718272}`

	out := map[string]interface{}{}
	err := jose_json.Unmarshal([]byte(inputJSON), &out)
	require.NoError(t, err)

	assert.Equal(t, float64(1493718272), out["exp"])

	out2 := map[string]interface{}{}
	err = enc_json.Unmarshal([]byte(inputJSON), &out2)
	require.NoError(t, err)

	assert.Equal(t, float64(1493718272), out2["exp"])
}

It is kind of expected, so old normalize was simply losing concrete type info. Fixed normalize helped with my case.
However during debugging it more I got caught into case where even after losing type (so having float64 where I specified int) encode/json Marshal was handling my float which was holding an int in a smart way and that fixed my AWS issue as well:

package main

import (
	enc_json "encoding/json"
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	jose_json "gopkg.in/square/go-jose.v2/json"
)

func TestJSONMarshallers(t *testing.T) {
	float := float64(1493718272)
	marshaledFloat, err := jose_json.Marshal(float)
	require.NoError(t, err)

	// Since it was float.. this output is expected.
	assert.Equal(t, []byte("1.493718272e+09"), marshaledFloat)

	marshaledFloatLikeInt, err := enc_json.Marshal(float)
	require.NoError(t, err)

	// Cool to see that encode/json marshaled my float without E notation,
	// since it is not needed.
	assert.Equal(t, []byte("1493718272"), marshaledFloatLikeInt)
}

This is what I called serializing as float. Sorry for confusion - smart behavior is not required. It was just surprising and in the same helpful for my case (: New normalize fixes my problem.

@csstaub
Copy link
Collaborator

csstaub commented May 3, 2017

Cool. Tagged release v2.1.1 with the normalize bug fix.

@csstaub csstaub closed this as completed Jun 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants