diff --git a/docs/guides/rar.md b/docs/guides/rar.md index 2f8783b2..4d22a433 100644 --- a/docs/guides/rar.md +++ b/docs/guides/rar.md @@ -46,7 +46,19 @@ later token request narrows the grant; left nil, `ValidateGrant` decides that too. Set it when `ValidateGrant` lets the server add a member at approval: consent may add one, but a token request should only keep or drop what was granted, never add a value of the client's own choosing to -a grant that holds none. +a grant that holds none. Leaving it nil there fails open: `ValidateGrant` +can't tell a client's token request from the server's own grant, so it +would accept the client's value. A type with a member the server mints +must set `ValidateNarrowing`. + +The hooks see objects decoded into your Go type, so they can't tell a +member that's absent, or `null`, from one holding its zero value: a +missing `actions` list passes a "subset of the requested actions" check, +while a resource server may read it as no restriction at all. +Floating-point numbers are compared after rounding to `float64`, while +the token carries the digits as sent. Where that matters, require the member in +`Validate`, or give it a pointer, `json.Number` or `json.RawMessage` +type. ## Authorization server: register, gate, approve @@ -99,6 +111,17 @@ and refuses anything malformed or wider with `invalid_authorization_details`. An empty array gets a token without details; no parameter gets the whole grant. The refresh token always keeps the whole grant, so a later refresh can ask for any of it. +Objects are matched greedily, in the order the client sends them: with a +hook that accepts more than an exact match, the client may be refused a +narrowing that it could have had by sending the same objects in another +order. Matching never accepts more than the grant allows. + +**Issuing tokens yourself.** `Server.IssueAccessToken` and +`IssueRefreshToken` grant the `AuthorizationDetails` you pass them +after checking them with `Parse`, not `ParseRequest`: you decide what +to grant, so `ValidateRequest` doesn't run. If you forward details a +client sent you (an OpenID4VCI pre-authorized code token request, for +example), run `RARRegistry.ParseRequest` on them yourself first. ## Client: ask with the same definition diff --git a/extension/rar.go b/extension/rar.go index ba5fd0d0..0b531ca8 100644 --- a/extension/rar.go +++ b/extension/rar.go @@ -52,6 +52,8 @@ type RARDefinition[T any] struct { // choosing a value that, with a policy that grants what was requested // (such as server.AllowRequestedAuthorizationDetails) and the default // exact-match ValidateGrant, would otherwise survive into the grant. + // A definition with such a member almost always needs + // ValidateNarrowing too: see its doc. ValidateRequest func(T) error // ValidateGrant, if non-nil, decides whether a granted detail object @@ -67,6 +69,17 @@ type RARDefinition[T any] struct { // consent, a server's RARPolicy decision. A client asking at the // token endpoint for less than its grant holds is judged by // ValidateNarrowing instead, falling back to ValidateGrant. + // + // The hook sees requested and granted decoded into T, so it can't + // tell a member that's absent, or null, from one holding T's zero + // value: a nil "actions" slice passes a "subset of the requested + // actions" check, while a resource server may read the absent member + // as no restriction at all. Floating-point members are compared + // after rounding to float64. Where that matters, make Validate + // require the member, or give it a pointer, json.Number or + // json.RawMessage type in T, so the hook sees what was sent. The + // same holds for ValidateNarrowing, where the client chooses the + // narrowed bytes the access token then carries. ValidateGrant func(requested, granted T) error // ValidateNarrowing, if non-nil, decides whether a detail object a @@ -86,7 +99,8 @@ type RARDefinition[T any] struct { // Without ValidateNarrowing, a grant that holds no such member would // let a token request add one of the client's own choosing, since // ValidateGrant can't tell a client's narrowing from the server's - // grant. + // grant: the fallback fails open at the token endpoint. A definition + // with a server-minted member must set ValidateNarrowing. ValidateNarrowing func(granted, narrowed T) error } @@ -236,8 +250,14 @@ func RARGet[T any](values RARValues, def RARDefinition[T]) ([]RARDetail[T], erro // validate data an authorization server doesn't trust from a client: // RARSet runs on the sending side, encoding data the caller just // constructed itself, so there is nothing to validate against. +// +// RARSet doesn't HTML-escape '<', '>' and '&' as json.Marshal does. +// That escaping grows each to six bytes, so details a consent page +// reads with RARGet and grants re-encoded with RARSet could grow past +// the MaxBytesPerObject and registry bounds the request was validated +// within, and be refused when the grant is completed. func RARSet[T any](def RARDefinition[T], v T) (json.RawMessage, error) { - encoded, err := json.Marshal(v) + encoded, err := strictjson.MarshalUnescaped(v) if err != nil { return nil, fmt.Errorf("extension: authorization_details type %q: %w", def.Type, err) } @@ -251,14 +271,14 @@ func RARSet[T any](def RARDefinition[T], v T) (json.RawMessage, error) { // object either. return nil, fmt.Errorf("extension: authorization_details type %q: value must encode as a JSON object", def.Type) } - typeValue, _ := json.Marshal(def.Type) // marshaling a string cannot fail + typeValue, _ := strictjson.MarshalUnescaped(def.Type) // marshaling a string cannot fail obj["type"] = typeValue // Every value in obj is already valid JSON — either captured verbatim // by the Unmarshal above (each json.RawMessage decode target only // ever holds one already-well-formed sub-value) or typeValue, // freshly marshaled just above — so re-marshaling the map cannot // fail. - out, _ := json.Marshal(obj) + out, _ := strictjson.MarshalUnescaped(obj) return out, nil } @@ -433,6 +453,13 @@ func (r *RARRegistry) ValidateGrant(requested, granted RARValues) error { // hook, then to exact match. Use it rather than ValidateGrant wherever the // client, not the authorization server, chooses the narrower objects. // +// Matching is greedy in array order, as ValidateGrant's is, and here the +// client chooses that order: with a hook that accepts more than an exact +// match (a lower amount, say), narrowed objects in one order may be +// refused while the same objects in another order are accepted. It never +// accepts a narrowing that a matcher trying every assignment would +// refuse: an accepted narrowing is one valid assignment. +// // Like ValidateGrant's granted, narrowed must already have been produced // by this same registry's own Parse. func (r *RARRegistry) ValidateNarrowing(granted, narrowed RARValues) error { diff --git a/extension/rar_test.go b/extension/rar_test.go index 8f5188fa..c3a9bd4f 100644 --- a/extension/rar_test.go +++ b/extension/rar_test.go @@ -445,6 +445,22 @@ func TestRARSetRoundTripsThroughRegistryParseAndRARGet(t *testing.T) { } } +// TestRARSetDoesNotEscapeHTML: RARSet keeps '<', '>' and '&' as they +// are, in the value's members and in def.Type alike, rather than growing +// each to a six-byte \u003c-style escape as json.Marshal does — an +// object read with RARGet and re-encoded with RARSet must stay within +// the bounds it was validated against. +func TestRARSetDoesNotEscapeHTML(t *testing.T) { + def := extension.RARDefinition[bareInstructedAmount]{Type: "a&c", MaxObjects: 1, MaxBytesPerObject: 128} + raw, err := extension.RARSet(def, bareInstructedAmount{InstructedAmt: "<1&2>"}) + if err != nil { + t.Fatalf("RARSet: %v", err) + } + if want := `{"instructed_amount":"<1&2>","type":"a&c"}`; string(raw) != want { + t.Fatalf("RARSet = %s, want %s", raw, want) + } +} + func TestRARSetRejectsNonObjectValue(t *testing.T) { def := extension.RARDefinition[string]{Type: "payment_initiation", MaxObjects: 1, MaxBytesPerObject: 128} if _, err := extension.RARSet(def, "not an object"); err == nil { diff --git a/server/attester_trust.go b/server/attester_trust.go index 5fe36226..9f71873c 100644 --- a/server/attester_trust.go +++ b/server/attester_trust.go @@ -117,6 +117,9 @@ func (r RegisteredAttesterKeys) validate() error { if r.Keys == nil { return errors.New("server: dependencies: attester trust: RegisteredAttesterKeys needs Keys, the attesters' own key source (e.g. keys.StaticAttesterKeys)") } + if found, ok := depcheck.FindTypedNil(r, map[string]string{"Keys": "Keys"}); ok { + return fmt.Errorf("server: dependencies: attester trust: RegisteredAttesterKeys.%s is a nil %s", found.Name, found.Type) + } return nil } @@ -273,14 +276,16 @@ func (c X5CAttesterChain) validate() error { if c.Revocation == nil { return fmt.Errorf("server: dependencies: attester_trust: X5CAttesterChain.Revocation is required (pass ClientCertificateCRLs{...} or NoClientCertificateRevocationCheck{} to explicitly decline)") } + // Before Revocation's own validate: a typed nil there (a nil + // *ClientCertificateCRLs) would panic in it. + if found, ok := depcheck.FindTypedNil(c, x5cAttesterChainFieldNames); ok { + return fmt.Errorf("server: dependencies: attester_trust: X5CAttesterChain.%s is a nil %s", found.Name, found.Type) + } if v, ok := c.Revocation.(interface{ validate() error }); ok { if err := v.validate(); err != nil { return fmt.Errorf("server: dependencies: attester_trust: X5CAttesterChain.Revocation: %w", err) } } - if found, ok := depcheck.FindTypedNil(c, x5cAttesterChainFieldNames); ok { - return fmt.Errorf("server: dependencies: attester_trust: X5CAttesterChain.%s is a nil %s", found.Name, found.Type) - } switch c.IssuerBinding { case AttesterIssuerInCertificate, AttesterIssuerByTrustAnchors, AttesterIssuerBoundToAnchor: default: diff --git a/server/client_certificate_trust.go b/server/client_certificate_trust.go index 1260f780..8aa83135 100644 --- a/server/client_certificate_trust.go +++ b/server/client_certificate_trust.go @@ -7,6 +7,8 @@ import ( "fmt" "slices" "time" + + "github.com/idfoundry/fapigo/internal/depcheck" ) // ClientCertificateTrust decides how chain-of-trust for a client @@ -142,6 +144,11 @@ func (t TrustedClientCAs) validate() error { if t.Revocation == nil { return errors.New("trusted client CAs: revocation is required (pass ClientCertificateCRLs{...} or NoClientCertificateRevocationCheck{} to explicitly decline)") } + // Before Revocation's own validate: a typed nil there (a nil + // *ClientCertificateCRLs) would panic in it. + if found, ok := depcheck.FindTypedNil(t, nil); ok { + return fmt.Errorf("trusted client CAs: %s is a nil %s", found.Name, found.Type) + } if v, ok := t.Revocation.(interface{ validate() error }); ok { if err := v.validate(); err != nil { return fmt.Errorf("trusted client CAs: %w", err) diff --git a/server/embedder_access_token.go b/server/embedder_access_token.go index ceb1f4c2..ef9b626b 100644 --- a/server/embedder_access_token.go +++ b/server/embedder_access_token.go @@ -39,6 +39,10 @@ type IssueAccessTokenRequest struct { // authenticated, so no ID token is issued. AuthorizationDetails must // parse under Config.RAR, in types the client is registered for; no // RARPolicy is consulted, since the embedder decided what to grant. + // They're parsed with RARRegistry.Parse, not ParseRequest, so no + // RARDefinition.ValidateRequest runs: an embedder forwarding a + // client's own authorization_details should run + // Config.RAR.ParseRequest on them first. Scope []string AuthorizationDetails []json.RawMessage diff --git a/server/embedder_refresh.go b/server/embedder_refresh.go index 97195cc3..07a26036 100644 --- a/server/embedder_refresh.go +++ b/server/embedder_refresh.go @@ -50,6 +50,10 @@ type IssueRefreshTokenRequest struct { // ID token. AuthorizationDetails must parse under Config.RAR, in // types the client is registered for; no RARPolicy is consulted, // since the embedder decided what to grant. + // They're parsed with RARRegistry.Parse, not ParseRequest, so no + // RARDefinition.ValidateRequest runs: an embedder forwarding a + // client's own authorization_details should run + // Config.RAR.ParseRequest on them first. Scope []string AuthorizationDetails []json.RawMessage diff --git a/server/rar_html_escape_test.go b/server/rar_html_escape_test.go index 2ae1bff6..1d5eaabb 100644 --- a/server/rar_html_escape_test.go +++ b/server/rar_html_escape_test.go @@ -16,18 +16,23 @@ import ( // rarHTMLObject is a payment detail object full of the characters // encoding/json escapes as HTML ('<', '>' and '&' each become a six-byte // \u003c-style escape), so re-encoding it with json.Marshal would grow it -// well past the bound below. -var rarHTMLObject = `{"type":"payment","actions":["approve"],"amount":"` + strings.Repeat("<>&", 20) + `"}` +// well past the bound below. Its members are in the order +// extension.RARSet encodes them, so a grant re-encoded with RARSet is +// byte for byte the object requested. +var rarHTMLObject = `{"actions":["approve"],"amount":"` + strings.Repeat("<>&", 20) + `","type":"payment"}` + +// rarHTMLDef is the "payment" definition rarHTMLHarness registers, with a +// MaxBytesPerObject only just above rarHTMLObject's own size. +var rarHTMLDef = extension.RARDefinition[testPaymentDetail]{ + Type: "payment", MaxObjects: 1, MaxBytesPerObject: len(rarHTMLObject) + 8, +} // rarHTMLHarness registers "payment" with a MaxBytesPerObject only just // above rarHTMLObject's own size, and a total bound only just above the // one-object array's. func rarHTMLHarness(t *testing.T) harness { t.Helper() - def := extension.RARDefinition[testPaymentDetail]{ - Type: "payment", MaxObjects: 1, MaxBytesPerObject: len(rarHTMLObject) + 8, - } - registry, err := extension.NewRARRegistry(len(rarHTMLObject)+16, 4, def) + registry, err := extension.NewRARRegistry(len(rarHTMLObject)+16, 4, rarHTMLDef) if err != nil { t.Fatalf("NewRARRegistry: %v", err) } @@ -38,6 +43,15 @@ func rarHTMLHarness(t *testing.T) harness { // rarHTMLIssueCode pushes a request for exactly rarHTMLObject, grants it // as the end user was shown it, and returns the authorization code. func rarHTMLIssueCode(t *testing.T, h harness) string { + t.Helper() + return rarHTMLIssueCodeGranting(t, h, func(*testing.T, server.InteractionRequest) []json.RawMessage { + return []json.RawMessage{json.RawMessage(rarHTMLObject)} + }) +} + +// rarHTMLIssueCodeGranting is rarHTMLIssueCode, granting the +// authorization_details grant returns for the interaction. +func rarHTMLIssueCodeGranting(t *testing.T, h harness, grant func(*testing.T, server.InteractionRequest) []json.RawMessage) string { t.Helper() push, err := h.server.PushAuthorizationRequest(context.Background(), server.PushAuthorizationRequest{ HTTP: server.FormRequest{Parameters: []server.FormParameter{ @@ -73,7 +87,7 @@ func rarHTMLIssueCode(t *testing.T, h harness) string { Handle: interaction.Handle, Result: server.Authorize(subject, authCtx, server.GrantedAuthorization{ Scope: []string{"openid", "accounts", "offline_access"}, - AuthorizationDetails: []json.RawMessage{json.RawMessage(rarHTMLObject)}, + AuthorizationDetails: grant(t, interaction.Interaction), }), }) if err != nil { @@ -135,14 +149,38 @@ func TestAuthorizationDetailsKeepTheirBytesThroughGrantAndRefresh(t *testing.T) } } +// TestAuthorizationDetailsKeepTheirBytesThroughRARSet is +// TestAuthorizationDetailsKeepTheirBytesThroughGrantAndRefresh for a +// consent page that reads the requested details with extension.RARGet +// and grants them re-encoded with extension.RARSet, as the RAR guide +// does: RARSet mustn't grow them past the bounds they were validated +// against by escaping '<', '>' and '&'. +func TestAuthorizationDetailsKeepTheirBytesThroughRARSet(t *testing.T) { + h := rarHTMLHarness(t) + code := rarHTMLIssueCodeGranting(t, h, func(t *testing.T, interaction server.InteractionRequest) []json.RawMessage { + t.Helper() + details, err := extension.RARGet(interaction.AuthorizationDetails, rarHTMLDef) + if err != nil || len(details) != 1 { + t.Fatalf("RARGet = %v, %v; want the one requested object", details, err) + } + granted, err := extension.RARSet(rarHTMLDef, details[0].Fields) + if err != nil { + t.Fatalf("RARSet: %v", err) + } + return []json.RawMessage{granted} + }) + result, err := rarExchangeCode(t, h, code, nil) + if err != nil { + t.Fatalf("ExchangeAuthorizationCode: %v", err) + } + assertRARHTMLIssued(t, h, result) +} + // TestEmbedderAuthorizationDetailsKeepTheirBytes is // TestAuthorizationDetailsKeepTheirBytesThroughGrantAndRefresh for // details an embedder grants itself (IssueRefreshTokenRequest.AuthorizationDetails). func TestEmbedderAuthorizationDetailsKeepTheirBytes(t *testing.T) { - def := extension.RARDefinition[testPaymentDetail]{ - Type: "payment", MaxObjects: 1, MaxBytesPerObject: len(rarHTMLObject) + 8, - } - registry, err := extension.NewRARRegistry(len(rarHTMLObject)+16, 4, def) + registry, err := extension.NewRARRegistry(len(rarHTMLObject)+16, 4, rarHTMLDef) if err != nil { t.Fatalf("NewRARRegistry: %v", err) } diff --git a/server/trust_typed_nil_test.go b/server/trust_typed_nil_test.go new file mode 100644 index 00000000..b717add7 --- /dev/null +++ b/server/trust_typed_nil_test.go @@ -0,0 +1,61 @@ +package server_test + +import ( + "crypto/x509" + "strings" + "testing" + + "github.com/idfoundry/fapigo/keys" + "github.com/idfoundry/fapigo/server" +) + +// TestNewRefusesTypedNilTrustFields covers interface fields nested in +// AttesterTrust and ClientCertificateTrust holding a typed nil (a nil +// *ClientCertificateCRLs, say): New refuses each, naming the field, +// rather than panicking in the field's own validation. +func TestNewRefusesTypedNilTrustFields(t *testing.T) { + x5c := server.X5CAttesterChain{ + TrustAnchors: server.StaticAttesterTrustAnchors{Roots: x509.NewCertPool()}, + IssuerBinding: server.AttesterIssuerInCertificate, + Revocation: (*server.ClientCertificateCRLs)(nil), + } + for name, tc := range map[string]struct { + attestation bool + set func(*server.Dependencies) + want string + }{ + "x5c chain revocation": {true, func(d *server.Dependencies) { d.AttesterTrust = x5c }, "X5CAttesterChain.Revocation is a nil *ClientCertificateCRLs"}, + "x5c chain by pointer revocation": {true, func(d *server.Dependencies) { d.AttesterTrust = &x5c }, "X5CAttesterChain.Revocation is a nil *ClientCertificateCRLs"}, + "registered attester keys": {true, func(d *server.Dependencies) { + d.AttesterTrust = server.RegisteredAttesterKeys{Keys: keys.StaticAttesterKeys(nil)} + }, "RegisteredAttesterKeys.Keys is a nil keys.StaticAttesterKeys"}, + "trusted client CAs revocation": {false, func(d *server.Dependencies) { + d.ClientCertificateTrust = server.TrustedClientCAs{Roots: x509.NewCertPool(), Revocation: (*server.ClientCertificateCRLs)(nil)} + }, "trusted client CAs: revocation is a nil *ClientCertificateCRLs"}, + } { + t.Run(name, func(t *testing.T) { + cfg := validConfig(t) + if tc.attestation { + cfg = validAttestationConfig(t) + } + deps := validDependencies() + tc.set(&deps) + assertNewRefuses(t, cfg, deps, tc.want) + }) + } +} + +// assertNewRefuses checks New refuses deps with an error containing +// want, rather than accepting them or panicking. +func assertNewRefuses(t *testing.T, cfg server.Config, deps server.Dependencies, want string) { + t.Helper() + defer func() { + if r := recover(); r != nil { + t.Fatalf("New panicked: %v", r) + } + }() + _, err := server.New(cfg, deps) + if err == nil || !strings.Contains(err.Error(), want) { + t.Fatalf("New = %v, want an error containing %q", err, want) + } +}