Add support for PKCE #265
Add support for PKCE #265
Conversation
Signed-off-by: Gaurav Dasson <[email protected]>
gdasson
requested review from
nacx,
ZackButcher and
sergicastro
as code owners
There was a problem hiding this comment.
Thanks! This looks great. A couple things:
- Can you run
make format? This should format the code and have CI pass thecheckstep. - One of the
e2etest suites uses Keycloak. Is there a way to configure it to force that a client uses PKCE? It would be great to have it enforced to make sure we're properly testing that path. Or we could create an additional suite, keycloak-pkce with this setup. WDYT?
| @@ -133,7 +133,7 @@ func TestSessionGenerator(t *testing.T) { | |||
| require.NotEqual(t, sg.GenerateNonce(), sg.GenerateNonce()) | |||
| }) | |||
| t.Run("static", func(t *testing.T) { | |||
| sg := NewStaticGenerator("sessionid", "nonce", "state") | |||
| sg := NewStaticGenerator("sessionid", "nonce", "state", "codeverifier") | |||
| require.Equal(t, sg.GenerateSessionID(), sg.GenerateSessionID()) | |||
| require.Equal(t, sg.GenerateState(), sg.GenerateState()) | |||
| require.Equal(t, sg.GenerateNonce(), sg.GenerateNonce()) | |||
There was a problem hiding this comment.
[nit] Update this test and the random one to also include a check for the code verifier.
There was a problem hiding this comment.
@nacx : Thanks for the review. Yes, there is a way to configure keycloak to enforce PKCE use by client. Infact, I also used keycloak for my local testing. In the UI, It's available under Client > Advanced > Proof Key for Code Exchange Code Challenge Method > Set it to S256. (However, I am not sure if the same setting is available through the Keycloak admin API)
Here is a screenshot from the UI:
There was a problem hiding this comment.
The client is created and configured here: https://github.com/istio-ecosystem/authservice/blob/main/e2e/keycloak/setup-keycloak.sh#L50-L58
Let's see if PKCE can be configured in that script.
Actually, there is probably no need to create another suite. The istio suite also uses Keycloak, so we can just change the keycloak suite and we should have all cases covered.
Testing ti locally should be easy:
make docker # the e2e tests need the update Docker image
make e2e/keycloak
# Or, if you want to see all requests/responses for the e2e tests logged:
E2E_TEST_OPTS="-v -count=1" make e2e/keycloak
Or you can start the infra and run the tests from your IDE: https://github.com/istio-ecosystem/authservice/blob/main/DEVELOPMENT.md#running-tests-from-your-ide
Keycloak will be accessible on https://localhost:9443 (admin / admin)
There was a problem hiding this comment.
Adding this to the keycloak setup script sould do the trick, to enforce PKCE server-side:
diff --git a/e2e/keycloak/setup-keycloak.sh b/e2e/keycloak/setup-keycloak.sh
index 7fc46af..ee3d340 100755
--- a/e2e/keycloak/setup-keycloak.sh
+++ b/e2e/keycloak/setup-keycloak.sh
@@ -51,6 +51,7 @@ set -ex
-s clientId="${CLIENT_ID}" \
-s secret="${CLIENT_SECRET}" \
-s "redirectUris=[\"${REDIRECT_URL}\"]" \
+ -s "attributes={\"pkce.code.challenge.method\":\"S256\"}" \
-s consentRequired=false \
--server "${KEYCLOAK_SERVER}" \
--realm "${REALM}" \There was a problem hiding this comment.
Added and ran e2e tests, they run fine. Also physically checked the setting in the UI before destroying infra.
There was a problem hiding this comment.
Nice! I run the e2e with this setting but without the changes in the PR and they were failing, complaining about the missing query params, so all good!
Signed-off-by: Gaurav Dasson <[email protected]>
…e tests Signed-off-by: Gaurav Dasson <[email protected]>
|
CI complains about project not being go mod tidied. Since we have code generation, the If you run locally |
Signed-off-by: Gaurav Dasson <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #265 +/- ##
=======================================
Coverage ? 90.71%
=======================================
Files ? 25
Lines ? 1615
Branches ? 0
=======================================
Hits ? 1465
Misses ? 107
Partials ? 43
|
Signed-off-by: Gaurav Dasson <[email protected]>
This PR adds support for PKCE (Proof Key for Code Exchange) to enhance the security of the OAuth 2.0 Authorization Code flow. It includes generating a code verifier and code challenge, and ensures these parameters are used during the authorization and token exchange processes. This update aligns with OAuth 2.0 best practices and improves compatibility with identity providers that require PKCE.
More details about PKCE here: