fix!: harden JWKS rotation, typ, issuer/redirect URL and Client-Cert parsing, kid-less client keys

GETTING_STARTED.md

@@ -1747,8 +1747,9 @@ in the right order:
    for your JWKS — this module's own `keys.NewJWKSIssuerKeySource` (a
    client resolving your keys) refetches on an unrecognized kid at most
    once per its minimum refresh interval (`keys.WithMinRefreshInterval`,
-   by default its cache TTL), so it picks up a new key within one TTL,
-   and you may have consumers you don't control caching longer.
+   by default a minute, or the cache TTL if shorter), but you may have
+   consumers you don't control that only refetch once their cache
+   expires.
    When in doubt, wait longer than you think you need to; this step
    costs nothing but time.
 4. Cut `Sign`/`PublicKey` over to the new key. New ID tokens, JWT

UPGRADING.md

@@ -164,6 +164,69 @@ result, err := srv.IssueAccessToken(ctx, server.IssueAccessTokenRequest{
 })
 ```
 
+### `server.New` and `client.New` refuse an issuer with a query (server, client)
+
+**Affects:** a server's or a client's `Config.Issuer` with a query,
+even an empty `?`. `fapi.ParseIssuerURL` has refused one since v0.65.0,
+so this only reaches an issuer built another way, such as with
+`fapi.ParseEndpointURL`, which allows a query.
+
+**Why:** RFC 8414 §2 and OpenID Connect Core §2 define an issuer as a
+URL with no query or fragment. v0.65.0 made `fapi.ParseIssuerURL`
+refuse a query, but `New` accepted any `fapi.URL` as the issuer, so a
+server could still publish and sign (`iss` in ID tokens, authorization
+responses and JARM) an issuer conformant clients refuse.
+
+**What fails:** `New` returns an error ending "issuer must not contain
+a query (RFC 8414 §2): parse it with fapi.ParseIssuerURL", at either
+assurance level.
+
+**What to change:** parse the issuer with `fapi.ParseIssuerURL` and
+give it without a query. An issuer that needs to tell tenants apart
+puts the tenant in the path:
+
+```go
+issuer, err := fapi.ParseIssuerURL("https://as.example/tenants/a") // not ParseEndpointURL("https://as.example/?tenant=a")
+if err != nil {
+	return err
+}
+cfg.Issuer = issuer
+```
+
+### An empty fragment (`#`) is refused in issuer, endpoint and redirect URIs (all packages, storage)
+
+**Affects:**
+
+- a URL ending in a bare `#` given to `fapi.ParseIssuerURL`,
+  `fapi.ParseEndpointURL` or `fapi.ParseRedirectURL`, including an
+  issuer a client discovers and a `redirect_uri` a client sends to a
+  server;
+- a client registered with `storage.NewRegisteredClient` (and so
+  `sqlstore`) whose `RedirectURIs` has a URI containing `#`, even a
+  web client's.
+
+**Why:** RFC 3986 §3.5 makes everything after the first `#` a fragment
+component, an empty one included. RFC 8414 §2 rules one out of an
+issuer and RFC 6749 §3.1.2 out of a redirect URI. The parsers refused
+a non-empty fragment but parsed a bare `#` away, silently rewriting the
+value. `NewRegisteredClient` checked a web client's redirect URIs for
+no fragment at all, and registered one that could only match a request
+spelled the same way, which the server's own parser then refused.
+
+**What fails:** the parsers return an error ending "URL must not
+contain a fragment". `NewRegisteredClient` returns an error saying the
+redirect URI "has a fragment, which a redirect URI must not (RFC 6749
+§3.1.2)". A client already stored in `sqlstore` with such a URI fails
+to resolve with `sqlstore.ErrClientConfig`, so requests naming it are
+refused until it is stored again without the `#`.
+
+**What to change:** drop the `#`, from the configuration and from each
+client's registered redirect URIs:
+
+```go
+cfg.RedirectURIs = []fapi.RegisteredRedirectURI{"https://rp.example/cb"} // not "https://rp.example/cb#"
+```
+
 ## v0.68.0
 
 ### `DefaultAttesterLeafPolicy` refuses more signing roles (server)

client/client.go

@@ -215,6 +215,9 @@ func validateRequiredConfig(cfg Config) error {
 	if cfg.Issuer.IsZero() {
 		return fmt.Errorf("client: config: issuer is required")
 	}
+	if u := cfg.Issuer.URL(); u.RawQuery != "" || u.ForceQuery {
+		return fmt.Errorf("client: config: issuer must not contain a query (RFC 8414 §2): parse it with fapi.ParseIssuerURL")
+	}
 	if cfg.ClientID == "" {
 		return fmt.Errorf("client: config: client ID is required")
 	}

client/config.go

@@ -447,7 +447,9 @@ type Config struct {
 	// Issuer is the authorization server this client talks to — the
 	// audience client assertions and request objects are addressed to,
 	// and the issuer authorization responses and ID tokens must come
-	// from.
+	// from. Parse it with fapi.ParseIssuerURL: New refuses one with a
+	// query, even an empty one, which RFC 8414 §2 and OpenID Connect
+	// Core §2 rule out.
 	Issuer fapi.URL
 
 	// ClientID is this client's identifier at the authorization server:

client/issuer_query_test.go

@@ -0,0 +1,31 @@
+package client_test
+
+import (
+	"strings"
+	"testing"
+
+	fapi "github.com/idfoundry/fapigo"
+	"github.com/idfoundry/fapigo/client"
+)
+
+// TestNewRefusesIssuerWithQuery: an issuer has no query component (RFC
+// 8414 §2, OpenID Connect Core §2), so New refuses one even when it was
+// built with a parser that allows a query, under either assurance level.
+func TestNewRefusesIssuerWithQuery(t *testing.T) {
+	for _, assurance := range []client.AssuranceLevel{client.AssuranceDevelopment, client.AssuranceProduction} {
+		for _, suffix := range []string{"?tenant=a", "?"} {
+			t.Run(suffix, func(t *testing.T) {
+				cfg := validConfig(t)
+				cfg.Assurance = assurance
+				issuer, err := fapi.ParseEndpointURL(cfg.Issuer.String() + suffix)
+				if err != nil {
+					t.Fatalf("ParseEndpointURL: %v", err)
+				}
+				cfg.Issuer = issuer
+				if _, err := client.New(cfg, validDependencies(t)); err == nil || !strings.Contains(err.Error(), "query") {
+					t.Fatalf("New(issuer %q, assurance %v) = %v, want the query refused", issuer, assurance, err)
+				}
+			})
+		}
+	}
+}

fapihttp/forwarded_certificate.go

@@ -58,13 +58,15 @@ var (
 // certificate, authenticating as that client and presenting its
 // certificate-bound tokens.
 //
-// The value is RFC 8941 §3.3.5's Byte Sequence: the DER certificate in
-// base64 between colons. A header occurring more than once (RFC 9440
-// §2.2 makes it a singleton), a value longer than
-// MaxForwardedCertificateBytes, anything but one Byte Sequence
-// (parameters included: RFC 9440 defines none), or bytes that aren't
-// exactly one X.509 certificate is ErrMalformedForwardedCertificate.
-// Missing "=" padding is accepted, as RFC 8941 §4.2.7 asks. A forwarded
+// The value is RFC 9651 §3.3.5's Byte Sequence: the DER certificate in
+// base64 between colons. RFC 9440 defines no parameters, but RFC 9651
+// §2.3 lets every Item carry some, so any that follow the Byte Sequence
+// (":…:;a=1") are accepted and ignored. A header occurring more than
+// once (RFC 9440 §2.2 makes it a singleton), a value longer than
+// MaxForwardedCertificateBytes, anything but one Byte Sequence and its
+// parameters, or bytes that aren't exactly one X.509 certificate is
+// ErrMalformedForwardedCertificate. Missing "=" padding is accepted, as
+// RFC 9651 §4.2.7 asks. A forwarded
 // PEM or URL-encoded certificate, which some proxies send in their own
 // headers, isn't RFC 9440's encoding and is refused: configure the proxy
 // to send Client-Cert instead.
@@ -97,21 +99,29 @@ func ForwardedClientCertificate(r *http.Request, header string, trusted func(*ht
 	return cert, nil
 }
 
-// parseByteSequence decodes a header field value that is one RFC 8941
-// Byte Sequence (§3.3.5, parsed per §4.2 and §4.2.7): optional spaces,
-// ":", base64, ":", optional spaces, and nothing else.
+// parseByteSequence decodes a header field value that is one RFC 9651
+// Item whose bare item is a Byte Sequence (§3.3.5, parsed per §4.2,
+// §4.2.3 and §4.2.7): optional spaces, ":", base64, ":", any
+// parameters (ignored), optional spaces, and nothing else.
 func parseByteSequence(value string) ([]byte, error) {
 	if len(value) > MaxForwardedCertificateBytes {
 		return nil, fmt.Errorf("the value is longer than %d bytes", MaxForwardedCertificateBytes)
 	}
-	// RFC 8941 §4.2: leading and trailing SP are discarded (net/http has
+	// RFC 9651 §4.2: leading and trailing SP are discarded (net/http has
 	// already removed any other optional whitespace).
 	value = strings.Trim(value, " ")
-	if len(value) < 2 || value[0] != ':' || value[len(value)-1] != ':' {
+	end := -1
+	if value != "" && value[0] == ':' {
+		end = strings.IndexByte(value[1:], ':')
+	}
+	if end < 0 {
 		return nil, errors.New("the value isn't a byte sequence (:base64:)")
 	}
-	body := value[1 : len(value)-1]
-	// Up to two "=" of padding, only at the end; RFC 8941 §4.2.7 asks a
+	body := value[1 : 1+end]
+	if err := skipParameters(value[2+end:]); err != nil {
+		return nil, err
+	}
+	// Up to two "=" of padding, only at the end; RFC 9651 §4.2.7 asks a
 	// parser not to fail when it is missing.
 	unpadded := strings.TrimRight(body, "=")
 	if unpadded == "" {
@@ -121,9 +131,9 @@ func parseByteSequence(value string) ([]byte, error) {
 	if len(body)-len(unpadded) > 2 || strings.ContainsRune(unpadded, '=') {
 		return nil, errors.New("the byte sequence's padding is malformed")
 	}
-	// encoding/base64 skips line breaks, which RFC 8941's base64 doesn't
+	// encoding/base64 skips line breaks, which RFC 9651's base64 doesn't
 	// allow, so the alphabet is checked first. Non-zero pad bits are
-	// accepted, as RFC 8941 §4.2.7 asks.
+	// accepted, as RFC 9651 §4.2.7 asks.
 	if strings.IndexFunc(unpadded, notBase64) >= 0 {
 		return nil, errors.New("the byte sequence isn't base64")
 	}

fapihttp/forwarded_certificate_fuzz_test.go

@@ -11,26 +11,32 @@ import (
 	"errors"
 	"math/big"
 	"net/http"
+	"net/url"
+	"regexp"
 	"strings"
 	"testing"
 	"time"
+	"unicode/utf8"
 )
 
 // FuzzParseByteSequence checks parseByteSequence, which reads the RFC
 // 9440 Client-Cert header a proxy forwards, against an independent
-// reading of RFC 8941 §3.3.5/§4.2.7: beyond never panicking,
+// reading of RFC 9651 §3.3.5/§4.2.3/§4.2.7: beyond never panicking,
 //
 //   - it accepts exactly a value that, once leading and trailing spaces
 //     are dropped, is ":", base64 characters, at most two "=" at the
-//     end, ":", within MaxForwardedCertificateBytes, whose base64 decodes
-//     to at least one byte;
+//     end, ":", then zero or more RFC 9651 Parameters, within
+//     MaxForwardedCertificateBytes, whose base64 decodes to at least one
+//     byte;
 //   - what it accepts re-encodes, canonically padded, to a Byte Sequence
 //     it reads back to the same bytes, whose base64 is the input's own
 //     up to pad bits.
 func FuzzParseByteSequence(f *testing.F) {
 	for _, seed := range []string{
 		":AQID:", " :AQID: ", ":AQI=:", ":AQ==:", ":AQ:", ":AR==:", ":=:", ":==:", "::", ":", "",
 		":AQ===:", ":A=Q=:", ":AQ\nID:", ":AQ-_:", "AQID", ":AQID:;p=1", "\t:AQID:", ":A:",
+		":AQID:;a", ":AQID:; a=?1;b=-12.5", `:AQID:;a="q\"";b=%"%c3%a9"`, ":AQID:;a=@-1;b=t:/x",
+		":AQID: ;a", ":AQID:;A", ":AQID:;a=1.", ":AQID:;a=%\"%c3\"", ":AQID:;a=:A=:", ":AQID:;",
 	} {
 		f.Add(seed)
 	}
@@ -47,17 +53,52 @@ func FuzzParseByteSequence(f *testing.F) {
 	})
 }
 
-// byteSequenceOracle is RFC 8941's Byte Sequence grammar as
+// sfParameter is one RFC 9651 §4.2.3.2 parameter as a regular
+// expression: ";", optional spaces, a key (§4.2.3.3), and optionally "="
+// and a bare item: a Decimal or Integer (§4.2.4), a String (§4.2.5), a
+// Token (§4.2.6), a Byte Sequence (§4.2.7), a Boolean (§4.2.8), a Date
+// (§4.2.9) or a Display String (§4.2.10), whose escaped content is the
+// one submatch, for parametersOracle to check it decodes to UTF-8.
+var sfParameter = regexp.MustCompile(`^; *[a-z*][a-z0-9_\-.*]*(?:=(?:` +
+	`-?[0-9]{1,12}\.[0-9]{1,3}|-?[0-9]{1,15}|` +
+	`"(?:[\x20\x21\x23-\x5b\x5d-\x7e]|\\["\\])*"|` +
+	"[A-Za-z*][!#$%&'*+\\-.^_`|~0-9A-Za-z:/]*|" +
+	`:[A-Za-z0-9+/=]*:|\?[01]|@-?[0-9]{1,15}|` +
+	`%"((?:[\x20\x21\x23\x24\x26-\x7e]|%[0-9a-f]{2})*)"))?`)
+
+// parametersOracle reports whether s is RFC 9651 Parameters: zero or
+// more of sfParameter, and nothing else.
+func parametersOracle(s string) bool {
+	for s != "" {
+		m := sfParameter.FindStringSubmatchIndex(s)
+		if m == nil {
+			return false
+		}
+		if m[2] >= 0 {
+			decoded, err := url.PathUnescape(s[m[2]:m[3]])
+			if err != nil || !utf8.ValidString(decoded) {
+				return false
+			}
+		}
+		s = s[m[1]:]
+	}
+	return true
+}
+
+// byteSequenceOracle is RFC 9651's Byte Sequence Item grammar as
 // parseByteSequence documents it, written without its code.
 func byteSequenceOracle(value string) bool {
 	if len(value) > MaxForwardedCertificateBytes {
 		return false
 	}
 	v := strings.Trim(value, " ")
-	if !strings.HasPrefix(v, ":") || !strings.HasSuffix(v, ":") || len(v) < 2 {
+	if !strings.HasPrefix(v, ":") {
+		return false
+	}
+	body, params, closed := strings.Cut(v[1:], ":")
+	if !closed || !parametersOracle(params) {
 		return false
 	}
-	body := v[1 : len(v)-1]
 	pad := len(body) - len(strings.TrimRight(body, "="))
 	chars := body[:len(body)-pad]
 	if pad > 2 {
@@ -85,9 +126,10 @@ func checkByteSequenceRoundTrip(t *testing.T, value string, der []byte) {
 	if err != nil || !bytes.Equal(again, der) {
 		t.Fatalf("parseByteSequence(%q) = %x, but its canonical form %q reads back as %x, %v", value, der, canonical, again, err)
 	}
-	body := strings.TrimRight(strings.Trim(strings.Trim(value, " "), ":"), "=")
+	body, _, _ := strings.Cut(strings.TrimPrefix(strings.Trim(value, " "), ":"), ":")
+	body = strings.TrimRight(body, "=")
 	reencoded := base64.RawStdEncoding.EncodeToString(der)
-	// Only the last character may differ, in pad bits RFC 8941 §4.2.7
+	// Only the last character may differ, in pad bits RFC 9651 §4.2.7
 	// asks a parser to ignore.
 	if len(body) != len(reencoded) || body[:len(body)-1] != reencoded[:len(reencoded)-1] {
 		t.Fatalf("parseByteSequence(%q) = %x, which re-encodes as %q", value, der, reencoded)

fapihttp/forwarded_certificate_test.go

@@ -106,7 +106,29 @@ func TestForwardedClientCertificateEncodings(t *testing.T) {
 		"padding missing":            {[]string{":" + raw + ":"}, true},
 		"twice":                      {[]string{":" + b64 + ":", ":" + b64 + ":"}, false},
 		"a list":                     {[]string{":" + b64 + ":, :" + b64 + ":"}, false},
-		"parameters":                 {[]string{":" + b64 + ":;a=1"}, false},
+		"a parameter":                {[]string{":" + b64 + ":;a=1"}, true},
+		"a boolean parameter":        {[]string{":" + b64 + ":;grease"}, true},
+		"parameters of every type":   {[]string{":" + b64 + `:;a=-1.5;b="x \" y";c=tok/en:x;d=:AQID:;e=?0;f=@1700000000;g=%"caf%c3%a9";*h;i`}, true},
+		"spaces after a semicolon":   {[]string{":" + b64 + ":;  a=1 "}, true},
+		"a space before a parameter": {[]string{":" + b64 + ": ;a=1"}, false},
+		"an uppercase key":           {[]string{":" + b64 + ":;A=1"}, false},
+		"a parameter with no value":  {[]string{":" + b64 + ":;a="}, false},
+		"a semicolon alone":          {[]string{":" + b64 + ":;"}, false},
+		"a four-digit fraction":      {[]string{":" + b64 + ":;a=1.2345"}, false},
+		"an unterminated string":     {[]string{":" + b64 + `:;a="x`}, false},
+		"a decimal date":             {[]string{":" + b64 + ":;a=@1.5"}, false},
+		"an invalid display string":  {[]string{":" + b64 + `:;a=%"%ff"`}, false},
+		"an inner list parameter":    {[]string{":" + b64 + ":;a=(1 2)"}, false},
+		"a sign with no digits":      {[]string{":" + b64 + ":;a=-"}, false},
+		"a bad string escape":        {[]string{":" + b64 + `:;a="\x"`}, false},
+		"a tab in a string":          {[]string{":" + b64 + ":;a=\"a\tb\""}, false},
+		"a non-base64 parameter":     {[]string{":" + b64 + ":;a=:A$:"}, false},
+		"an open byte sequence":      {[]string{":" + b64 + ":;a=:AQID"}, false},
+		"a boolean of 2":             {[]string{":" + b64 + ":;a=?2"}, false},
+		"a display string, no quote": {[]string{":" + b64 + ":;a=%x"}, false},
+		"an uppercase escape":        {[]string{":" + b64 + `:;a=%"%C3%A9"`}, false},
+		"a tab in a display string":  {[]string{":" + b64 + ":;a=%\"a\tb\""}, false},
+		"an open display string":     {[]string{":" + b64 + `:;a=%"abc`}, false},
 		"no colons":                  {[]string{b64}, false},
 		"one colon":                  {[]string{":" + b64}, false},
 		"empty":                      {[]string{"::"}, false},