diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index eb1dc1f9..8d7c8b58 100644 --- a/GETTING_STARTED.md +++ b/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 diff --git a/UPGRADING.md b/UPGRADING.md index ebabbccb..dd5b82ee 100644 --- a/UPGRADING.md +++ b/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) diff --git a/client/client.go b/client/client.go index 587aadb0..63c6de9b 100644 --- a/client/client.go +++ b/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") } diff --git a/client/config.go b/client/config.go index 719764b8..11a22a2f 100644 --- a/client/config.go +++ b/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: diff --git a/client/issuer_query_test.go b/client/issuer_query_test.go new file mode 100644 index 00000000..1336c200 --- /dev/null +++ b/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) + } + }) + } + } +} diff --git a/fapihttp/forwarded_certificate.go b/fapihttp/forwarded_certificate.go index 65360fc0..456b9e56 100644 --- a/fapihttp/forwarded_certificate.go +++ b/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") } diff --git a/fapihttp/forwarded_certificate_fuzz_test.go b/fapihttp/forwarded_certificate_fuzz_test.go index 0e7349f7..14b6f98b 100644 --- a/fapihttp/forwarded_certificate_fuzz_test.go +++ b/fapihttp/forwarded_certificate_fuzz_test.go @@ -11,19 +11,23 @@ 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. @@ -31,6 +35,8 @@ 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) diff --git a/fapihttp/forwarded_certificate_test.go b/fapihttp/forwarded_certificate_test.go index 7830669d..8ead28ff 100644 --- a/fapihttp/forwarded_certificate_test.go +++ b/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}, diff --git a/fapihttp/structured_field.go b/fapihttp/structured_field.go new file mode 100644 index 00000000..bb6e4756 --- /dev/null +++ b/fapihttp/structured_field.go @@ -0,0 +1,196 @@ +package fapihttp + +import ( + "errors" + "strings" + "unicode/utf8" +) + +// This file reads just enough of RFC 9651 (Structured Field Values for +// HTTP, which obsoletes RFC 8941) to skip an Item's Parameters: RFC 9440 +// §2.2 makes Client-Cert a Byte Sequence, and RFC 9651 §2.3 has every +// Item allow Parameters "as an extensibility mechanism", discouraging a +// field from treating an unrecognized one as an error. Each function +// checks one RFC 9651 §4.2 production at the start of s and returns +// what follows it, so a value that isn't a valid Item is still refused. + +var errMalformedParameters = errors.New("the byte sequence's parameters are malformed") + +// skipParameters checks that s is zero or more Parameters (RFC 9651 +// §4.2.3.2: ";", optional spaces, a key, optionally "=" and a bare item) +// and nothing else. Their keys and values are ignored. +func skipParameters(s string) error { + for s != "" { + if s[0] != ';' { + return errors.New("the value isn't a byte sequence (:base64:)") + } + rest, ok := skipKey(strings.TrimLeft(s[1:], " ")) + if !ok { + return errMalformedParameters + } + if rest != "" && rest[0] == '=' { + if rest, ok = skipBareItem(rest[1:]); !ok { + return errMalformedParameters + } + } + s = rest + } + return nil +} + +// skipKey skips a key (RFC 9651 §4.2.3.3): a lowercase letter or "*", +// then lowercase letters, digits, "_", "-", "." and "*". +func skipKey(s string) (string, bool) { + if s == "" || !isLCAlpha(s[0]) && s[0] != '*' { + return s, false + } + i := 1 + for i < len(s) && (isLCAlpha(s[i]) || isDigit(s[i]) || strings.IndexByte("_-.*", s[i]) >= 0) { + i++ + } + return s[i:], true +} + +// skipBareItem skips one bare item (RFC 9651 §4.2.3.1), whichever of +// its types s starts with. +func skipBareItem(s string) (string, bool) { + if s == "" { + return s, false + } + switch c := s[0]; { + case c == '-' || isDigit(c): + rest, _, ok := skipNumber(s) + return rest, ok + case c == '"': + return skipString(s[1:]) + case c == '*' || isAlpha(c): + return skipToken(s[1:]), true + case c == ':': + return skipByteSequence(s[1:]) + case c == '?': + return skipBoolean(s[1:]) + case c == '@': + rest, decimal, ok := skipNumber(s[1:]) + return rest, ok && !decimal + case c == '%': + return skipDisplayString(s[1:]) + } + return s, false +} + +// skipNumber skips an Integer or Decimal (RFC 9651 §4.2.4): an optional +// "-", then at most 15 digits, or at most 12 digits, "." and one to +// three digits. decimal reports which. +func skipNumber(s string) (rest string, decimal bool, ok bool) { + i := 0 + if i < len(s) && s[i] == '-' { + i++ + } + start := i + for i < len(s) && isDigit(s[i]) { + i++ + } + whole := i - start + if whole == 0 { + return s, false, false + } + if i == len(s) || s[i] != '.' { + return s[i:], false, whole <= 15 + } + i++ + fracStart := i + for i < len(s) && isDigit(s[i]) { + i++ + } + fraction := i - fracStart + return s[i:], true, whole <= 12 && fraction >= 1 && fraction <= 3 +} + +// skipString skips the rest of a String (RFC 9651 §4.2.5) after its +// opening quote: visible ASCII and spaces, "\"" and "\\" escapes, then +// the closing quote. +func skipString(s string) (string, bool) { + for i := 0; i < len(s); i++ { + switch c := s[i]; { + case c == '"': + return s[i+1:], true + case c == '\\': + i++ + if i == len(s) || s[i] != '"' && s[i] != '\\' { + return s, false + } + case c < 0x20 || c > 0x7e: + return s, false + } + } + return s, false +} + +// skipToken skips the rest of a Token (RFC 9651 §4.2.6) after its first +// character: tchar (RFC 9110 §5.6.2), ":" and "/". +func skipToken(s string) string { + i := 0 + for i < len(s) && (isAlpha(s[i]) || isDigit(s[i]) || strings.IndexByte("!#$%&'*+-.^_`|~:/", s[i]) >= 0) { + i++ + } + return s[i:] +} + +// skipByteSequence skips the rest of a Byte Sequence (RFC 9651 §4.2.7) +// after its opening colon: base64 characters, then the closing colon. +func skipByteSequence(s string) (string, bool) { + end := strings.IndexByte(s, ':') + if end < 0 || strings.IndexFunc(s[:end], func(c rune) bool { return notBase64(c) && c != '=' }) >= 0 { + return s, false + } + return s[end+1:], true +} + +// skipBoolean skips the rest of a Boolean (RFC 9651 §4.2.8) after its +// "?": "0" or "1". +func skipBoolean(s string) (string, bool) { + if s == "" || s[0] != '0' && s[0] != '1' { + return s, false + } + return s[1:], true +} + +// skipDisplayString skips the rest of a Display String (RFC 9651 +// §4.2.10) after its "%": a quote, visible ASCII and spaces with "%" +// escaping two lowercase hex digits, a closing quote, and the decoded +// bytes valid UTF-8. +func skipDisplayString(s string) (string, bool) { + if s == "" || s[0] != '"' { + return s, false + } + var decoded []byte + for i := 1; i < len(s); i++ { + switch c := s[i]; { + case c == '"': + return s[i+1:], utf8.Valid(decoded) + case c == '%': + if i+2 >= len(s) || !isLCHex(s[i+1]) || !isLCHex(s[i+2]) { + return s, false + } + decoded = append(decoded, lcHexValue(s[i+1])<<4|lcHexValue(s[i+2])) + i += 2 + case c < 0x20 || c > 0x7e: + return s, false + default: + decoded = append(decoded, c) + } + } + return s, false +} + +func isDigit(c byte) bool { return '0' <= c && c <= '9' } +func isLCAlpha(c byte) bool { return 'a' <= c && c <= 'z' } +func isAlpha(c byte) bool { return isLCAlpha(c) || 'A' <= c && c <= 'Z' } +func isLCHex(c byte) bool { return isDigit(c) || 'a' <= c && c <= 'f' } + +func lcHexValue(c byte) byte { + if isDigit(c) { + return c - '0' + } + return c - 'a' + 10 +} diff --git a/internal/jose/typ.go b/internal/jose/typ.go index 08895104..73e16c5d 100644 --- a/internal/jose/typ.go +++ b/internal/jose/typ.go @@ -4,6 +4,8 @@ import ( "errors" "slices" "strings" + + "github.com/idfoundry/fapigo/internal/canonical" ) // ErrOtherExplicitType reports a JWT whose "typ" names a different, @@ -15,15 +17,17 @@ var ErrOtherExplicitType = errors.New("jose: header typ names another kind of JW var ErrUnexpectedType = errors.New(`jose: header typ is neither absent, "JWT", nor the expected media type`) // TypeIs reports whether typ names the media type want, comparing as -// RFC 7515 §4.1.9 asks: case-insensitively (RFC 2045), and with -// "application/" implied for a value containing no '/'. So -// "application/at+jwt" and "AT+JWT" are both TypeIs(_, "at+jwt"). +// RFC 7515 §4.1.9 asks: case-insensitively (RFC 2045), in ASCII only, +// and with "application/" implied for a value containing no '/'. So +// "application/at+jwt" and "AT+JWT" are both TypeIs(_, "at+jwt"), but +// a non-ASCII letter that Unicode folds onto an ASCII one (the Kelvin +// sign U+212A onto "k") doesn't match it. func TypeIs(typ, want string) bool { return mediaType(typ) == mediaType(want) } func mediaType(typ string) string { - typ = strings.ToLower(typ) + typ = canonical.LowerASCII(typ) if !strings.Contains(typ, "/") { typ = "application/" + typ } diff --git a/internal/jose/typ_test.go b/internal/jose/typ_test.go index af152a26..29331d2d 100644 --- a/internal/jose/typ_test.go +++ b/internal/jose/typ_test.go @@ -20,6 +20,11 @@ func TestTypeIs(t *testing.T) { {"text/at+jwt", "at+jwt", false}, {"at+jwt ", "at+jwt", false}, {"dpop+jwt", "at+jwt", false}, + // Unicode folds the Kelvin sign U+212A onto "k"; the media type + // comparison is ASCII-only. + {"trust-mar\u212a+jwt", "trust-mark+jwt", false}, + {"jw\u212a-set+jwt", "jwk-set+jwt", false}, + {"TRUST-MARK+JWT", "trust-mark+jwt", true}, } { if got := TypeIs(tc.typ, tc.want); got != tc.is { t.Errorf("TypeIs(%q, %q) = %v, want %v", tc.typ, tc.want, got, tc.is) diff --git a/keys/client_jwks.go b/keys/client_jwks.go index a10f190e..d80d7179 100644 --- a/keys/client_jwks.go +++ b/keys/client_jwks.go @@ -91,8 +91,10 @@ func WithClientJWKSCacheTTL(d time.Duration) ClientJWKSOption { // WithClientJWKSMinRefreshInterval bounds how often a request naming a // kid a client's cached set lacks may force a refetch of that client's -// set, as WithMinRefreshInterval does for JWKSIssuerKeySource. Defaults -// to the cache TTL; a zero or negative duration means that default. +// set, as WithMinRefreshInterval does for JWKSIssuerKeySource: at most +// one forced refetch per interval, counted from the last forced +// refetch. Defaults to min(cache TTL, 1 minute); a zero or negative +// duration means that default. func WithClientJWKSMinRefreshInterval(d time.Duration) ClientJWKSOption { return func(s *ClientJWKSSource) { s.timing.minRefreshInterval = d } } diff --git a/keys/client_jwks_internal_test.go b/keys/client_jwks_internal_test.go index ab4a1597..cfb0858c 100644 --- a/keys/client_jwks_internal_test.go +++ b/keys/client_jwks_internal_test.go @@ -194,32 +194,62 @@ func TestClientJWKSSourceCachesPerClient(t *testing.T) { } // TestClientJWKSSourceRefetchesOnRotation: a kid the cached set lacks -// (the client rotated keys) forces a refetch, once the minimum refresh -// interval has passed, and the new key then resolves. +// (the client rotated keys) forces a refetch at once, however long the +// cache TTL; a second rotation within the minimum refresh interval of +// that forced refetch waits for the interval, then resolves. func TestClientJWKSSourceRefetchesOnRotation(t *testing.T) { doc, _ := es256JWKS(t, "old") src, srv, _ := newURISource(t, map[fapi.ClientID][]byte{"a": doc}, WithClientJWKSCacheTTL(time.Hour), WithClientJWKSMinRefreshInterval(time.Minute)) clock := newManualClock() src.now = clock.Now + assertClientRotations(t, src, srv, clock, time.Minute) +} + +// TestClientJWKSSourceDefaultRefetchesOnRotation: the same with the +// default options, whose 5-minute cache TTL used to rate-limit forced +// refetches out of existence (every TTL refresh reset the limit). +func TestClientJWKSSourceDefaultRefetchesOnRotation(t *testing.T) { + doc, _ := es256JWKS(t, "old") + src, srv, _ := newURISource(t, map[fapi.ClientID][]byte{"a": doc}) + clock := newManualClock() + src.now = clock.Now + assertClientRotations(t, src, srv, clock, defaultMinRefreshInterval) +} +// assertClientRotations drives two key rotations of client "a" against +// src, whose minimum refresh interval is interval. +func assertClientRotations(t *testing.T, src *ClientJWKSSource, srv *jwksServer, clock *manualClock, interval time.Duration) { + t.Helper() if got, err := resolveES256(t, src, "a", "old"); err != nil || len(got) != 1 { t.Fatalf("old kid: got %d keys, err %v", len(got), err) } rotated, pubs := es256JWKS(t, "new") srv.set("/a", rotated) + clock.Advance(time.Second) + assertClientKey(t, src, "new", pubs["new"]) + if n := srv.fetchCount("/a"); n != 2 { + t.Fatalf("fetches after the first rotation = %d, want 2", n) + } - // Within the minimum refresh interval of the first fetch, the miss - // doesn't refetch. - if got, err := resolveES256(t, src, "a", "new"); err != nil || len(got) != 0 { - t.Fatalf("new kid within the interval: got %d keys, err %v; want none", len(got), err) + rotatedAgain, pubsAgain := es256JWKS(t, "newer") + srv.set("/a", rotatedAgain) + clock.Advance(interval / 2) + if got, err := resolveES256(t, src, "a", "newer"); err != nil || len(got) != 0 { + t.Fatalf("newer kid within the interval: got %d keys, err %v; want none", len(got), err) } - clock.Advance(2 * time.Minute) - got, err := resolveES256(t, src, "a", "new") - if err != nil || len(got) != 1 || !got[0].PublicKey.(*ecdsa.PublicKey).Equal(pubs["new"]) { - t.Fatalf("new kid after the interval: got %+v, err %v; want the rotated key", got, err) + clock.Advance(interval / 2) + assertClientKey(t, src, "newer", pubsAgain["newer"]) + if n := srv.fetchCount("/a"); n != 3 { + t.Fatalf("fetches after the second rotation = %d, want 3", n) } - if n := srv.fetchCount("/a"); n != 2 { - t.Fatalf("fetches = %d, want 2", n) +} + +// assertClientKey asserts that kid resolves for client "a" to want. +func assertClientKey(t *testing.T, src *ClientJWKSSource, kid string, want *ecdsa.PublicKey) { + t.Helper() + got, err := resolveES256(t, src, "a", kid) + if err != nil || len(got) != 1 || !got[0].PublicKey.(*ecdsa.PublicKey).Equal(want) { + t.Fatalf("kid %q: got %+v, err %v; want the rotated key", kid, got, err) } } diff --git a/keys/client_static.go b/keys/client_static.go index 2f99e06b..e97bd098 100644 --- a/keys/client_static.go +++ b/keys/client_static.go @@ -36,12 +36,30 @@ type ClientKeyMaterial struct { } // clientKeySet is one client's validated keys: those that verify its -// signatures and those to encrypt to it. +// signatures and those to encrypt to it. thumbprints[i] is the RFC 7638 +// JWK thumbprint of verification[i] when that key has no KeyID, the kid +// it also answers to (see VerificationKey.KeyID), and "" otherwise. type clientKeySet struct { verification []VerificationKey + thumbprints []string encryption []ClientEncryptionKey } +// addVerificationKey appends k, an already validated key, to set, +// with its thumbprint if it has no KeyID. +func (set *clientKeySet) addVerificationKey(k VerificationKey) error { + thumbprint := "" + if k.KeyID == "" { + var err error + if thumbprint, err = keyIDFor("", k.PublicKey, k.Algorithm); err != nil { + return err + } + } + set.verification = append(set.verification, k) + set.thumbprints = append(set.thumbprints, thumbprint) + return nil +} + // StaticClientKeys is a ClientKeySource and ClientEncryptionKeySource // over keys registered up front, client by client — for a deployment // that keeps its clients' public keys (their registered "jwks", or keys @@ -99,7 +117,9 @@ func validateClientKeyMaterial(material ClientKeyMaterial) (clientKeySet, error) if err := jose.ValidateCallerVerificationKey(k.PublicKey, k.Algorithm); err != nil { return clientKeySet{}, fmt.Errorf("verification key %d (kid %q): %w", i, k.KeyID, err) } - set.verification = append(set.verification, k) + if err := set.addVerificationKey(k); err != nil { + return clientKeySet{}, fmt.Errorf("verification key %d: %w", i, err) + } } for i, k := range material.EncryptionKeys { if err := jose.ValidateCallerEncryptionKey(k.PublicKey, k.Algorithm); err != nil { @@ -143,11 +163,12 @@ func (s *StaticClientKeys) Capabilities() KeySourceCapabilities { } // matchVerificationKeys is set's verification keys for alg (and kid, if -// set), in a new slice. +// set), in a new slice. A key registered without a KeyID matches a kid +// that is its RFC 7638 thumbprint, and no other. func matchVerificationKeys(set clientKeySet, alg fapi.SignatureAlgorithm, kid string) []VerificationKey { var matched []VerificationKey - for _, k := range set.verification { - if k.Algorithm == alg && (kid == "" || k.KeyID == kid) { + for i, k := range set.verification { + if k.Algorithm == alg && (kid == "" || k.KeyID == kid || k.KeyID == "" && set.thumbprints[i] == kid) { matched = append(matched, k) } } @@ -185,7 +206,9 @@ func parseClientJWKS(body []byte) (clientKeySet, error) { } var out clientKeySet for _, k := range verification { - out.verification = append(out.verification, VerificationKey{KeyID: k.KeyID, Algorithm: k.Algorithm, PublicKey: k.PublicKey}) + if err := out.addVerificationKey(VerificationKey{KeyID: k.KeyID, Algorithm: k.Algorithm, PublicKey: k.PublicKey}); err != nil { + return clientKeySet{}, err + } } for _, k := range encryption { out.encryption = append(out.encryption, ClientEncryptionKey{KeyID: k.KeyID, Algorithm: k.Algorithm, PublicKey: k.PublicKey}) diff --git a/keys/client_static_test.go b/keys/client_static_test.go index b7ed8446..9b350ed6 100644 --- a/keys/client_static_test.go +++ b/keys/client_static_test.go @@ -8,6 +8,7 @@ import ( "crypto/elliptic" "crypto/rand" "crypto/rsa" + "encoding/base64" "math/big" "strings" "testing" @@ -77,6 +78,55 @@ func TestStaticClientKeysServesKeysGivenDirectly(t *testing.T) { } } +// TestStaticClientKeysMatchesThumbprintOfKeyWithoutKeyID: a key +// registered without a KeyID matches a request naming its RFC 7638 +// thumbprint, the kid NewKeyManagerFromSigners derives for the same key +// given without one, so the two sides interoperate. It matches no other +// kid, and a key with a KeyID still matches only that kid. +func TestStaticClientKeysMatchesThumbprintOfKeyWithoutKeyID(t *testing.T) { + rp, other := newP256(t), newP256(t) + km, err := keys.NewKeyManagerFromSigners([]keys.SignerSpec{{Purpose: keys.ClientAuthentication, Algorithm: fapi.ES256, Signer: rp}}) + if err != nil { + t.Fatalf("NewKeyManagerFromSigners: %v", err) + } + signing, err := km.PublicKey(context.Background(), keys.ClientAuthentication, fapi.ES256) + if err != nil || signing.KeyID == "" { + t.Fatalf("PublicKey: %+v, %v; want a derived kid", signing, err) + } + point, err := rp.PublicKey.Bytes() // 0x04 || X || Y + if err != nil { + t.Fatalf("Bytes: %v", err) + } + src, err := keys.NewStaticClientKeys(map[fapi.ClientID]keys.ClientKeyMaterial{ + "rp": {VerificationKeys: []keys.VerificationKey{{Algorithm: fapi.ES256, PublicKey: &rp.PublicKey}}}, + "named": {VerificationKeys: []keys.VerificationKey{{KeyID: "named", Algorithm: fapi.ES256, PublicKey: &rp.PublicKey}}}, + "other": {VerificationKeys: []keys.VerificationKey{{Algorithm: fapi.ES256, PublicKey: &other.PublicKey}}}, + "jwks": {JWKS: []byte(`{"keys":[{"kty":"EC","crv":"P-256","alg":"ES256","use":"sig","x":"` + base64.RawURLEncoding.EncodeToString(point[1:33]) + `","y":"` + base64.RawURLEncoding.EncodeToString(point[33:]) + `"}]}`)}, + }) + if err != nil { + t.Fatalf("NewStaticClientKeys: %v", err) + } + for _, tc := range []struct { + client fapi.ClientID + kid string + want int + }{ + {"rp", signing.KeyID, 1}, + {"rp", "", 1}, + {"rp", "some-other-kid", 0}, + {"named", signing.KeyID, 0}, + {"named", "named", 1}, + {"other", signing.KeyID, 0}, + {"jwks", signing.KeyID, 1}, + {"jwks", "some-other-kid", 0}, + } { + set, err := src.ResolveVerificationKeys(context.Background(), keys.ClientKeyRequest{ClientID: tc.client, Purpose: keys.ClientAssertionVerification, Algorithm: fapi.ES256, KeyID: tc.kid}) + if err != nil || len(set.Keys) != tc.want { + t.Errorf("client %q, kid %q: got %d keys, err %v; want %d", tc.client, tc.kid, len(set.Keys), err, tc.want) + } + } +} + func TestStaticClientKeysResolvesEncryptionKeys(t *testing.T) { priv, err := ecdh.P256().GenerateKey(rand.Reader) if err != nil { diff --git a/keys/ephemeral/clientkeysource.go b/keys/ephemeral/clientkeysource.go index 2fc3214a..7a69104f 100644 --- a/keys/ephemeral/clientkeysource.go +++ b/keys/ephemeral/clientkeysource.go @@ -19,6 +19,10 @@ import ( // WithCacheTTL. const defaultClientJWKSCacheTTL = 5 * time.Minute +// defaultMinRefreshInterval caps how often, by default, a request naming +// a kid a client's cached JWKS lacks may force a refetch of it. +const defaultMinRefreshInterval = time.Minute + // ClientKeySpec is one registered client's key material — // either an inline static JWKS or a JWKS URI to fetch live. Deliberately // carries only what ClientKeySource needs (not a storage.RegisteredClient @@ -47,11 +51,13 @@ func WithCacheTTL(d time.Duration) Option { // WithMinRefreshInterval bounds how often a request naming a kid the // cached JWKS doesn't have may force a live refetch of that client's -// JWKS. Defaults to the cache TTL, as keys.JWKSIssuerKeySource's own -// option does: a rotated key is still picked up within one TTL, while a -// caller sending distinct unknown kids (taken from an unverified JWT -// header, so attacker-controlled) forces at most one fetch per client -// per interval. A zero or negative duration means that default. +// JWKS. Defaults to min(cache TTL, 1 minute), as +// keys.JWKSIssuerKeySource's own option does: a rotated key is picked up +// within one interval of its first use, while a caller sending distinct +// unknown kids (taken from an unverified JWT header, so +// attacker-controlled) forces at most one fetch per client per +// interval. Only forced refetches count against it, not the TTL ones. A +// zero or negative duration means that default. func WithMinRefreshInterval(d time.Duration) Option { return func(s *ClientKeySource) { s.minRefreshInterval = d } } @@ -82,6 +88,7 @@ type clientKeyEntry struct { cached *clientKeys cachedAt time.Time lastAttempt time.Time // the last fetch, successful or not + lastForced time.Time // the last fetch made while the cached set was fresh lastErr error // the last fetch's error, nil if it succeeded // fetchMu serializes fetches, so concurrent callers that all need @@ -133,7 +140,7 @@ func NewClientKeySource(fetcher *fapihttp.Client, specs []ClientKeySpec, opts .. return nil, fmt.Errorf("ephemeral: cache TTL must be positive") } if s.minRefreshInterval <= 0 { - s.minRefreshInterval = s.cacheTTL + s.minRefreshInterval = min(s.cacheTTL, defaultMinRefreshInterval) } for _, spec := range specs { if spec.ClientID == "" { @@ -235,21 +242,23 @@ func (s *ClientKeySource) ResolveEncryptionKeys(ctx context.Context, req keys.Cl // while fresh, refetched once stale, with the same stale-key handling // keys.JWKSIssuerKeySource applies: a fresh set without wantKeyID (a // client that may have rotated keys since the last fetch) is refetched -// too, but at most once per minRefreshInterval, since wantKeyID comes -// from an unverified JWT header. Fetches are serialized per client, and +// too, but at most once per minRefreshInterval of the last such forced +// refetch, since wantKeyID comes from an unverified JWT header, and not +// if the set was fetched since the request arrived. Fetches are serialized per client, and // a failed one isn't retried for refreshBackoff — its error is // returned instead, or the cached set while it is still fresh. func (s *ClientKeySource) current(ctx context.Context, entry *clientKeyEntry, wantKeyID string, kids func(clientKeys) []string) (clientKeys, error) { if entry.jwksURL == nil { return entry.static, nil } - if keys, done, err := s.cachedOrRateLimited(entry, wantKeyID, kids); done { + arrived := s.now() + if keys, done, err := s.cachedOrRateLimited(entry, wantKeyID, kids, arrived); done { return keys, err } entry.fetchMu.Lock() defer entry.fetchMu.Unlock() // Another caller may have fetched while this one waited. - if keys, done, err := s.cachedOrRateLimited(entry, wantKeyID, kids); done { + if keys, done, err := s.cachedOrRateLimited(entry, wantKeyID, kids, arrived); done { return keys, err } return s.refetch(ctx, entry) @@ -258,9 +267,11 @@ func (s *ClientKeySource) current(ctx context.Context, entry *clientKeyEntry, wa // cachedOrRateLimited reports (done=true) the answer current gives // without fetching: the cached set when it is fresh and has wantKeyID // (or none was asked for), or when a fetch for a missing kid isn't -// allowed yet; the last fetch's error while it is within +// allowed yet (the set was fetched since the request arrived at +// arrived, or the last forced refetch was within minRefreshInterval); +// the last fetch's error while it is within // refreshBackoff. done=false means current should fetch now. -func (s *ClientKeySource) cachedOrRateLimited(entry *clientKeyEntry, wantKeyID string, kids func(clientKeys) []string) (clientKeys, bool, error) { +func (s *ClientKeySource) cachedOrRateLimited(entry *clientKeyEntry, wantKeyID string, kids func(clientKeys) []string, arrived time.Time) (clientKeys, bool, error) { entry.mu.Lock() defer entry.mu.Unlock() now := s.now() @@ -275,7 +286,7 @@ func (s *ClientKeySource) cachedOrRateLimited(entry *clientKeyEntry, wantKeyID s } return clientKeys{}, true, entry.lastErr } - if fresh && sinceAttempt < s.minRefreshInterval { + if fresh && (!entry.cachedAt.Before(arrived) || now.Sub(entry.lastForced) < s.minRefreshInterval) { return *entry.cached, true, nil } return clientKeys{}, false, nil @@ -293,6 +304,9 @@ func (s *ClientKeySource) refetch(ctx context.Context, entry *clientKeyEntry) (c entry.mu.Lock() defer entry.mu.Unlock() entry.lastAttempt = s.now() + if entry.cached != nil && entry.lastAttempt.Sub(entry.cachedAt) < s.cacheTTL { + entry.lastForced = entry.lastAttempt + } entry.lastErr = err if err != nil { return clientKeys{}, err diff --git a/keys/ephemeral/clientkeysource_refresh_test.go b/keys/ephemeral/clientkeysource_refresh_test.go index 372c1522..cde22b11 100644 --- a/keys/ephemeral/clientkeysource_refresh_test.go +++ b/keys/ephemeral/clientkeysource_refresh_test.go @@ -73,7 +73,7 @@ func (h refreshHarness) resolve(kid string) (keys.VerificationKeySet, error) { // TestClientKeySourceBoundsUnknownKeyIDRefetches: a kid is taken from an // unverified JWT header, so a stream of distinct unknown kids within -// the minimum refresh interval forces no fetch beyond the first. +// the minimum refresh interval forces one refetch, and no more. func TestClientKeySourceBoundsUnknownKeyIDRefetches(t *testing.T) { h := newRefreshHarness(t) if _, err := h.resolve("kid-1"); err != nil { @@ -86,8 +86,8 @@ func TestClientKeySourceBoundsUnknownKeyIDRefetches(t *testing.T) { t.Fatalf("resolve(unknown kid) = %+v, %v; want no keys, no error", set.Keys, err) } } - if got := h.fetches.Load(); got != 1 { - t.Fatalf("fetches = %d after 50 unknown kids within the interval, want 1", got) + if got := h.fetches.Load(); got != 2 { + t.Fatalf("fetches = %d after 50 unknown kids within the interval, want 2 (the first lookup plus one forced refetch)", got) } } @@ -169,27 +169,30 @@ func TestNewClientKeySourceRejectsNonPositiveCacheTTL(t *testing.T) { } } -// TestClientKeySourceMinRefreshIntervalDefaultsToCacheTTL: without -// WithMinRefreshInterval, an unknown kid forces at most one refetch per -// cache TTL, as keys.JWKSIssuerKeySource's own default does. -func TestClientKeySourceMinRefreshIntervalDefaultsToCacheTTL(t *testing.T) { - h := newRefreshHarness(t, WithCacheTTL(time.Minute)) - if _, err := h.resolve("kid-1"); err != nil { - t.Fatalf("resolve(kid-1): %v", err) - } - *h.now = h.now.Add(59 * time.Second) - if _, err := h.resolve("unknown"); err != nil { - t.Fatalf("resolve(unknown): %v", err) - } - if got := h.fetches.Load(); got != 1 { - t.Fatalf("fetches = %d for an unknown kid within the cache TTL, want 1", got) - } - *h.now = h.now.Add(2 * time.Second) - if _, err := h.resolve("unknown"); err != nil { - t.Fatalf("resolve(unknown): %v", err) - } - if got := h.fetches.Load(); got != 2 { - t.Fatalf("fetches = %d once the cache TTL has passed, want 2", got) +// TestClientKeySourceMinRefreshIntervalDefault: without +// WithMinRefreshInterval, an unknown kid forces a refetch at once, then +// at most one more per minute however long the cache TTL, as +// keys.JWKSIssuerKeySource's own default does. +func TestClientKeySourceMinRefreshIntervalDefault(t *testing.T) { + h := newRefreshHarness(t, WithCacheTTL(time.Hour)) + steps := []struct { + advance time.Duration + kid string + fetches int32 + }{ + {0, "kid-1", 1}, + {time.Second, "unknown-1", 2}, + {59 * time.Second, "unknown-2", 2}, + {time.Second, "unknown-3", 3}, + } + for _, step := range steps { + *h.now = h.now.Add(step.advance) + if _, err := h.resolve(step.kid); err != nil { + t.Fatalf("resolve(%s): %v", step.kid, err) + } + if got := h.fetches.Load(); got != step.fetches { + t.Fatalf("fetches = %d after resolve(%s), want %d", got, step.kid, step.fetches) + } } } diff --git a/keys/ephemeral/ephemeral_test.go b/keys/ephemeral/ephemeral_test.go index 2272fd6a..b0f63f74 100644 --- a/keys/ephemeral/ephemeral_test.go +++ b/keys/ephemeral/ephemeral_test.go @@ -264,8 +264,9 @@ func TestClientKeySourceCachesWithinTTL(t *testing.T) { // TestClientKeySourceRefetchesOnUnknownKeyIDEvenWhenCacheFresh covers // current's stale-key handling: a client that has rotated keys since the // last fetch isn't stuck with a cached set that lacks the newly -// requested kid until the TTL expires — it is refetched once the minimum -// refresh interval has passed, and not before. +// requested kid until the TTL expires — it is refetched at once, and a +// further unknown kid forces another refetch only once the minimum +// refresh interval has passed. func TestClientKeySourceRefetchesOnUnknownKeyIDEvenWhenCacheFresh(t *testing.T) { priv1, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) if err != nil { @@ -321,28 +322,33 @@ func TestClientKeySourceRefetchesOnUnknownKeyIDEvenWhenCacheFresh(t *testing.T) mu.Lock() rotated = true mu.Unlock() - resolveKid2 := func() keys.VerificationKeySet { + resolve := func(kid string) keys.VerificationKeySet { t.Helper() set, err := src.ResolveVerificationKeys(context.Background(), keys.ClientKeyRequest{ - ClientID: "client-1", Algorithm: fapi.ES256, KeyID: "kid-2", + ClientID: "client-1", Algorithm: fapi.ES256, KeyID: kid, }) if err != nil { - t.Fatalf("ResolveVerificationKeys(kid-2): %v", err) + t.Fatalf("ResolveVerificationKeys(%s): %v", kid, err) } return set } + // The rotated-in kid forces a refetch at once, well within the TTL. + now = now.Add(time.Second) + if set := resolve("kid-2"); len(set.Keys) != 1 || set.Keys[0].KeyID != "kid-2" || fetches != 2 { + t.Fatalf("rotated kid: keys = %+v, fetches = %d; want kid-2 and 2 (rotation should force a refetch)", set.Keys, fetches) + } + + // Another unknown kid within the minimum refresh interval of that + // forced refetch doesn't force another. now = now.Add(5 * time.Second) - if set := resolveKid2(); len(set.Keys) != 0 || fetches != 1 { - t.Fatalf("within the minimum refresh interval: keys = %+v, fetches = %d; want none and 1", set.Keys, fetches) + if set := resolve("kid-3"); len(set.Keys) != 0 || fetches != 2 { + t.Fatalf("within the minimum refresh interval: keys = %+v, fetches = %d; want none and 2", set.Keys, fetches) } now = now.Add(6 * time.Second) - if set := resolveKid2(); len(set.Keys) != 1 || set.Keys[0].KeyID != "kid-2" { - t.Fatalf("Keys = %+v, want one key with kid-2 (rotation should force a refetch)", set.Keys) - } - if fetches != 2 { - t.Fatalf("fetches = %d, want 2 (an unknown kid forces a refetch within TTL once the interval has passed)", fetches) + if set := resolve("kid-3"); len(set.Keys) != 0 || fetches != 3 { + t.Fatalf("after the interval: keys = %+v, fetches = %d; want none and 3 (an unknown kid forces a refetch once the interval has passed)", set.Keys, fetches) } } diff --git a/keys/jwkscache.go b/keys/jwkscache.go index 8342e523..0be48cdb 100644 --- a/keys/jwkscache.go +++ b/keys/jwkscache.go @@ -15,7 +15,12 @@ import ( // refetch, at most once per minRefreshInterval, with a miss for that // kid negative-cached for as long, since the kid comes from an // unverified JWT header and so is attacker-controlled. Concurrent -// fetches are single-flighted. +// fetches are single-flighted. The forced-refetch rate limit counts +// only forced refetches, never the plain TTL refreshes, so a rotated-in +// kid is picked up within minRefreshInterval of its first use whatever +// the TTL (OIDC Core §10.1.1: "The verifier knows to go back to the +// jwks_uri location to re-retrieve the keys when it sees an unfamiliar +// kid value"). // // It is shared by JWKSIssuerKeySource (one cache per source) and // ClientJWKSSource (one per client), and stays unexported: each source @@ -32,7 +37,7 @@ type jwksCache[V any] struct { cached V haveCached bool cachedAt time.Time - lastAttempt time.Time + lastForced time.Time lastRefreshErr error lastRefreshErrAt time.Time inflight *refreshCall[V] @@ -55,6 +60,13 @@ type refreshCall[V any] struct { // occasional extra refresh, which minRefreshInterval already bounds. const maxNegativeKIDs = 1024 +// defaultMinRefreshInterval is how often, by default, an unknown kid +// may force a refetch of a JWK Set: short enough that a signer's key +// rotation is picked up within a minute of the new kid's first use, +// long enough that a flood of random kids costs the JWKS endpoint at +// most one extra fetch a minute per set. +const defaultMinRefreshInterval = time.Minute + // jwksCacheTiming is a jwksCache's three intervals. type jwksCacheTiming struct { cacheTTL time.Duration @@ -63,10 +75,11 @@ type jwksCacheTiming struct { } // withDefaults fills t's unset intervals: minRefreshInterval defaults to -// cacheTTL, refreshBackoff to min(cacheTTL, 5s). +// min(cacheTTL, defaultMinRefreshInterval), refreshBackoff to +// min(cacheTTL, 5s). func (t jwksCacheTiming) withDefaults() jwksCacheTiming { if t.minRefreshInterval <= 0 { - t.minRefreshInterval = t.cacheTTL + t.minRefreshInterval = min(t.cacheTTL, defaultMinRefreshInterval) } if t.refreshBackoff <= 0 { t.refreshBackoff = min(t.cacheTTL, 5*time.Second) @@ -89,6 +102,7 @@ func newJWKSCache[V any](timing jwksCacheTiming, now func() time.Time, fetch fun // is set and matches nothing, it forces one refresh (rate-limited and // single-flighted, see jwksCache) before giving up with an empty result. func resolveFromJWKSCache[V, K any](ctx context.Context, c *jwksCache[V], kid string, match func(V) []K) ([]K, error) { + arrived := c.now() current, err := c.current(ctx) if err != nil { return nil, err @@ -97,7 +111,7 @@ func resolveFromJWKSCache[V, K any](ctx context.Context, c *jwksCache[V], kid st if len(matched) > 0 || kid == "" { return matched, nil } - if !c.shouldForceRefresh(kid) { + if !c.reserveForcedRefresh(kid, arrived) { return nil, nil } fresh, err := c.refreshSingleflight(ctx) @@ -135,20 +149,30 @@ func (c *jwksCache[V]) current(ctx context.Context) (V, error) { return c.refreshSingleflight(ctx) } -// shouldForceRefresh reports whether an unknown-kid miss on kid may -// force a live refresh right now: it may not if kid is still -// negative-cached, or if the last refresh attempt was within -// minRefreshInterval. -func (c *jwksCache[V]) shouldForceRefresh(kid string) bool { +// reserveForcedRefresh reports whether an unknown-kid miss on kid, in +// a request that arrived at arrived, may force a live refresh right +// now, and if so records it as the last forced refresh. It may not if +// kid is still negative-cached, or if the last forced refresh was +// within minRefreshInterval. A plain TTL refresh doesn't count against +// that limit (if it did, a minRefreshInterval as long as cacheTTL would +// let a kid miss force nothing at all), but a set fetched since the +// request arrived is already as fresh as a forced refresh would make +// it, so a miss on it is negative-cached instead of refetched. +func (c *jwksCache[V]) reserveForcedRefresh(kid string, arrived time.Time) bool { c.mu.Lock() defer c.mu.Unlock() now := c.now() if retryAt, ok := c.negativeKIDs[kid]; ok && now.Before(retryAt) { return false } - if !c.lastAttempt.IsZero() && now.Sub(c.lastAttempt) < c.minRefreshInterval { + if c.haveCached && !c.cachedAt.Before(arrived) { + c.rememberMissLocked(kid, now) + return false + } + if !c.lastForced.IsZero() && now.Sub(c.lastForced) < c.minRefreshInterval { return false } + c.lastForced = now return true } @@ -159,7 +183,11 @@ func (c *jwksCache[V]) shouldForceRefresh(kid string) bool { func (c *jwksCache[V]) rememberMiss(kid string) { c.mu.Lock() defer c.mu.Unlock() - now := c.now() + c.rememberMissLocked(kid, c.now()) +} + +// rememberMissLocked is rememberMiss with c.mu held. +func (c *jwksCache[V]) rememberMissLocked(kid string, now time.Time) { if len(c.negativeKIDs) >= maxNegativeKIDs { for k, retryAt := range c.negativeKIDs { if !now.Before(retryAt) { @@ -185,7 +213,6 @@ func (c *jwksCache[V]) refreshSingleflight(ctx context.Context) (V, error) { } call := &refreshCall[V]{done: make(chan struct{})} c.inflight = call - c.lastAttempt = c.now() c.mu.Unlock() go func() { diff --git a/keys/jwkscache_internal_test.go b/keys/jwkscache_internal_test.go index f2e7d0f6..8cea8b69 100644 --- a/keys/jwkscache_internal_test.go +++ b/keys/jwkscache_internal_test.go @@ -21,7 +21,7 @@ func TestJWKSCacheBoundsNegativeKeyIDs(t *testing.T) { if n := len(c.negativeKIDs); n != maxNegativeKIDs { t.Fatalf("negative cache holds %d kids, want %d", n, maxNegativeKIDs) } - if c.shouldForceRefresh("kid-0") { + if c.reserveForcedRefresh("kid-0", clock.Now()) { t.Fatalf("a negative-cached kid may force a refresh") } clock.Advance(2 * time.Hour) diff --git a/keys/jwksissuer.go b/keys/jwksissuer.go index 0ceafdb5..2f287759 100644 --- a/keys/jwksissuer.go +++ b/keys/jwksissuer.go @@ -47,12 +47,13 @@ type JWKSIssuerKeySource struct { type JWKSOption func(*JWKSIssuerKeySource) // WithMinRefreshInterval bounds how often an unknown-kid miss may force -// a live refetch, independent of CacheTTL. Defaults to CacheTTL, which -// means an unknown kid can force at most one extra fetch per TTL -// window — a real key rotation is still picked up within one TTL, the -// same worst case as plain TTL expiry, while an attacker sending -// requests with distinct unknown kids cannot force more than one -// upstream fetch per interval. +// a live refetch, independent of CacheTTL: at most one forced refetch +// per interval, counted from the last forced refetch (the plain TTL +// refreshes don't count). Defaults to min(CacheTTL, 1 minute), so a +// real key rotation is picked up within a minute of the new kid's first +// use (OIDC Core §10.1.1), while an attacker sending requests with +// distinct unknown kids cannot force more than one upstream fetch per +// interval. A zero or negative duration means that default. func WithMinRefreshInterval(d time.Duration) JWKSOption { return func(s *JWKSIssuerKeySource) { s.timing.minRefreshInterval = d } } diff --git a/keys/jwksissuer_internal_test.go b/keys/jwksissuer_internal_test.go index fbf26d1f..9bfbd2f7 100644 --- a/keys/jwksissuer_internal_test.go +++ b/keys/jwksissuer_internal_test.go @@ -87,16 +87,17 @@ func TestUnknownKIDBurstCoalescesToSingleFetch(t *testing.T) { if err != nil { t.Fatalf("NewJWKSIssuerKeySource: %v", err) } + // A stopped clock: every call arrives no later than the fetch. + src.now = newManualClock().Now // No priming: the cache starts empty, so every one of these // concurrent, distinct-unknown-kid calls independently discovers a - // cache miss and races to populate it via currentKeys' own + // cache miss and races to populate it via current's own // refreshSingleflight call — this is the single-flight coalescing - // this test exercises. Once that first fetch completes, each call's - // own forced-refresh attempt (the kid still won't match) is in turn - // suppressed by minRefreshInterval, since it was just satisfied by - // the same fetch a moment ago — so the total across all 50 must - // still be exactly one fetch. + // this test exercises. Once that first fetch completes, no call + // forces another: the set each one misses in was fetched since it + // arrived — so the total across all 50 must still be exactly one + // fetch. const burst = 50 var wg sync.WaitGroup wg.Add(burst) @@ -139,39 +140,37 @@ func TestUnknownKIDRateLimited(t *testing.T) { src.now = clock.Now // No priming: the cache starts empty, so this first unknown-kid - // lookup does the initial population fetch itself (via - // currentKeys), which also stamps lastAttempt. - if _, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "unknown-1", - }); err != nil { - t.Fatalf("ResolveIssuerKeys(unknown-1): %v", err) - } - if got := atomic.LoadInt32(&fetches); got != 1 { - t.Fatalf("fetches after first miss = %d, want 1", got) - } + // lookup does the initial population fetch itself (via current); + // the set it misses in was fetched for it, so no forced refresh. + assertFetchesAfterMiss(t, src, "unknown-1", &fetches, 1) - // A second, different unknown kid within minRefreshInterval must not - // force another fetch. + // Another unknown kid, on a set fetched before it arrived, forces + // one refetch: a plain fetch doesn't count against the limit. clock.Advance(30 * time.Second) - if _, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "unknown-2", - }); err != nil { - t.Fatalf("ResolveIssuerKeys(unknown-2): %v", err) - } - if got := atomic.LoadInt32(&fetches); got != 1 { - t.Fatalf("fetches after second miss within interval = %d, want 1 (rate limit not enforced)", got) - } + assertFetchesAfterMiss(t, src, "unknown-2", &fetches, 2) + + // A third, different unknown kid within minRefreshInterval of that + // forced refetch must not force another. + clock.Advance(59 * time.Second) + assertFetchesAfterMiss(t, src, "unknown-3", &fetches, 2) // Once minRefreshInterval has elapsed, a further unknown kid is // allowed to force a new fetch. - clock.Advance(31 * time.Second) + clock.Advance(time.Second) + assertFetchesAfterMiss(t, src, "unknown-4", &fetches, 3) +} + +// assertFetchesAfterMiss resolves the unknown kid against src and +// asserts that the upstream has then been fetched want times. +func assertFetchesAfterMiss(t *testing.T, src *JWKSIssuerKeySource, kid string, fetches *int32, want int32) { + t.Helper() if _, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "unknown-3", + Algorithm: fapi.ES256, KeyID: kid, }); err != nil { - t.Fatalf("ResolveIssuerKeys(unknown-3): %v", err) + t.Fatalf("ResolveIssuerKeys(%s): %v", kid, err) } - if got := atomic.LoadInt32(&fetches); got != 2 { - t.Fatalf("fetches after interval elapsed = %d, want 2", got) + if got := atomic.LoadInt32(fetches); got != want { + t.Fatalf("fetches after a miss on %s = %d, want %d", kid, got, want) } } @@ -207,15 +206,34 @@ func TestNegativeCacheSuppressesRefetch(t *testing.T) { } } +// TestRotationStillResolvesAfterInterval: a rotated-in kid forces a +// refetch at once, however long the cache TTL; a second rotation within +// the minimum refresh interval of that forced refetch waits for the +// interval, then resolves. func TestRotationStillResolvesAfterInterval(t *testing.T) { - var newKID atomic.Bool + assertIssuerRotations(t, time.Hour, time.Minute, WithMinRefreshInterval(time.Minute)) +} + +// TestRotationResolvesWithDefaultOptions: the same with no options, +// where the minimum refresh interval used to default to the cache TTL +// and every TTL refresh reset it, so an unknown kid never forced a +// refetch (OIDC Core §10.1.1). +func TestRotationResolvesWithDefaultOptions(t *testing.T) { + assertIssuerRotations(t, 15*time.Minute, defaultMinRefreshInterval) +} + +// assertIssuerRotations drives two key rotations against a +// JWKSIssuerKeySource built with cacheTTL and opts, whose minimum +// refresh interval is interval. +func assertIssuerRotations(t *testing.T, cacheTTL, interval time.Duration, opts ...JWKSOption) { + t.Helper() + var kid atomic.Value + kid.Store("kid-original") + var fetches atomic.Int32 ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fetches.Add(1) w.Header().Set("Content-Type", "application/json") - if newKID.Load() { - fmt.Fprint(w, testP256JWK(t, "kid-rotated")) - } else { - fmt.Fprint(w, testP256JWK(t, "kid-original")) - } + fmt.Fprint(w, testP256JWK(t, kid.Load().(string))) })) defer ts.Close() @@ -224,42 +242,39 @@ func TestRotationStillResolvesAfterInterval(t *testing.T) { t.Fatalf("ParseEndpointURL: %v", err) } clock := newManualClock() - src, err := NewJWKSIssuerKeySource(testFetcher(t, ts), jwksURI, time.Hour, WithMinRefreshInterval(time.Minute)) + src, err := NewJWKSIssuerKeySource(testFetcher(t, ts), jwksURI, cacheTTL, opts...) if err != nil { t.Fatalf("NewJWKSIssuerKeySource: %v", err) } src.now = clock.Now - if _, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "kid-original", - }); err != nil { - t.Fatalf("prime: %v", err) + assertIssuerKeyCount(t, src, "kid-original", 1) + kid.Store("kid-rotated") + clock.Advance(time.Second) + assertIssuerKeyCount(t, src, "kid-rotated", 1) + if n := fetches.Load(); n != 2 { + t.Fatalf("fetches after the first rotation = %d, want 2", n) } - // The priming resolve above already stamped lastAttempt at clock's - // current value, so this immediate query for the (now rotated) kid - // is still within minRefreshInterval and must not force a refresh. - newKID.Store(true) - set, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "kid-rotated", - }) - if err != nil { - t.Fatalf("ResolveIssuerKeys(kid-rotated, within interval): %v", err) - } - if len(set.Keys) != 0 { - t.Fatalf("Keys = %+v, want none yet (forced refresh should still be rate-limited)", set.Keys) + kid.Store("kid-rotated-again") + clock.Advance(interval / 2) + assertIssuerKeyCount(t, src, "kid-rotated-again", 0) + clock.Advance(interval / 2) + assertIssuerKeyCount(t, src, "kid-rotated-again", 1) + if n := fetches.Load(); n != 3 { + t.Fatalf("fetches after the second rotation = %d, want 3", n) } +} - // After minRefreshInterval elapses, the same kid resolves. - clock.Advance(2 * time.Minute) - set, err = src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "kid-rotated", - }) +// assertIssuerKeyCount asserts that src resolves want ES256 keys for kid. +func assertIssuerKeyCount(t *testing.T, src *JWKSIssuerKeySource, kid string, want int) { + t.Helper() + set, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{Algorithm: fapi.ES256, KeyID: kid}) if err != nil { - t.Fatalf("ResolveIssuerKeys(kid-rotated, after interval): %v", err) + t.Fatalf("ResolveIssuerKeys(%s): %v", kid, err) } - if len(set.Keys) != 1 || set.Keys[0].KeyID != "kid-rotated" { - t.Fatalf("Keys = %+v, want one key with kid-rotated", set.Keys) + if len(set.Keys) != want { + t.Fatalf("ResolveIssuerKeys(%s) = %+v, want %d keys", kid, set.Keys, want) } } diff --git a/keys/verification.go b/keys/verification.go index b60d9199..27d2eddf 100644 --- a/keys/verification.go +++ b/keys/verification.go @@ -48,6 +48,12 @@ type ClientKeyRequest struct { // type, so an external implementation of ClientKeySource never needs to // depend on this module's internal JWK representation. type VerificationKey struct { + // KeyID is the key's "kid". A request naming no kid matches any + // key of its algorithm; one naming a kid matches a key whose KeyID + // is that kid. A key given without one (to NewStaticClientKeys, or + // in a client's JWK Set) matches a request naming its RFC 7638 JWK + // thumbprint, the kid NewKeyManagerFromSigners and SignerSpec derive + // for a key given without one, and no other kid. KeyID string Algorithm fapi.SignatureAlgorithm PublicKey crypto.PublicKey diff --git a/server/config.go b/server/config.go index dfd56cc5..947e193e 100644 --- a/server/config.go +++ b/server/config.go @@ -404,7 +404,9 @@ type Config struct { // Issuer is this server's issuer identifier — the audience client // assertions and request objects must be addressed to, and every // access token's audience unless the token is for resources (RFC - // 8707 resource indicators; see Resources). + // 8707 resource indicators; see Resources). 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 Endpoints Endpoints diff --git a/server/issuer_query_test.go b/server/issuer_query_test.go new file mode 100644 index 00000000..8ecccc68 --- /dev/null +++ b/server/issuer_query_test.go @@ -0,0 +1,38 @@ +package server_test + +import ( + "strings" + "testing" + + fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/server" +) + +// 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) { + prodCfg, prodDeps := productionConfigAndDeps(t) + configs := map[string]struct { + cfg server.Config + deps server.Dependencies + }{ + "development": {validConfig(t), validDependencies()}, + "production": {prodCfg, prodDeps}, + } + for level, c := range configs { + for _, suffix := range []string{"?tenant=a", "?"} { + t.Run(level+" "+suffix, func(t *testing.T) { + cfg := c.cfg + issuer, err := fapi.ParseEndpointURL(cfg.Issuer.String() + suffix) + if err != nil { + t.Fatalf("ParseEndpointURL: %v", err) + } + cfg.Issuer = issuer + if _, err := server.New(cfg, c.deps); err == nil || !strings.Contains(err.Error(), "query") { + t.Fatalf("New(issuer %q) = %v, want the query refused", issuer, err) + } + }) + } + } +} diff --git a/server/server.go b/server/server.go index c1e27aee..7f729a49 100644 --- a/server/server.go +++ b/server/server.go @@ -173,6 +173,9 @@ func validateRequiredEndpoints(cfg Config) error { if cfg.Issuer.IsZero() { return fmt.Errorf("server: config: issuer is required") } + if u := cfg.Issuer.URL(); u.RawQuery != "" || u.ForceQuery { + return fmt.Errorf("server: config: issuer must not contain a query (RFC 8414 §2): parse it with fapi.ParseIssuerURL") + } if cfg.Endpoints.Token.IsZero() { return fmt.Errorf("server: config: endpoints.token is required") } diff --git a/storage/client_repository.go b/storage/client_repository.go index 870123e6..dd072ed7 100644 --- a/storage/client_repository.go +++ b/storage/client_repository.go @@ -249,7 +249,8 @@ type RegisteredClientConfig struct { // kept byte for byte and the authorization response's parameters // added after it (RFC 6749 §3.1.2), so a query naming one of them — // code, state, iss, error, error_description, error_uri or - // response — is refused. + // response — is refused, as is a URI with a fragment, even an empty + // "#" (RFC 6749 §3.1.2). RedirectURIs []fapi.RegisteredRedirectURI // ApplicationType is ApplicationTypeWeb (the zero value) or @@ -944,6 +945,19 @@ func (c RegisteredClient) ApplicationType() ApplicationType { return c.applicati // iss, and JARM's response. var authorizationResponseParameters = []string{"code", "state", "iss", "error", "error_description", "error_uri", "response"} +// checkRedirectURIFragments refuses a redirect URI with a fragment +// component, even an empty one ("https://rp.example/cb#"): RFC 6749 +// §3.1.2 says "The endpoint URI MUST NOT include a fragment component", +// and RFC 3986 §3.5 makes everything after the first "#" one. +func checkRedirectURIFragments(cfg RegisteredClientConfig) error { + for _, uri := range cfg.RedirectURIs { + if strings.Contains(string(uri), "#") { + return fmt.Errorf("storage: client %q: redirect URI %q has a fragment, which a redirect URI must not (RFC 6749 §3.1.2)", cfg.ID, uri) + } + } + return nil +} + // checkRedirectURIQueries refuses a redirect URI whose query names an // authorization response parameter (authorizationResponseParameters). // RFC 6749 §3.1.2 has the server retain the registered query and add @@ -1033,7 +1047,7 @@ func withoutLoopbackPort(uri string) (string, bool) { return "", false } u, err := url.Parse(uri) - if err != nil || u.User != nil || u.Fragment != "" || !strings.HasPrefix(uri[len(prefix):], u.Host) { + if err != nil || u.User != nil || strings.Contains(uri, "#") || !strings.HasPrefix(uri[len(prefix):], u.Host) { return "", false } if ip := net.ParseIP(u.Hostname()); ip == nil || !ip.IsLoopback() { diff --git a/storage/client_subject.go b/storage/client_subject.go index 001f9ea4..d0808384 100644 --- a/storage/client_subject.go +++ b/storage/client_subject.go @@ -25,10 +25,13 @@ func (c RegisteredClient) SubjectType() fapi.SubjectType { return c.subjectType // can't have pairwise subjects). func (c RegisteredClient) Sector() string { return c.sector } -// checkApplicationTypeAndSector runs checkRedirectURIQueries and -// checkApplicationType, then clientSector, whose sector derivation -// depends on the application type. +// checkApplicationTypeAndSector runs checkRedirectURIFragments, +// checkRedirectURIQueries and checkApplicationType, then clientSector, +// whose sector derivation depends on the application type. func checkApplicationTypeAndSector(cfg RegisteredClientConfig) (string, error) { + if err := checkRedirectURIFragments(cfg); err != nil { + return "", err + } if err := checkRedirectURIQueries(cfg); err != nil { return "", err } diff --git a/storage/redirect_uris_test.go b/storage/redirect_uris_test.go index 4d0d5452..91c94294 100644 --- a/storage/redirect_uris_test.go +++ b/storage/redirect_uris_test.go @@ -97,3 +97,33 @@ func TestNewRegisteredClientRefusesRedirectQueryNamingResponseParameter(t *testi }) } } + +// RFC 6749 §3.1.2: a redirect URI "MUST NOT include a fragment +// component", and "#" alone is one (RFC 3986 §3.5). NewRegisteredClient +// refuses it for a web or a native client, rather than registering a +// URI that matches only a request spelled with the same bare "#". +func TestNewRegisteredClientRefusesRedirectFragment(t *testing.T) { + for _, tc := range []struct { + uri string + native bool + }{ + {"https://rp.example/cb#", false}, + {"https://rp.example/cb#frag", false}, + {"https://rp.example/cb?a=1#", false}, + {"http://127.0.0.1/cb#", true}, + {"com.example.app:/cb#", true}, + } { + t.Run(tc.uri, func(t *testing.T) { + cfg := RegisteredClientConfig{ + ID: "c", RedirectURIs: []fapi.RegisteredRedirectURI{fapi.RegisteredRedirectURI(tc.uri)}, + ClientAssertionAlgorithm: fapi.ES256, AllowedScopes: []string{"openid"}, + } + if tc.native { + cfg.ApplicationType = ApplicationTypeNative + } + if c, err := NewRegisteredClient(cfg); err == nil { + t.Fatalf("NewRegisteredClient(%q) accepted it; HasRedirectURI = %v", tc.uri, c.HasRedirectURI(tc.uri)) + } + }) + } +} diff --git a/url.go b/url.go index 89641d34..ebca22d1 100644 --- a/url.go +++ b/url.go @@ -16,7 +16,7 @@ import ( // ParseIssuerURL, ParseEndpointURL or ParseRedirectURL, which enforce: // absolute, HTTPS (except an explicitly enabled loopback exception, or a // native app's private-use scheme for a redirect), no embedded -// credentials, no fragment, a host in ASCII (an internationalized +// credentials, no fragment (not even an empty "#"), a host in ASCII (an internationalized // domain name given as its xn-- A-label), and a normalized scheme and // host (their ASCII letters lowercased). type URL struct { @@ -65,7 +65,7 @@ func ParseRedirectURL(raw string, opts ...URLOption) (URL, error) { } if o.allowPrivateUseScheme { if parsed, err := url.Parse(raw); err == nil && parsed.Scheme != "http" && parsed.Scheme != "https" && parsed.Scheme != "" { - u, err := parsePrivateUseURL(parsed) + u, err := parsePrivateUseURL(raw, parsed) if err != nil { return URL{}, fmt.Errorf("fapi: parse redirect URL: %w", err) } @@ -79,9 +79,9 @@ func ParseRedirectURL(raw string, opts ...URLOption) (URL, error) { return u, nil } -// parsePrivateUseURL validates parsed as a private-use URI scheme -// redirect (RFC 8252 §7.1, §8.4). -func parsePrivateUseURL(parsed *url.URL) (URL, error) { +// parsePrivateUseURL validates parsed, raw's parse, as a private-use +// URI scheme redirect (RFC 8252 §7.1, §8.4). +func parsePrivateUseURL(raw string, parsed *url.URL) (URL, error) { if !isReverseDomainScheme(parsed.Scheme) { return URL{}, fmt.Errorf("private-use scheme %q must be a reverse-order domain name, such as com.example.app", parsed.Scheme) } @@ -91,12 +91,21 @@ func parsePrivateUseURL(parsed *url.URL) (URL, error) { if parsed.Opaque != "" || parsed.Host != "" || parsed.User != nil || !strings.HasPrefix(parsed.Path, "/") || strings.HasPrefix(parsed.Path, "//") || strings.HasPrefix(parsed.String(), parsed.Scheme+"://") { return URL{}, fmt.Errorf("private-use redirect URI must be %s:/path, with a single slash after the scheme (RFC 8252 §7.1), not %s://…", parsed.Scheme, parsed.Scheme) } - if parsed.Fragment != "" { + if hasFragment(raw) { return URL{}, fmt.Errorf("URL must not contain a fragment") } return URL{value: *parsed}, nil } +// hasFragment reports whether raw has a fragment component, even an +// empty one: RFC 3986 §3.5's fragment is everything after the first +// "#", so "https://rp.example/cb#" has one. net/url parses that "#" +// away, leaving url.URL.Fragment empty, and a URL that kept it would +// drop it from its string form, silently rewriting the value. +func hasFragment(raw string) bool { + return strings.Contains(raw, "#") +} + // isReverseDomainScheme reports whether scheme has the form RFC 8252 // §7.1 requires of a private-use URI scheme: a domain name in reverse // order, at least two dot-separated labels, each of letters, digits and @@ -184,7 +193,7 @@ func parseSecureURL(raw string, opts []URLOption) (URL, error) { if parsed.User != nil { return URL{}, fmt.Errorf("URL must not contain embedded credentials") } - if parsed.Fragment != "" { + if hasFragment(raw) { return URL{}, fmt.Errorf("URL must not contain a fragment") } diff --git a/url_test.go b/url_test.go index 117da517..8ea9fc2f 100644 --- a/url_test.go +++ b/url_test.go @@ -258,6 +258,25 @@ func TestParseURLRefusesNonASCIIHost(t *testing.T) { } } +// TestParseURLRefusesEmptyFragment: "#" alone is a fragment component +// (RFC 3986 §3.5), which an issuer (RFC 8414 §2) and a redirect URI (RFC +// 6749 §3.1.2) must not have, so every parser refuses it rather than +// parsing it away; a private-use redirect too. +func TestParseURLRefusesEmptyFragment(t *testing.T) { + for name, parse := range parsers { + for _, raw := range []string{ + "https://as.example.com/#", + "https://as.example.com#", + "https://client.example.com/cb?a=1#", + "com.example.app:/cb#", + } { + if u, err := parse(raw); err == nil { + t.Errorf("%s(%q) = %q, want an error", name, raw, u.String()) + } + } + } +} + // TestParseURLLowercasesASCIIHost: a mixed-case ASCII host or scheme is // lowercased, and an internationalized name in its A-label form is // accepted.