Fixes JWS signing bug where JSON unmarshalling breaks verification

[EthanHeilman]

This fixes an bug where marshalling and unmarshalling the PK Token could break verification of the PK Token in semi-rare circumstances.

Bug Description

PR #45 Use jws signatures introduced a bug that in some circumstances break signature verification.

JSON Marshalling/Unmarshalling json result in a different json representation. For instance the json string {"c": 1, "b": 2, "a": 3} could be marshalled and unmarshalled to json string (with alphabetized keys): {"a": 3, "b": 2, "c": 1}
as order of keys is not guaranteed to be preserved during marshaling. A signature over {"c": 1, "b": 2, "a": 3} will not verify for {"a": 3, "b": 2, "c": 1} even if semantically they are same object. This issue is not limited to only alphabetization, a similar problem exists for whitespace.

Prior to PR #45 we stored the original signed bytes of the OP, CIC and COS in the PK Token to ensure that the bytes we signed are the same bytes we used to construct the marshalled version of the PK Token for verification.

type PKToken struct {
	Payload []byte
	OpPH    []byte
	OpSig   []byte
...

PR #45 altered this behavior so that the bytes we signed were unmarshalled into a jws.signature object.

type Signature = jws.Signature

type PKToken struct {
	raw []byte // the original, raw representation of the object

	Payload []byte     // decoded payload
	Op      *Signature // Provider Signature
	Cic     *Signature // Client Signature
	Cos     *Signature // Cosigner Signature

This did not break verification for our current OP and it was only until experimenting with Azure that this bug was discovered.

Fix

We fix this bug storing the signed tokens in the PK Token.

OpToken  []byte // Base64 encoded ID Token signed by the OP
CicToken []byte // Base64 encoded Token signed by the Client
CosToken []byte // Base64 encoded Token signed by the Cosigner

Then we plug these bytes for the fields in the json PK Token so that you always verify the exact bytes that were signed.

Additionally we needed to change the pkt.Compact function. pkt.Compact takes an unmarshalled signature and then produces the a compact token for verification. This assumes that the unmarshalled signature is safe to use for verificatio but there are several circumstances in which the returned compact token my not verify. To keep the lines of code in this PR low, this PR does not fully remove pkt.Compact. Instead it changes pkt.Compact` to simply return the saved compact token in the PK Token making it safe to use. We have marked FromCompact as depreciated and plan to remove it completely in a future PR.

Instead of pkt.Compact, developers should simply use the compact token saved to the PK Token.
Rather than:

cicToken, err := pkt.Compact(pkt.Cic)

do this instead:

cicToken := pkt.CicToken

In a future PR pkt.Compact will be removed.

Regression Tests

We also created a regression test which creates a PK Token raw json strings and then we marshal and unmarshal the PK Token and check if the signed values are still equal to the original signed values.

I tested this PR against the Google and MFA example tests on windows.

TODOs:

• Change unittests use a table test
• Fix strange memory test failures that seem to happen randomly after this code change

[mrjoelkamp]

LGTM, just one thing in simplejws.go

[lgmugnier]

If this PR is deprecating a function, it seems like we should be replacing everywhere we internally use that function in this PR, otherwise I'd doubt it would ever be done and therefore we would never be able to remove the function

[EthanHeilman]

Next PR after this one removes Compact. I wanted to keep this PR small and fast to review so I'm adding the bugfix here and them removing it later.

I created an issue to track this #107

[lgmugnier]

if we're creating out own types here that are safe from the issues with the jws library we were using, then why do we need to also save the raw tokens as values when we can replace the jws.Signature with our own and always be able to create them from the object as saved?

[EthanHeilman]

Eventually I hope to remove all dependencies on jwx. That is too large of a change to make in a single PR and impacts too much. This PR introduces simplejws as the acorn to fix the bug and set that feature work up.
The general sketch of this work is:

1. We move to compact tokens internally in the pktoken (this PR)
2. Then we remove the Compact function from pktoken
3. Hopefully by this point we have moved CIC creation and verification into the providers.
4. Slow start moving away from jwx

[EthanHeilman]

@lgmugnier Any objection to merging?

[EthanHeilman]

Lucie is good with merging this