From 526a646ff4b3c5d46e0ecea6318f8d6d147761e0 Mon Sep 17 00:00:00 2001 From: Michael DOUBEZ Date: Thu, 5 Jan 2023 23:58:27 +0100 Subject: [PATCH] Add config option to enable PKCE --- .../plugins/oic/OicSecurityRealm.java | 24 ++++++++-- .../plugins/oic/OicSecurityRealm/config.jelly | 3 ++ .../OicSecurityRealm/help-pkceEnabled.html | 3 ++ .../org/jenkinsci/plugins/oic/PluginTest.java | 44 ++++++++++++++++++- .../plugins/oic/ConfigurationAsCode.yml | 1 + .../plugins/oic/ConfigurationAsCodeExport.yml | 1 + 6 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/help-pkceEnabled.html diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index f7e26229..a09cfc13 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -165,6 +165,10 @@ public static enum TokenAuthMethod { client_secret_basic, client_secret_post }; */ private boolean sendScopesInTokenRequest = false; + /** Flag to enable PKCE challenge + */ + private boolean pkceEnabled = false; + /** old field that had an '/' implicitly added at the end, * transient because we no longer want to have this value stored * but it's still needed for backwards compatibility */ @@ -394,6 +398,10 @@ public boolean isSendScopesInTokenRequest() { return sendScopesInTokenRequest; } + public boolean isPkceEnabled() { + return pkceEnabled; + } + public boolean isAutoConfigure() { return "auto".equals(this.automanualconfigure); } @@ -536,6 +544,11 @@ public void setSendScopesInTokenRequest(boolean sendScopesInTokenRequest) { this.sendScopesInTokenRequest = sendScopesInTokenRequest; } + @DataBoundSetter + public void setPkceEnabled(boolean pkceEnabled) { + this.pkceEnabled = pkceEnabled; + } + @Override public String getLoginUrl() { //Login begins with our doCommenceLogin(String,String) method @@ -620,7 +633,7 @@ protected AuthorizationCodeFlow buildAuthorizationCodeFlow() { tokenAccessMethod = BearerToken.authorizationHeaderAccessMethod(); authInterceptor = new BasicAuthentication(clientId, Secret.toString(clientSecret)); } - return new AuthorizationCodeFlow.Builder( + AuthorizationCodeFlow.Builder builder = new AuthorizationCodeFlow.Builder( tokenAccessMethod, httpTransport, JSON_FACTORY, @@ -629,8 +642,13 @@ protected AuthorizationCodeFlow buildAuthorizationCodeFlow() { clientId, authorizationServerUrl ) - .setScopes(Arrays.asList(this.getScopes())) - .build(); + .setScopes(Arrays.asList(this.getScopes())); + + if(pkceEnabled) { + builder.enablePKCE(); + } + + return builder.build(); } /** diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.jelly b/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.jelly index 8520a7bb..a1724dc3 100644 --- a/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/config.jelly @@ -89,6 +89,9 @@ + + + diff --git a/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/help-pkceEnabled.html b/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/help-pkceEnabled.html new file mode 100644 index 00000000..2ce7e9ff --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/oic/OicSecurityRealm/help-pkceEnabled.html @@ -0,0 +1,3 @@ +
+ Enable Proof Key for Code Exchange (PKCE).. +
diff --git a/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java b/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java index ac48183c..7d3681de 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/PluginTest.java @@ -32,6 +32,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.matching; import static com.github.tomakehurst.wiremock.client.WireMock.notMatching; import static com.github.tomakehurst.wiremock.client.WireMock.post; import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor; @@ -117,7 +118,7 @@ public void setUp() { assertTrue("User should be part of group " + TEST_USER_GROUPS[1], user.getAuthorities().contains(TEST_USER_GROUPS[1])); verify(getRequestedFor(urlPathEqualTo("/authorization")) - . withQueryParam("scope", equalTo("openid email"))); + .withQueryParam("scope", equalTo("openid email"))); verify(postRequestedFor(urlPathEqualTo("/token")) .withRequestBody(notMatching(".*&scope=.*"))); } @@ -157,11 +158,50 @@ public void setUp() { webClient.goTo(jenkins.getSecurityRealm().getLoginUrl()); verify(getRequestedFor(urlPathEqualTo("/authorization")) - . withQueryParam("scope", equalTo("openid email"))); + .withQueryParam("scope", equalTo("openid email"))); verify(postRequestedFor(urlPathEqualTo("/token")) .withRequestBody(containing("&scope=openid+email&"))); } + @Test public void testLoginWithPkceEnabled() throws Exception { + KeyPair keyPair = createKeyPair(); + + wireMockRule.stubFor(get(urlPathEqualTo("/authorization")).willReturn( + aResponse() + .withStatus(302) + .withHeader("Content-Type", "text/html; charset=utf-8") + .withHeader("Location", jenkins.getRootUrl()+"securityRealm/finishLogin?state=state&code=code") + .withBody("") + )); + Map keyValues = new HashMap<>(); + keyValues.put(EMAIL_FIELD, TEST_USER_EMAIL_ADDRESS); + keyValues.put(FULL_NAME_FIELD, TEST_USER_FULL_NAME); + keyValues.put(GROUPS_FIELD, TEST_USER_GROUPS); + + wireMockRule.stubFor(post(urlPathEqualTo("/token")).willReturn( + aResponse() + .withHeader("Content-Type", "text/html; charset=utf-8") + .withBody("{" + + "\"id_token\": \""+createIdToken(keyPair.getPrivate(), keyValues)+"\"," + + "\"access_token\":\"AcCeSs_ToKeN\"," + + "\"token_type\":\"example\"," + + "\"expires_in\":3600," + + "\"refresh_token\":\"ReFrEsH_ToKeN\"," + + "\"example_parameter\":\"example_value\"" + + "}") + )); + + + TestRealm oidcSecurityRealm = new TestRealm(wireMockRule); + oidcSecurityRealm.setPkceEnabled(true); + jenkins.setSecurityRealm(oidcSecurityRealm); + webClient.goTo(jenkins.getSecurityRealm().getLoginUrl()); + + verify(getRequestedFor(urlPathEqualTo("/authorization")) + .withQueryParam("code_challenge_method", equalTo("S256")) + .withQueryParam("code_challenge", matching(".+"))); + } + @Test public void testLoginWithMinimalConfiguration() throws Exception { KeyPair keyPair = createKeyPair(); diff --git a/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCode.yml b/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCode.yml index ca51ba02..dbabdce4 100644 --- a/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCode.yml +++ b/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCode.yml @@ -19,3 +19,4 @@ jenkins: userNameField: userNameField rootURLFromRequest: true sendScopesInTokenRequest: true + pkceEnabled: true diff --git a/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCodeExport.yml b/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCodeExport.yml index 077ed28b..3310c1c3 100644 --- a/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCodeExport.yml +++ b/src/test/resources/org/jenkinsci/plugins/oic/ConfigurationAsCodeExport.yml @@ -7,6 +7,7 @@ escapeHatchGroup: "escapeHatchGroup" escapeHatchUsername: "escapeHatchUsername" fullNameFieldName: "fullNameFieldName" groupsFieldName: "groupsFieldName" +pkceEnabled: true rootURLFromRequest: true scopes: "scopes" sendScopesInTokenRequest: true