RFC 8705 Support - #8
Conversation
Adds support for OAuth 2.0 Mutual-TLS Client Authentication and Certificate-Bound Access Tokens, letting a client authenticate with a certificate presented during the TLS handshake instead of a shared secret. - Added `TLS_CLIENT_AUTH` and `SELF_SIGNED_TLS_CLIENT_AUTH` to `ClientAuthMethod`, with an `isMutualTls()` helper. - Added the `MutualTlsCertificate` value object, which carries the certificate, private key and optional passphrase, exposes the Guzzle request options that present it during the handshake, and computes the base64url SHA-256 thumbprint used in the `cnf` / `x5t#S256` claim. - Added the `MutualTls` trait, which resolves the mutual-TLS endpoint aliases, suppresses the client secret when the certificate authenticates the client, and verifies that an access token is bound to the configured certificate. - Suppressed the client secret across every token endpoint request: authorization code, refresh, token exchange, introspection and revocation. The latter two previously hardcoded Basic authentication. - Parsed `mtls_endpoint_aliases` and `tls_client_certificate_bound_access_tokens` during auto discovery, and used the aliased endpoints automatically. - Registered the mutual-TLS authentication method during dynamic registration, and stopped requiring a client secret that mutual-TLS clients do not have. - Selected an announced mutual-TLS method only when a certificate is configured, so existing secret-based clients keep their current behaviour. - Added a self-contained Docker test stack (Keycloak, certificate generation, `docker/run-tests.sh`) covering the happy paths plus the negative cases: a request without a certificate, a certificate with a mismatched subject DN, and a certificate-bound token replayed without its certificate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UGmjsaiMbu1LxgCzWB9Djx
…certificate `cert_path` was only applied when `verify_ssl` was `false`, because `($verify_ssl ?: $cert_path) ?? false` short-circuits to `true` whenever `verify_ssl` is true. A CA bundle was therefore silently ignored in the default configuration, and the only way to make it take effect was to also disable verification — which is exactly what the README documented. The bundle is now always used when one is given, so a private CA can be trusted without turning verification off: | verify_ssl | cert_path | before | after | |------------|-----------|------------|------------| | true | null | true | true | | true | bundle | true | bundle | | false | null | false | false | | false | bundle | bundle | bundle | - Replaced the expression with `$this->cert_path ?? $this->verify_ssl`, so verification is only disabled when it is turned off *and* no bundle is given. No configuration verifies less than it did before. - Updated Example 3, the Development Environments section and the constructor PHPDoc, which all documented `verify_ssl: false` as the way to enable `cert_path`. - Added regression tests covering the four combinations, including one asserting that no configuration verifies less than the original expression did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UGmjsaiMbu1LxgCzWB9Djx
The docker/ stack is local development tooling rather than part of the distributed package, so remove it from version control and ignore it. The files stay on disk for local RFC 8705 testing. Drop the README section that pointed at ./docker/run-tests.sh, since that script is no longer present in a fresh clone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Port the mutual-TLS checks from the local Docker harness into tests/Feature so the RFC 8705 support is covered by the repo's own suite. The tests generate their own certificates and configure clients without a provider_url, so they need neither fixture files nor a running provider. Covered: certificate request options and thumbprints, auth method selection, dropping the client secret under mutual TLS, endpoint aliases, certificate bound token verification, and that a CA bundle keeps verifying the provider even alongside verify_ssl: false. The handshake behaviour of a real provider — a token request being rejected without a certificate, and a bound token being refused at userinfo — still needs a live mutual-TLS provider and is not covered here. Add a setProperty() reflection helper to TestCase alongside getProperty(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docker/ ignore rule only matters to this working copy, so keep it out of the committed .gitignore and move it to .git/info/exclude. The stack is still ignored locally; the shared ignore file returns to upstream defaults. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…applies Two RFC 8705 endpoint-alias bugs. `tls_client_certificate_bound_access_tokens` was parsed from discovery and sent during registration, but never consulted when resolving endpoints: `mtlsEndpoint()` keyed solely off the presence of a certificate. A client authenticating with a secret while asking for certificate-bound tokens therefore kept using the regular token endpoint, where no certificate is requested during the handshake, so the token could not be bound. Resolution now goes through `usesMutualTls()`, covering both reasons the aliases exist: mutual-TLS client authentication, and certificate-bound access tokens. This also stops a certificate that is used for neither purpose from diverting endpoints, which the old check did. `register()` posted to `registration_endpoint` directly while the other four endpoints were resolved through the aliases. It is where a mutual-TLS client announces its `token_endpoint_auth_method`, so it is the call that most needs the certificate presented. Verified against the local Keycloak stack (29/29) and the Pest suite, including an end-to-end check that `register()` posts to the aliased URL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 30 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe client adds RFC 8705 mutual-TLS authentication and certificate-bound access-token support. It adds certificate handling, endpoint discovery and registration support, mutual-TLS token routing, certificate verification, tests, and documentation. ChangesMutual-TLS support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MutualTls
participant Token
participant Provider
Client->>MutualTls: resolve authentication method and endpoint
MutualTls->>Token: apply certificate request options
Token->>Provider: send token, introspection, or revocation request
Provider-->>Token: return OAuth response
Token-->>Client: return parsed result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/Traits/DynamicRegistration.php (1)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote the extra RFC 8705 registration metadata.
RFC 8705 requires more than the authentication method at registration.
tls_client_authneeds exactly one subject identifier, for exampletls_client_auth_subject_dnortls_client_auth_san_dns(section 2.1.2).self_signed_tls_client_authneedsjwksorjwks_uri(section 2.2.2). A caller can supply these through$params, so registration still works. Document that requirement in the README section for dynamic registration, so users do not get an opaque provider error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Traits/DynamicRegistration.php` around lines 35 - 46, Update the README section documenting dynamic registration to explain the RFC 8705 metadata requirements: tls_client_auth must include exactly one subject identifier such as tls_client_auth_subject_dn or tls_client_auth_san_dns, while self_signed_tls_client_auth must include jwks or jwks_uri. Note that callers provide these fields through the registration parameters.tests/Feature/MutualTlsTest.php (1)
53-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
tempnamfile is left behind.
tempnam()creates a file at the returned path. Appending.pemproduces a different path, so the created file is never used and never deleted. Each test run leaks one empty temp file. Reuse the temp path directly, or build the bundle inside the existing test directory used bycertificate().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/MutualTlsTest.php` around lines 53 - 59, Update the bundle setup in the test “certificate without a separate private key omits the ssl_key option” to avoid creating an unused tempnam file: reuse the path returned by tempnam directly when writing the PEM bundle, or create it within the existing certificate test directory. Ensure the temporary bundle is cleaned up after the test.tests/Pest.php (1)
67-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the generated key file and check the OpenSSL results.
mkdiruses 0700, butfile_put_contentswrites the private key with the default umask, usually 0644. Restrict the key file to match the directory intent. Also,openssl_pkey_new,openssl_csr_new, andopenssl_csr_signreturnfalseon failure. An unchecked failure surfaces later as a confusing certificate error instead of a clear test failure.♻️ Proposed change
if (!isset($certificates[$name])) { $key = openssl_pkey_new(['private_key_bits' => 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); + if ($key === false) { + throw new RuntimeException('Unable to generate a test key pair: ' . openssl_error_string()); + } $csr = openssl_csr_new(['commonName' => $name], $key, ['digest_alg' => 'sha256']); openssl_x509_export(openssl_csr_sign($csr, null, $key, 1, ['digest_alg' => 'sha256']), $certificate); openssl_pkey_export($key, $private_key); @@ file_put_contents("$directory/$name.crt", $certificate); file_put_contents("$directory/$name.key", $private_key); + chmod("$directory/$name.key", 0600);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Pest.php` around lines 67 - 89, Update certificate() to validate the results of openssl_pkey_new, openssl_csr_new, and openssl_csr_sign, failing immediately with a clear test error if any returns false. After writing the private key in certificate(), restrict its file permissions to 0600, while preserving the existing certificate caching and MutualTlsCertificate construction.src/Traits/MutualTls.php (1)
58-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-enum entries in
token_endpoint_auth_methods_supportedcause a TypeError.The closure declares
ClientAuthMethod $method.token_endpoint_auth_methods_supportedis a public mutable array, and a caller can pass method names as strings.AutoDiscoverynormalizes discovered values, but constructor input is not normalized. Accept both shapes to keep the selection tolerant.♻️ Proposed change
return collect($this->token_endpoint_auth_methods_supported) - ->first(static fn (ClientAuthMethod $method): bool => $method->isMutualTls()); + ->map(static fn (ClientAuthMethod|string $method): ?ClientAuthMethod => $method instanceof ClientAuthMethod + ? $method + : ClientAuthMethod::tryFrom($method)) + ->first(static fn (?ClientAuthMethod $method): bool => $method?->isMutualTls() === true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Traits/MutualTls.php` around lines 58 - 70, Update getClientAuthMethod so its token_endpoint_auth_methods_supported selection tolerates both ClientAuthMethod instances and string method names in the public mutable array. Remove the closure’s strict ClientAuthMethod parameter type, validate or normalize each entry before calling isMutualTls, and preserve returning the first mutual-TLS method or null.src/MutualTlsCertificate.php (1)
87-99: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the
preg_replaceresult before decoding.
preg_replacereturnsnullwhen the PCRE engine fails, for example on backtrack limits with a large bundle.base64_decode(null)then raises a deprecation in PHP 8.1+ and returns an empty string. Cast the result so the failure path stays a cleanOIDCClientException.♻️ Proposed change
- $der = base64_decode(preg_replace('/\s+/', '', $matches[1]), true); + $der = base64_decode((string) preg_replace('/\s+/', '', $matches[1]), true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MutualTlsCertificate.php` around lines 87 - 99, Guard the whitespace-stripping preg_replace result in the certificate parsing flow before passing it to base64_decode, using a string fallback when preg_replace returns null. Keep the existing first-certificate extraction behavior and ensure failures continue to reach the clean OIDCClientException path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Traits/Token.php`:
- Around line 93-99: Update the introspection and revokeToken authentication
flows to use applyMutualTlsClientAuth() instead of relying on
usesMutualTlsClientAuth() before falling back to Basic authentication. This must
raise the established OIDCClientException when mutual-TLS is configured without
a certificate, while preserving certificate-based client authentication and
existing Basic-auth behavior for non-mutual-TLS clients.
In `@tests/Feature/MutualTlsTest.php`:
- Around line 154-160: Update the `$data['client_id']` assertion in the `no
client secret is sent under mutual tls` test to use a null-coalescing access,
preserving the expected `mtls-client` value while avoiding PHPStan’s
offsetAccess.notFound error after the by-reference `invokeMethod` call.
---
Nitpick comments:
In `@src/MutualTlsCertificate.php`:
- Around line 87-99: Guard the whitespace-stripping preg_replace result in the
certificate parsing flow before passing it to base64_decode, using a string
fallback when preg_replace returns null. Keep the existing first-certificate
extraction behavior and ensure failures continue to reach the clean
OIDCClientException path.
In `@src/Traits/DynamicRegistration.php`:
- Around line 35-46: Update the README section documenting dynamic registration
to explain the RFC 8705 metadata requirements: tls_client_auth must include
exactly one subject identifier such as tls_client_auth_subject_dn or
tls_client_auth_san_dns, while self_signed_tls_client_auth must include jwks or
jwks_uri. Note that callers provide these fields through the registration
parameters.
In `@src/Traits/MutualTls.php`:
- Around line 58-70: Update getClientAuthMethod so its
token_endpoint_auth_methods_supported selection tolerates both ClientAuthMethod
instances and string method names in the public mutable array. Remove the
closure’s strict ClientAuthMethod parameter type, validate or normalize each
entry before calling isMutualTls, and preserve returning the first mutual-TLS
method or null.
In `@tests/Feature/MutualTlsTest.php`:
- Around line 53-59: Update the bundle setup in the test “certificate without a
separate private key omits the ssl_key option” to avoid creating an unused
tempnam file: reuse the path returned by tempnam directly when writing the PEM
bundle, or create it within the existing certificate test directory. Ensure the
temporary bundle is cleaned up after the test.
In `@tests/Pest.php`:
- Around line 67-89: Update certificate() to validate the results of
openssl_pkey_new, openssl_csr_new, and openssl_csr_sign, failing immediately
with a clear test error if any returns false. After writing the private key in
certificate(), restrict its file permissions to 0600, while preserving the
existing certificate caching and MutualTlsCertificate construction.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 32a1659a-576a-40b0-9fa0-c18e7e7f0008
📒 Files selected for processing (11)
README.mdsrc/Client.phpsrc/ClientAuthMethod.phpsrc/MutualTlsCertificate.phpsrc/Traits/AutoDiscovery.phpsrc/Traits/DynamicRegistration.phpsrc/Traits/MutualTls.phpsrc/Traits/Token.phptests/Feature/MutualTlsTest.phptests/Pest.phptests/TestCase.php
Introspection and revocation used usesMutualTlsClientAuth(), which is false when a mutual-TLS method is configured without a certificate. Both then fell back to Basic authentication with a secret a mutual-TLS client does not have, and the provider answered with an opaque 401. They now go through applyMutualTlsClientAuth() like refreshToken() and requestTokens() already did, so the misconfiguration raises a clear OIDCClientException instead. The client_id override both methods accept is preserved. getClientAuthMethod() typed its closure parameter as ClientAuthMethod, but token_endpoint_auth_methods_supported is a public array typed only by docblock and constructor input is never normalised — only discovered values are. Method names passed as strings raised a TypeError, so entries are now normalised through tryFrom(). preg_replace() returns null when the PCRE engine fails, for example on a backtrack limit with a large bundle. base64_decode(null) is deprecated on the PHP versions this package supports and yields an empty string, so the result is cast to keep the clean OIDCClientException path. Tests: tempnam() creates a file, so appending '.pem' leaked an empty temp file on every run; the returned path is reused and cleaned up instead. The openssl calls behind the test certificates are checked for failure, the generated key is restricted to 0600 to match the 0700 directory, and one assertion uses a coalescing access that PHPStan can follow through invokeArgs(). README: document the RFC 8705 metadata callers must pass to register(), a subject identifier for tls_client_auth (section 2.1.2) and jwks/jwks_uri for self_signed_tls_client_auth (section 2.2.2). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
revokeToken() called acceptJson() without asForm(), so the body went out as
JSON. RFC 7009 section 2.1 requires application/x-www-form-urlencoded, and a
provider that cannot read client_id from the body rejects the request with
invalid_client — revocation never worked for any client.
Verified against Keycloak: same endpoint and same credentials, only the encoding
differs.
acceptJson only: 401 {"error":"invalid_client", ...}
asForm + acceptJson: 200
This predates the RFC 8705 work, but the mutual-TLS tests are what reached the
revocation endpoint for the first time and surfaced it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
RFC 8705 - Mutual TLS Client Auth against an IdP.
What was added
Clients can now authenticate with a certificate presented during the TLS handshake instead of a shared secret. Both RFC 8705 methods are supported:
ClientAuthMethod::TLS_CLIENT_AUTH- certificate issued by a CA the provider trustsClientAuthMethod::SELF_SIGNED_TLS_CLIENT_AUTH- provider holds the certificate itselfCertificate-bound access tokens:
verifyCertificateBinding()checks a token's cnf.x5t#S256 thumbprint against the configured certificate. It returns false if the token isn't bound at all, throws if it's bound to someone else's certificate. The authoritative check still belongs to the resource server; this is a client-side sanity check.mtls_endpoint_aliases handling: Providers may serve mTLS endpoints on a separate host/port that requests a client certificate. Those aliases are picked up from discovery and used automatically for token, userinfo, introspection, revocation, and registration. Certificate-bound tokens need the aliased token endpoint too, even when the client authenticates with a secret, that case is handled.
Dynamic registration now sends
token_endpoint_auth_methodandtls_client_certificate_bound_access_tokens, and no longer errors out when the provider returns no client_secret (mTLS clients don't get one).Other changes
cert_pathused to only take effect whenverify_sslwasfalse. The documented way to trust a private CA (cert_path + verify_ssl: false) actually disabled verification entirely and silently trusted any certificate. A CA bundle is now always honored, and verification is only off whenverify_ssl: falseand no bundle is set.Tests
31 new Pest tests in tests/Feature/MutualTlsTest.php, covering thumbprint computation (including certs with a bundled key), Guzzle option construction, endpoint alias resolution, secret suppression, registration, binding verification, and the verify semantics above. Test certificates are generated at runtime via openssl.
This is tested and working with Keycloak.
Summary by CodeRabbit
New Features
Documentation