Change how JWK validation works

[MicahParks]

Currently, validation in this project uses the result of marshaling the cryptographic key and reflect.DeepEqual() to ensure the jwkset.JWK is valid.

jwkset/jwk.go

Lines 311 to 314 in a0de971

	ok := reflect.DeepEqual(j.marshal, marshalled)
	if !ok {
	return fmt.Errorf("%w: marshaled JWK does not match original JWK", ErrJWKValidation)
	}

Rework validation to compare JWK parameters in a less strict way. This will allow the project to be compatible with other JWK projects that are not RFC compliant.

One case to note in particular is ECDSA key parameters with leading padding. The integers should be compared, not the base64 encoded strings. See #18 for additional context.

[chenzeshenga]

could we expose these options?

[kperreau]

Hi, not sure if it's related but i got the same error when i'm trying to fetch JWK from firebase app check.
URL: https://firebaseappcheck.googleapis.com/v1/jwks
Error:

ERROR Failed to refresh HTTP JWK Set from remote HTTP resource. error="failed to create JWK from JWK Marshal: failed to validate JSON Web Key: failed to validate JWK: marshaled JWK does not match original JWK" url=https://firebaseappcheck.googleapis.com/v1/jwks

[MicahParks]

@chenzeshenga, I'm not sure exactly what the request is, but yes, new validation options will be exposed via jwkset.ValidateOptions when validation has been reworked to be less strict by default.

@kperreau thank you for posting the link to the JWK Set. It looks like this is a bug with this project.

RFC 7518 Section 6.3.1.1 notes that:

Note that implementers have found that some cryptographic libraries prefix an extra zero-valued octet to the modulus representations they return, for instance, returning 257 octets for a 2048-bit key, rather than 256. Implementations using such libraries will need to take care to omit the extra octet from the base64url-encoded representation.

When I implemented the "n" RSA JWK parameter, I treated it as a normal Base64urlUInt-encoded value, ignoring this note about needing to account for extra padding. This should be another issue marked as a bug and will be fixed in this validation update, which I plan on working on today. See #23.

Edit:
I think I read that wrong just now, seems like Firebase should be the one to respect the Base64urlUInt-encoded no padding format.

Small Go program showing one of the modulus, "n", from the Firebase JWK Set.

package main

import (
	"encoding/base64"
	"math/big"
)

func main() {
	noPad := "6kXl3CgKmZbZOHmQFooNmbKA6XJBImbcmrAa89NNWIcKzhmK2UsX3Nr34M_E_fOg1qvIRiUV8uFg4VVLuZSMwa0s1OQtp8FIgigXbyzL_PUau7EnZoVyFrMGKbEjUHQxdMp0jDhXbrySWs4QI7kUhwdqvsRX1bRepOgWifehW8dbDeqQ9Xij4DsZ2uHdiHu9FQT9PlcItgP2M7jWcCpiFZhEY34rG7nXHpUxrn3lNdOunOBgYNtrUwtDMH2pwdEi9GyS3saHwzTOBzKjpzqO-S1oTOHSPaR0vYkIAUeExdRl51FQ6l6qQR6GnEMNHQN6LPtQ-Ux7qXw62z3tabfntuBqMNzGepoYYQXkYiSvvEfDXkOtN85Y9jfl_SweM1M6f0BTZs9ZPLqgdk-GgWMd1I2ndQ4i6ed_V5dGVNrthnk8uZsUXKM01KdB1w4eArHWUkTEmpcIAnakGSnR1frm6KoIvPhZkFyYIcZOiLia9DnCkWqBQn_o3wdek_lktn6F"
	pad := "AOpF5dwoCpmW2Th5kBaKDZmygOlyQSJm3JqwGvPTTViHCs4ZitlLF9za9-DPxP3zoNaryEYlFfLhYOFVS7mUjMGtLNTkLafBSIIoF28sy_z1GruxJ2aFchazBimxI1B0MXTKdIw4V268klrOECO5FIcHar7EV9W0XqToFon3oVvHWw3qkPV4o-A7Gdrh3Yh7vRUE_T5XCLYD9jO41nAqYhWYRGN-Kxu51x6VMa595TXTrpzgYGDba1MLQzB9qcHRIvRskt7Gh8M0zgcyo6c6jvktaEzh0j2kdL2JCAFHhMXUZedRUOpeqkEehpxDDR0Deiz7UPlMe6l8Ots97Wm357bgajDcxnqaGGEF5GIkr7xHw15DrTfOWPY35f0sHjNTOn9AU2bPWTy6oHZPhoFjHdSNp3UOIunnf1eXRlTa7YZ5PLmbFFyjNNSnQdcOHgKx1lJExJqXCAJ2pBkp0dX65uiqCLz4WZBcmCHGToi4mvQ5wpFqgUJ_6N8HXpP5ZLZ-hQ"
	b, err := base64.RawURLEncoding.DecodeString(noPad)
	if err != nil {
		panic(err)
	}
	noPadI := new(big.Int).SetBytes(b)
	b, err = base64.RawURLEncoding.DecodeString(pad)
	if err != nil {
		panic(err)
	}
	padI := new(big.Int).SetBytes(b)
	if noPadI.Cmp(padI) != 0 {
		println("noPadI != padI")
	}
	println("noPadI == padI")
}

[MicahParks]

@kperreau, sorry for the back and forth, but I think I read that note wrong just now. I think that implementation note is saying that implementers need to respect the Base64urlUInt-encoded format. So not a bug in this project, but a bug I will report to Firebase. See this comment: #23 (comment)

Regardless, this project will be updated to account for any padding after this validation rework is implemented.

[kperreau]

All right, thank you for the explanation and your work! 🙏

[MicahParks]

@kperreau and @chenzeshenga I'm doing a release for the new validation soon. It will be less strict and handle invalid padding by default.

Any questions or comments are welcome. I think community feedback is important.

[chenzeshenga]

@MicahParks thanks for the quick update, and it would be really helpful.

For exposing the option, the original background is the jwks inside our company doesn't have the kid key, so I want to skip this validation, but not other members.

And I checked the document, the kid isn't a required member.

https://www.rfc-editor.org/rfc/rfc7517#section-4.5
The "kid" (key ID) parameter is used to match a specific key. This is used, for instance, to choose among a set of keys within a JWK Set during key rollover. The structure of the "kid" value is unspecified. When "kid" values are used within a JWK Set, different keys within the JWK Set SHOULD use distinct "kid" values. (One example in which different keys might use the same "kid" value is if they have different "kty" (key type) values but are considered to be equivalent alternatives by the application using them.) The "kid" value is a case-sensitive string. Use of this member is OPTIONAL. When used with JWS or JWE, the "kid" value is used to match a JWS or JWE "kid" Header Parameter value.

So if posible, could we expose some option like skipKid and recheck other members as well?

[MicahParks]

@chenzeshenga I don't fully understand why an option to skip validation of the JWK kid parameter is desired. Does the undesired behavior happen when creating a JWK Set or consuming one?

I would like to add an option to skip validating the kid if I can understand the use case. Right now, I believe the project should validate a JWK kid parameter if it's populated or empty. Perhaps another part of the project relies on the kid parameter being present?

An example would be best to convey your meaning. Would you please provide a runnable example of this jwkset project failing validation or other undesired behavior? You can use the below as a template:

package main

import (
	"context"
	"crypto/ed25519"
	"crypto/rand"
	"log/slog"

	"github.com/MicahParks/jwkset"
)

const (
	logErr = "error"
)

func main() {
	l := slog.Default()
	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	pub, _, err := ed25519.GenerateKey(rand.Reader)
	if err != nil {
		l.ErrorContext(ctx, "Failed to generate Ed25519 key.",
			logErr, err,
		)
		return
	}

	options := jwkset.JWKOptions{
		Metadata: jwkset.JWKMetadataOptions{
			KID: "",
		},
	}
	jwk, err := jwkset.NewJWKFromKey(pub, options)
	if err != nil {
		l.ErrorContext(ctx, "Failed to create JWK.",
			logErr, err,
		)
		return
	}

	_ = jwk
}

[kperreau]

Everything works fine now for me, thx!

[chenzeshenga]

@MicahParks never mind, I have found out the way to use jwkset to do the validation.
Thanks for ur kindly support and reply