From ad8f3d56cd85864cd652504b06670f1c073decd6 Mon Sep 17 00:00:00 2001 From: terra tauri Date: Sun, 27 Sep 2026 23:18:39 -0700 Subject: [PATCH 1/2] harden(keys,verifier): validate JWK points via ecdsa.ParseUncompressedPublicKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both places that reconstruct a P-256 public key from JWK x/y coordinates built an ecdsa.PublicKey by hand and validated it with elliptic.Curve.IsOnCurve, deprecated since Go 1.21 (SA1019). The use was sound — a public point, a boolean on-curve test, no secret — but the stdlib now does this better. Route both through keys.PublicFromXY, which builds the SEC 1 uncompressed encoding and calls ecdsa.ParseUncompressedPublicKey (Go 1.25+). That rejects an off-curve point and the point at infinity in one validated step, removes the deprecated API, and drops verifier/remote.go's duplicate decode so there is one source of truth. Coordinates are left-padded to the 32 bytes P-256 requires (tolerating a stripped leading zero) and an over-long coordinate is rejected. Tests: a valid key round-trips; the minimal (stripped) encoding parses; an off-curve point, the point at infinity, an over-long coordinate, and a non-base64url coordinate are each rejected. Review-only; not for merge. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SDfMmeRXGsanqzDDSjL1uU --- keys/keys.go | 34 +++++++++++++++--- keys/parsexy_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++++ verifier/remote.go | 24 +++---------- 3 files changed, 118 insertions(+), 23 deletions(-) create mode 100644 keys/parsexy_test.go diff --git a/keys/keys.go b/keys/keys.go index 5f7054f..7c5c8a5 100644 --- a/keys/keys.go +++ b/keys/keys.go @@ -16,7 +16,6 @@ import ( "encoding/json" "errors" "fmt" - "math/big" "sync" "time" ) @@ -237,9 +236,36 @@ func PublicFromXY(x, y string) (*ecdsa.PublicKey, error) { if err != nil { return nil, fmt.Errorf("keys: y is not base64url") } - pub := &ecdsa.PublicKey{Curve: elliptic.P256(), X: new(big.Int).SetBytes(xb), Y: new(big.Int).SetBytes(yb)} - if !pub.Curve.IsOnCurve(pub.X, pub.Y) { - return nil, fmt.Errorf("keys: point is not on P-256") + xb, ok := coordBytes(xb) + if !ok { + return nil, fmt.Errorf("keys: x is not a P-256 coordinate") + } + yb, ok = coordBytes(yb) + if !ok { + return nil, fmt.Errorf("keys: y is not a P-256 coordinate") + } + // Build the SEC 1 uncompressed encoding (0x04 || x || y) and let the stdlib + // parse and validate it in one step. ParseUncompressedPublicKey rejects a + // point that is not on the curve or is the point at infinity — replacing a + // manual (and since Go 1.21 deprecated) Curve.IsOnCurve check. + uncompressed := make([]byte, 0, 1+len(xb)+len(yb)) + uncompressed = append(append(append(uncompressed, 4), xb...), yb...) + pub, err := ecdsa.ParseUncompressedPublicKey(elliptic.P256(), uncompressed) + if err != nil { + return nil, fmt.Errorf("keys: invalid P-256 public key: %w", err) } return pub, nil } + +// coordBytes left-pads an affine coordinate to the 32 bytes P-256 requires, +// rejecting an over-long input. A compliant JWK carries exactly 32 bytes +// (RFC 7518 §6.2.1.2); left-padding also tolerates a producer that stripped +// leading zeros, while a longer value is not a valid coordinate. +func coordBytes(b []byte) ([]byte, bool) { + if len(b) > 32 { + return nil, false + } + out := make([]byte, 32) + copy(out[32-len(b):], b) + return out, true +} diff --git a/keys/parsexy_test.go b/keys/parsexy_test.go new file mode 100644 index 0000000..de74f9b --- /dev/null +++ b/keys/parsexy_test.go @@ -0,0 +1,83 @@ +package keys + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "encoding/base64" + "math/big" + "strings" + "testing" +) + +func b64(b []byte) string { return base64.RawURLEncoding.EncodeToString(b) } + +// pad32 mirrors the on-wire coordinate encoding (left-padded to the P-256 size). +func pad32(n *big.Int) []byte { + b := make([]byte, 32) + n.FillBytes(b) + return b +} + +func TestPublicFromXY_roundTripsAValidKey(t *testing.T) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + got, err := PublicFromXY(b64(pad32(priv.X)), b64(pad32(priv.Y))) + if err != nil { + t.Fatalf("a valid P-256 point must parse: %v", err) + } + if got.X.Cmp(priv.X) != 0 || got.Y.Cmp(priv.Y) != 0 { + t.Fatal("round-tripped key does not match the original coordinates") + } +} + +func TestPublicFromXY_toleratesMinimalCoordinateEncoding(t *testing.T) { + // big.Int.Bytes() returns the minimal big-endian form (leading zeros + // stripped), which a lenient producer may emit. Left-padding must accept it + // whatever its length. + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + if _, err := PublicFromXY(b64(priv.X.Bytes()), b64(priv.Y.Bytes())); err != nil { + t.Fatalf("the minimal (stripped) coordinate encoding of a valid point must parse: %v", err) + } +} + +func TestPublicFromXY_rejectsOffCurvePoint(t *testing.T) { + // A valid X with Y := Y+1 is (almost surely) not on the curve. + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + badY := new(big.Int).Add(priv.Y, big.NewInt(1)) + if _, err := PublicFromXY(b64(pad32(priv.X)), b64(pad32(badY))); err == nil { + t.Fatal("an off-curve point must be rejected") + } +} + +func TestPublicFromXY_rejectsPointAtInfinity(t *testing.T) { + zero := b64(make([]byte, 32)) + if _, err := PublicFromXY(zero, zero); err == nil { + t.Fatal("the point at infinity (0,0) must be rejected") + } +} + +func TestPublicFromXY_rejectsOverlongCoordinate(t *testing.T) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + overlong := b64(append([]byte{0}, pad32(priv.X)...)) // 33 bytes + if _, err := PublicFromXY(overlong, b64(pad32(priv.Y))); err == nil { + t.Fatal("an over-long (>32 byte) coordinate must be rejected") + } +} + +func TestPublicFromXY_rejectsNonBase64(t *testing.T) { + if _, err := PublicFromXY("!!!", "!!!"); err == nil || !strings.Contains(err.Error(), "base64url") { + t.Fatalf("non-base64url coordinate must be rejected as such, got: %v", err) + } +} diff --git a/verifier/remote.go b/verifier/remote.go index 65f8902..79f3e32 100644 --- a/verifier/remote.go +++ b/verifier/remote.go @@ -3,11 +3,8 @@ package verifier import ( "context" "crypto/ecdsa" - "crypto/elliptic" - "encoding/base64" "encoding/json" "fmt" - "math/big" "net/http" "sync" "time" @@ -201,23 +198,12 @@ func jwkToPublic(j keys.JWK) (*ecdsa.PublicKey, error) { if j.Use != "" && j.Use != "sig" { return nil, fmt.Errorf("key is not for signatures") } - x, err := base64.RawURLEncoding.DecodeString(j.X) + // Coordinate decode and on-curve validation live in keys.PublicFromXY, + // which uses the stdlib's ParseUncompressedPublicKey to reject an off-curve + // point or the point at infinity. One source of truth, no deprecated API. + pub, err := keys.PublicFromXY(j.X, j.Y) if err != nil { - return nil, fmt.Errorf("x is not base64url") - } - y, err := base64.RawURLEncoding.DecodeString(j.Y) - if err != nil { - return nil, fmt.Errorf("y is not base64url") - } - pub := &ecdsa.PublicKey{ - Curve: elliptic.P256(), - X: new(big.Int).SetBytes(x), - Y: new(big.Int).SetBytes(y), - } - // Reject a point that is not on the curve. Without this check a - // malformed or hostile key set could supply an invalid point. - if !pub.Curve.IsOnCurve(pub.X, pub.Y) { - return nil, fmt.Errorf("point is not on P-256") + return nil, err } // The kid is defined as the RFC 7638 thumbprint of the key, so verify the // declared kid actually derives from this key. Otherwise a publisher could From d46a07175d39f9e2f8266a337da643ae24d09378 Mon Sep 17 00:00:00 2001 From: terra tauri Date: Sun, 27 Sep 2026 23:23:49 -0700 Subject: [PATCH 2/2] ci(lint): add golangci-lint as a CI step, with config, across all modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI had two fuzzers, govulncheck, gitleaks, a stdlib-only gate, load and benchmark-regression gates — but no golangci-lint, so errcheck/ineffassign/ staticcheck findings went unseen. Add a lint job that runs golangci-lint (v2.11.4) over all three modules, plus a .golangci.yml that enables the standard set (errcheck, govet, ineffassign, staticcheck, unused) tuned to stay green on real defects: the std-error-handling preset for unactionable Close/Fprintln/response-write errors, errcheck relaxed in _test.go, and the QF* refactor "quick fixes" left off (style, not correctness). The SA* bug checks run everywhere. Two real findings surfaced and fixed here (both in keys_test.go): a possible nil-pointer dereference after a non-fatal nil check (SA5011), now t.Fatal; and an ineffectual assignment, now a blank identifier. The SA1019 deprecation this config would otherwise flag is fixed in the parent commit. Review-only; not for merge. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SDfMmeRXGsanqzDDSjL1uU --- .github/workflows/ci.yaml | 22 ++++++++++++++++++++++ .golangci.yml | 39 +++++++++++++++++++++++++++++++++++++++ keys/keys_test.go | 9 +++------ 3 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b3a2a6b..4a53d01 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -238,3 +238,25 @@ jobs: govulncheck ./... (cd tsnetid && govulncheck ./...) (cd gateway && govulncheck ./...) + + # Static analysis across every module. The standard linter set (errcheck, + # govet, ineffassign, staticcheck, unused); configuration is in + # .golangci.yml at the repo root, which each module picks up by upward search. + lint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: {go-version: '1.26'} + - name: Install golangci-lint + run: | + set -euo pipefail + curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh \ + | sh -s -- -b "$(go env GOPATH)/bin" v2.11.4 + - name: Lint every module + run: | + set -euo pipefail + export PATH="$(go env GOPATH)/bin:$PATH" + golangci-lint run ./... + (cd tsnetid && golangci-lint run ./...) + (cd gateway && golangci-lint run ./...) diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..d1a2b25 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,39 @@ +# golangci-lint configuration. +# +# The standard set (errcheck, govet, ineffassign, staticcheck, unused) is the +# high-signal core. The tuning below keeps it green on real defects without +# nagging on patterns this codebase deliberately accepts. +version: "2" + +linters: + default: standard + settings: + staticcheck: + # The SA* bug checks plus golangci-lint's default set. The ST* doc/naming + # checks below are golangci-lint's own defaults (off), kept explicit here + # because setting `checks` replaces the default rather than extending it. + checks: + - all + - -ST1000 + - -ST1003 + - -ST1016 + - -ST1020 + - -ST1021 + - -ST1022 + # QF* are refactor "quick fixes" (tagged switch, drop embedded field), + # not defects. Leave them off; they are style, not correctness. + - -QF1002 + - -QF1008 + exclusions: + # A failed write to an HTTP response, a deferred Close, or an Fprintln to a + # ResponseWriter cannot be meaningfully handled; the standard preset covers + # exactly those and keeps errcheck focused on returns that matter. + presets: + - std-error-handling + rules: + # Unchecked error returns in test setup (e.g. Rotate, json.Unmarshal) are + # low value: a failure there fails the test anyway. Keep errcheck strict + # for non-test code. + - path: _test\.go + linters: + - errcheck diff --git a/keys/keys_test.go b/keys/keys_test.go index 341aec3..eb734b0 100644 --- a/keys/keys_test.go +++ b/keys/keys_test.go @@ -56,11 +56,8 @@ func TestRotateOldKeyVerifiesDuringOverlapWindow(t *testing.T) { // Manually verify the key is in the set pub1, ok := set.PublicKey(kid1) - if !ok { - t.Error("Current key not found in set after NewSet") - } - if pub1 == nil { - t.Error("PublicKey returned nil for current kid") + if !ok || pub1 == nil { + t.Fatal("PublicKey did not return the current key after NewSet") } // Verify it matches the private key @@ -139,7 +136,7 @@ func TestRotateOldKeyDoesNotVerifyAfterOverlap(t *testing.T) { laterSet.current = set.current laterSet.retired = set.retired - pub, exists = laterSet.PublicKey(kid1) + _, exists = laterSet.PublicKey(kid1) if exists { t.Error("old key should not exist after overlap window expired") }