Skip to content

Commit

Permalink
Reimplement PKCE to serialize verifier in session
Browse files Browse the repository at this point in the history
  • Loading branch information
@michael-doubez
michael-doubez committed Apr 7, 2024
1 parent 94cdf03 commit 5b5f4d4
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();

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

View check run for this annotation

ci.jenkins.io / SpotBugs

DM_DEFAULT_ENCODING 

HIGH:
Found reliance on default encoding in new org.jenkinsci.plugins.oic.OicSession$PKCE(String): String.getBytes()
Raw output 
<p> Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behaviour to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly. </p>
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 5b5f4d4

Please sign in to comment.