diff --git a/UPGRADING.md b/UPGRADING.md index e978875d..4e297478 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -140,6 +140,25 @@ following §5.3.1 item 4 refuses a longer one. **What to change:** set it to 60 minutes or less. `RecommendedLimits` uses 60 seconds. +### The federation entity key must not be a protocol key (server) + +**Affects:** a server with `Config.Federation.EntityID` set whose +`Dependencies.Keys` returns the same key for +`keys.FederationEntitySigning` as for any protocol signing purpose (ID +tokens, JARM, UserInfo, or access tokens). + +**Why:** the entity key signs this server's Entity Configuration, which +vouches for its protocol keys. Reusing one of those for it lets anything +that key signs be presented as a statement of the entity itself. +`Config.Federation` already said the key is never reused; now `New` +checks. + +**What fails:** `server.New` returns an error saying the federation +entity key is also a protocol key. + +**What to change:** give `keys.FederationEntitySigning` its own key in +your key manager. + ### Attestation-based client authentication follows draft-12 (server, client) Not a breaking change: clients built to draft-07 keep working, and so diff --git a/federation/automatic_registration.go b/federation/automatic_registration.go index 67fabb48..59887447 100644 --- a/federation/automatic_registration.go +++ b/federation/automatic_registration.go @@ -432,25 +432,30 @@ func (a *AutomaticClientRepository) resolve(ctx context.Context, id fapi.ClientI a.mu.Unlock() return cachedClient{}, failed.err } - if call, ok := a.inflight[id]; ok { - a.mu.Unlock() - select { - case <-call.done: - return call.entry, call.err - case <-ctx.Done(): - return cachedClient{}, ctx.Err() - } + call, ok := a.inflight[id] + if !ok { + call = &inflightResolution{done: make(chan struct{})} + a.inflight[id] = call + // The resolution is shared by every caller for id, so it runs + // detached from this one's cancellation: one caller giving up + // (or an attacker cancelling its own request on purpose) must + // not fail the others. It keeps ctx's values, and the fetcher's + // own limits bound how long it can take. + go func() { + call.entry, call.err = a.resolveUncached(context.WithoutCancel(ctx), id) + a.mu.Lock() + delete(a.inflight, id) + a.mu.Unlock() + close(call.done) + }() } - call := &inflightResolution{done: make(chan struct{})} - a.inflight[id] = call - a.mu.Unlock() - - call.entry, call.err = a.resolveUncached(ctx, id) - a.mu.Lock() - delete(a.inflight, id) a.mu.Unlock() - close(call.done) - return call.entry, call.err + select { + case <-call.done: + return call.entry, call.err + case <-ctx.Done(): + return cachedClient{}, ctx.Err() + } } // resolveUncached resolves id's Trust Chain and builds its registration, @@ -654,10 +659,10 @@ type relyingPartyMetadata struct { JWKS json.RawMessage `json:"jwks"` JWKSURI string `json:"jwks_uri"` TLSClientCertificateBoundAccessTokens bool `json:"tls_client_certificate_bound_access_tokens"` - IDTokenEncryptedResponseAlg string `json:"id_token_encrypted_response_alg"` - IDTokenEncryptedResponseEnc string `json:"id_token_encrypted_response_enc"` - UserinfoEncryptedResponseAlg string `json:"userinfo_encrypted_response_alg"` - UserinfoEncryptedResponseEnc string `json:"userinfo_encrypted_response_enc"` + IDTokenEncryptedResponseAlg *string `json:"id_token_encrypted_response_alg"` + IDTokenEncryptedResponseEnc *string `json:"id_token_encrypted_response_enc"` + UserinfoEncryptedResponseAlg *string `json:"userinfo_encrypted_response_alg"` + UserinfoEncryptedResponseEnc *string `json:"userinfo_encrypted_response_enc"` // BackchannelAuthenticationRequestSigningAlg/BackchannelTokenDeliveryMode/ // BackchannelClientNotificationEndpoint are read only when @@ -1053,20 +1058,29 @@ func (a *AutomaticClientRepository) applyBackchannelAuthenticationFields(cfg *st // encLabel name the two metadata fields, for error messages. Shared by // registeredClientConfigFromMetadata's id_token and userinfo encryption // handling, which are otherwise identical. -func parseEncryptionAlgorithmPair(algValue, encValue, algLabel, encLabel string) (fapi.KeyManagementAlgorithm, fapi.ContentEncryptionAlgorithm, error) { - algSet := algValue != "" - encSet := encValue != "" - if algSet != encSet { +// +// Each is nil when the metadata leaves it out; one that is present must +// name an algorithm. An empty string would otherwise read as "no +// encryption" while counting as present, so a superior's "essential" +// policy on the parameter would be met by a value that disables it. +func parseEncryptionAlgorithmPair(algValue, encValue *string, algLabel, encLabel string) (fapi.KeyManagementAlgorithm, fapi.ContentEncryptionAlgorithm, error) { + if algValue != nil && *algValue == "" { + return 0, 0, fmt.Errorf("%s must not be empty", algLabel) + } + if encValue != nil && *encValue == "" { + return 0, 0, fmt.Errorf("%s must not be empty", encLabel) + } + if (algValue != nil) != (encValue != nil) { return 0, 0, fmt.Errorf("%s and %s must both be set, or neither", algLabel, encLabel) } - if !algSet { + if algValue == nil { return 0, 0, nil } - alg, err := fapi.ParseKeyManagementAlgorithm(algValue) + alg, err := fapi.ParseKeyManagementAlgorithm(*algValue) if err != nil { return 0, 0, fmt.Errorf("%s: %w", algLabel, err) } - enc, err := fapi.ParseContentEncryptionAlgorithm(encValue) + enc, err := fapi.ParseContentEncryptionAlgorithm(*encValue) if err != nil { return 0, 0, fmt.Errorf("%s: %w", encLabel, err) } diff --git a/federation/automatic_registration_internal_test.go b/federation/automatic_registration_internal_test.go index 8167799b..17134ad7 100644 --- a/federation/automatic_registration_internal_test.go +++ b/federation/automatic_registration_internal_test.go @@ -788,3 +788,34 @@ func TestResolveDropsAnExpiredRegistration(t *testing.T) { t.Fatal("expired registration still cached after its lookup") } } + +// TestRegisteredClientConfigFromMetadataRejectsEmptyEncryptionAlgorithms: +// a present but empty encryption algorithm would read as "no encryption" +// while counting as present, so a superior's essential policy on it +// would be met by a value that turns it off. It's refused; null, which +// states nothing, is no encryption, as an absent one is. +func TestRegisteredClientConfigFromMetadataRejectsEmptyEncryptionAlgorithms(t *testing.T) { + repo := newTestAutomaticClientRepository(AutomaticRegistrationConfig{AllowedScopes: []string{"openid"}}) + metadata := func(fields string) string { + return `{ + "redirect_uris": ["https://rp.example.org/cb"], + "token_endpoint_auth_method": "private_key_jwt", + "token_endpoint_auth_signing_alg": "ES256", + ` + fields + ` + "jwks": ` + testRPJWKS + ` + }` + } + for _, fields := range []string{ + `"id_token_encrypted_response_alg": "", "id_token_encrypted_response_enc": "",`, + `"id_token_encrypted_response_alg": "", "id_token_encrypted_response_enc": "A256GCM",`, + `"userinfo_encrypted_response_alg": "ECDH-ES+A256KW", "userinfo_encrypted_response_enc": "",`, + } { + if _, _, err := call(t, repo, metadata(fields)); err == nil || !strings.Contains(err.Error(), "must not be empty") { + t.Errorf("registeredClientConfigFromMetadata(%s) = %v, want an empty-value error", fields, err) + } + } + cfg, _, err := call(t, repo, metadata(`"id_token_encrypted_response_alg": null, "id_token_encrypted_response_enc": null,`)) + if err != nil || cfg.IDTokenEncryptionKeyManagement != 0 { + t.Errorf("registeredClientConfigFromMetadata(null) = %v, %v; want no encryption", cfg.IDTokenEncryptionKeyManagement, err) + } +} diff --git a/federation/fetch_limits_test.go b/federation/fetch_limits_test.go index cff96f52..03e44a5f 100644 --- a/federation/fetch_limits_test.go +++ b/federation/fetch_limits_test.go @@ -2,6 +2,7 @@ package federation_test import ( "context" + "errors" "net/http" "net/http/httptest" "strings" @@ -135,3 +136,60 @@ func TestAutomaticRegistrationSharesConcurrentResolution(t *testing.T) { t.Fatalf("entity configuration fetched %d times, want 1", n) } } + +// TestAutomaticRegistrationSharedResolutionOutlivesTheFirstCaller: the +// first caller for a client_id starts the resolution every other caller +// waits on. Cancelling that first request returns context.Canceled to +// it alone; the others get the resolution's own result, so one client +// can't fail everyone's requests naming an RP by cancelling its own. +func TestAutomaticRegistrationSharedResolutionOutlivesTheFirstCaller(t *testing.T) { + started := make(chan struct{}) + release := make(chan struct{}) + var once sync.Once + ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + once.Do(func() { close(started) }) + select { + case <-release: + case <-r.Context().Done(): + } + http.NotFound(w, nil) + })) + t.Cleanup(ts.Close) + + resolver, err := federation.NewResolver(federation.Config{ + TrustAnchors: []federation.TrustAnchor{{EntityID: "https://ta.example.org", JWKS: jwksFor(t, "a", generateKey(t))}}, + Limits: federation.Limits{MaxPathLength: 5, MaxAuthorityHints: 5, MaxStatementLifetime: 2 * time.Hour, MaxClockSkew: 5 * time.Second}, + }, federation.Dependencies{HTTP: fetcherFor(t, ts), Clock: federation.SystemClock{}}) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + repo, err := federation.NewAutomaticClientRepository(memstore.NewClientRepository(nil), resolver, fetcherFor(t, ts), + federation.AutomaticRegistrationConfig{AllowedScopes: []string{"openid"}, MaxCacheAge: time.Minute}, federation.SystemClock{}) + if err != nil { + t.Fatalf("NewAutomaticClientRepository: %v", err) + } + + id := fapi.ClientID(ts.URL) + firstCtx, cancelFirst := context.WithCancel(context.Background()) + first := make(chan error, 1) + go func() { + _, err := repo.ResolveClient(firstCtx, id) + first <- err + }() + <-started + second := make(chan error, 1) + go func() { + _, err := repo.ResolveClient(context.Background(), id) + second <- err + }() + time.Sleep(100 * time.Millisecond) // let the second caller join the in-flight resolution + cancelFirst() + if err := <-first; !errors.Is(err, context.Canceled) { + t.Fatalf("first ResolveClient = %v, want context.Canceled", err) + } + close(release) + err = <-second + if err == nil || errors.Is(err, context.Canceled) { + t.Fatalf("second ResolveClient = %v, want the fetch failure, not the first caller's cancellation", err) + } +} diff --git a/federation/resolve_endpoint.go b/federation/resolve_endpoint.go index 3c35cd39..79053791 100644 --- a/federation/resolve_endpoint.go +++ b/federation/resolve_endpoint.go @@ -5,6 +5,7 @@ import ( "crypto" "fmt" "net/url" + "slices" "github.com/idfoundry/fapigo/fapihttp" intfed "github.com/idfoundry/fapigo/internal/federation" @@ -94,6 +95,12 @@ func (r *Resolver) ResolveViaEndpoint(ctx context.Context, req ResolveRequest) ( if req.TrustAnchor == "" { return ResolveResponseClaims{}, fmt.Errorf("federation: trust anchor is empty") } + if !slices.ContainsFunc(r.cfg.TrustAnchors, func(ta TrustAnchor) bool { return ta.EntityID == req.TrustAnchor }) { + // The resolver would answer for a Trust Anchor this Resolver + // doesn't trust, and the caller would take the metadata as + // resolved under one it does. + return ResolveResponseClaims{}, fmt.Errorf("federation: trust anchor %q is not one of this Resolver's Config.TrustAnchors", req.TrustAnchor) + } if err := ValidEntityID(req.ExpectedIssuer); err != nil { return ResolveResponseClaims{}, fmt.Errorf("federation: expected resolver issuer: %w", err) } diff --git a/federation/resolve_endpoint_test.go b/federation/resolve_endpoint_test.go index 7dbe1b81..cf384133 100644 --- a/federation/resolve_endpoint_test.go +++ b/federation/resolve_endpoint_test.go @@ -273,3 +273,17 @@ func TestResolveViaEndpointRefusesAnotherIssuer(t *testing.T) { t.Fatalf("ResolveViaEndpoint(response from another issuer) = %v, want refusal before resolving that issuer", err) } } + +// TestResolveViaEndpointRefusesAnUnconfiguredTrustAnchor: a Trust Anchor +// this Resolver doesn't trust is refused before anything is fetched, +// so the caller can't take a response as resolved under one it trusts. +func TestResolveViaEndpointRefusesAnUnconfiguredTrustAnchor(t *testing.T) { + metadata := map[string]json.RawMessage{"openid_provider": json.RawMessage(`{}`)} + entityID, resolveEndpoint, resolver, _ := selfAnchoredResolveEndpoint(t, "https://op.example.org", metadata) + _, err := resolver.ResolveViaEndpoint(context.Background(), federation.ResolveRequest{ + Endpoint: resolveEndpoint, Subject: "https://op.example.org", TrustAnchor: "https://other-ta.example.org", ExpectedIssuer: entityID, + }) + if err == nil || !strings.Contains(err.Error(), "not one of this Resolver's Config.TrustAnchors") { + t.Fatalf("ResolveViaEndpoint(unconfigured trust anchor) = %v, want refused", err) + } +} diff --git a/federation/trustmark.go b/federation/trustmark.go index 513a5726..6d7d94d4 100644 --- a/federation/trustmark.go +++ b/federation/trustmark.go @@ -147,10 +147,16 @@ func checkTrustMarkAccreditation(trustAnchor ResolvedEntity, claims TrustMarkCla // checkTrustMarkDelegation requires and validates a "delegation" claim // when trustAnchor names the mark's type in its "trust_mark_owners" -// claim (OpenID Federation 1.0 §7.3 steps 8-9). +// claim (OpenID Federation 1.0 §7.3 step 8), and validates one the mark +// carries anyway (step 9): one whose type has no owner there can't be +// validated, since the owner's keys come from that claim (§7.2.2 step +// 9), so the mark is refused rather than its delegation ignored. func (r *Resolver) checkTrustMarkDelegation(trustAnchor ResolvedEntity, tm intfed.TrustMark, claims TrustMarkClaims) error { owner, required := trustAnchor.TrustMarkOwners[claims.TrustMarkType] if !required { + if tm.ClaimedDelegation() != "" { + return fmt.Errorf("the trust mark carries a delegation claim, but trust_mark_type %q has no owner in the trust anchor's trust_mark_owners claim to validate it against", claims.TrustMarkType) + } return nil } if tm.ClaimedDelegation() == "" { diff --git a/federation/trustmark_test.go b/federation/trustmark_test.go index 9444dbd0..0e68c835 100644 --- a/federation/trustmark_test.go +++ b/federation/trustmark_test.go @@ -9,6 +9,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -436,12 +437,13 @@ func TestVerifyTrustMarkRejectsMalformedDelegationJWT(t *testing.T) { } } -func TestVerifyTrustMarkIgnoresDelegationWhenTypeNotOwned(t *testing.T) { - // TA's own trust_mark_owners is nil (no type named at all), so no - // delegation is required and none is present — the existing - // TestVerifyTrustMarkSucceeds already covers this baseline; this - // test instead confirms a PRESENT delegation is simply not checked - // when the type isn't owned, even a broken one. +// TestVerifyTrustMarkRefusesDelegationWhenTypeNotOwned: OpenID +// Federation 1.0 §7.3 step 9 has a validator validate any delegation +// claim a mark carries, and §7.2.2 step 9 takes the owner's keys from +// the Trust Anchor's trust_mark_owners. A type with no owner there +// leaves nothing to validate it against, so the mark is refused rather +// than its delegation ignored. +func TestVerifyTrustMarkRefusesDelegationWhenTypeNotOwned(t *testing.T) { f := setupTrustMarkFederation(t, func(taID, leID string) map[string]any { return map[string]any{"delegation": "not-a-jwt"} }, nil) @@ -451,8 +453,8 @@ func TestVerifyTrustMarkIgnoresDelegationWhenTypeNotOwned(t *testing.T) { if err != nil { t.Fatalf("Resolve: %v", err) } - if _, err := r.VerifyTrustMark(context.Background(), resolved, resolved.TrustMarks[0], federation.AcceptAnyFederationIssuer); err != nil { - t.Fatalf("VerifyTrustMark(broken delegation, type not owned, federation.AcceptAnyFederationIssuer): %v, want nil error", err) + if _, err := r.VerifyTrustMark(context.Background(), resolved, resolved.TrustMarks[0], federation.AcceptAnyFederationIssuer); err == nil || !strings.Contains(err.Error(), "has no owner") { + t.Fatalf("VerifyTrustMark(delegation, type not owned) = %v, want refused", err) } } diff --git a/internal/federation/claims.go b/internal/federation/claims.go index 1c41b0b4..cef0f74e 100644 --- a/internal/federation/claims.go +++ b/internal/federation/claims.go @@ -201,6 +201,9 @@ func parseClaims(payload []byte) (Claims, error) { if err != nil { return Claims{}, err } + if err := checkParsedClaimPlacement(raw, iss == sub); err != nil { + return Claims{}, err + } jwks, hasJWKS := raw["jwks"] if !hasJWKS { return Claims{}, fmt.Errorf("%w: missing \"jwks\"", ErrMalformedClaims) @@ -278,6 +281,33 @@ func parseClaims(payload []byte) (Claims, error) { return c, nil } +// subordinateOnlyClaims and entityConfigurationOnlyClaims are the +// claims OpenID Federation 1.0 §3.2 (steps 14 to 23) allows in only one +// kind of Entity Statement. +var ( + subordinateOnlyClaims = []string{"metadata_policy", "metadata_policy_crit", "constraints", "source_endpoint"} + entityConfigurationOnlyClaims = []string{"authority_hints", "trust_marks", "trust_mark_issuers", "trust_mark_owners"} +) + +// checkParsedClaimPlacement refuses a statement carrying a claim its +// kind may not: §3.2's validation steps 14 to 23, e.g. "If the +// metadata_policy Claim is present, the Entity Statement MUST be a +// Subordinate Statement". selfSigned is iss == sub, an Entity +// Configuration. Create checks the same when building one +// (validateClaimMixing). +func checkParsedClaimPlacement(raw map[string]json.RawMessage, selfSigned bool) error { + forbidden, kind := subordinateOnlyClaims, "an Entity Configuration" + if !selfSigned { + forbidden, kind = entityConfigurationOnlyClaims, "a Subordinate Statement" + } + for _, claim := range forbidden { + if _, ok := raw[claim]; ok { + return fmt.Errorf("%w: %q may not appear in %s (OpenID Federation 1.0 §3.2)", ErrMalformedClaims, claim, kind) + } + } + return nil +} + // parseTrustMarks parses raw's "trust_marks" claim into c, if present — // split out of parseClaims purely to keep that function's own // cognitive complexity manageable. @@ -405,6 +435,31 @@ func validateKeyIDs(jwks json.RawMessage) error { return nil } +// privateKeyMembers are the JWK members that carry private or secret key +// material (RFC 7518 §6.2.2, §6.3.2, §6.4.1; RFC 8037 §2): never part of +// a published key. +var privateKeyMembers = []string{"d", "p", "q", "dp", "dq", "qi", "oth", "k"} + +// refusePrivateKeyMembers refuses a JWK Set any of whose keys carries +// private or secret key material, so a statement this package signs +// never publishes a key handed to it as a private JWK. +func refusePrivateKeyMembers(jwks json.RawMessage) error { + var set struct { + Keys []map[string]json.RawMessage `json:"keys"` + } + if err := json.Unmarshal(jwks, &set); err != nil { + return fmt.Errorf("%w: %v", ErrMalformedJWKS, err) + } + for i, key := range set.Keys { + for _, member := range privateKeyMembers { + if _, ok := key[member]; ok { + return fmt.Errorf("%w: key %d has the private member %q: publish only public keys", ErrMalformedJWKS, i, member) + } + } + } + return nil +} + // rejectCriticalClaims rejects a statement whose "crit" claim names any // claim: OpenID Federation 1.0 §3.2 step 13 requires each named extension // claim to be understood and processed, and this package implements diff --git a/internal/federation/create.go b/internal/federation/create.go index 230ff7d0..b1cc2f6d 100644 --- a/internal/federation/create.go +++ b/internal/federation/create.go @@ -138,6 +138,17 @@ func validateCreateParams(p CreateParams) error { if err := validateKeyIDs(p.JWKS); err != nil { return err } + if err := refusePrivateKeyMembers(p.JWKS); err != nil { + return err + } + for trustMarkType, owner := range p.TrustMarkOwners { + if len(owner.JWKS) == 0 { + continue + } + if err := refusePrivateKeyMembers(owner.JWKS); err != nil { + return fmt.Errorf("trust_mark_owners %q: %w", trustMarkType, err) + } + } return validateClaimMixing(p) } diff --git a/internal/federation/federation_test.go b/internal/federation/federation_test.go index f115ece4..ccbd836c 100644 --- a/internal/federation/federation_test.go +++ b/internal/federation/federation_test.go @@ -6,6 +6,7 @@ import ( "crypto/rand" "encoding/json" "errors" + "fmt" "testing" "time" @@ -908,3 +909,67 @@ func TestStatementClaimedConstraintsAbsent(t *testing.T) { t.Errorf("ClaimedConstraints() = %+v, want nil", constraints) } } + +// TestCreateRefusesPrivateKeyMembers: a statement publishes its jwks +// (and a trust mark owner's) as given, so one handed a private JWK would +// publish the private key. Create refuses any key carrying private or +// secret key material. +func TestCreateRefusesPrivateKeyMembers(t *testing.T) { + key := generateKey(t) + now := time.Now() + private := json.RawMessage(`{"keys":[{"kty":"EC","crv":"P-256","kid":"k","x":"AAAA","y":"AAAA","d":"secret"}]}`) + + params := entityConfigParams(t, key, "https://rp.example.org", now) + params.JWKS = private + if _, err := Create(params); !errors.Is(err, ErrMalformedJWKS) { + t.Errorf("Create(jwks with d) = %v, want ErrMalformedJWKS", err) + } + + params = entityConfigParams(t, key, "https://ta.example.org", now) + params.AuthorityHints = nil + params.TrustMarkOwners = map[string]TrustMarkOwner{"https://marks.example.org/x": {Subject: "https://owner.example.org", JWKS: json.RawMessage(`{"keys":[{"kty":"oct","kid":"o","k":"c2VjcmV0"}]}`)}} + if _, err := Create(params); !errors.Is(err, ErrMalformedJWKS) { + t.Errorf("Create(trust mark owner jwks with k) = %v, want ErrMalformedJWKS", err) + } + + params.TrustMarkOwners = map[string]TrustMarkOwner{"https://marks.example.org/x": {Subject: "https://owner.example.org", JWKS: json.RawMessage(`not json`)}} + if _, err := Create(params); !errors.Is(err, ErrMalformedJWKS) { + t.Errorf("Create(malformed trust mark owner jwks) = %v, want ErrMalformedJWKS", err) + } + params.TrustMarkOwners = map[string]TrustMarkOwner{"https://marks.example.org/x": {Subject: "https://owner.example.org"}} + if _, err := Create(params); errors.Is(err, ErrMalformedJWKS) { + t.Errorf("Create(trust mark owner without jwks) = %v, want no jwks error", err) + } +} + +// TestParseRefusesMisplacedClaims: OpenID Federation 1.0 §3.2 (steps 14 +// to 23) has a validator refuse a claim the statement's kind may not +// carry, as Create refuses to build one: a Subordinate Statement claim +// in an Entity Configuration, or the reverse. +func TestParseRefusesMisplacedClaims(t *testing.T) { + key := generateKey(t) + now := time.Now().Unix() + jwks := string(testJWKS(t, key)) + for name, tc := range map[string]struct { + sub, extra string + wantErr bool + }{ + "metadata_policy in an Entity Configuration": {"https://a.example.org", `"metadata_policy":{}`, true}, + "constraints in an Entity Configuration": {"https://a.example.org", `"constraints":{}`, true}, + "source_endpoint in an Entity Configuration": {"https://a.example.org", `"source_endpoint":"https://a.example.org/fetch"`, true}, + "authority_hints in a Subordinate Statement": {"https://b.example.org", `"authority_hints":["https://ta.example.org"]`, true}, + "trust_mark_owners in a Subordinate Statement": {"https://b.example.org", `"trust_mark_owners":{}`, true}, + "authority_hints in an Entity Configuration": {"https://a.example.org", `"authority_hints":["https://ta.example.org"]`, false}, + "metadata_policy in a Subordinate Statement": {"https://b.example.org", `"metadata_policy":{}`, false}, + } { + payload := fmt.Sprintf(`{"iss":"https://a.example.org","sub":%q,"iat":%d,"exp":%d,"jwks":%s,%s}`, tc.sub, now, now+60, jwks, tc.extra) + token, err := jose.Sign(key, jose.Header{Algorithm: fapi.ES256, Type: jwtType, KeyID: "test-kid"}, []byte(payload)) + if err != nil { + t.Fatal(err) + } + _, err = Parse(token) + if gotErr := errors.Is(err, ErrMalformedClaims); gotErr != tc.wantErr { + t.Errorf("%s: Parse = %v, want ErrMalformedClaims %v", name, err, tc.wantErr) + } + } +} diff --git a/internal/federation/policy.go b/internal/federation/policy.go index 48141408..be52d6d0 100644 --- a/internal/federation/policy.go +++ b/internal/federation/policy.go @@ -449,6 +449,12 @@ func applyEntityTypePolicy(params map[string]PolicyOperators, crit critSet, curr resolved := make(map[string]json.RawMessage, len(claims)) for claim := range claims { value, present := current[claim] + if present && isJSONNull(value) { + // A parameter whose value is null states nothing: it is + // treated as absent, so it can't stand in for one an + // "essential" operator requires, and is left out. + value, present = nil, false + } value, present, err := applyClaimPolicy(params[claim], crit, value, present) if err != nil { return nil, fmt.Errorf("%s: %w", claim, err) diff --git a/internal/federation/policy_test.go b/internal/federation/policy_test.go index 14e73a99..44b9b78a 100644 --- a/internal/federation/policy_test.go +++ b/internal/federation/policy_test.go @@ -555,3 +555,21 @@ func TestApplyPolicyRefusesNonObjectMetadata(t *testing.T) { t.Fatalf("ApplyPolicy(non-object metadata) = %v, want a refusal naming the entity type", err) } } + +// TestApplyPolicyTreatsNullAsAbsent: a metadata parameter whose value +// is null states nothing, so it doesn't meet an essential operator, and +// without a policy it's left out of the resolved metadata. +func TestApplyPolicyTreatsNullAsAbsent(t *testing.T) { + metadata := map[string]json.RawMessage{"openid_relying_party": json.RawMessage(`{"id_token_encrypted_response_alg": null, "client_name": null}`)} + policy := MetadataPolicy{"openid_relying_party": {"id_token_encrypted_response_alg": {opEssential: json.RawMessage(`true`)}}} + if _, err := ApplyPolicy(policy, nil, metadata); err == nil { + t.Fatal("ApplyPolicy(essential parameter null) = nil error, want refused") + } + resolved, err := ApplyPolicy(MetadataPolicy{}, nil, metadata) + if err != nil { + t.Fatal(err) + } + if string(resolved["openid_relying_party"]) != `{}` { + t.Errorf("resolved = %s, want the null parameters left out", resolved["openid_relying_party"]) + } +} diff --git a/server/automatic_registration_subject_types_test.go b/server/automatic_registration_subject_types_test.go index 046267db..f122f703 100644 --- a/server/automatic_registration_subject_types_test.go +++ b/server/automatic_registration_subject_types_test.go @@ -197,6 +197,7 @@ func TestEntityConfigurationAdvertisesAutomaticRegistrationSubjectTypes(t *testi cfg, deps := automaticRegistrationTestConfig(t, f, func(cfg *server.Config, deps *server.Dependencies) { cfg.Subjects = fedPairwiseSubjects(t, fapi.SubjectTypePairwise, server.SubjectTypesPerClient) cfg.Federation = server.FederationConfig{EntityID: testIssuer, Lifetime: time.Hour, Algorithm: fapi.ES256} + deps.Keys.(*fakeKeyManager).federationKey = generateKey(t) deps.PairwiseSubjects = hmacDeriver(t) deps.AccessTokens = server.OpaqueAccessTokens{Store: memstore.NewAccessTokenStore()} }) diff --git a/server/federation_test.go b/server/federation_test.go index 28fdca9f..276ea617 100644 --- a/server/federation_test.go +++ b/server/federation_test.go @@ -62,7 +62,7 @@ func TestServerEntityConfiguration(t *testing.T) { cfg := federationConfiguredConfig(t) cfg.Federation.AuthorityHints = []string{"https://ta.example.org"} deps := validDependencies() - deps.Keys = &fakeKeyManager{key: generateKey(t), keyID: "as-fed-kid"} + deps.Keys = &fakeKeyManager{key: generateKey(t), keyID: "as-fed-kid", federationKey: generateKey(t)} srv, err := server.New(cfg, deps) if err != nil { @@ -167,7 +167,7 @@ func TestServerEntityConfigurationRejectsEmptyKeyID(t *testing.T) { func TestServerEntityConfigurationPublishesTrustMarks(t *testing.T) { key := generateKey(t) deps := validDependencies() - deps.Keys = &fakeKeyManager{key: key, keyID: "as-fed-kid"} + deps.Keys = &fakeKeyManager{key: generateKey(t), keyID: "as-fed-kid", federationKey: key} srv, err := server.New(federationConfiguredConfig(t), deps) if err != nil { t.Fatalf("New: %v", err) @@ -212,3 +212,33 @@ func TestNewRejectsMissingFederationSigningKey(t *testing.T) { t.Fatalf("New(no federation signing key) = %v, want it refused naming federation_entity_signing", err) } } + +// TestNewRefusesFederationKeyReusedAsProtocolKey: the federation entity +// key vouches for the whole Entity Configuration, so it is never also a +// protocol signing key; New refuses one that is. +func TestNewRefusesFederationKeyReusedAsProtocolKey(t *testing.T) { + deps := validDependencies() + deps.Keys = &fakeKeyManager{key: generateKey(t), keyID: "as-kid"} + if _, err := server.New(federationConfiguredConfig(t), deps); err == nil || !strings.Contains(err.Error(), "never reused as a protocol key") { + t.Fatalf("New(federation key = protocol key) = %v, want refused", err) + } + deps.Keys = &fakeKeyManager{key: generateKey(t), keyID: "as-kid", federationKey: generateKey(t)} + if _, err := server.New(federationConfiguredConfig(t), deps); err != nil { + t.Fatalf("New(distinct federation key) = %v", err) + } +} + +// TestNewRefusesFederationKeyReusedDuringARotation: with a rotating key +// manager, every key it still publishes for a protocol purpose counts, +// the outgoing one included. +func TestNewRefusesFederationKeyReusedDuringARotation(t *testing.T) { + entityKey := generateKey(t) + deps := validDependencies() + deps.Keys = &rotatingKeyManager{ + outgoing: &fakeKeyManager{key: entityKey, keyID: "old"}, + newest: &fakeKeyManager{key: generateKey(t), keyID: "new", federationKey: entityKey}, + } + if _, err := server.New(federationConfiguredConfig(t), deps); err == nil || !strings.Contains(err.Error(), "never reused as a protocol key") { + t.Fatalf("New(outgoing protocol key = federation key) = %v, want refused", err) + } +} diff --git a/server/jwks.go b/server/jwks.go index 6aa05ff5..773d759e 100644 --- a/server/jwks.go +++ b/server/jwks.go @@ -2,6 +2,7 @@ package server import ( "context" + "crypto" "fmt" "time" @@ -125,9 +126,56 @@ func (s *Server) checkSigningKeys() error { if _, err := s.PublicJWKS(ctx); err != nil { return fmt.Errorf("server: dependencies: the JWK Set at endpoints.jwks can't be built: %w", err) } + if s.cfg.Federation.EntityID != "" { + return checkFederationKeyDistinct(ctx, uses[len(published)], published) + } + return nil +} + +// checkFederationKeyDistinct refuses a federation entity signing key that +// is also one of the server's protocol signing keys. The entity key +// vouches for this server's whole Entity Configuration, its protocol +// keys included; one of those keys reused for it would let anything +// that key signs (an ID token, a JARM response) be presented as a +// statement of the entity itself, which is why Config.Federation says +// it is never reused. +func checkFederationKeyDistinct(ctx context.Context, federation namedSigningKeyUse, published []namedSigningKeyUse) error { + entityKeys := signingPublicKeys(ctx, federation.SigningKeyUse) + for _, use := range published { + for _, protocolKey := range signingPublicKeys(ctx, use.SigningKeyUse) { + for _, entityKey := range entityKeys { + if equal, ok := entityKey.(interface{ Equal(crypto.PublicKey) bool }); ok && equal.Equal(protocolKey) { + return fmt.Errorf("server: dependencies: the %v key is also the %s %v key: a federation entity key is never reused as a protocol key", keys.FederationEntitySigning, use.field, use.Purpose) + } + } + } + } return nil } +// signingPublicKeys returns every public key use's manager currently +// holds for it: each one a keys.RotatingKeyManager reports, or the one +// PublicKey returns. checkSigningKeys has resolved each use already, so +// a key that can't be read now is simply left out. +func signingPublicKeys(ctx context.Context, use keys.SigningKeyUse) []crypto.PublicKey { + if rotating, ok := use.Manager.(keys.RotatingKeyManager); ok { + set, err := rotating.PublicKeys(ctx, use.Purpose, use.Algorithm) + if err != nil { + return nil + } + out := make([]crypto.PublicKey, 0, len(set.Keys)) + for _, info := range set.Keys { + out = append(out, info.PublicKey) + } + return out + } + info, err := use.Manager.PublicKey(ctx, use.Purpose, use.Algorithm) + if err != nil { + return nil + } + return []crypto.PublicKey{info.PublicKey} +} + // probeSigningKey signs a throwaway payload with use's key, through the // same path tokens take, and verifies it against the public key the // manager reports. The result is discarded. diff --git a/server/par_test.go b/server/par_test.go index f697816d..b0900ad8 100644 --- a/server/par_test.go +++ b/server/par_test.go @@ -335,17 +335,30 @@ func (f *fakeGrantStore) Capabilities() storage.Capabilities { type fakeKeyManager struct { key *ecdsa.PrivateKey keyID string + // federationKey, when set, is the keys.FederationEntitySigning key, + // under the kid "fed-" + keyID: New refuses a federation entity key + // that is also a protocol key. + federationKey *ecdsa.PrivateKey +} + +// keyFor is the key f holds for purpose. +func (f *fakeKeyManager) keyFor(purpose keys.SigningPurpose) (*ecdsa.PrivateKey, string) { + if purpose == keys.FederationEntitySigning && f.federationKey != nil { + return f.federationKey, "fed-" + f.keyID + } + return f.key, f.keyID } func (f *fakeKeyManager) Sign(_ context.Context, req keys.SigningRequest) (keys.Signature, error) { + key, keyID := f.keyFor(req.Purpose) // keys.Signature.Value must match crypto.Signer's contract for the // key type — ASN.1 DER for ECDSA, exactly what *ecdsa.PrivateKey.Sign // would produce — not the fixed-width R||S JWS uses on the wire. - der, err := ecdsa.SignASN1(rand.Reader, f.key, req.Digest) + der, err := ecdsa.SignASN1(rand.Reader, key, req.Digest) if err != nil { return keys.Signature{}, err } - return keys.Signature{KeyID: f.keyID, Value: der}, nil + return keys.Signature{KeyID: keyID, Value: der}, nil } // KeyCustody is a test-only assertion — see fakeClientRepository.Capabilities. @@ -353,8 +366,9 @@ func (f *fakeKeyManager) KeyCustody() keys.KeyCustody { return keys.KeyCustody{Durable: true, CrossInstanceConsistent: true} } -func (f *fakeKeyManager) PublicKey(_ context.Context, _ keys.SigningPurpose, _ fapi.SignatureAlgorithm) (keys.PublicKeyInfo, error) { - return keys.PublicKeyInfo{KeyID: f.keyID, PublicKey: &f.key.PublicKey}, nil +func (f *fakeKeyManager) PublicKey(_ context.Context, purpose keys.SigningPurpose, _ fapi.SignatureAlgorithm) (keys.PublicKeyInfo, error) { + key, keyID := f.keyFor(purpose) + return keys.PublicKeyInfo{KeyID: keyID, PublicKey: &key.PublicKey}, nil } type fakeAuditSink struct {