Hacktoberfest 2026: le issue che i maintainer hanno segnato per ottobre, aperte e adatte ai principianti. Sfoglia le issue Hacktoberfest

Consolidate duplicated JWKS caching and IP classification

Aperta
  • Qualcosa non va?
#6,232 1 commento 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
5/5
Tempo stimato
Più di una settimana
Idoneità per principianti
42/100
Tipo di issue
Refactoring
Chiarezza
Abbastanza chiara
Stato di attività
Tranquilla
Stack tecnologico
go
Ambito
networking, security

Direzione di ricerca

Inizia dalla registrazione JWKS in pkg/auth/token.go e pkg/authserver/server/tokenexchange/multi_issuer_validator.go, quindi confronta le protezioni aggiunte in #6149. Esamina i siti di chiamata di net.ParseIP elencati, in particolare pkg/networking/utilities.go e il validatore dello scambio di token, insieme a pkg/oauthproto/cimd/fetch.go. Il lavoro è completato quando il comportamento JWKS condiviso è consolidato e tutti i controlli degli indirizzi elencati gestiscono in modo coerente i literal con zona e gli host risolti non classificabili.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

enhancement good-tenth-issue

Summary

Two pieces of security-relevant logic exist in more than one place in this repo, and in both cases the copies have drifted: one copy got hardened and the other didn't. This proposes consolidating them, which also closes the gaps the drift left behind.

Both were found while reviewing #6149 (RFC 8693 token exchange with external OIDC issuers), which fixed its own copies. Neither is an urgent exploit; the point is that fixing the copies individually leaves the next drift to happen again.

1. Two JWKS caches over the same library

pkg/auth/token.go (TokenValidator, ~1370 lines) and pkg/authserver/server/tokenexchange/multi_issuer_validator.go both:

  • build an HTTP client with networking.NewHttpClientBuilder()
  • create a jwk.Cache over httprc
  • register lazily on first use, under a mutex, caching the registration error
  • handle httprc.ErrNotReady() explicitly to distinguish "registered but first fetch pending" from "not registered"

They were written independently and converged on the same design, including the same non-obvious ErrNotReady insight. But #6149 added two protections that pkg/auth/token.go does not have:

No response-body cap on the JWKS path. token.go caps its discovery reads with io.LimitReader(resp.Body, maxResponseSize) (token.go:141, :324), but the JWKS fetch goes through jwx, whose own ceiling is httprc.MaxBufferSize (~1 GB), and jwx then does an unbounded io.ReadAll under that before parsing. #6149 wraps the client's transport with http.MaxBytesReader to restore the cap; token.go registers at :715 with only jwk.WithHTTPClient.

No bound on the refresh interval. Without jwk.WithConstantInterval, httprc derives the refresh interval from the endpoint's own Cache-Control/Expires, clamped only to [DefaultMinInterval, DefaultMaxInterval] = [15 minutes, 30 days]. So the issuer being validated chooses how long its keys stay cached. A key revoked upstream but still present locally keeps validating tokens until the next refresh — the unknown-kid refresh path doesn't help, since the kid is still known.

Worth noting the background refresh is autonomous: only the first registration is request-driven, every later fetch is dispatched by httprc's timer with no request involved, so neither gap is gated by client authentication.

Proposal: extract the narrow shared piece — register a JWKS URL with a body-capped client and a pinned refresh interval — into one helper (pkg/auth/ already has discovery/ and dcr/ subpackages that could host it), and have both validators call it. The rest of the two implementations differs legitimately: token.go is single-issuer, the token-exchange one is multi-issuer with per-issuer transport policy.

2. Zone-blind IP classification, repeated

net.ParseIP returns nil for a zoned IPv6 literal (::1%lo0, fe80::1%en0, ::ffff:127.0.0.1%zzz), while netip.ParseAddr parses them and Go's own resolver accepts them — net.lookupIPAddr short-circuits literals through netip.ParseAddr and preserves the zone.

Several guards classify addresses with the ip := net.ParseIP(host); ip != nil && ... shape, which therefore skips classification entirely for a zoned literal rather than rejecting it:

  • pkg/networking/utilities.go — AddressReferencesPrivateIp, used by the dial-time guard installed when WithPrivateIPs(false)
  • pkg/authserver/server/tokenexchange/multi_issuer_validator.go — ValidateJWKSURL
  • pkg/authserver/config.go — validateJWKSEndpointURL (delegates to the above)
  • pkg/skills/gitresolver/reference.go
  • pkg/plugins/pluginsvc/oci.go (preceded by an IsLocalhost check, so partially covered)
  • cmd/thv-operator/pkg/validation/url_validation.go

Measured: through a client built with WithPrivateIPs(false), a request to http://[::ffff:127.0.0.1%25zzz]:<port>/ reaches a loopback listener and the handler runs, while plain http://127.0.0.1:<port>/ is correctly blocked. The 4-in-6 form is the one that matters — TCPAddr.family() sees an IPv4 address, drops the zone, and issues an ordinary connect.

This is hardening rather than a readily exploitable path from ToolHive: the affected inputs are operator configuration or a discovery document from an already-trusted issuer, and on HTTPS paths certificate verification against a loopback literal fails anyway. But the guards don't do what they are written to do.

Proposal: one zone-aware helper in pkg/networking (netip.ParseAddr-based, returning nil for non-literals so hostnames still pass through), used at the sites above. Separately, AddressReferencesPrivateIp should fail closed on any host it cannot classify — it only ever receives a resolved literal from the dialer's Control hook (net/sock_posix.go passes raddr.String()), never a hostname, so refusing the unclassifiable case is safe there and fixes the general problem rather than just the zone instance.

Note pkg/oauthproto/cimd/fetch.go is currently safe only incidentally — net.ParseIP returning nil leads to "no valid address found" rather than a dial — so it's worth including so a later refactor can't silently open it.

Scope

Not blocking #6149, which fixed its own copies. Filed so the consolidation happens once rather than each caller rediscovering it.

Lingua principale
Go
Stelle
2.3k
Fork
318
Merge medio
2g 8h
PR unite (30g)
79

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di stacklok/toolhive

Tutte le issue di stacklok/toolhive

Issue simili

Altre issue su Go

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.