diff --git a/cmd/ateapi/internal/k8sjwt/k8sjwt.go b/cmd/ateapi/internal/k8sjwt/k8sjwt.go index f121e93fd..014d46b37 100644 --- a/cmd/ateapi/internal/k8sjwt/k8sjwt.go +++ b/cmd/ateapi/internal/k8sjwt/k8sjwt.go @@ -402,73 +402,105 @@ func discoverKeysForIssuer(ctx context.Context, httpClient *http.Client, issuer slog.InfoContext(ctx, "Fetched JWK set", slog.Any("jwkSet", jwkSet)) var ret []*KeyAndID + skipped := 0 for _, jwk := range jwkSet.Keys { - if jwk.KeyID == "" { - return nil, fmt.Errorf("JWKs endpoint returned key without key ID") + key, err := parseJWK(jwk) + if err != nil { + // Skip an unusable key instead of failing the whole issuer; it's safe because + // keys are selected by kid and the signature is still verified, so a skipped + // key can't be abused. Debug, not Warn: unsupported key types are a normal + // config and discovery runs on every Verify (no cache yet), so Warn would spam. + slog.DebugContext(ctx, "Skipping unusable JWK from issuer", + slog.String("kid", jwk.KeyID), slog.String("kty", jwk.KeyType), slog.Any("err", err)) + skipped++ + continue } + ret = append(ret, key) + } - switch jwk.KeyType { - case "EC": - curve, err := ellipticCurveForJWK(jwk.EllipticCurve) - if err != nil { - return nil, err - } - if jwk.EllipticX == "" || jwk.EllipticY == "" { - return nil, fmt.Errorf("EC JWK is missing the x or y coordinate") - } - xBytes, err := base64.RawURLEncoding.DecodeString(jwk.EllipticX) - if err != nil { - return nil, fmt.Errorf("while base64-decoding EC x coordinate: %w", err) - } - yBytes, err := base64.RawURLEncoding.DecodeString(jwk.EllipticY) - if err != nil { - return nil, fmt.Errorf("while base64-decoding EC y coordinate: %w", err) - } - x := new(big.Int).SetBytes(xBytes) - y := new(big.Int).SetBytes(yBytes) - // Reject coordinates outside the field. This is a cheap sanity check; the - // authoritative on-curve validation happens in ecdsa.Verify, which returns - // false for a public key whose point is not on the curve. - p := curve.Params().P - if x.Cmp(p) >= 0 || y.Cmp(p) >= 0 { - return nil, fmt.Errorf("EC JWK coordinate is out of range for curve %q", jwk.EllipticCurve) - } - ret = append(ret, &KeyAndID{ - KeyID: jwk.KeyID, - PublicKey: &ecdsa.PublicKey{Curve: curve, X: x, Y: y}, - }) - - case "RSA": - nBytes, err := base64.RawURLEncoding.DecodeString(jwk.RSAN) - if err != nil { - return nil, fmt.Errorf("while base64-decoding n: %w", err) - } - n := &big.Int{} - n.SetBytes(nBytes) - - eBytes, err := base64.RawURLEncoding.DecodeString(jwk.RSAE) - if err != nil { - return nil, fmt.Errorf("while base64-decoding e: %w", err) - } - e := &big.Int{} - e.SetBytes(eBytes) - - ret = append(ret, &KeyAndID{ - KeyID: jwk.KeyID, - PublicKey: &rsa.PublicKey{ - N: n, - E: int(e.Int64()), - }, - }) - - default: - return nil, fmt.Errorf("unhandled key type %q", jwk.KeyType) + // None usable: fail here (reasons logged above) rather than return an empty set + // that fails later as a vaguer "unknown key ID". + if len(ret) == 0 { + if len(jwkSet.Keys) == 0 { + return nil, fmt.Errorf("issuer %q published an empty JWKS", issuer) } + return nil, fmt.Errorf("no usable keys in JWKS for issuer %q (%d skipped)", issuer, skipped) + } + + if skipped > 0 { + slog.DebugContext(ctx, "Skipped unusable JWKs from issuer", + slog.String("issuer", issuer), slog.Int("skipped", skipped), slog.Int("usable", len(ret))) } return ret, nil } +// parseJWK converts a single JWK into a verification key, returning an error for a key +// the verifier cannot use (missing key ID, unsupported key type or curve, or malformed +// parameters). +func parseJWK(jwk jwkT) (*KeyAndID, error) { + if jwk.KeyID == "" { + return nil, fmt.Errorf("JWK has no key ID") + } + switch jwk.KeyType { + case "EC": + curve, err := ellipticCurveForJWK(jwk.EllipticCurve) + if err != nil { + return nil, err + } + if jwk.EllipticX == "" || jwk.EllipticY == "" { + return nil, fmt.Errorf("EC JWK is missing the x or y coordinate") + } + xBytes, err := base64.RawURLEncoding.DecodeString(jwk.EllipticX) + if err != nil { + return nil, fmt.Errorf("while base64-decoding EC x coordinate: %w", err) + } + yBytes, err := base64.RawURLEncoding.DecodeString(jwk.EllipticY) + if err != nil { + return nil, fmt.Errorf("while base64-decoding EC y coordinate: %w", err) + } + x := new(big.Int).SetBytes(xBytes) + y := new(big.Int).SetBytes(yBytes) + // Reject coordinates outside the field. This is a cheap sanity check; the + // authoritative on-curve validation happens in ecdsa.Verify, which returns + // false for a public key whose point is not on the curve. + p := curve.Params().P + if x.Cmp(p) >= 0 || y.Cmp(p) >= 0 { + return nil, fmt.Errorf("EC JWK coordinate is out of range for curve %q", jwk.EllipticCurve) + } + return &KeyAndID{ + KeyID: jwk.KeyID, + PublicKey: &ecdsa.PublicKey{Curve: curve, X: x, Y: y}, + }, nil + + case "RSA": + nBytes, err := base64.RawURLEncoding.DecodeString(jwk.RSAN) + if err != nil { + return nil, fmt.Errorf("while base64-decoding n: %w", err) + } + n := &big.Int{} + n.SetBytes(nBytes) + + eBytes, err := base64.RawURLEncoding.DecodeString(jwk.RSAE) + if err != nil { + return nil, fmt.Errorf("while base64-decoding e: %w", err) + } + e := &big.Int{} + e.SetBytes(eBytes) + + return &KeyAndID{ + KeyID: jwk.KeyID, + PublicKey: &rsa.PublicKey{ + N: n, + E: int(e.Int64()), + }, + }, nil + + default: + return nil, fmt.Errorf("unhandled key type %q", jwk.KeyType) + } +} + func fetchJSON[T any](httpClient *http.Client, url string) (T, error) { var parsedBody T if httpClient == nil { diff --git a/cmd/ateapi/internal/k8sjwt/k8sjwt_test.go b/cmd/ateapi/internal/k8sjwt/k8sjwt_test.go index 233da9ab3..ad3d80ac4 100644 --- a/cmd/ateapi/internal/k8sjwt/k8sjwt_test.go +++ b/cmd/ateapi/internal/k8sjwt/k8sjwt_test.go @@ -28,6 +28,7 @@ import ( "math/big" "net/http" "net/http/httptest" + "strings" "sync" "testing" "time" @@ -276,6 +277,81 @@ func TestVerifyMixedJWKS(t *testing.T) { } } +// TestVerifyUnusableJWKSKeySkipped pins the resilient discovery contract: a key the +// verifier cannot parse (here an unsupported P-192 curve) is skipped rather than +// failing discovery for the whole issuer, so a token signed by a supported key in +// the same JWKS still verifies — while a token that references the skipped key is +// still rejected (skipping is not accepting). +func TestVerifyUnusableJWKSKeySkipped(t *testing.T) { + ti := newTestIssuer(t) + rsaKey := testRSAKey(t) + ti.addRSA("rsa-1", &rsaKey.PublicKey) + ti.jwks.Keys = append(ti.jwks.Keys, jwkT{ + KeyType: "EC", KeyID: "ec-bad", EllipticCurve: "P-192", EllipticX: "AA", EllipticY: "AA", + }) + + good := mintJWT(t, "RS256", "rsa-1", rsaKey, validClaims(ti.issuer())) + if _, err := Verify(context.Background(), ti.server.Client(), good, ti.issuer(), testAudience, time.Now()); err != nil { + t.Fatalf("Verify with an unusable key in the JWKS = %v, want nil (bad key should be skipped)", err) + } + + referencesSkipped := mintJWT(t, "RS256", "ec-bad", rsaKey, validClaims(ti.issuer())) + if _, err := Verify(context.Background(), ti.server.Client(), referencesSkipped, ti.issuer(), testAudience, time.Now()); err == nil { + t.Fatal("Verify accepted a token whose kid names a key that was skipped") + } +} + +// TestDiscoverKeysAllUnusable pins that an issuer whose JWKS contains only keys the +// verifier can't use fails at discovery (naming the cause) rather than returning an +// empty key set. +func TestDiscoverKeysAllUnusable(t *testing.T) { + ti := newTestIssuer(t) + ti.jwks.Keys = append(ti.jwks.Keys, jwkT{ + KeyType: "EC", KeyID: "ec-bad", EllipticCurve: "P-192", EllipticX: "AA", EllipticY: "AA", + }) + _, err := discoverKeysForIssuer(context.Background(), ti.server.Client(), ti.issuer()) + if err == nil { + t.Fatal("discoverKeysForIssuer returned nil error for an issuer with no usable keys") + } + if !strings.Contains(err.Error(), "no usable keys") { + t.Errorf("error = %q, want it to name the cause (contain %q)", err, "no usable keys") + } +} + +// TestDiscoverKeysEmptyJWKS pins the distinct error for an issuer that publishes no +// keys at all, versus one whose keys are all unusable. +func TestDiscoverKeysEmptyJWKS(t *testing.T) { + ti := newTestIssuer(t) // no keys registered + _, err := discoverKeysForIssuer(context.Background(), ti.server.Client(), ti.issuer()) + if err == nil { + t.Fatal("discoverKeysForIssuer returned nil error for an empty JWKS") + } + if !strings.Contains(err.Error(), "empty JWKS") { + t.Errorf("error = %q, want it to mention %q", err, "empty JWKS") + } +} + +func TestParseJWKRejects(t *testing.T) { + tests := []struct { + name string + jwk jwkT + }{ + {"no key ID", jwkT{KeyType: "RSA", KeyID: ""}}, + {"unknown key type", jwkT{KeyType: "OKP", KeyID: "k"}}, + {"unsupported EC curve", jwkT{KeyType: "EC", KeyID: "k", EllipticCurve: "P-192", EllipticX: "AA", EllipticY: "AA"}}, + {"EC missing coordinate", jwkT{KeyType: "EC", KeyID: "k", EllipticCurve: "P-256", EllipticX: "AA"}}, + {"EC malformed x", jwkT{KeyType: "EC", KeyID: "k", EllipticCurve: "P-256", EllipticX: "!!!", EllipticY: "AA"}}, + {"RSA malformed n", jwkT{KeyType: "RSA", KeyID: "k", RSAN: "!!!", RSAE: "AQAB"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if _, err := parseJWK(tc.jwk); err == nil { + t.Errorf("parseJWK(%s) = nil, want error", tc.name) + } + }) + } +} + func TestEllipticCurveForJWK(t *testing.T) { for _, crv := range []string{"P-256", "P-384", "P-521"} { if _, err := ellipticCurveForJWK(crv); err != nil {