Repository navigation
Answer invalid_client when token endpoint client authentication fails - #363
Merged
Merged
Conversation
Nothing tested a token request without a secret, or with PKCE, end to end. ClientRepositoryTest covers validateClient() directly and the conformance plans cover successful client_secret_basic and client_secret_post flows, but code_verifier appeared nowhere in tests/. Add two integration tests, written from the ones Ioannis Igoumenos contributed in PR 362 (981ea18) and left out of its merge because they needed a signing key CI does not have. A confidential client, with and without PKCE, is refused without its secret and with a wrong one, and gets tokens with the right one. With PKCE it is also refused with a wrong verifier. Each refusal asserts the error type. The tests generate their signing key pair in a temporary directory and point SimpleSAMLphp's certdir at it, so they need neither vendor/ nor the keys in docker/ssp/, which wip-version-7 no longer tracks. They boot the module Kernel instead of Services\Container, which wip-version-7 no longer has, so the same file runs on both branches. Merging this into wip-version-7 needs one edit, which git will not ask for: ERROR_WITHOUT_CLIENT_AUTHENTICATION must become invalid_client, the answer that branch gives. master answers access_denied. Co-authored-by: Ioannis Igoumenos <ioigoume@gmail.com> Assisted-by: Claude Code (claude-opus-5-5, implementation & canary mutations)
The token endpoint answered access_denied to a confidential client which presented no authentication, and to one whose client assertion did not hold: no JWKS available, a signature which did not verify, or the wrong Issuer, Subject or Audience. RFC 6749 section 5.2 names invalid_client for the first and RFC 7521 section 4.2.1 requires it for the second. A wrong secret already got it, and wip-version-7 answers it for all of these. The status stays HTTP 401, and League adds its WWW-Authenticate challenge when the request used the Authorization header. The response no longer carries the hint access_denied had, and a client which compares the error member with the old value needs the new one. Since ErrorResponder logs nothing on this branch, the rule logs the reason for each of these refusals at warning level with the client_id. The backstop in AuthCodeGrant answers the same. No request reaches it today, and it runs before the code is decrypted, so it cannot tell an invalid grant from missing authentication as wip-version-7's does. The end-to-end test now has one constant for the answer, and adds the challenge and private_key_jwt, whose checks in the rule had no test on this branch. It passes unedited on wip-version-7. Assisted-by: Claude Code (claude-opus-5-5, implementation & integration tests) Assisted-by: Google Antigravity (gemini-3.8-flash-high, review)
The release adds the option refusing the implicit flow for confidential clients. The token endpoint now answers invalid_client where it answered access_denied, which a client comparing the error member has to follow, and the reason has moved from the response to the log. Assisted-by: Claude Code (claude-opus-5-5, upgrade note)
15fb6b4 logs why client authentication failed. One reason quotes the message of the signature check, and the library's answer to an unknown algorithm quotes the alg header of an assertion nobody has verified yet. SimpleSAMLphp's file log handler writes messages as they are, so a caller with no credentials who knows the client_id of a client with a usable JWKS could write lines of their own into the log. The reason is now cut at 1000 bytes, and every run of bytes outside printable ASCII becomes one space. Stripping control characters alone would leave U+0085 and U+2028, which some consumers break lines on. wip-version-7 logs the same message as it is, in AuthenticatedOAuth2ClientResolver. A merge of this keeps that branch's rule, so the new test fails there until that line gets the same treatment. Assisted-by: Claude Code (claude-opus-5-5, implementation & integration tests) Assisted-by: Google Antigravity (gemini-3.8-flash-high, review)
A client assertion which did not parse left ClientAuthenticationRule as an uncaught JwsException. Its message quotes raw claim values, and SimpleSAMLphp's exception handling logs it as it is, so a caller with no credentials who knows the client_id of a confidential client could write lines of their own into the log, with no JWKS or valid signature involved. The client also got the error page instead of a token error. The rule now catches JwsException around the parse and refuses like any other failed client authentication: invalid_client, with the reason cut and reduced to printable ASCII in the log. This also covers an expired assertion, a missing claim, and a Subject other than the Issuer. The catch stops at JwsException, which the library wraps every failure of the token in, so a parser which can not start stays an error of the OP. The upgrade note also said the old answers carried a hint. They had no such member: OidcServerException appends the hint to error_description, and the note now says that. wip-version-7 refuses these in AuthenticatedOAuth2ClientResolver. Merging this keeps that branch's rule, so both log tests fail there until the line named in af7c2aa gets the same treatment. Assisted-by: Claude Code (claude-opus-5-5, implementation & integration tests) Assisted-by: OpenAI Codex (gpt-6.1-sol, xhigh review)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
============================================
+ Coverage 54.64% 58.71% +4.06%
- Complexity 1516 1517 +1
============================================
Files 141 141
Lines 6097 6114 +17
============================================
+ Hits 3332 3590 +258
+ Misses 2765 2524 -241 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
A confidential client which presents no client authentication, or a client assertion which fails validation, should get
invalid_client(RFC 6749 section 5.2, RFC 7521 section 4.2.1), andwip-version-7already answers that. master answeredaccess_denied, and an assertion which did not parse ended on the SimpleSAMLphp error page with HTTP 500.Changes
invalid_client, HTTP 401, with aWWW-Authenticatechallenge when the request used theAuthorizationheader. This covers no client authentication, no JWKS to check the assertion with, a signature which does not verify, the wrong Issuer or Audience, and an assertion which can not be parsed (expired, a missing or malformed claim, a Subject other than its Issuer). The backstop inAuthCodeGrant, unreachable today, answers the same.error_descriptionno longer says why. The reason goes to the OP log atwarninglevel with theclient_id, cut at 1000 bytes and reduced to printable ASCII, because it can quote an assertion nobody has verified yet.invalid_requestanswers, for a wrong assertion type and a reused assertion, are unchanged.ConfidentialClientTokenEndpointTestchecks these answers and the log lines through the real authorization server. It grew out of the two token endpoint tests Ioannis Igoumenos wrote in Add an option refusing the implicit flow for confidential clients #362. No test makes the parser fail to start.Version 6.4 to 6.5section.Client-visible
A client which compares the
errormember withaccess_deniedneeds the new value. This is meant for 6.5.0.Checks
On PHP 8.3: CI's integration step 17 tests / 94 assertions, unit suite 545 tests / 980 assertions, phpcs and psalm clean. No conformance plan in CI expects the old answer; the plans run on this PR.
Merging into wip-version-7
Keep that branch's
ClientAuthenticationRuleandAuthCodeGrant. The test file arrives unedited, and its two log tests fail until the log line inAuthenticatedOAuth2ClientResolver::forAnySupportedMethod()gets the same treatment.docs/6-oidc-upgrade.mdis CRLF here and LF there, so it conflicts over the whole file: keep that branch's and insert the new section aboveVersion 6.4.3 to 6.4.4.Assisted-by: Claude Code (claude-opus-5-5, implementation, integration tests & upgrade note)
Assisted-by: OpenAI Codex (gpt-6.1-sol, xhigh review)
Assisted-by: Google Antigravity (gemini-3.8-flash-high, review)