feat: add fixed-identity mode to cmd/conformance-client for hosted RP certification - #248
Open
osanderson wants to merge 7 commits into
Open
feat: add fixed-identity mode to cmd/conformance-client for hosted RP certification#248osanderson wants to merge 7 commits into
osanderson wants to merge 7 commits into
Conversation
… certification The hosted certification.openid.net suite requires a plan to be created through its own guided web UI, with a fixed alias/client_id/ certificate registered up front - incompatible with this driver's existing self-generate-everything-and-create-a-new-plan behavior, and confirmed against OIDF's own reference RP client that the plan creation REST endpoint needs authenticated access this driver doesn't have anyway. Adds -issuer (plus -client-id/-redirect-uri/-accounts-endpoint/ -client-cert/-client-key/-test-name): drives a single flow attempt against an already-created hosted plan's fixed values instead, skipping plan/module creation entirely - the flow-driving logic itself (discover, authorize-redirect interception, token exchange, resource call) is unchanged, extracted into a shared driveAuthorizationFlow so both modes use identical, unmodified protocol logic. 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 first live run of fixed-identity mode against the real hosted suite failed with "authorization response is missing iss" — turned out to be a scope mismatch: the suite validates the requested scope against the plan's own configured value and, on a mismatch, redirects to its own internal log-detail page instead of a normal OAuth error redirect, which this driver had no way to distinguish from a malformed response. Confirmed against the real suite's own log viewer and fixed by making scope configurable and documenting the failure mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mode Checked against OIDF's actual RP certification submission requirements (logs from both the suite and the client implementation) and the reference client's own evidence-capture approach (bare console output) to confirm this driver's -evidence-dir output already covers the client-implementation-log half. Along the way found that fixed-identity mode's evidence file falsely claimed "module instance was never created" for SUITE LOG — it does exist, it was created through the suite's own guided UI, this driver just never queried its ID via API. Corrected to point at the suite's plan-detail page instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed live against the real hosted suite with fapi2-security-profile-final-client-test-discovery-issuer-mismatch (suite graded PASS): this driver's own issuer anti-spoofing check (internal/metadata's ErrIssuerMismatch) already rejects a mismatched discovery document correctly, but runFixedIdentity treated any buildModuleClient failure as fatal — no evidence file, just a log.Fatalf exit — even though a discovery-time rejection is exactly the correct, desired outcome for a negative-test module, not a driver crash. Also fixes a real time-waster along the way: buildModuleClient's own discovery/algorithm-mismatch failure paths always called awaitVerdict, which polls the suite's api/info/<moduleID> endpoint — with fixed- identity mode's empty moduleID, that call was hitting a malformed "api/info/" URL and burning its own full 45s timeout before giving up, every time. New verdictOrDriverErr skips that poll entirely when moduleID is empty, since there was never a module ID to poll with. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed against openid.net/certification/how-to-submit-your-certification-request/: submission is a form at submissions.openid.net wanting ZIP files, one per profile, combining the suite's own official export (its "Publish for certification" button) with this driver's own -evidence-dir output — OIDF's instructions explicitly require both, and its one-file-per-test naming rule already matches what -evidence-dir produces without any renaming. Verified end-to-end against the real hosted suite: ran all 20 modules of the fapigo-sp-rp-mtls-mtls plan through -issuer mode, every one suite-graded PASS, -evidence-dir producing one correctly-detailed log file per module throughout — including every negative-test module this session's earlier fixes (-scope, verdictOrDriverErr, the buildModuleClient-failure handling in runFixedIdentity) were needed for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewed against the sample RP app and this evidence file directly:
the DRIVER line alone states an outcome ("completed successfully" or a
specific error) but never shows what actually happened to reach it —
no record of the requests/responses the client made. Confirmed by
re-checking the OIDF reference client's own source
(gitlab.com/openid/sample-openbanking-client-nodejs entrypoint.js): it
logs every PAR request/response, the authorization URL, the full
authentication response query, and the specific checks run against the
callback — all of which our evidence was missing.
New interactionRecorder + loggingRoundTripper (interactions.go) wraps
the shared *http.Client's own Transport once, so every request the
driver makes from that point on — discovery, JWKS, PAR, the authorize
redirect (including its Location header), token exchange, the resource
call — is recorded generically, with no call-site-specific
instrumentation. Bearer credentials (access_token, refresh_token,
client_assertion, client_secret) are redacted before recording;
everything else, including a full id_token, is kept as-is since its
claims are exactly what most of these tests are about.
Verified live against the real hosted suite: reran
fapi2-security-profile-final-client-test-happy-path, suite still
graded PASS (confirms the new logging is transparent to actual
behavior), and the resulting evidence file now shows the complete
discover/PAR/authorize/token/JWKS/accounts transcript.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's lint job flagged the plain switch{case cl == nil:...} this
session's earlier commit introduced.
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
certification.openid.netsuite requires a plan to be created through its own guided web UI first, with a fixed alias/client_id/certificate registered up front. This driver's existingrun()always self-generates a random identity and always creates its own new plan viaPOST /api/plan— incompatible with a plan that already exists with fixed values, and confirmed (both against the live hosted UI and OIDF's own official reference RP client,sample-openbanking-client-nodejs) that the hosted suite's plan-creation endpoint needs authenticated access this driver has no support for anyway.-issuer: drives a single flow attempt against externally-supplied-client-id/-redirect-uri/-accounts-endpoint/-client-cert/-client-key/-test-name, skipping plan/module creation entirely. No suite-graded verdict is available in this mode (no module ID to poll) — the suite's own plan-detail page is the source of truth for PASS/FAIL, same as it's always been for certification submission.driveAuthorizationFlowso both the existing self-generating mode and the new fixed-identity mode run identical, unmodified protocol logic.runModule's own behavior is byte-for-byte unchanged by this refactor.tls.LoadX509KeyPairwhen-client-cert/-client-keyare set, instead ofselfSignedClientCertgenerating a throwaway one — sametls.Certificateshape, somtlsSuiteHTTPClientneeds no change.private_key_jwtprofiles (private key+DPoP, private key+MTLS) — this pass only has a working credential path forclient_auth_type=mtls, which needs no signed assertion.keys.NewKeyManagerFromSignersis the existing, already-tested mechanism a follow-up would build on.conformance/client/scripts/README.mdupdated: replaces the previous (unverified, and now confirmed incorrect) guidance suggesting-suite=https://certification.openid.net/could be used with the existing self-generating mode, with the real, confirmed process.Test plan
TestFixedIdentityConfigValidate— every required-companion-flag combination under-issuer(missing client-id/redirect-uri/accounts-endpoint, missing cert/key under mTLS, all present, reports every missing field at once)go build ./...,go vet ./cmd/conformance-client/...,gofmt -l cmd/conformance-clientall cleango test ./cmd/conformance-client/...and fullgo test ./...both passfapigo-sp-rp-mtls-mtlsplan (MTLS+MTLS) — not yet run as of this PR; local dev-mode suite wasn't running in this environment to do a live smoke test first, so the architectural claim (no plan/module REST calls) is verified by direct code tracing rather than an integration test in this pass🤖 Generated with Claude Code
https://claude.ai/code/session_01KWEP7ddZm9T8pVotywwFfF