feat: gate ClientKeySource/IssuerKeySource/ClientEncryptionKeySource behind KeySourceAssurance - #359
Merged
Merged
Conversation
…behind KeySourceAssurance
keys.ClientKeySource, keys.IssuerKeySource, and
keys.ClientEncryptionKeySource are each a single-method, open
interface — any integrator-supplied implementation qualifies, with
only a doc comment ("must apply the same SSRF... protections") asking
nicely. Unlike a storage.Store, which AssuranceProduction already
refuses to accept without a self-declared storage.StoreAssurance, a
key source doing an unprotected live fetch (or an X.509-chain-based
resolver with no root-of-trust check) has no structural signal to the
library that it's taken on that responsibility itself.
Adds keys.KeySourceAssurance (mirrors storage.StoreAssurance exactly):
an optional interface a key source implementation can declare
keys.KeySourceCapabilities{LiveFetchHardened: bool} through. There is
no default — server.AssuranceProduction now requires it of
Dependencies.ClientKeys (always) and ClientEncryptionKeys (when set);
client.AssuranceProduction requires it of Dependencies.IssuerKeys. An
implementation that doesn't implement the interface at all is rejected
rather than assumed adequate, the same "declaring capabilities is not
optional" stance storage.StoreAssurance already takes.
keys.JWKSIssuerKeySource (this module's only bundled production-shaped
key source, unconditionally built on *fapihttp.Client) now declares
LiveFetchHardened. federation.AutomaticClientKeySource forwards its
Underlying's own declaration rather than asserting anything on its
own, since Underlying is tried first and this type's own
federation-derived fallback path being hardened doesn't make an
unhardened Underlying safe. keys/ephemeral.ClientKeySource
deliberately does NOT implement the new interface: that package's own
doc comment already states "local development and testing only, never
production", so it stays correctly rejected under AssuranceProduction
rather than accidentally starting to pass because its live fetch
happens to also go through fapihttp.
Not a breaking change: AssuranceDevelopment (this module's existing
default) asks no KeySourceAssurance question at all, exactly like it
already does for StoreAssurance.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The two new negative tests only asserted err != nil, and Dependencies.Sessions' own, independent AssuranceProduction check runs before the new IssuerKeys check in validateDependencies — with validDependencies(t)'s default Sessions (no StoreAssurance declaration) still in place, both tests were actually failing on Sessions, never reaching the IssuerKeys code path they claimed to test. Found via codecov/patch flagging checkKeySourceAssurance's "declare but insufficient" and "reject" branches as uncovered in client/assurance.go despite dedicated tests existing for them. Fixed by giving both tests a fully-declared Sessions dependency (same as the existing positive test already had) and asserting the error message actually mentions issuer_keys, so this class of bug — a generic err == nil check silently passing for the wrong reason underneath — can't recur unnoticed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
From the same cross-project security rubric review as #358: FAPIgo's "extension-point interfaces" score was STRONG for stores (capability-gated via
storage.StoreAssurance) but only ADEQUATE for key sources —keys.ClientKeySource,keys.IssuerKeySource, andkeys.ClientEncryptionKeySourceare each a single-method, open interface with only a doc comment asking an implementation to be careful about SSRF/trust. No structural signal, unlike a store underAssuranceProduction.Adds
keys.KeySourceAssurance— mirrorsstorage.StoreAssuranceexactly: an optional interface a key source can declarekeys.KeySourceCapabilities{LiveFetchHardened: bool}through. No default:server.AssuranceProductionnow requires it ofDependencies.ClientKeys(always) andClientEncryptionKeys(when set);client.AssuranceProductionrequires it ofDependencies.IssuerKeys. An implementation that doesn't implement the interface at all is rejected rather than assumed adequate — same "declaring capabilities is not optional" stancestorage.StoreAssurancealready takes.keys.JWKSIssuerKeySource(the module's only bundled production-shaped key source, unconditionally built on*fapihttp.Client) now declaresLiveFetchHardened.federation.AutomaticClientKeySourceforwards itsUnderlying's own declaration rather than asserting anything on its own —Underlyingis tried first, so this type's own hardened federation-derived fallback path doesn't make an unhardenedUnderlyingsafe.keys/ephemeral.ClientKeySourcedeliberately does not implement the new interface: that package's own doc comment already says "local development and testing only, never production" — it stays correctly rejected underAssuranceProductionrather than accidentally starting to pass just because its live fetch happens to also go throughfapihttp.Not a breaking change:
AssuranceDevelopment(the existing default) asks noKeySourceAssurancequestion at all, exactly like it already does forStoreAssurance.Test plan
gofmt -l .cleango build ./...go vet ./...go test -race ./...— all packages passgolangci-lint run ./...— 0 issuesserver(reject-without-declaration, reject-when-declared-false, accept-when-hardened, for bothClientKeysandClientEncryptionKeys, plus a skip-when-not-configured case for the latter),client(same shape forIssuerKeys)federation.AutomaticClientKeySource.Capabilities()tested both ways: forwardstruefrom a declaringUnderlying, forwards the zero value from a non-declaring onekeys.JWKSIssuerKeySource.Capabilities()tested directly🤖 Generated with Claude Code
https://claude.ai/code/session_01JCpqoxEg7XUkkGg312zXyS