Skip to content

Commit

Permalink
Reimplement PKCE to serialize verifier in session (#291)
Browse files Browse the repository at this point in the history
  • Loading branch information
@michael-doubez
michael-doubez committed Apr 8, 2024
1 parent 94cdf03 commit 4124503
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 20 deletions.
11 changes: 6 additions & 5 deletions src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,10 +824,6 @@ protected AuthorizationCodeFlow buildAuthorizationCodeFlow() {
authorizationServerUrl)
.setScopes(Arrays.asList(this.getScopes()));

if (pkceEnabled) {
builder.enablePKCE();
}

return builder.build();
}

Expand Down Expand Up @@ -867,6 +863,9 @@ public HttpResponse onSuccess(String authorizationCode, AuthorizationCodeFlow fl
AuthorizationCodeTokenRequest tokenRequest = flow.newTokenRequest(authorizationCode)
.setRedirectUri(buildOAuthRedirectUrl())
.setResponseClass(OicTokenResponse.class);
if (this.pkceVerifierCode != null) {
tokenRequest.set("code_verifier", this.pkceVerifierCode);
}
if (!sendScopesInTokenRequest) {
tokenRequest.setScopes(Collections.emptyList());
}
Expand Down Expand Up @@ -907,7 +906,9 @@ public HttpResponse onSuccess(String authorizationCode, AuthorizationCodeFlow fl
return HttpResponses.error(500, e);
}
}
}.commenceLogin(isNonceDisabled(), buildAuthorizationCodeFlow());
}.withNonceDisabled(isNonceDisabled())
.withPkceEnabled(isPkceEnabled())
.commenceLogin(buildAuthorizationCodeFlow());
}

@SuppressFBWarnings(
Expand Down
98 changes: 86 additions & 12 deletions src/main/java/org/jenkinsci/plugins/oic/OicSession.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,18 @@
import com.google.api.client.auth.oauth2.AuthorizationCodeRequestUrl;
import com.google.api.client.auth.oauth2.AuthorizationCodeResponseUrl;
import com.google.api.client.auth.openidconnect.IdToken;
import com.google.api.client.util.Base64;
import com.google.common.annotations.VisibleForTesting;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.model.Failure;
import hudson.remoting.Base64;
import java.io.IOException;
import java.io.Serializable;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.UUID;
import javax.servlet.http.HttpSession;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.DoNotUse;
import org.kohsuke.stapler.HttpRedirect;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.Stapler;
Expand All @@ -60,13 +61,13 @@ abstract class OicSession implements Serializable {
* An opaque value used by the client to maintain state between the request and callback.
*/
@VisibleForTesting
String state = Base64.encode(UUID.randomUUID().toString().getBytes(StandardCharsets.UTF_8))
String state = Base64.encodeBase64URLSafeString(UUID.randomUUID().toString().getBytes(StandardCharsets.UTF_8))
.substring(0, 20);
/**
* More random state, this time extending to the id token.
*/
@VisibleForTesting
String nonce = UUID.randomUUID().toString();
String nonce;
/**
* The url the user was trying to navigate to.
*/
Expand All @@ -79,10 +80,79 @@ abstract class OicSession implements Serializable {
* ID Token needed to logout from OpenID Provider
*/
private String idToken;
/**
* PKCE Verifier code if activated
*/
String pkceVerifierCode;

OicSession(String from, String redirectUrl) {
this.from = from;
this.redirectUrl = redirectUrl;
this.withNonceDisabled(false);
}

/**
* Activate or disable Nonce
*/
public OicSession withNonceDisabled(boolean isDisabled) {
if (isDisabled) {
this.nonce = null;
} else {
if (this.nonce == null) {
this.nonce = UUID.randomUUID().toString();
}
}
return this;
}

/**
* Helper class to compute PKCE Challenge
*/
private static class PKCE {
/** Challenge code of verifier code */
public String challengeCode;
/** Methode used for computing challenge code */
public String challengeCodeMethod;

public PKCE(String verifierCode) {
try {
byte[] bytes = verifierCode.getBytes(StandardCharsets.UTF_8);
MessageDigest md = MessageDigest.getInstance("SHA-256");
md.update(bytes, 0, bytes.length);
byte[] digest = md.digest();
challengeCode = Base64.encodeBase64URLSafeString(digest);
challengeCodeMethod = "S256";
} catch (NoSuchAlgorithmException e) {
challengeCode = verifierCode;
challengeCodeMethod = "plain";

Check warning on line 127 in src/main/java/org/jenkinsci/plugins/oic/OicSession.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 125-127 are not covered by tests
}
}

/**
* Generate base64 verifier code
*/
public static String generateVerifierCode() {
try {
SecureRandom random = SecureRandom.getInstanceStrong();
byte[] code = new byte[32];
random.nextBytes(code);
return Base64.encodeBase64URLSafeString(code);
} catch (NoSuchAlgorithmException e) {
return null;

Check warning on line 141 in src/main/java/org/jenkinsci/plugins/oic/OicSession.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 140-141 are not covered by tests
}
}
}

/**
* Activate or disable PKCE
*/
public OicSession withPkceEnabled(boolean isEnabled) {
if (isEnabled) {
this.pkceVerifierCode = PKCE.generateVerifierCode();
} else {
this.pkceVerifierCode = null;
}
return this;
}

/**
Expand All @@ -98,16 +168,20 @@ private void setupOicSession(HttpSession session) {
* Starts the login session.
* @return an {@link HttpResponse}
*/
@Restricted(DoNotUse.class)
public HttpResponse commenceLogin(boolean disableNonce, AuthorizationCodeFlow flow) {
public HttpResponse commenceLogin(AuthorizationCodeFlow flow) {
setupOicSession(Stapler.getCurrentRequest().getSession());
AuthorizationCodeRequestUrl authorizationCodeRequestUrl =
flow.newAuthorizationUrl().setState(state).setRedirectUri(redirectUrl);
if (disableNonce) {
this.nonce = null;
} else {
AuthorizationCodeRequestUrl authorizationCodeRequestUrl = flow.newAuthorizationUrl()
.setState(state)
.setRedirectUri(redirectUrl);
if (this.nonce != null) {
authorizationCodeRequestUrl.set("nonce", this.nonce); // no @Key field defined in AuthorizationRequestUrl
}

if (this.pkceVerifierCode != null) {
PKCE pkce = new PKCE(this.pkceVerifierCode);
authorizationCodeRequestUrl.setCodeChallengeMethod(pkce.challengeCodeMethod);
authorizationCodeRequestUrl.setCodeChallenge(pkce.challengeCode);
}
return new HttpRedirect(authorizationCodeRequestUrl.toString());
}

Expand Down
40 changes: 37 additions & 3 deletions src/test/java/org/jenkinsci/plugins/oic/PluginTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
import com.google.api.client.json.webtoken.JsonWebSignature;
import com.google.api.client.json.webtoken.JsonWebToken;
import com.google.api.client.util.ArrayMap;
import com.google.api.client.util.Base64;
import com.google.gson.JsonElement;
import hudson.model.User;
import hudson.tasks.Mailer;
import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.PrivateKey;
import java.util.ArrayList;
Expand All @@ -20,6 +22,8 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import jenkins.model.Jenkins;
import org.acegisecurity.Authentication;
import org.junit.Before;
Expand All @@ -34,6 +38,7 @@
import static com.github.tomakehurst.wiremock.client.WireMock.absent;
import static com.github.tomakehurst.wiremock.client.WireMock.containing;
import static com.github.tomakehurst.wiremock.client.WireMock.equalTo;
import static com.github.tomakehurst.wiremock.client.WireMock.findAll;
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;
Expand Down Expand Up @@ -139,7 +144,8 @@ public void testLoginWithDefaults() throws Exception {
verify(getRequestedFor(urlPathEqualTo("/authorization"))
.withQueryParam("scope", equalTo("openid email"))
.withQueryParam("nonce", matching(".+")));
verify(postRequestedFor(urlPathEqualTo("/token")).withRequestBody(notMatching(".*&scope=.*")));
verify(postRequestedFor(urlPathEqualTo("/token"))
.withRequestBody(notMatching(".*&scope=.*")));
}

@Test
Expand Down Expand Up @@ -175,8 +181,10 @@ public void testLoginWithScopesInTokenRequest() throws Exception {
jenkins.setSecurityRealm(oidcSecurityRealm);
webClient.goTo(jenkins.getSecurityRealm().getLoginUrl());

verify(getRequestedFor(urlPathEqualTo("/authorization")).withQueryParam("scope", equalTo("openid email")));
verify(postRequestedFor(urlPathEqualTo("/token")).withRequestBody(containing("&scope=openid+email&")));
verify(getRequestedFor(urlPathEqualTo("/authorization"))
.withQueryParam("scope", equalTo("openid email")));
verify(postRequestedFor(urlPathEqualTo("/token"))
.withRequestBody(containing("&scope=openid+email&")));
}

@Test
Expand Down Expand Up @@ -215,6 +223,32 @@ public void testLoginWithPkceEnabled() throws Exception {
verify(getRequestedFor(urlPathEqualTo("/authorization"))
.withQueryParam("code_challenge_method", equalTo("S256"))
.withQueryParam("code_challenge", matching(".+")));
verify(postRequestedFor(urlPathEqualTo("/token"))
.withRequestBody(matching(".*&code_verifier=[^&]+.*")));

// check PKCE
// - get codeChallenge
final String codeChallenge = findAll(getRequestedFor(urlPathEqualTo("/authorization")))
.get(0)
.queryParameter("code_challenge")
.values()
.get(0);
// - get verifierCode
Matcher m = Pattern.compile(".*&code_verifier=([^&]+).*")
.matcher(findAll(postRequestedFor(urlPathEqualTo("/token")))
.get(0)
.getBodyAsString());
assertTrue(m.find());
final String codeVerifier = m.group(1);

// - hash verifierCode
byte[] bytes = codeVerifier.getBytes();
MessageDigest md = MessageDigest.getInstance("SHA-256");
md.update(bytes, 0, bytes.length);
byte[] digest = md.digest();
final String verifyChallenge = Base64.encodeBase64URLSafeString(digest);

assertEquals(verifyChallenge, codeChallenge);
}

@Test
Expand Down

0 comments on commit 4124503

Please sign in to comment.