Robert Navarro
opened 1 week ago
Commits were merged into target branch
-
Reviewed commit baa51a05. No actionable correctness or security issues found in the patch. Checked verifier randomness, S256 challenge generation, state validation before verifier consumption, token-request serialization, and inherited connector behavior against Nimbus 11.28 sources and RFC 7636.
Validation:
mvn -U -pl server-plugin/server-plugin-sso-openid -am -DskipTests compilepassed, including reactor dependencies. Live provider logins were not exercised. No code changes. -
-
Previous Value Current Value CREATE_MERGE_COMMIT
SQUASH_SOURCE_BRANCH_COMMITS
-
Previous Value Current Value SQUASH_SOURCE_BRANCH_COMMITS
CREATE_MERGE_COMMIT
-
| Submitter | Robert Navarro |
| Target | main |
| Source | forks/rnavarro/server:feat/oidc-pkce |
Reviewers
Merge Strategy
Create Merge Commit
Watchers (3)
I'm running OneDev 16.8.6 with Kanidm as the OpenID Connect provider. Kanidm enforces PKCE on OAuth2 clients by default, and OneDev's OpenID connector sends no
code_challenge, so I turned PKCE off for the OneDev client before the first login. I didn't try an unpatched login with PKCE enforced; Kanidm's code rejects an authorization request withoutcode_challengein that mode. I searched the tracker and found no existing issue or PR for PKCE.This adds PKCE with the S256 method to
OpenIdConnector.buildAuthUrlcreates a code verifier for each login and keeps it in the session next to the state, and the authorization request carries the matchingcode_challenge.handleAuthResponsetakes the verifier out of the session and sends it ascode_verifieron the token request. A response without a stored verifier is rejected with the existing "Unsolicited OIDC authentication response" message.The GitHub, Entra ID and Twitch connectors extend
OpenIdConnector, so they pick this up too. Discord extendsSsoConnectordirectly and is unchanged.PKCE is always on. RFC 7636 asks clients to send it to every server, and RFC 6749 requires servers to ignore parameters they don't recognize, so a provider without PKCE support sees a normal code flow. GitHub and Entra ID document S256 support. Twitch doesn't mention PKCE and I couldn't test it. If you'd prefer a per-provider setting to turn it off, I can add one.
I applied the patch to my 16.8.6 server and turned PKCE enforcement back on for the Kanidm client. The SSO login works, and the authorization request carries
code_challenge_method=S256. The module has no test sources, so I didn't add a unit test.I used an AI coding assistant to help write this change, and I reviewed and tested it myself.