feat: direct OIDC IdP federation (closes #88, partial)#124
Open
safayavatsal wants to merge 4 commits intohighflame-ai:mainfrom
Open
feat: direct OIDC IdP federation (closes #88, partial)#124safayavatsal wants to merge 4 commits intohighflame-ai:mainfrom
safayavatsal wants to merge 4 commits intohighflame-ai:mainfrom
Conversation
…ee PR) Spec-aligned alternative to the trusted-service broker path for ingesting upstream user identity. RFC 8693 token-exchange with subject_token_type=urn:ietf:params:oauth:token-type:id_token now routes to a new code path that verifies the upstream IdP's signature against a configured JWKS, then mints a ZeroID token carrying user_id_iss as IdP-granular provenance. Implementation: - domain/external_issuer.go: ExternalIssuerConfig with Validate/Defaults - internal/service/external_issuer_registry.go: per-issuer JWKS clients, fail-fast on startup, lifecycle hooks into Server.Shutdown - internal/service/oauth_external_idp.go: externalIDTokenExchange — alg gate, signature verify, iss/aud/exp/nbf checks, iat staleness cap, claim mapping, auth_time/acr/amr opt-in propagation - internal/service/oauth.go: dispatch for subject_token_type=id_token before the actor_token check; bypasses TrustedServiceValidator - server.go: registry construction, lifecycle close - config.go: ExternalIssuers section + per-entry validation loop - docs/identity-model.md: direct-federation-first guide - zeroid.yaml: example external_issuers stanza Tests: domain config validation, registry lifecycle (incl. fail-fast on unreachable JWKS), claim-mapping evaluator, algorithm allow-list gate. Full HTTP-stack rejection matrix deferred to follow-up — needs the shared-test-server helper to support a second federation-configured server alongside the existing broker-configured one.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Open
8 tasks
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements direct OIDC IdP federation, enabling ZeroID to verify upstream ID tokens directly via RFC 8693 token exchange. Key additions include an ExternalIssuerRegistry for managing JWKS clients, updated configuration validation, and comprehensive documentation on the identity model. Review feedback suggests aligning error codes with RFC 6749 by using invalid_grant for unknown issuers, simplifying reserved claim logic, and using strconv.FormatFloat instead of fmt.Sprintf to prevent scientific notation when processing numeric subject identifiers.
Resolve conflicts and migrate the new federation code to jwx v4 (per main PR highflame-ai#114, which upgraded the rest of the codebase). - config.go: keep both validation additions — wimse_domain (from main) runs before the external_issuers loop (this PR). - zeroid.yaml: keep both new top-level sections — attestation (from main) followed by the commented external_issuers example. - internal/service/oauth_external_idp.go: migrate from jwx/v2 to jwx/v4. Token.Issuer/IssuedAt are now (T, bool) tuples; AsMap dropped, use Claims() iter.Seq2. Replace jws.Parse-based alg gate with internal/jwtalg.Validate (also covers JWT-SVID alg allow-list) plus a small base64 header decoder for kid/alg lookup. - internal/service/external_issuer_registry_test.go: jwk.FromRaw → jwk.Import[jwk.Key]; jwa.ES256 → jwa.ES256().
Contributor
Author
|
Changes since last commit: Merged
Migrated the new federation code from jwx v2 to jwx v4 to match main #114:
PR is now |
- Add ErrUnknownExternalIssuer sentinel; wrap onto *OAuthError so callers
can errors.Is while the handler still picks up the OAuth error code via
errors.As. Kept invalid_request rather than invalid_grant — see review
reply for the RFC 6749 §5.2 reasoning.
- Add user_id_iss to the global reservedClaims map in oauth.go;
federation path now uses the single-sourced check.
- extractMappedClaimString: switch float64 path from fmt.Sprintf("%v")
to strconv.FormatFloat(tv, 'f', -1, 64); large numeric subjects no
longer render in scientific notation. int64 path moved to FormatInt
for symmetry. Regression test for 16-digit subject.
- Tests for the dual-signaling unknown-issuer error path.
Contributor
Author
|
Changes since last commit: addressed three Gemini review comments.
New tests:
|
…n items 2 & 3
- New zeroid.WithExternalIssuerJWKSOption build-time option threads an
authjwt.JWKSOption into the external-issuer registry. Lets tests inject
an insecure HTTP client when pointing the registry at a fake TLS JWKS.
- tests/integration/external_idp_test.go: end-to-end
TestExternalIDTokenFederation_EndToEnd with two subtests:
* federation_happy_path_emits_user_id_iss — Okta-shaped ID token ->
issued JWT carries user_id_iss=upstream iss, plus acr/amr/auth_time
propagation and token_exchange=external_id_token.
* broker_dispatch_unchanged_when_subject_token_type_is_omitted —
request without subject_token_type still routes to the broker
(proven by the broker-only "external principal exchange is not
configured" error fingerprint).
- tests/integration/external_idp_helpers_test.go: fake upstream IdP key
material, jws header builder, payload-segment decoder.
- helpers_test.go: capture sharedDBURL so federation tests can build a
second NewServer pointed at the same Postgres.
All four packages PASS under \`make test\`.
Contributor
Author
|
Changes since last commit: test-plan items 2 and 3 are now automated.
PR description updated — test-plan checkboxes ticked. |
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
external_issuersconfig;subject_token_type=id_tokentoken-exchange requests now route to a new verifier instead of the broker.user_id_iss(= upstream iss) for IdP-granular provenance; optionalauth_time/acr/amrpropagation when configured AND present upstream.Acceptance criteria status
external_issuersconfig section parses and validatessubject_token_type = id_token; rejects withinvalid_requestwhen no matching issuer is configuredmax_token_ageuser_id_isscarried on issued tokens; auth_time/acr/amr propagated per configdocs/identity-model.mdleads with direct federationuser_id_iss/acr/amr/auth_time/sub/user_emailflow through (Okta-shaped fixture)subject_token_typeis omittedTest plan
make testpasses (all four packages green)user_id_iss = https://upstream.idp.test(automated asTestExternalIDTokenFederation_EndToEnd/federation_happy_path_emits_user_id_iss)subject_token_typeis omitted (automated asTestExternalIDTokenFederation_EndToEnd/broker_dispatch_unchanged_when_subject_token_type_is_omitted)