From d99ba502125fa53b93c2a6bab1a0b2bc5622742b Mon Sep 17 00:00:00 2001 From: David Henning Date: Tue, 8 Sep 2026 22:46:22 +0200 Subject: [PATCH 1/4] fix: close review findings on claims, parsing, and input Six confirmed review findings: - Pre-check exp/nbf/iat through claimTime before the golang-jwt validator. parseNumericDate discards the Float64 error and casts to int64, so a value outside int64 range wrapped and produced the opposite verdict ("nbf":1e400 reported Claims: VALID). An unrepresentable timestamp is now an explicit invalid reason naming the claim, shared by the human and --json paths. - Reject a null JWT header or payload with "expected JSON object", the guard the JWE protected header already had. A null payload previously rendered as {} with a VALID claim verdict. - Resolve the key before printParsedJWT writes anything, so a bad --key emits nothing on stdout. Verification splits into verifyLoadedKeySignature (the core, taking a loaded key) and verifyJWTSignature (the key-argument wrapper the --json path uses); the alg allowlist stays in the core. - Report an empty or whitespace-only token argument as "no token provided" instead of as a malformed token. - Bound the stdin read with io.LimitReader at maxStdinTokenBytes (16 MiB) and fail with a clear message instead of truncating. - Remove the production-dead isJWT and verifyClaims; verifyClaims moves to helpers_test.go beside verifySignature. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0125xMARDzdnbkbMTbNxSihH --- AGENTS.md | 14 ++--- claims.go | 77 ++++++++++++++++++----- claims_test.go | 162 ++++++++++++++++++++++++++++++++++++++++++++++++ helpers_test.go | 24 +++++-- jwe.go | 7 --- main.go | 74 +++++++++++++++++----- main_test.go | 112 +++++++++++++++++++++++++++++++++ 7 files changed, 424 insertions(+), 46 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ffa217b..d6230a7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,14 +11,14 @@ All functionality lives in package `main`, split across six source files: ### `main.go` - CLI, token input, and the JWT/JWS path - `main()` / `newRootCommand()` - Build and execute the Cobra root command with the `--key`/`-k`, `--json`, and `--color` flags; suppress Cobra's automatic usage/error output so runtime errors are rendered once, while invalid-signature details are not duplicated -- `run()` / `readToken()` / `decodeJWTHuman()` - Resolves the token from arguments, stdin pipe, or interactive readline prompt; falls back to `JWTD_KEY` when `--key` is not set; applies the color mode, then dispatches to the JWT/JWE handler or, under `--json`, to the JSON handler. `decodeJWTHuman` wraps `decodeAndPrint` and, when claim validation was requested, prints a Claims section after it; both the signature and claim checks run so their sections show together, and the command exits nonzero if either fails (the signature verdict takes precedence for the returned sentinel). Claim flags on a JWE emit a stderr note and are otherwise skipped +- `run()` / `readToken()` / `decodeJWTHuman()` - Resolves the token from arguments, stdin pipe, or interactive readline prompt. An empty or whitespace-only token *argument* is reported as `errNoToken`, the same as an empty pipe or an empty interactive line, rather than being handed on as a malformed token. A piped stdin is read through an `io.LimitReader` bounded by `maxStdinTokenBytes` (16 MiB) with one byte of headroom, so input past the limit is an explicit error instead of a silent truncation into a different token. Falls back to `JWTD_KEY` when `--key` is not set; applies the color mode, then dispatches to the JWT/JWE handler or, under `--json`, to the JSON handler. `decodeJWTHuman` wraps `decodeAndPrint` and, when claim validation was requested, prints a Claims section after it; both the signature and claim checks run so their sections show together, and the command exits nonzero if either fails (the signature verdict takes precedence for the returned sentinel). Claim flags on a JWE emit a stderr note and are otherwise skipped - `applyColorMode()` - Maps `--color` onto `fatih/color`'s global `NoColor`: `auto` leaves TTY/`NO_COLOR` detection untouched, `always` forces color, `never` disables it; `--json` always forces color off - `headerKID()` - Extracts the token's `kid` header (or `""`) so JWK Set verification/decryption selects the key the token names - `printKeyInterpretation()` - Notes on stderr how a key argument was read when it was not read as a file, so precedence-based detection cannot silently take a value the user meant one way and use it another; adds the process-list exposure warning for `--key` values, which `JWTD_KEY` does not carry (`/proc//cmdline` is world-readable, `/proc//environ` is owner-only). Diagnostics go to stderr so stdout stays parseable - `readInteractive()` - Prompts for a token interactively using `chzyer/readline` -- `decodeAndPrint()` / `printParsedJWT()` - `decodeAndPrint` parses the JWT with `parseUnverifiedJWT` and hands the result to `printParsedJWT`, which orchestrates output and verifies the signature when a key is provided. `printParsedJWT` runs `formatTimestamps` on a `maps.Clone` of the claims, so the display rewrite never touches the authoritative parse and the same `parsedJWT` can still be validated -- `parsedJWT` / `parseUnverifiedJWT()` / `splitCompactJWT()` / `decodeJSON()` / `isJSONWhitespace()` - Strictly decode the header, claims, and other displayed JSON with exact `json.Number` values and reject malformed or trailing JSON data. `decodeJSON` decides the ordinary no-trailing-data case with `isJSONWhitespace` over what follows the value, and only hands the input back to the decoder — which is what phrases the rejection — when something else is there. That set is JSON's own four whitespace characters, not Unicode's: a vertical tab after a value is trailing data and must still reach the decoder that says so. `parseUnverifiedJWT` returns a `parsedJWT` (the segments, decoded header and claims, signing method, and decoded signature bytes) that is threaded through the decode, signature, and claim steps, so one run parses the token once instead of once per step. It does the segment work itself rather than calling `jwt.ParseUnverified`, whose plain `json.Unmarshal` of the header and claims loses number precision and accepts trailing data — every one of its decodes would be discarded and redone strictly here. The remaining steps mirror `ParseUnverified` exactly, `splitCompactJWT`'s rejection of extra delimiters and the refusal of a missing or unknown `alg` included, so the set of tokens jwtd decodes is unchanged -- `verifyJWTSignature()` / `validMethodsForKey()` / `printSignatureVerdict()` - `verifyJWTSignature` takes a `parsedJWT` and does the cryptographic check without printing, calling the parsed token's `SigningMethod.Verify` over its own segments rather than re-parsing the compact string through `jwt.Parse`; the claims are never consulted, so the result reflects only the signature, not expiry. **It must check the header `alg` against `validMethodsForKey` before verifying.** That allowlist is what `jwt.WithValidMethods` applied while verification went through `jwt.Parse`; verifying from an already-parsed token makes it jwtd's own check, and without it an HS256 token signed with the bytes of a published public key verifies against that key. `TestVerifyJWTSignature_RejectsAlgOutsideKeyTypeAllowlist` holds it down across every key type. The allowlist fails closed: `validMethodsForKey` returns an empty list for a key type it does not know (an X25519 key, which parses as a valid JWE key but can verify no JWS), and `verifyJWTSignature` rejects such a key outright instead of skipping the check and leaving the refusal to each `Verify` implementation's type assertion; `TestVerifyJWTSignature_RejectsKeyTypeWithoutJWSAlgorithms` pins that. `verifyJWTSignature` returns `valid`, an invalid-signature `reason`, and a separate hard `err` (unusable key). `printSignatureVerdict` renders `Signature: VALID`/`INVALID` from it and returns the `errInvalidSignature` sentinel on failure so the CLI exits nonzero. The `--json` path reuses the same core. Production code always holds a `parsedJWT` at this point, so there is no string-taking wrapper in the package; the tests keep one (`verifySignature` in `helpers_test.go`) for the cases that start from a compact string +- `decodeAndPrint()` / `printParsedJWT()` - `decodeAndPrint` parses the JWT with `parseUnverifiedJWT` and hands the result to `printParsedJWT`, which orchestrates output and verifies the signature when a key is provided. **`printParsedJWT` resolves the key before it writes anything**: `loadKeyForKID` runs ahead of the first `printSection`, so an unusable `--key` fails with an error alone instead of three decoded sections followed by one — the JWE path and both `--json` paths already resolved their key up front, and this is what makes the human JWT path agree with them. `printParsedJWT` runs `formatTimestamps` on a `maps.Clone` of the claims, so the display rewrite never touches the authoritative parse and the same `parsedJWT` can still be validated +- `parsedJWT` / `parseUnverifiedJWT()` / `splitCompactJWT()` / `decodeJSON()` / `isJSONWhitespace()` - Strictly decode the header, claims, and other displayed JSON with exact `json.Number` values and reject malformed or trailing JSON data. A header or payload segment that is the literal `null` decodes without error and leaves the target map nil, so both are additionally rejected with an `expected JSON object` error — the guard `jweProtectedHeaderMap` already carried; without it a `null` payload rendered as `{}` with a VALID claim verdict. `decodeJSON` decides the ordinary no-trailing-data case with `isJSONWhitespace` over what follows the value, and only hands the input back to the decoder — which is what phrases the rejection — when something else is there. That set is JSON's own four whitespace characters, not Unicode's: a vertical tab after a value is trailing data and must still reach the decoder that says so. `parseUnverifiedJWT` returns a `parsedJWT` (the segments, decoded header and claims, signing method, and decoded signature bytes) that is threaded through the decode, signature, and claim steps, so one run parses the token once instead of once per step. It does the segment work itself rather than calling `jwt.ParseUnverified`, whose plain `json.Unmarshal` of the header and claims loses number precision and accepts trailing data — every one of its decodes would be discarded and redone strictly here. The remaining steps mirror `ParseUnverified` exactly, `splitCompactJWT`'s rejection of extra delimiters and the refusal of a missing or unknown `alg` included, so the set of tokens jwtd decodes is unchanged +- `verifyJWTSignature()` / `verifyLoadedKeySignature()` / `validMethodsForKey()` / `printSignatureVerdict()` - `verifyLoadedKeySignature` is the verification core: it takes a `parsedJWT` and an **already-loaded** key and does the cryptographic check without printing, calling the parsed token's `SigningMethod.Verify` over its own segments rather than re-parsing the compact string through `jwt.Parse`; the claims are never consulted, so the result reflects only the signature, not expiry. **It must check the header `alg` against `validMethodsForKey` before verifying.** That allowlist is what `jwt.WithValidMethods` applied while verification went through `jwt.Parse`; verifying from an already-parsed token makes it jwtd's own check, and without it an HS256 token signed with the bytes of a published public key verifies against that key. `TestVerifyJWTSignature_RejectsAlgOutsideKeyTypeAllowlist` holds it down across every key type. The allowlist fails closed: `validMethodsForKey` returns an empty list for a key type it does not know (an X25519 key, which parses as a valid JWE key but can verify no JWS), and `verifyJWTSignature` rejects such a key outright instead of skipping the check and leaving the refusal to each `Verify` implementation's type assertion; `TestVerifyJWTSignature_RejectsKeyTypeWithoutJWSAlgorithms` pins that. Everything `verifyLoadedKeySignature` can report is a verdict on the signature, so it returns only `valid` plus an invalid-signature `reason`; the one hard failure, an unusable key argument, belongs to whoever loaded the key. `printSignatureVerdict` takes the loaded key too — that is what lets `printParsedJWT` fail before printing — renders `Signature: VALID`/`INVALID`, and returns the `errInvalidSignature` sentinel on failure so the CLI exits nonzero. `verifyJWTSignature` is the key-argument-taking wrapper (`loadKeyForKID` plus the core, returning the separate hard `err`) that the `--json` path and the benchmarks use, so both output paths share one verification. Production code always holds a `parsedJWT` at this point, so there is no string-taking wrapper in the package; the tests keep `verifySignature` and `verifyClaims` in `helpers_test.go` for the cases that start from a compact string - `publicKeyForVerification()` - Extracts the public key from RSA/ECDSA/Ed25519 private keys ### `formatter.go` - Colored JSON rendering @@ -35,14 +35,14 @@ All functionality lives in package `main`, split across six source files: ### `claims.go` - Opt-in claim validation - `claimChecks` / `requested()` - Holds the `--verify-claims`, `--aud`, and `--iss` flag values. The zero value requests nothing, so the default stays decode-only and the exit code keeps reflecting the signature alone; `requested()` treats an expected audience or issuer as implying validation, so those flags work without also passing `--verify-claims` -- `validateClaimsSet()` - Runs the requested RFC 7519 checks against the already-parsed claims via `jwt.NewValidator` with `jwt.WithTimeFunc(timeNow)` (so the verdict shares the display clock and is deterministic under `pinTime`), returning `valid` plus a reason. Temporal claims (`exp`, `nbf`) that are present are always checked; an expected `aud`/`iss` is additionally required to be present and match. A missing `exp` is not treated as expired. No signature verification happens here — the human and `--json` paths share this core -- `verifyClaims()` / `printClaimsVerdict()` / `claimReason()` - `printClaimsVerdict` runs the core against already-parsed claims and renders `Claims: VALID`/`INVALID` with the reason, returning the `errInvalidClaims` sentinel on failure; it must be given the raw parse, not the display copy `formatTimestamps` rewrites. `verifyClaims` is the wrapper that parses the token first (and returns a hard error on an unparseable one). `claimReason` flattens the validator's newline-joined multi-error onto one `; `-separated line so the dim reason and wrapped error stay readable +- `validateClaimsSet()` / `unrepresentableTimeClaim()` / `truncateClaimValue()` - Runs the requested RFC 7519 checks against the already-parsed claims via `jwt.NewValidator` with `jwt.WithTimeFunc(timeNow)` (so the verdict shares the display clock and is deterministic under `pinTime`), returning `valid` plus a reason. Temporal claims (`exp`, `nbf`) that are present are always checked; an expected `aud`/`iss` is additionally required to be present and match. A missing `exp` is not treated as expired. No signature verification happens here — the human and `--json` paths share this core. **`unrepresentableTimeClaim` must run before the validator.** golang-jwt's `parseNumericDate` converts a claim with a `Float64` whose error it discards and then casts to `int64` (`map_claims.go`), so a value outside `int64` range wraps — typically to `math.MinInt64` — and the validator returns the *opposite* verdict: `"exp":1e400` reads as long expired, `"nbf":1e400` as long since valid, neither visible in the output because `claimTime` declines to annotate a timestamp it cannot represent. So `exp`, `nbf`, and `iat` are pre-checked through `claimTime` (output.go), in that fixed order, and an unrepresentable numeric value becomes an explicit invalid reason naming the claim. The representability rule is `claimTime`'s own, so the verdict and the display cannot drift. Only numeric values are examined — a non-numeric temporal claim is not a timestamp at all and keeps the validator's own message — and `truncateClaimValue` caps the literal quoted in the reason, which is rendered on one line +- `printClaimsVerdict()` / `claimReason()` - `printClaimsVerdict` runs the core against already-parsed claims and renders `Claims: VALID`/`INVALID` with the reason, returning the `errInvalidClaims` sentinel on failure; it must be given the raw parse, not the display copy `formatTimestamps` rewrites. Production code always holds a `parsedJWT` here, so the token-parsing wrapper lives in `helpers_test.go` (`verifyClaims`) alongside `verifySignature`. `claimReason` flattens the validator's newline-joined multi-error onto one `; `-separated line so the dim reason and wrapped error stay readable **Claim validation is display-and-exit-code only, and deliberately separate from signature verification.** It performs no cryptography and runs whether or not a key is given, so `--verify-claims` on an unverified token still reports expiry — the two verdicts (`Signature:` and `Claims:`) are shown independently and either failing exits nonzero. This keeps the pre-existing invariant that a bare decode never fails on expiry intact: nothing validates claims unless a claim flag is passed. ### `jwe.go` - JWE parsing and decryption -- `isJWE()` / `isJWT()` / `isJWEBytes()` / `isJWTBytes()` - Detect JWE (5 dot-separated parts) and JWS/JWT (3 parts) compact serialization. Every token-shape dispatch goes through one of these, so the shape rules are not restated inline; all four read their delimiter counts from the `jweDelimiters`/`jwtDelimiters` constants, so the string and byte forms cannot disagree. The byte forms exist for `printDecryptedPayload`, where the candidate is a decrypted payload that may be large and must not be copied into a string just to be measured +- `isJWE()` / `isJWEBytes()` / `isJWTBytes()` - Detect JWE (5 dot-separated parts) and JWS/JWT (3 parts) compact serialization. Every token-shape dispatch goes through one of these, so the shape rules are not restated inline; all three read their delimiter counts from the `jweDelimiters`/`jwtDelimiters` constants, so the string and byte forms cannot disagree. There is no string-form `isJWT`: the CLI dispatches on `isJWE` alone and everything else is handed to the JWT parser, so a JWT-shaped string predicate had no production caller. The byte forms exist for `printDecryptedPayload`, where the candidate is a decrypted payload that may be large and must not be copied into a string just to be measured - `decodeAndPrintJWE()` / `jweProtectedHeaderMap()` - Parse a JWE with `go-jose` and decode every field in the compact protected header for display; without a key print encrypted part metadata, with a key decrypt and print the payload - `jweEncryptedParts()` / `printEncryptedParts()` / `partSize()` - Encrypted part metadata shown when no key is provided. `jweEncryptedParts` splits the compact serialization into its five segments for both the human and `--json` paths; `partSize` renders `base64URLLen`'s count (or its `-1`) as display text, so measuring and formatting are not implemented twice diff --git a/claims.go b/claims.go index 744abec..f5fe7de 100644 --- a/claims.go +++ b/claims.go @@ -1,9 +1,11 @@ package main import ( + "encoding/json" "errors" "fmt" "io" + "strconv" "strings" "github.com/golang-jwt/jwt/v5" @@ -45,24 +47,17 @@ func validateClaimsSet(claims jwt.MapClaims, c claimChecks) (bool, error) { if c.issuer != "" { opts = append(opts, jwt.WithIssuer(c.issuer)) } - if err := jwt.NewValidator(opts...).Validate(claims); err != nil { + // The validator has to be shielded from timestamps it cannot convert: see + // unrepresentableTimeClaim, which would otherwise silently invert the + // verdict. + if err := unrepresentableTimeClaim(claims); err != nil { return false, err } - return true, nil -} -// verifyClaims parses the token, runs the requested claim validations, and -// prints "Claims: VALID" or "Claims: INVALID" with the reason. It returns the -// errInvalidClaims sentinel (wrapping the reason) when a check fails so the CLI -// exits nonzero, mirroring signature verification; an unparseable token returns -// a hard error instead. Claim validation is independent of the signature: it -// runs with or without a key. -func verifyClaims(w io.Writer, tokenStr string, c claimChecks) error { - p, err := parseUnverifiedJWT(tokenStr) - if err != nil { - return err + if err := jwt.NewValidator(opts...).Validate(claims); err != nil { + return false, err } - return printClaimsVerdict(w, p.claims, c) + return true, nil } // printClaimsVerdict runs the requested checks against already-parsed claims @@ -87,3 +82,57 @@ func printClaimsVerdict(w io.Writer, claims jwt.MapClaims, c claimChecks) error func claimReason(err error) string { return strings.ReplaceAll(err.Error(), "\n", "; ") } + +// temporalClaimKeys are the numeric date claims the validator consults, in a +// fixed order so the reported reason does not depend on map iteration. +var temporalClaimKeys = [...]string{"exp", "nbf", "iat"} + +// unrepresentableTimeClaim reports the first temporal claim whose numeric value +// does not name a time jwtd can represent. +// +// It has to run before the validator. golang-jwt converts these claims with a +// Float64 whose error it discards and then casts to int64 (map_claims.go), so a +// value outside int64 range wraps - typically to math.MinInt64 - and the +// validator returns the opposite verdict: "exp":1e400 reads as long expired, +// "nbf":1e400 as long since valid. Neither is visible in the output, because +// claimTime declines to annotate a timestamp it cannot represent. Rejecting the +// value up front makes the verdict say what is actually wrong and keeps it +// agreeing with the displayed claims; the representability rule is claimTime's +// own, so the two cannot drift. +// +// Only numeric values are examined: a non-numeric temporal claim is not a +// timestamp at all, and the validator rejects it with its own message. +func unrepresentableTimeClaim(claims jwt.MapClaims) error { + for _, key := range temporalClaimKeys { + val, ok := claims[key] + if !ok { + continue + } + + var text string + switch num := val.(type) { + case json.Number: + text = num.String() + case float64: + text = strconv.FormatFloat(num, 'f', -1, 64) + default: + continue + } + + if _, ok := claimTime(text); !ok { + return fmt.Errorf("%s claim %s is not a representable timestamp", key, truncateClaimValue(text)) + } + } + return nil +} + +// truncateClaimValue shortens a claim literal for an error message: a numeric +// claim comes from the token and can be arbitrarily long, while the reason is +// rendered on one line. +func truncateClaimValue(text string) string { + const max = 32 + if len(text) <= max { + return text + } + return text[:max] + "..." +} diff --git a/claims_test.go b/claims_test.go index 204ef15..5104e4f 100644 --- a/claims_test.go +++ b/claims_test.go @@ -2,6 +2,7 @@ package main import ( "bytes" + "encoding/json" "errors" "strings" "testing" @@ -301,3 +302,164 @@ func TestDecodeJWTJSON_SignatureTakesPrecedenceOverClaims(t *testing.T) { t.Errorf("expected both verdicts false in JSON, got %q", out) } } + +// golang-jwt converts the temporal claims with a Float64 whose error it +// discards and then casts to int64, so a value outside int64 range wraps and +// the validator returns the opposite verdict: an absurd "exp" reads as expired +// and an absurd "nbf" as long since valid. Such a value must be reported as +// what it is instead, and the verdict must not depend on which way it wrapped. +func TestValidateClaimsSet_RejectsUnrepresentableTemporalClaims(t *testing.T) { + pinTime(t, 1000) + + tests := []struct { + name string + claims jwt.MapClaims + want string + }{ + { + name: "exp past int64 range", + claims: jwt.MapClaims{"exp": json.Number("10000000000000000000")}, + want: "exp claim 10000000000000000000 is not a representable timestamp", + }, + { + name: "exp with an absurd exponent", + claims: jwt.MapClaims{"exp": json.Number("1e400")}, + want: "exp claim 1e400 is not a representable timestamp", + }, + { + name: "nbf with an absurd exponent", + claims: jwt.MapClaims{"nbf": json.Number("1e400")}, + want: "nbf claim 1e400 is not a representable timestamp", + }, + { + name: "nbf below int64 range", + claims: jwt.MapClaims{"nbf": json.Number("-10000000000000000000")}, + want: "nbf claim -10000000000000000000 is not a representable timestamp", + }, + { + name: "iat unrepresentable alongside a live exp", + claims: jwt.MapClaims{"iat": json.Number("1e400"), "exp": json.Number("2000")}, + want: "iat claim 1e400 is not a representable timestamp", + }, + { + name: "float64 claim past the representable range", + claims: jwt.MapClaims{"exp": 1e300}, + want: "exp claim", + }, + { + name: "exp reported before nbf", + claims: jwt.MapClaims{ + "exp": json.Number("1e400"), + "nbf": json.Number("1e400"), + }, + want: "exp claim", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + valid, reason := validateClaimsSet(tt.claims, claimChecks{verify: true}) + if valid { + t.Fatalf("expected an invalid verdict for %v", tt.claims) + } + if !strings.Contains(reason.Error(), tt.want) { + t.Errorf("reason %q missing %q", reason.Error(), tt.want) + } + }) + } +} + +// The pre-check must not change the verdict for values it can represent, and +// must leave a non-numeric temporal claim to the validator's own message. +func TestValidateClaimsSet_RepresentableAndNonNumericClaimsUnaffected(t *testing.T) { + pinTime(t, 1000) + + tests := []struct { + name string + claims jwt.MapClaims + wantValid bool + wantAbout string + }{ + { + name: "ordinary integer seconds", + claims: jwt.MapClaims{"exp": json.Number("2000"), "iat": json.Number("900")}, + wantValid: true, + }, + { + name: "fractional seconds", + claims: jwt.MapClaims{"exp": json.Number("2000.5")}, + wantValid: true, + }, + { + name: "exponent form within range", + claims: jwt.MapClaims{"exp": json.Number("2e3")}, + wantValid: true, + }, + { + name: "string exp is the validator's to reject", + claims: jwt.MapClaims{"exp": "tomorrow"}, + wantValid: false, + wantAbout: "exp", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + valid, reason := validateClaimsSet(tt.claims, claimChecks{verify: true}) + if valid != tt.wantValid { + t.Fatalf("valid = %v, want %v (reason: %v)", valid, tt.wantValid, reason) + } + if tt.wantValid { + return + } + if strings.Contains(reason.Error(), "representable") { + t.Errorf("a non-numeric claim must not be reported as unrepresentable: %v", reason) + } + if !strings.Contains(reason.Error(), tt.wantAbout) { + t.Errorf("reason %q missing %q", reason.Error(), tt.wantAbout) + } + }) + } +} + +// An unrepresentable claim reaches both output paths through the shared core, +// so neither can report the wrapped verdict. +func TestUnrepresentableClaimFailsBothOutputPaths(t *testing.T) { + pinTime(t, 1000) + token := makeJWT(`{"alg":"HS256"}`, `{"nbf":1e400}`, "sig") + + t.Run("human", func(t *testing.T) { + var buf bytes.Buffer + err := decodeJWTHuman(&buf, token, "", claimChecks{verify: true}) + if !errors.Is(err, errInvalidClaims) { + t.Fatalf("expected errInvalidClaims, got %v", err) + } + out := stripANSI(buf.String()) + if !strings.Contains(out, "Claims: INVALID") || !strings.Contains(out, "nbf claim") { + t.Errorf("expected an INVALID verdict naming nbf, got %q", out) + } + }) + + t.Run("json", func(t *testing.T) { + var buf bytes.Buffer + err := decodeJWTJSON(&buf, token, "", claimChecks{verify: true}) + if !errors.Is(err, errInvalidClaims) { + t.Fatalf("expected errInvalidClaims, got %v", err) + } + if !strings.Contains(buf.String(), `"claimsValid": false`) { + t.Errorf("expected claimsValid false, got %q", buf.String()) + } + }) +} + +// The reason is rendered on one line, so a very long claim literal is truncated +// rather than pasted into it whole. +func TestTruncateClaimValue(t *testing.T) { + got := truncateClaimValue(strings.Repeat("9", 200)) + if len(got) != 35 || !strings.HasSuffix(got, "...") { + t.Errorf("expected a 32-character prefix plus an ellipsis, got %q", got) + } + if short := truncateClaimValue("1758000000"); short != "1758000000" { + t.Errorf("a short value must be kept verbatim, got %q", short) + } +} diff --git a/helpers_test.go b/helpers_test.go index feeb218..283f6d6 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -365,15 +365,31 @@ func randomSymmetricKey(t *testing.T, size int) []byte { return key } -// verifySignature parses a token and renders its signature verdict. Production -// code always has a parsedJWT in hand and calls printSignatureVerdict directly; -// this wrapper exists so the signature tests can work from a compact string. +// verifySignature parses a token, loads the key, and renders the signature +// verdict. Production code always has a parsedJWT and a loaded key in hand and +// calls printSignatureVerdict directly; this wrapper exists so the signature +// tests can work from a compact string and a key argument. func verifySignature(w io.Writer, tokenStr, keyStr string) error { p, err := parseUnverifiedJWT(tokenStr) if err != nil { return fmt.Errorf("signature verification: %w", err) } - return printSignatureVerdict(w, p, keyStr) + key, err := loadKeyForKID(keyStr, headerKID(p.header)) + if err != nil { + return fmt.Errorf("signature verification: error loading key: %w", err) + } + return printSignatureVerdict(w, p, key) +} + +// verifyClaims parses a token and renders its claim verdict. Production code +// holds a parsedJWT at this point and calls printClaimsVerdict directly; this +// wrapper exists so the claim tests can work from a compact string. +func verifyClaims(w io.Writer, tokenStr string, c claimChecks) error { + p, err := parseUnverifiedJWT(tokenStr) + if err != nil { + return err + } + return printClaimsVerdict(w, p.claims, c) } // --- JWS signature verification ----------------------------------------------- diff --git a/jwe.go b/jwe.go index 4a82de9..8fc3262 100644 --- a/jwe.go +++ b/jwe.go @@ -56,13 +56,6 @@ func isJWE(token string) bool { return strings.Count(token, ".") == jweDelimiters } -// isJWT returns true if the token string looks like a JWS/JWT compact -// serialization (3 dot-separated parts). It is the counterpart of isJWE, so -// token-shape dispatch has one definition per form. -func isJWT(token string) bool { - return strings.Count(token, ".") == jwtDelimiters -} - // isJWEBytes and isJWTBytes are the byte forms, used where the candidate is a // decrypted payload that may be large: testing its shape must not cost a full // copy of it just to reach the string predicates. diff --git a/main.go b/main.go index da880b3..bc6e05f 100644 --- a/main.go +++ b/main.go @@ -198,10 +198,22 @@ func printKeyInterpretation(w io.Writer, keyStr string, fromFlag bool) { _, _ = fmt.Fprintln(w, note) } +// maxStdinTokenBytes bounds how much of a piped stdin is read. A compact token +// is a few kilobytes at most, so the limit is generous; it exists so a stray +// pipe from an unbounded source (a log file, /dev/zero) fails with a clear +// message instead of being buffered whole into memory. +const maxStdinTokenBytes = 16 << 20 // 16 MiB + // readToken resolves the JWT string from arguments, stdin pipe, or interactive prompt. func readToken(args []string) (string, error) { if len(args) > 0 { - return sanitizeToken(args[0]), nil + // An empty or whitespace-only argument carries no token, so it is + // reported the same way an empty pipe is rather than as malformed. + token := sanitizeToken(args[0]) + if token == "" { + return "", errNoToken + } + return token, nil } // A nil FileInfo (Stat failed, e.g. a closed or detached stdin) is treated @@ -209,10 +221,15 @@ func readToken(args []string) (string, error) { // dereferencing nil. stat, err := os.Stdin.Stat() if err == nil && stat != nil && (stat.Mode()&os.ModeCharDevice) == 0 { - data, err := io.ReadAll(os.Stdin) + // One byte past the limit is read so exceeding it is detected rather + // than silently truncating the token. + data, err := io.ReadAll(io.LimitReader(os.Stdin, maxStdinTokenBytes+1)) if err != nil { return "", fmt.Errorf("reading stdin: %w", err) } + if len(data) > maxStdinTokenBytes { + return "", fmt.Errorf("reading stdin: input exceeds the %d byte limit", maxStdinTokenBytes) + } token := sanitizeToken(string(data)) if token == "" { return "", errNoToken @@ -268,6 +285,19 @@ func decodeAndPrint(w io.Writer, tokenStr, keyStr string) error { // claims (claim validation) or the header (signature verification) parse once // and share the result instead of decoding the same segments again. func printParsedJWT(w io.Writer, p *parsedJWT, keyStr string) error { + // The key is resolved before anything is written, so a bad key argument + // fails with an error alone instead of three decoded sections followed by + // one. The JWE and --json paths resolve their key up front for the same + // reason. + var key any + if keyStr != "" { + loaded, err := loadKeyForKID(keyStr, headerKID(p.header)) + if err != nil { + return fmt.Errorf("signature verification: error loading key: %w", err) + } + key = loaded + } + f := newFormatter() if err := printSection(w, f, "Header", p.header); err != nil { @@ -296,7 +326,7 @@ func printParsedJWT(w io.Writer, p *parsedJWT, keyStr string) error { if _, err := fmt.Fprintln(w); err != nil { return err } - if err := printSignatureVerdict(w, p, keyStr); err != nil { + if err := printSignatureVerdict(w, p, key); err != nil { return err } } @@ -342,6 +372,12 @@ func parseUnverifiedJWT(tokenStr string) (*parsedJWT, error) { if err := decodeJSON(headerData, &header); err != nil { return nil, fmt.Errorf("parsing JWT header: %w", err) } + // A literal "null" decodes without error and leaves the map nil, which + // would otherwise be rendered as an empty object. The JWE header decoder + // carries the same guard. + if header == nil { + return nil, fmt.Errorf("parsing JWT header: expected JSON object") + } payload, err := base64.RawURLEncoding.DecodeString(parts[1]) if err != nil { @@ -352,6 +388,9 @@ func parseUnverifiedJWT(tokenStr string) (*parsedJWT, error) { if err := decodeJSON(payload, &claims); err != nil { return nil, fmt.Errorf("parsing JWT claims: %w", err) } + if claims == nil { + return nil, fmt.Errorf("parsing JWT claims: expected JSON object") + } alg, ok := header["alg"].(string) if !ok { @@ -393,13 +432,12 @@ func splitCompactJWT(tokenStr string) ([]string, bool) { } // printSignatureVerdict renders the signature verdict for an already-parsed -// token and returns the errInvalidSignature sentinel on failure so the CLI -// exits nonzero. -func printSignatureVerdict(w io.Writer, p *parsedJWT, keyStr string) error { - valid, reason, err := verifyJWTSignature(p, keyStr) - if err != nil { - return fmt.Errorf("signature verification: %w", err) - } +// token and an already-loaded key, and returns the errInvalidSignature sentinel +// on failure so the CLI exits nonzero. It takes the loaded key rather than the +// key argument so its caller can fail on an unusable key before printing +// anything. +func printSignatureVerdict(w io.Writer, p *parsedJWT, key any) error { + valid, reason := verifyLoadedKeySignature(p, key) if !valid { if werr := printVerdict(w, "Signature", false, reason.Error()); werr != nil { @@ -421,7 +459,15 @@ func verifyJWTSignature(p *parsedJWT, keyStr string) (valid bool, reason error, if err != nil { return false, nil, fmt.Errorf("error loading key: %w", err) } + valid, reason = verifyLoadedKeySignature(p, key) + return valid, reason, nil +} +// verifyLoadedKeySignature is the verification core, taking a key that has +// already been loaded. Everything that can fail here is a verdict on the +// signature, so there is no separate hard error: the one hard failure, an +// unusable key argument, was decided by the caller that loaded it. +func verifyLoadedKeySignature(p *parsedJWT, key any) (valid bool, reason error) { // Extract the public key from private keys for verification. key = publicKeyForVerification(key) @@ -438,19 +484,19 @@ func verifyJWTSignature(p *parsedJWT, keyStr string) (valid bool, reason error, alg := p.method.Alg() methods := validMethodsForKey(key) if len(methods) == 0 { - return false, fmt.Errorf("%w: key type %T cannot verify a JWS", jwt.ErrTokenSignatureInvalid, key), nil + return false, fmt.Errorf("%w: key type %T cannot verify a JWS", jwt.ErrTokenSignatureInvalid, key) } if !slices.Contains(methods, alg) { - return false, fmt.Errorf("%w: signing method %v is invalid", jwt.ErrTokenSignatureInvalid, alg), nil + return false, fmt.Errorf("%w: signing method %v is invalid", jwt.ErrTokenSignatureInvalid, alg) } // Only the signature is checked here: the claims are never consulted, so // the verdict reflects the cryptography alone and not token expiry. signingInput := p.parts[0] + "." + p.parts[1] if verr := p.method.Verify(signingInput, p.signature, key); verr != nil { - return false, fmt.Errorf("%w: %w", jwt.ErrTokenSignatureInvalid, verr), nil + return false, fmt.Errorf("%w: %w", jwt.ErrTokenSignatureInvalid, verr) } - return true, nil, nil + return true, nil } // headerKID returns the token's "kid" header as a string, or "" when it is diff --git a/main_test.go b/main_test.go index 503a098..6efbea7 100644 --- a/main_test.go +++ b/main_test.go @@ -299,8 +299,120 @@ func TestDecodeAndPrint_TimestampsFormatted(t *testing.T) { } } +// A JSON "null" decodes without error and leaves the target map nil, which +// would otherwise be rendered as an empty object with a VALID claim verdict. +// Both segments must be rejected as not naming a JSON object, the way the JWE +// protected header already is. +func TestParseUnverifiedJWT_RejectsNullHeaderAndPayload(t *testing.T) { + tests := []struct { + name string + token string + want string + }{ + { + name: "null payload", + token: makeJWT(`{"alg":"none"}`, `null`, ""), + want: "parsing JWT claims: expected JSON object", + }, + { + name: "null header", + token: makeJWT(`null`, `{"sub":"x"}`, ""), + want: "parsing JWT header: expected JSON object", + }, + { + name: "non-object payload", + token: makeJWT(`{"alg":"none"}`, `[1,2]`, ""), + want: "parsing JWT claims", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if _, err := parseUnverifiedJWT(tt.token); err == nil { + t.Fatalf("expected an error for %q", tt.token) + } else if !strings.Contains(err.Error(), tt.want) { + t.Errorf("error %q missing %q", err, tt.want) + } + }) + } +} + +// A null header or payload must not reach the output at all: a nil map renders +// as "{}", which would silently misreport the token's contents. +func TestDecodeAndPrint_NullPayloadWritesNothing(t *testing.T) { + var buf bytes.Buffer + if err := decodeAndPrint(&buf, makeJWT(`{"alg":"none"}`, `null`, ""), ""); err == nil { + t.Fatal("expected an error for a null payload") + } + if buf.Len() != 0 { + t.Errorf("nothing should be written for a null payload, got %q", buf.String()) + } +} + +// The key is resolved before any section is written, so an unusable key +// argument produces an error alone instead of a decoded token followed by one. +// The JWE and --json paths behave the same way. +func TestPrintParsedJWT_KeyErrorWritesNothing(t *testing.T) { + token := makeJWT(`{"alg":"HS256"}`, `{"sub":"x"}`, "sig") + p, err := parseUnverifiedJWT(token) + if err != nil { + t.Fatalf("parsing token: %v", err) + } + + var buf bytes.Buffer + err = printParsedJWT(&buf, p, filepath.Join(t.TempDir(), "missing.pem")) + if err == nil { + t.Fatal("expected an error for an unusable key") + } + if !strings.Contains(err.Error(), "error loading key") { + t.Errorf("error should name the key load failure, got: %v", err) + } + if buf.Len() != 0 { + t.Errorf("no section may be written before the key is resolved, got %q", buf.String()) + } +} + // --- readToken --------------------------------------------------------------- +// An empty or whitespace-only token argument carries no token, so it is +// reported the same way an empty pipe is instead of as a malformed token. +func TestReadToken_EmptyArgIsNoToken(t *testing.T) { + for _, arg := range []string{"", " ", "\n\t "} { + if _, err := readToken([]string{arg}); !errors.Is(err, errNoToken) { + t.Errorf("readToken(%q): expected errNoToken, got %v", arg, err) + } + } +} + +// stdin is bounded, so an unbounded pipe fails with a clear message rather than +// being buffered whole or silently truncated into a wrong token. +func TestReadToken_StdinOverLimitErrors(t *testing.T) { + origStdin := os.Stdin + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("creating pipe: %v", err) + } + + go func() { + // Exactly one byte past the limit: enough to be detected, and fully + // consumed by the reader so this goroutine finishes. + _, _ = w.Write(bytes.Repeat([]byte("a"), maxStdinTokenBytes+1)) + w.Close() + }() + + os.Stdin = r + defer func() { os.Stdin = origStdin }() + + _, err = readToken([]string{}) + if err == nil { + t.Fatal("expected an error for stdin past the limit") + } + if !strings.Contains(err.Error(), "exceeds") { + t.Errorf("error should report the limit, got: %v", err) + } +} + func TestReadToken_FromArgs(t *testing.T) { token, err := readToken([]string{"my.jwt.token"}) if err != nil { From 04d810e662431752c098634bdfe8223d4cbfabef Mon Sep 17 00:00:00 2001 From: David Henning Date: Tue, 8 Sep 2026 22:46:45 +0200 Subject: [PATCH 2/4] fix: close key-loading review findings Four confirmed review findings around key resolution: - An "oct" JWK bypassed the empty-secret gate: parseJWK returned jwk.Key directly, so {"kty":"oct","k":""} yielded an empty HMAC key and a token forged with the empty secret verified. Every JWK path now unwraps through jwkKey, which routes []byte through symmetricKey. errEmptyKey joins errKIDNotFound as a final verdict (finalKeyError), so the rejection cannot degrade into a base64 retry or the misleading "pass it as hmac:" hint. - headerKID collapsed a non-string "kid" to "", which parseJWK read as "no kid named" and answered with the first JWK Set entry. RFC 7515 requires a string, so a present non-string kid is now errNonStringKID; the JWT, --json, and JWE call sites all propagate it. - A directory passed as --key was misreported as base64 key material. It gets its own keySource and a clear "is a directory, not a key file" error; printKeyInterpretation still narrates only the readings that actually apply. - Removed the production-dead loadKey wrapper (kept as a test helper in helpers_test.go) and loadInlineKey's unreachable decode-failure branch. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0125xMARDzdnbkbMTbNxSihH --- AGENTS.md | 8 ++-- helpers_test.go | 24 ++++++++++ jsonout.go | 6 ++- jwe.go | 6 ++- jwe_test.go | 37 +++++++++++++++ keys.go | 101 +++++++++++++++++++++++++++------------- keys_test.go | 99 +++++++++++++++++++++++++++++++++++++--- main.go | 37 +++++++++++---- main_test.go | 119 ++++++++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 384 insertions(+), 53 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ffa217b..74864a3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ All functionality lives in package `main`, split across six source files: - `main()` / `newRootCommand()` - Build and execute the Cobra root command with the `--key`/`-k`, `--json`, and `--color` flags; suppress Cobra's automatic usage/error output so runtime errors are rendered once, while invalid-signature details are not duplicated - `run()` / `readToken()` / `decodeJWTHuman()` - Resolves the token from arguments, stdin pipe, or interactive readline prompt; falls back to `JWTD_KEY` when `--key` is not set; applies the color mode, then dispatches to the JWT/JWE handler or, under `--json`, to the JSON handler. `decodeJWTHuman` wraps `decodeAndPrint` and, when claim validation was requested, prints a Claims section after it; both the signature and claim checks run so their sections show together, and the command exits nonzero if either fails (the signature verdict takes precedence for the returned sentinel). Claim flags on a JWE emit a stderr note and are otherwise skipped - `applyColorMode()` - Maps `--color` onto `fatih/color`'s global `NoColor`: `auto` leaves TTY/`NO_COLOR` detection untouched, `always` forces color, `never` disables it; `--json` always forces color off -- `headerKID()` - Extracts the token's `kid` header (or `""`) so JWK Set verification/decryption selects the key the token names +- `headerKID()` - Extracts the token's `kid` header (or `""` when absent) so JWK Set verification/decryption selects the key the token names. **A present but non-string `kid` is the `errNonStringKID` error, not an absent one.** RFC 7515 requires the value to be a string, and returning `""` for `{"kid":123}` made `parseJWK` read it as "no kid named" and select the first JWK Set entry — a signature made with a key the token never pointed at, reported as `VALID`. Every call site (`verifyJWTSignature`, `decodeAndPrintJWE`, `decodeJWEJSON`) propagates the error, so the human, `--json`, and JWE paths fail closed alike; `TestHeaderKID`, `TestVerifySignature_RejectsNonStringKID`, and `TestDecodeJWE_RejectsNonStringKID` pin that (go-jose rejects such a header while parsing a JWE, so the JWE refusal arrives even earlier) - `printKeyInterpretation()` - Notes on stderr how a key argument was read when it was not read as a file, so precedence-based detection cannot silently take a value the user meant one way and use it another; adds the process-list exposure warning for `--key` values, which `JWTD_KEY` does not carry (`/proc//cmdline` is world-readable, `/proc//environ` is owner-only). Diagnostics go to stderr so stdout stays parseable - `readInteractive()` - Prompts for a token interactively using `chzyer/readline` - `decodeAndPrint()` / `printParsedJWT()` - `decodeAndPrint` parses the JWT with `parseUnverifiedJWT` and hands the result to `printParsedJWT`, which orchestrates output and verifies the signature when a key is provided. `printParsedJWT` runs `formatTimestamps` on a `maps.Clone` of the claims, so the display rewrite never touches the authoritative parse and the same `parsedJWT` can still be validated @@ -48,13 +48,13 @@ All functionality lives in package `main`, split across six source files: ### `keys.go` - Key loading and format detection -- `loadKey()` / `loadKeyForKID()` / `parseKeyData()` / `parseAnyDER()` / `parseDERKey()` / `parseJWK()` - Resolve `raw:` and `hmac:`, then an existing file path, then base64/base64url; parse loaded data as JWK/JWK Set, PEM, or DER (PKCS#1/PKCS#8/SEC 1/PKIX) keys and X.509 certificates, and error on anything else. `parseAnyDER` holds the one list of understood DER encodings, used both for raw DER and as `parseDERKey`'s fallback for an unrecognized PEM block type, so a format cannot be added to one and missed by the other. `parseKeyData` only detects formats: it never phrases a rejection, because `unsupportedKeyError` is the single place that knows the user-facing subject and the symmetric-form hint. Trailing newlines are trimmed only for ASCII `hmac:` files limited to printable bytes plus tab/CR/LF, while UTF-8/non-ASCII and other binary files remain byte-exact. `loadKeyForKID` threads the token's `kid` so a JWK Set selects the matching entry (`loadKey` is the `kid=""` wrapper); a `kid` that matches nothing returns the `errKIDNotFound` sentinel, which short-circuits the base64/unsupported-format fallbacks so a JWK Set miss fails closed with a clear message instead of degrading into other key material +- `loadKeyForKID()` / `parseKeyData()` / `parseAnyDER()` / `parseDERKey()` / `parseJWK()` / `jwkKey()` - Resolve `raw:` and `hmac:`, then an existing file path, then base64/base64url; parse loaded data as JWK/JWK Set, PEM, or DER (PKCS#1/PKCS#8/SEC 1/PKIX) keys and X.509 certificates, and error on anything else. `parseAnyDER` holds the one list of understood DER encodings, used both for raw DER and as `parseDERKey`'s fallback for an unrecognized PEM block type, so a format cannot be added to one and missed by the other. `parseKeyData` only detects formats: it never phrases a rejection, because `unsupportedKeyError` is the single place that knows the user-facing subject and the symmetric-form hint. Trailing newlines are trimmed only for ASCII `hmac:` files limited to printable bytes plus tab/CR/LF, while UTF-8/non-ASCII and other binary files remain byte-exact. `loadKeyForKID` threads the token's `kid` so a JWK Set selects the matching entry (production code always holds a token header and so always knows the kid, possibly `""`; the tests keep a `loadKey` wrapper in `helpers_test.go` for the cases that do not care). A `kid` that matches nothing returns the `errKIDNotFound` sentinel and empty symmetric material the `errEmptyKey` sentinel; `finalKeyError` marks both as definitive, short-circuiting the base64 retry and the unsupported-format error so understood-but-unusable material fails closed with a clear message instead of degrading into another reading — in particular, an empty `oct` JWK must not be re-offered as "pass it as `hmac:`", which would turn the JWK's own JSON text into a secret. **`jwkKey` is where every JWK path (single key, first-of-set, kid-selected entry) unwraps the key, and it routes an `oct` JWK's `[]byte` through `symmetricKey`.** Without it `{"kty":"oct","k":""}` yielded an empty HMAC key — go-jose sets a typed-nil `[]byte`, so the `jwk.Key != nil` guard passes, `validMethodsForKey` admits `HS*`, and jwt v5's HMAC `Verify` accepts an empty key — so a token forged with the empty secret verified, while `--key raw:` correctly rejected the same material. `TestLoadKeyForKID_RejectsEmptyOctJWK` and `TestVerifySignature_RejectsForgedHMACFromEmptyOctJWK` hold that shut - `unsupportedKeyError()` - Explains a rejection and names the explicit symmetric form to use, substituting the user's own path so the fix is copy-pasteable; SSH keys get a conversion hint instead -- `classifyKeyArg()` / `loadSymmetricKeyFile()` / `loadKeyFile()` / `loadInlineKey()` - `classifyKeyArg` decides which reading applies (`raw:` literal, `hmac:` secret file, existing file, base64, or unusable) and `loadKeyForKID` switches on it to call the matching loader, so the precedence exists once: the reading the CLI hint reports is by construction the reading that happens, not a mirror of it. It uses `Stat` rather than a read, so classifying never consumes the key source; a consequence is that an existing but unreadable file now reports the read error instead of falling through to a base64 attempt on its path +- `classifyKeyArg()` / `loadSymmetricKeyFile()` / `loadKeyFile()` / `loadInlineKey()` - `classifyKeyArg` decides which reading applies (`raw:` literal, `hmac:` secret file, existing file, base64, directory, or unusable) and `loadKeyForKID` switches on it to call the matching loader, so the precedence exists once: the reading the CLI hint reports is by construction the reading that happens, not a mirror of it. It uses `Stat` rather than a read, so classifying never consumes the key source; a consequence is that an existing but unreadable file now reports the read error instead of falling through to a base64 attempt on its path. A **directory** is its own `keySource` and is rejected by name ("is a directory, not a key file") rather than falling through to a base64 reading of its own path text, which reported a nonsense interpretation of a path the user clearly meant as a key file. `printKeyInterpretation` narrates only the literal and base64 readings; a directory, like any other unusable value, is left to `loadKeyForKID`'s error, so the hint and the outcome cannot disagree. Because `classifyKeyArg` has already decoded the argument before returning `keySourceBase64`, `loadInlineKey`'s decode cannot fail and carries no unreachable rejection branch - `decodeBase64Key()` / `symmetricKey()` - Decode whitespace-tolerant base64/base64url key material (applied to text key files as well as inline arguments, so the same bytes mean the same key either way) and gate symmetric secrets, rejecting empty key material - `isTextKey()` / `isSSHPublicKey()` / `isSSHKeyType()` / `sshBlobHasType()` - Distinguish ASCII text from binary for newline trimming, and detect SSH public keys for the targeted error -**Symmetric secrets are explicit, and that is the security boundary.** Before 5.0.0, key material jwtd could not parse became an HMAC secret. Any *public* key reaching that path was forgeable: a public key is a published value, so an attacker who knew its bytes could sign an HS256 token that verified. Three formats hit it in practice (OpenSSH keys, RFC 4716 armor, base64 key material in a file), but the class was open to any format nobody had thought of. Unparseable material is now an error, so no new format can reopen it — verified against PKCS#12, which was never enumerated. `isSSHPublicKey()` exists only to give a better message; it covers OpenSSH one-line keys (verified through the SSH wire-format type prefix, so a secret merely starting with `ssh-rsa` is not misread), `authorized_keys` option prefixes, and RFC 4716 armor, whose four-dash BEGIN marker is not a PEM marker. Empty key material is rejected because the empty secret is known to everyone. `keys_test.go` and `TestVerifySignature_RejectsForgedHMACFromPublishedKeyFile` in `main_test.go` hold these properties down. +**Symmetric secrets are explicit, and that is the security boundary.** Before 5.0.0, key material jwtd could not parse became an HMAC secret. Any *public* key reaching that path was forgeable: a public key is a published value, so an attacker who knew its bytes could sign an HS256 token that verified. Three formats hit it in practice (OpenSSH keys, RFC 4716 armor, base64 key material in a file), but the class was open to any format nobody had thought of. Unparseable material is now an error, so no new format can reopen it — verified against PKCS#12, which was never enumerated. `isSSHPublicKey()` exists only to give a better message; it covers OpenSSH one-line keys (verified through the SSH wire-format type prefix, so a secret merely starting with `ssh-rsa` is not misread), `authorized_keys` option prefixes, and RFC 4716 armor, whose four-dash BEGIN marker is not a PEM marker. Empty key material is rejected because the empty secret is known to everyone — that gate covers **every** symmetric form, `raw:`, `hmac:`, and an `oct` JWK alike, which is why `jwkKey` exists. `keys_test.go` and `TestVerifySignature_RejectsForgedHMACFromPublishedKeyFile` in `main_test.go` hold these properties down. Removing the fallback deleted the heuristics that existed only to decide it (`isStructuredKeyData`, `hasPEMMarker`, `hasJWKMember`, `jsonStringEnd`, `isCompleteDER`) — about 100 lines of the most error-prone code in the package. Do not reintroduce a "looks like a secret" inference to make an unsupported format work; add a parser, or let the user say `hmac:`. diff --git a/helpers_test.go b/helpers_test.go index feeb218..c745765 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -376,6 +376,14 @@ func verifySignature(w io.Writer, tokenStr, keyStr string) error { return printSignatureVerdict(w, p, keyStr) } +// loadKey resolves a key argument without a kid, the shape most key tests +// exercise. Production code always has a token header at this point and so +// always knows the kid (possibly ""), which is why this wrapper lives with the +// test helpers rather than in keys.go. +func loadKey(keyStr string) (any, error) { + return loadKeyForKID(keyStr, "") +} + // --- JWS signature verification ----------------------------------------------- // signJWT creates a signed JWT with the given claims and RSA private key. @@ -403,6 +411,22 @@ func signJWTWithHMAC(t *testing.T, key []byte, claims jwt.MapClaims) string { return signed } +// signJWTWithHMACHeader signs with HMAC-SHA256 like signJWTWithHMAC, but lets +// the caller set extra header members — including ones RFC 7515 does not +// allow, such as a non-string "kid". +func signJWTWithHMACHeader(t *testing.T, key []byte, header map[string]any, claims jwt.MapClaims) string { + t.Helper() + token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) + for k, v := range header { + token.Header[k] = v + } + signed, err := token.SignedString(key) + if err != nil { + t.Fatalf("signing JWT: %v", err) + } + return signed +} + type failOnWriteWriter struct { failedWrite int writes int diff --git a/jsonout.go b/jsonout.go index 3acf8c2..d1a0a42 100644 --- a/jsonout.go +++ b/jsonout.go @@ -107,7 +107,11 @@ func decodeJWEJSON(w io.Writer, tokenStr, keyStr string) error { return writeJSON(w, out) } - key, err := loadKeyForKID(keyStr, headerKID(header)) + kid, err := headerKID(header) + if err != nil { + return err + } + key, err := loadKeyForKID(keyStr, kid) if err != nil { return fmt.Errorf("loading decryption key: %w", err) } diff --git a/jwe.go b/jwe.go index 4a82de9..6a4a24d 100644 --- a/jwe.go +++ b/jwe.go @@ -92,7 +92,11 @@ func decodeAndPrintJWE(w io.Writer, tokenStr, keyStr string) error { // fails with the error alone instead of a partial section ahead of it. var key any if keyStr != "" { - key, err = loadKeyForKID(keyStr, headerKID(header)) + kid, kerr := headerKID(header) + if kerr != nil { + return kerr + } + key, err = loadKeyForKID(keyStr, kid) if err != nil { return fmt.Errorf("loading decryption key: %w", err) } diff --git a/jwe_test.go b/jwe_test.go index ca0071c..d5db672 100644 --- a/jwe_test.go +++ b/jwe_test.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "encoding/base64" "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -143,6 +144,42 @@ func TestDecodeAndPrintJWE_WithDecryption(t *testing.T) { } } +// The JWE path resolves its decryption key through the same headerKID, so a +// non-string "kid" must fail closed here too instead of silently taking the +// first entry of a JWK Set. go-jose refuses such a header while parsing, so +// the refusal arrives before headerKID is even reached; this pins the outcome +// so a future parser change cannot let it through unnoticed. +func TestDecodeJWE_RejectsNonStringKID(t *testing.T) { + key := generateRSAKey(t) + keyPath := writeKeyFile(t, key) + token := encryptJWE(t, key, []byte(`{"sub":"user1"}`)) + + // Rewrite only the protected header segment; the key is resolved from it + // before any decryption is attempted. + _, rest, _ := strings.Cut(token, ".") + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"RSA-OAEP","enc":"A256GCM","kid":123}`)) + tampered := header + "." + rest + + for _, tc := range []struct { + name string + fn func(w *bytes.Buffer) error + }{ + {name: "human", fn: func(w *bytes.Buffer) error { return decodeAndPrintJWE(w, tampered, keyPath) }}, + {name: "json", fn: func(w *bytes.Buffer) error { return decodeJWEJSON(w, tampered, keyPath) }}, + } { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + err := tc.fn(&buf) + if err == nil { + t.Fatalf("JWE with a non-string kid accepted:\n%s", buf.String()) + } + if !errors.Is(err, errNonStringKID) && !strings.Contains(err.Error(), "key ID") { + t.Errorf("error should reject the kid header, got %v", err) + } + }) + } +} + func TestDecodeAndPrintJWE_WithTimestampFormatting(t *testing.T) { key := generateRSAKey(t) token := encryptJWE(t, key, []byte(`{"sub":"user1","iat":1516239022}`)) diff --git a/keys.go b/keys.go index c9a9350..d342644 100644 --- a/keys.go +++ b/keys.go @@ -21,25 +21,35 @@ import ( // surfaces it as-is. var errKIDNotFound = errors.New("kid not found in JWK Set") -// loadKey resolves a key argument. Symmetric secrets must be requested -// explicitly, with "raw:" for a literal or "hmac:" for a file of -// secret bytes. Everything else must parse as structured key material: JWK/JWK -// Set, PEM, DER, or an X.509 certificate, read from a file path or from inline -// base64. -// -// The explicit prefixes are the security boundary. Inferring "symmetric -// secret" from "did not parse" made every unsupported key format forgeable: a -// public key is a published value, so anyone who knew its bytes could sign an -// HS256 token that verified against it. Unparseable material is now an error, -// so the failure direction is always closed no matter what format shows up. -func loadKey(keyStr string) (any, error) { - return loadKeyForKID(keyStr, "") +// errEmptyKey marks empty symmetric key material. Like errKIDNotFound it is a +// definitive answer: the material parsed, it is simply unusable, so it must not +// degrade into a base64 retry or an "unsupported format" message that would +// point the user at a workaround for a key that is deliberately refused. +var errEmptyKey = errors.New("key is empty") + +// finalKeyError reports whether a parse failure is a definitive verdict on the +// key material rather than a reason to try the next reading. Both sentinels +// mean the bytes were understood, so reinterpreting them can only pick a key +// the user did not mean. +func finalKeyError(err error) bool { + return errors.Is(err, errKIDNotFound) || errors.Is(err, errEmptyKey) } // loadKeyForKID resolves a key argument, selecting the entry that matches kid // when the material is a JWK Set. kid is the token's "kid" header ("" when the // token carries none); it only affects JWK Set selection and is ignored for // every other key form. +// +// Symmetric secrets must be requested explicitly, with "raw:" for a +// literal or "hmac:" for a file of secret bytes. Everything else must +// parse as structured key material: JWK/JWK Set, PEM, DER, or an X.509 +// certificate, read from a file path or from inline base64. +// +// The explicit prefixes are the security boundary. Inferring "symmetric +// secret" from "did not parse" made every unsupported key format forgeable: a +// public key is a published value, so anyone who knew its bytes could sign an +// HS256 token that verified against it. Unparseable material is now an error, +// so the failure direction is always closed no matter what format shows up. func loadKeyForKID(keyStr, kid string) (any, error) { // The precedence lives in classifyKeyArg and is applied here, so the // reading the CLI reports is by construction the reading that happens. @@ -52,6 +62,8 @@ func loadKeyForKID(keyStr, kid string) (any, error) { return loadKeyFile(keyStr, kid) case keySourceBase64: return loadInlineKey(keyStr, kid) + case keySourceDirectory: + return nil, fmt.Errorf("key path %q is a directory, not a key file", keyStr) default: return nil, fmt.Errorf("key is neither a valid file path nor base64-encoded data") } @@ -89,9 +101,10 @@ func loadKeyFile(path, kid string) (any, error) { if err == nil { return key, nil } - // The file is a valid JWK Set; a kid miss is final, not a reason to try - // base64 or fall through to the unsupported-format error. - if errors.Is(err, errKIDNotFound) { + // The file parsed as JWK material; a kid miss or an empty secret is final, + // not a reason to try base64 or fall through to the unsupported-format + // error. + if finalKeyError(err) { return nil, err } @@ -101,7 +114,7 @@ func loadKeyFile(path, kid string) (any, error) { if err == nil { return key, nil } - if errors.Is(err, errKIDNotFound) { + if finalKeyError(err) { return nil, err } } @@ -112,13 +125,12 @@ func loadKeyFile(path, kid string) (any, error) { // loadInlineKey parses base64/base64url key material given directly on the // command line or in JWTD_KEY. func loadInlineKey(keyStr, kid string) (any, error) { - decoded, ok := decodeBase64Key([]byte(keyStr)) - if !ok { - return nil, fmt.Errorf("key is neither a valid file path nor base64-encoded data") - } + // classifyKeyArg only returns keySourceBase64 after decoding this same + // argument, so the decode cannot fail here. + decoded, _ := decodeBase64Key([]byte(keyStr)) key, err := parseKeyData(decoded, kid) if err != nil { - if errors.Is(err, errKIDNotFound) { + if finalKeyError(err) { return nil, err } return nil, unsupportedKeyError(decoded, "inline key", "raw: or hmac:") @@ -143,7 +155,7 @@ func unsupportedKeyError(data []byte, subject, symmetricForm string) error { // empty secret, so accepting it would report forged HMAC tokens as valid. func symmetricKey(data []byte) (any, error) { if len(data) == 0 { - return nil, fmt.Errorf("key is empty") + return nil, errEmptyKey } return data, nil } @@ -163,7 +175,12 @@ const ( keySourceSecretFile // keySourceBase64 is inline base64/base64url key material. keySourceBase64 - // keySourceUnusable is neither, and loadKey will reject it. + // keySourceDirectory is an existing path that is a directory. It can never + // hold key material, and must not fall through to a base64 reading of its + // own path text, which would report a nonsense interpretation of a path + // the user clearly meant as a key file. + keySourceDirectory + // keySourceUnusable is none of these, and loadKeyForKID will reject it. keySourceUnusable ) @@ -180,7 +197,10 @@ func classifyKeyArg(keyStr string) keySource { if strings.HasPrefix(keyStr, "hmac:") { return keySourceSecretFile } - if info, err := os.Stat(keyStr); err == nil && !info.IsDir() { + if info, err := os.Stat(keyStr); err == nil { + if info.IsDir() { + return keySourceDirectory + } return keySourceFile } if _, ok := decodeBase64Key([]byte(keyStr)); ok { @@ -277,9 +297,10 @@ func parseKeyData(data []byte, kid string) (any, error) { // Try JWK / JWK Set (JSON-based formats). if key, err := parseJWK(data, kid); err == nil { return key, nil - } else if errors.Is(err, errKIDNotFound) { - // The data is a valid JWK Set; the kid simply did not match. That is - // a final answer, not a reason to reinterpret the bytes as PEM/DER. + } else if finalKeyError(err) { + // The data is valid JWK material: the kid simply did not match, or the + // selected entry holds an empty secret. Either is a final answer, not + // a reason to reinterpret the bytes as PEM/DER. return nil, err } @@ -363,7 +384,7 @@ func parseJWK(data []byte, kid string) (any, error) { // Try single JWK. var jwk jose.JSONWebKey if err := json.Unmarshal(data, &jwk); err == nil && jwk.Key != nil { - return jwk.Key, nil + return jwkKey(jwk) } // Try JWK Set ({"keys": [...]}). @@ -376,10 +397,28 @@ func parseJWK(data []byte, kid string) (any, error) { } // Multiple entries can share a kid (e.g. one per use/alg). The // first match is deterministic and matches go-jose's own order. - return matches[0].Key, nil + return jwkKey(matches[0]) } - return jwks.Keys[0].Key, nil + return jwkKey(jwks.Keys[0]) } return nil, fmt.Errorf("not a valid JWK or JWK Set") } + +// jwkKey unwraps a parsed JWK into the key it holds. An "oct" JWK carries a +// symmetric secret, which go-jose hands back as a []byte, so it goes through +// symmetricKey like every other symmetric form: {"kty":"oct","k":""} otherwise +// yields an empty HMAC key, and the empty secret is known to everyone, so an +// HS256 token forged with it would verify. Every JWK path — single key, +// first-of-set, and the kid-selected entry — routes through here, so none of +// them can skip that gate. +func jwkKey(jwk jose.JSONWebKey) (any, error) { + if secret, ok := jwk.Key.([]byte); ok { + key, err := symmetricKey(secret) + if err != nil { + return nil, fmt.Errorf("oct JWK (kid %q): %w", jwk.KeyID, err) + } + return key, nil + } + return jwk.Key, nil +} diff --git a/keys_test.go b/keys_test.go index 669158d..65708b0 100644 --- a/keys_test.go +++ b/keys_test.go @@ -11,6 +11,7 @@ import ( "encoding/base64" "encoding/json" "encoding/pem" + "errors" "path/filepath" "strings" "testing" @@ -589,17 +590,101 @@ func TestClassifyKeyArg(t *testing.T) { }) } - // loadKey cannot read a directory, so it must not be reported as a key - // file. Which of the remaining readings applies depends on whether the - // path itself happens to be valid base64, so only the file reading is - // ruled out here. - t.Run("directory is not a key file", func(t *testing.T) { - if got := classifyKeyArg(filepath.Dir(keyPath)); got == keySourceFile { - t.Error("a directory must not classify as a key file") + // A directory can hold no key material. It gets its own classification so + // it neither reports as a key file nor degrades into a base64 reading of + // its own path text, which would be a nonsense interpretation of a path + // the user clearly meant as a key file. + t.Run("directory is classified as a directory", func(t *testing.T) { + if got := classifyKeyArg(filepath.Dir(keyPath)); got != keySourceDirectory { + t.Errorf("a directory must classify as keySourceDirectory, got %d", got) } }) } +// A directory passed as --key must say so, not be reinterpreted as base64 of +// its own path or reported as unsupported key material. +func TestLoadKey_RejectsDirectory(t *testing.T) { + dir := t.TempDir() + + loaded, err := loadKey(dir) + if err == nil { + t.Fatalf("directory accepted as a %T key", loaded) + } + for _, want := range []string{dir, "directory"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q should mention %q", err, want) + } + } + if strings.Contains(err.Error(), "base64") { + t.Errorf("a directory must not be reported as a base64 reading, got %v", err) + } +} + +// An "oct" JWK carries a symmetric secret, so it must pass the same empty-key +// gate as raw: and hmac:. {"kty":"oct","k":""} otherwise yields an empty HMAC +// key, and every attacker knows the empty secret. +func TestLoadKeyForKID_RejectsEmptyOctJWK(t *testing.T) { + tests := []struct { + name string + data string + kid string + }{ + { + name: "single JWK", + data: `{"kty":"oct","k":""}`, + }, + { + name: "first entry of a JWK Set", + data: `{"keys":[{"kty":"oct","kid":"a","k":""}]}`, + }, + { + name: "kid-selected entry of a JWK Set", + data: `{"keys":[{"kty":"oct","kid":"a","k":"c2VjcmV0LXNlY3JldC1zZWNyZXQtMzJieXRlcyE"},{"kty":"oct","kid":"b","k":""}]}`, + kid: "b", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := writeTextKeyFile(t, "oct.jwk", tt.data) + + loaded, err := loadKeyForKID(path, tt.kid) + if err == nil { + t.Fatalf("empty oct JWK accepted as a %T key", loaded) + } + if !errors.Is(err, errEmptyKey) { + t.Errorf("expected errEmptyKey, got %v", err) + } + // The material parsed as a JWK, so the rejection is final: it + // must not degrade into the "pass it as hmac:" hint, which + // would turn the JWK's own JSON text into a secret. + if strings.Contains(err.Error(), "hmac:") { + t.Errorf("empty oct JWK must not suggest the hmac: fallback, got %v", err) + } + }) + } +} + +// The gate must not break ordinary oct JWKs: a non-empty secret still loads as +// the symmetric key bytes it encodes. +func TestLoadKeyForKID_AcceptsOctJWK(t *testing.T) { + secret := []byte("a-32-byte-symmetric-test-secret!") + path := writeTextKeyFile(t, "oct.jwk", `{"keys":[{"kty":"oct","kid":"a","k":"`+ + base64.RawURLEncoding.EncodeToString(secret)+`"}]}`) + + loaded, err := loadKeyForKID(path, "a") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + symKey, ok := loaded.([]byte) + if !ok { + t.Fatalf("expected []byte, got %T", loaded) + } + if !bytes.Equal(symKey, secret) { + t.Errorf("expected the encoded secret, got %q", symKey) + } +} + // --- public key material must never degrade into a symmetric secret -------- // // Public keys are published values. If key material jwtd cannot parse fell diff --git a/main.go b/main.go index da880b3..3129471 100644 --- a/main.go +++ b/main.go @@ -187,8 +187,10 @@ func printKeyInterpretation(w io.Writer, keyStr string, fromFlag bool) { case keySourceBase64: note = fmt.Sprintf("Note: %s is not an existing file; decoded as base64 key material.", origin) default: - // A file, including an hmac: secret file, is the expected reading, - // and unusable values produce an error that speaks for itself. + // A file, including an hmac: secret file, is the expected reading. + // Directories and otherwise unusable values are rejected by + // loadKeyForKID with an error that speaks for itself, so there is no + // reading to narrate. return } @@ -417,7 +419,11 @@ func printSignatureVerdict(w io.Writer, p *parsedJWT, keyStr string) error { // hard failures (unparseable token, unusable key) that are not a verdict on the // signature itself. func verifyJWTSignature(p *parsedJWT, keyStr string) (valid bool, reason error, err error) { - key, err := loadKeyForKID(keyStr, headerKID(p.header)) + kid, err := headerKID(p.header) + if err != nil { + return false, nil, err + } + key, err := loadKeyForKID(keyStr, kid) if err != nil { return false, nil, fmt.Errorf("error loading key: %w", err) } @@ -453,13 +459,26 @@ func verifyJWTSignature(p *parsedJWT, keyStr string) (valid bool, reason error, return true, nil, nil } -// headerKID returns the token's "kid" header as a string, or "" when it is -// absent or not a string. It selects the matching key from a JWK Set. -func headerKID(header map[string]any) string { - if kid, ok := header["kid"].(string); ok { - return kid +// errNonStringKID rejects a "kid" header that is present but not a string. +var errNonStringKID = errors.New(`token header "kid" must be a string (RFC 7515)`) + +// headerKID returns the token's "kid" header, which selects the matching entry +// from a JWK Set, or "" when the token carries none. +// +// A present but non-string "kid" is an error, not an absent one. RFC 7515 +// requires the value to be a string, and treating {"kid":123} as "no kid +// named" would silently select the first JWK Set entry instead of the one the +// token points at — a key mismatch reported as a valid signature. +func headerKID(header map[string]any) (string, error) { + raw, ok := header["kid"] + if !ok { + return "", nil + } + kid, ok := raw.(string) + if !ok { + return "", errNonStringKID } - return "" + return kid, nil } // validMethodsForKey returns the JWS algorithm names compatible with the diff --git a/main_test.go b/main_test.go index 503a098..fb343dc 100644 --- a/main_test.go +++ b/main_test.go @@ -614,6 +614,125 @@ func TestVerifySignature_RejectsForgedHMACFromPublishedKeyFile(t *testing.T) { } } +// An "oct" JWK with empty key material is the same forgery as `raw:`: the +// empty secret is a published value, so a token HMAC'd with it must never +// verify, in the human path or under --json. +func TestVerifySignature_RejectsForgedHMACFromEmptyOctJWK(t *testing.T) { + keyPath := writeTextKeyFile(t, "empty.jwk", `{"kty":"oct","k":""}`) + forged := signJWTWithHMAC(t, []byte{}, jwt.MapClaims{"sub": "attacker", "role": "admin"}) + + var buf bytes.Buffer + if err := verifySignature(&buf, forged, keyPath); err == nil { + t.Fatal("token forged with the empty JWK secret accepted") + } + if output := stripANSI(buf.String()); strings.Contains(output, "Signature: VALID") { + t.Errorf("forged HMAC token reported as valid:\n%s", output) + } + + var jsonBuf bytes.Buffer + if err := decodeJWTJSON(&jsonBuf, forged, keyPath, claimChecks{}); err == nil { + t.Errorf("--json accepted the token forged with the empty JWK secret:\n%s", jsonBuf.String()) + } +} + +// RFC 7515 requires "kid" to be a string. A present non-string kid must be an +// error: treating it as "no kid named" would silently select the first JWK Set +// entry and report a signature made with a key the token never pointed at. +func TestVerifySignature_RejectsNonStringKID(t *testing.T) { + first := []byte("first-key-32-bytes-of-secret-abc") + second := []byte("second-key-32-bytes-of-secret-xy") + setPath := writeTextKeyFile(t, "jwks.json", `{"keys":[`+ + `{"kty":"oct","kid":"a","k":"`+base64.RawURLEncoding.EncodeToString(first)+`"},`+ + `{"kty":"oct","kid":"b","k":"`+base64.RawURLEncoding.EncodeToString(second)+`"}]}`) + + tests := []struct { + name string + kid any + }{ + {name: "number", kid: 123}, + {name: "null", kid: nil}, + {name: "bool", kid: true}, + {name: "array", kid: []any{"a"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + token := signJWTWithHMACHeader(t, first, map[string]any{"kid": tt.kid}, + jwt.MapClaims{"sub": "attacker"}) + + var buf bytes.Buffer + err := verifySignature(&buf, token, setPath) + if err == nil { + t.Fatal("token with a non-string kid accepted") + } + if !strings.Contains(err.Error(), "kid") { + t.Errorf("error should name the kid header, got %v", err) + } + if output := stripANSI(buf.String()); strings.Contains(output, "Signature: VALID") { + t.Errorf("token with a non-string kid reported as valid:\n%s", output) + } + + var jsonBuf bytes.Buffer + if err := decodeJWTJSON(&jsonBuf, token, setPath, claimChecks{}); err == nil { + t.Errorf("--json accepted a token with a non-string kid:\n%s", jsonBuf.String()) + } + }) + } + + // A well-formed kid still selects its entry, so the check does not break + // ordinary JWK Set verification. + t.Run("string kid still verifies", func(t *testing.T) { + token := signJWTWithHMACHeader(t, second, map[string]any{"kid": "b"}, + jwt.MapClaims{"sub": "test"}) + + var buf bytes.Buffer + if err := verifySignature(&buf, token, setPath); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if output := stripANSI(buf.String()); !strings.Contains(output, "Signature: VALID") { + t.Errorf("expected a valid signature, got:\n%s", output) + } + }) +} + +func TestHeaderKID(t *testing.T) { + tests := []struct { + name string + header map[string]any + want string + wantErr bool + }{ + {name: "absent", header: map[string]any{"alg": "HS256"}}, + {name: "string", header: map[string]any{"kid": "key-1"}, want: "key-1"}, + {name: "empty string", header: map[string]any{"kid": ""}}, + {name: "number", header: map[string]any{"kid": json.Number("123")}, wantErr: true}, + {name: "null", header: map[string]any{"kid": nil}, wantErr: true}, + {name: "bool", header: map[string]any{"kid": true}, wantErr: true}, + {name: "object", header: map[string]any{"kid": map[string]any{}}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := headerKID(tt.header) + if tt.wantErr { + if err == nil { + t.Fatalf("expected an error, got %q", got) + } + if !errors.Is(err, errNonStringKID) { + t.Errorf("expected errNonStringKID, got %v", err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("headerKID = %q, want %q", got, tt.want) + } + }) + } +} + func TestDecodeAndPrint_SignatureValid_RSA(t *testing.T) { key := generateRSAKey(t) keyPath := writeKeyFile(t, key) From 6ae9e31aaea2027ac9a18f863cdfa8b0efdb6ea6 Mon Sep 17 00:00:00 2001 From: David Henning Date: Tue, 8 Sep 2026 22:47:04 +0200 Subject: [PATCH 3/4] fix: apply the key to nested tokens and fix output edge cases - thread the key through printDecryptedPayload into the nested JWE/JWT decode, so --key keeps applying one level down: a JWE wrapping another token to the same key now decrypts all the way, and a nested JWS gets a real Signature verdict whose errInvalidSignature drives the exit code. A nested token the key does not fit is retried keyless, keeping its previous output. - emit "decryptedPayload": null in --json for a JWE whose plaintext is the JSON literal null; it was a nil interface that omitempty dropped, leaving neither encrypted nor decryptedPayload in the object. - compute exp/nbf annotations from Unix seconds instead of time.Time.Sub, whose Duration saturates at ~292 years and rendered every distant claim as the same bogus 106751d. - measure a base64url part arithmetically instead of decoding a possibly huge ciphertext just to report its length. - AGENTS.md: describe the changed behaviour, correct the golang-jwt row (the segment work is parseUnverifiedJWT's, not jwt.ParseUnverified's), and add the test-only gopkg.in/yaml.v3 dependency. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0125xMARDzdnbkbMTbNxSihH --- AGENTS.md | 13 +++--- jsonout.go | 37 +++++++++++++-- jsonout_test.go | 77 ++++++++++++++++++++++++++++++ jwe.go | 2 +- jwe_test.go | 91 ++++++++++++++++++++++++++++++++++-- output.go | 121 ++++++++++++++++++++++++++++++++++++++---------- output_test.go | 111 ++++++++++++++++++++++++++++++++++++++------ 7 files changed, 397 insertions(+), 55 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ffa217b..28d993f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,8 +29,8 @@ All functionality lives in package `main`, split across six source files: ### `jsonout.go` - Machine-readable `--json` output -- `decodeJWTJSON()` / `decodeJWEJSON()` - Emit one JSON object per token. A JWT carries `header`, `payload`, `signature`, and (with a key) `signatureValid`; an invalid signature still writes the JSON and then returns `errInvalidSignature` for the exit code. A JWE carries `protectedHeader` plus either encrypted part sizes (no key) or `decryptedPayload` (with a key) -- `jsonPayloadValue()` / `base64URLLen()` / `writeJSON()` - Decode a decrypted payload as structured JSON when possible (else a string); report part sizes; and encode with `encoding/json`, which preserves `json.Number` exactly and escapes control characters including ESC. Timestamps are left as raw numeric claim values here — `formatTimestamps` is intentionally not applied — so consumers do their own date math +- `decodeJWTJSON()` / `decodeJWEJSON()` - Emit one JSON object per token. A JWT carries `header`, `payload`, `signature`, and (with a key) `signatureValid`; an invalid signature still writes the JSON and then returns `errInvalidSignature` for the exit code. A JWE carries `protectedHeader` plus either encrypted part sizes (no key) or `decryptedPayload` (with a key). `decryptedPayload` is a **pointer** field: a plaintext that is the JSON literal `null` decodes to a nil interface, which `omitempty` on a plain `any` would drop, leaving an object with neither `encrypted` nor `decryptedPayload` — the pointer keeps presence (a key was given) separate from content (`null`). The `--json` JWE path deliberately does not recurse into a nested token the way the human path does: `jsonPayloadValue` reports it as its compact string, so consumers decode it themselves +- `jsonPayloadValue()` / `base64URLLen()` / `writeJSON()` - Decode a decrypted payload as structured JSON when possible (else a string); report part sizes; and encode with `encoding/json`, which preserves `json.Number` exactly and escapes control characters including ESC. Timestamps are left as raw numeric claim values here — `formatTimestamps` is intentionally not applied — so consumers do their own date math. `base64URLLen` derives the decoded size from the encoded text (alphabet check, CR/LF skipped as `encoding/base64` skips them, a remainder of one character rejected, then `DecodedLen`) instead of decoding the part: a JWE ciphertext is unbounded and this only ever reports its length. `TestBase64URLLenMatchesDecode` holds it to exactly what `base64.RawURLEncoding.DecodeString` answers, invalid input included ### `claims.go` - Opt-in claim validation @@ -43,7 +43,7 @@ All functionality lives in package `main`, split across six source files: ### `jwe.go` - JWE parsing and decryption - `isJWE()` / `isJWT()` / `isJWEBytes()` / `isJWTBytes()` - Detect JWE (5 dot-separated parts) and JWS/JWT (3 parts) compact serialization. Every token-shape dispatch goes through one of these, so the shape rules are not restated inline; all four read their delimiter counts from the `jweDelimiters`/`jwtDelimiters` constants, so the string and byte forms cannot disagree. The byte forms exist for `printDecryptedPayload`, where the candidate is a decrypted payload that may be large and must not be copied into a string just to be measured -- `decodeAndPrintJWE()` / `jweProtectedHeaderMap()` - Parse a JWE with `go-jose` and decode every field in the compact protected header for display; without a key print encrypted part metadata, with a key decrypt and print the payload +- `decodeAndPrintJWE()` / `jweProtectedHeaderMap()` - Parse a JWE with `go-jose` and decode every field in the compact protected header for display; without a key print encrypted part metadata, with a key decrypt and print the payload — the key is passed on to `printDecryptedPayload`, so it keeps applying to whatever the plaintext turns out to be - `jweEncryptedParts()` / `printEncryptedParts()` / `partSize()` - Encrypted part metadata shown when no key is provided. `jweEncryptedParts` splits the compact serialization into its five segments for both the human and `--json` paths; `partSize` renders `base64URLLen`'s count (or its `-1`) as display text, so measuring and formatting are not implemented twice ### `keys.go` - Key loading and format detection @@ -60,8 +60,8 @@ Removing the fallback deleted the heuristics that existed only to decide it (`is ### `output.go` - Formatting, escaping, and colored printing -- `printDecryptedPayload()` / `escapeTerminalText()` / `escapeFormattedJSONControls()` / `isPlainASCIIText()` / `isBelowDEL()` - Recursively decode nested JWTs/JWEs and pretty-print JSON objects or arrays; raw plaintext escapes C0 controls except newline/tab, DEL, C1 controls, invalid UTF-8 bytes, and targeted bidi controls, while formatted JSON sanitizes C1, DEL, and the same targeted bidi controls. Both escapers open with a byte-level shortcut that returns text needing no escaping as a single copy instead of rebuilding it rune by rune — a decrypted payload is unbounded in size, and this is the only path that prints one verbatim. **They use different predicates, and that is deliberate.** `escapeTerminalText` requires printable ASCII (plus newline and tab) via `isPlainASCIIText`, because it does escape the C0 controls. `escapeFormattedJSONControls` uses `isBelowDEL`, which admits them: everything it rewrites is DEL or above, and its input is JSON the formatter already rendered, so with color on it always carries the ESC bytes of the formatter's own ANSI codes — reusing the stricter predicate left the fast path unused for every colored render, the interactive default, at about 6x the cost. Both scans are **byte-level, not `bytes.ContainsFunc`**: rune decoding maps every invalid UTF-8 byte to `RuneError`, which no escape predicate matches, so a rune-level fast path would wave malformed bytes through unescaped. `TestEscapeTerminalText_FastPathMatchesSlowPath` and `TestEscapeFormattedJSONControls_FastPathMatchesSlowPath` pin each shortcut as a pure optimization -- `formatTimestamps()` / `claimTime()` / `representableTime()` / `timestampStatus()` / `humanizeDuration()` - Convert exact `iat`, `exp`, `nbf` Unix numeric values, including fractions, to RFC3339 strings (original value shown in parentheses); `exp` is annotated with the time remaining or elapsed (`expires in 14m` / `expired 2h ago`) and a future `nbf` with `not yet valid, in 5m` (an already-valid `nbf` gets no note). `claimTime` does the conversion. Whole seconds — what every ordinary token carries — are matched against JSON's own integer grammar by `jsonIntegerSeconds` and converted with `strconv.ParseInt`. Every other form is re-validated with `json.Valid` before the exact `big.Rat` path, because a `json.Number` can hold arbitrary text and `big.Rat.SetString` accepts ratios, hex, and binary exponents that JSON does not; that path costs about thirty allocations per claim, and `json.Valid` a copy of the literal, so neither runs for an integer. What `jsonIntegerSeconds` admits is a strict subset of what `json.Valid` would, so the check it skips could not have rejected the value. `humanizeDuration` renders the largest whole unit (s/m/h/d), truncating toward zero for deterministic output. This is display-only and never affects verification or the exit code; `timeNow` is a package variable so the annotations are testable. The `--json` path skips this entirely and keeps raw numeric claims +- `printDecryptedPayload()` / `renderNested()` / `escapeTerminalText()` / `escapeFormattedJSONControls()` / `isPlainASCIIText()` / `isBelowDEL()` - Recursively decode nested JWTs/JWEs and pretty-print JSON objects or arrays. **The key is threaded into the recursion**: a JWE wrapping another token to the same key decrypts all the way down, and a nested JWS gets a real `Signature:` verdict whose `errInvalidSignature` is propagated so the exit code reflects the innermost check. `renderNested` owns that: it buffers the nested render, treats an invalid signature as a verdict rather than a decode failure (the sections are printed, the sentinel only drives the exit), and retries **keyless** when the nested token cannot use the key at all — a different key type, or one that fails to load for it — so such a token keeps exactly the output it had before the key was threaded through. A nested token that does not decode either way falls back to the JSON/raw handling. Raw plaintext escapes C0 controls except newline/tab, DEL, C1 controls, invalid UTF-8 bytes, and targeted bidi controls, while formatted JSON sanitizes C1, DEL, and the same targeted bidi controls. Both escapers open with a byte-level shortcut that returns text needing no escaping as a single copy instead of rebuilding it rune by rune — a decrypted payload is unbounded in size, and this is the only path that prints one verbatim. **They use different predicates, and that is deliberate.** `escapeTerminalText` requires printable ASCII (plus newline and tab) via `isPlainASCIIText`, because it does escape the C0 controls. `escapeFormattedJSONControls` uses `isBelowDEL`, which admits them: everything it rewrites is DEL or above, and its input is JSON the formatter already rendered, so with color on it always carries the ESC bytes of the formatter's own ANSI codes — reusing the stricter predicate left the fast path unused for every colored render, the interactive default, at about 6x the cost. Both scans are **byte-level, not `bytes.ContainsFunc`**: rune decoding maps every invalid UTF-8 byte to `RuneError`, which no escape predicate matches, so a rune-level fast path would wave malformed bytes through unescaped. `TestEscapeTerminalText_FastPathMatchesSlowPath` and `TestEscapeFormattedJSONControls_FastPathMatchesSlowPath` pin each shortcut as a pure optimization +- `formatTimestamps()` / `claimTime()` / `representableTime()` / `timestampStatus()` / `secondsBetween()` / `humanizeSeconds()` - Convert exact `iat`, `exp`, `nbf` Unix numeric values, including fractions, to RFC3339 strings (original value shown in parentheses); `exp` is annotated with the time remaining or elapsed (`expires in 14m` / `expired 2h ago`) and a future `nbf` with `not yet valid, in 5m` (an already-valid `nbf` gets no note). `claimTime` does the conversion. Whole seconds — what every ordinary token carries — are matched against JSON's own integer grammar by `jsonIntegerSeconds` and converted with `strconv.ParseInt`. Every other form is re-validated with `json.Valid` before the exact `big.Rat` path, because a `json.Number` can hold arbitrary text and `big.Rat.SetString` accepts ratios, hex, and binary exponents that JSON does not; that path costs about thirty allocations per claim, and `json.Valid` a copy of the literal, so neither runs for an integer. What `jsonIntegerSeconds` admits is a strict subset of what `json.Valid` would, so the check it skips could not have rejected the value. `humanizeSeconds` renders the largest whole unit (s/m/h/d), truncating toward zero for deterministic output. The difference it is given comes from `secondsBetween`, **not `time.Time.Sub`**: a `time.Duration` saturates at roughly ±292 years, so every `exp` further out than that used to be annotated with the same bogus `106751d`. `secondsBetween` subtracts Unix second counts (correcting the sub-second parts so the truncation stays toward zero), which covers the whole year range `representableTime` admits. This is display-only and never affects verification or the exit code; `timeNow` is a package variable so the annotations are testable. The `--json` path skips this entirely and keeps raw numeric claims - `newFormatter()` / `style` / `newStyle()` / `escapesFor()` - `newFormatter` builds a `jsonFormatter` (see `formatter.go`) from the project color scheme. Each color in that scheme is a `style`, which resolves the ANSI sequences it wraps text in **once** instead of on every use: `fatih/color` rebuilds both sequences with `fmt.Sprintf` per call, which is affordable for a handful of labels but not for a token of JSON. `escapesFor` reads the sequences out of the color package by having it wrap a sentinel byte and splitting on it — the only way to reach sequences it keeps unexported, and the reason jwtd cannot drift from its reset handling. It captures **two** closing sequences, because the library uses two: the `Sprint` family closes with a reset per attribute, the `Fprintf` family with a plain reset around the whole write. `TestStyleMatchesColorPackage` compares both against the library, in both color states; it is what caught the two being conflated - `printSection()` / `printSignature()` / `printVerdict()` / `writeFormattedJSON()` - Formatted output through the styles above. `writeFormattedJSON` writes already-formatted JSON straight to the writer when nothing needs escaping, so the ordinary case does not copy output that can be megabytes. `printSection` takes any JSON-marshalable value, so objects and arrays share one path; `printVerdict` renders the `