Skip to content

Commit

Permalink
Gracefully handle missing and invalid idtoken (#292)
Browse files Browse the repository at this point in the history
  • Loading branch information
@michael-doubez
michael-doubez committed Apr 9, 2024
1 parent 0021f71 commit 325750a
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 25 deletions.
55 changes: 30 additions & 25 deletions src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,25 +261,25 @@ public OicSecurityRealm(
|| (Util.fixNull(automanualconfigure).isEmpty()
&& !Util.fixNull(wellKnownOpenIDConfigurationUrl).isEmpty())) {
this.automanualconfigure = "auto";
this.wellKnownOpenIDConfigurationUrl = Util.fixEmpty(wellKnownOpenIDConfigurationUrl);
this.wellKnownOpenIDConfigurationUrl = Util.fixEmptyAndTrim(wellKnownOpenIDConfigurationUrl);
this.loadWellKnownOpenIDConfigurationUrl();
} else {
this.automanualconfigure = "manual";
this.wellKnownOpenIDConfigurationUrl = null; // Remove the autoconfig URL
}

this.setTokenFieldToCheckKey(Util.fixEmpty(tokenFieldToCheckKey));
this.setTokenFieldToCheckValue(Util.fixEmpty(tokenFieldToCheckValue));
this.setUserNameField(Util.fixEmpty(userNameField) == null ? "sub" : userNameField);
this.setFullNameFieldName(Util.fixEmpty(fullNameFieldName));
this.setEmailFieldName(Util.fixEmpty(emailFieldName));
this.setGroupsFieldName(Util.fixEmpty(groupsFieldName));
this.setTokenFieldToCheckKey(tokenFieldToCheckKey);
this.setTokenFieldToCheckValue(tokenFieldToCheckValue);
this.setUserNameField(userNameField);
this.setFullNameFieldName(fullNameFieldName);
this.setEmailFieldName(emailFieldName);
this.setGroupsFieldName(groupsFieldName);
this.logoutFromOpenidProvider = Util.fixNull(logoutFromOpenidProvider, Boolean.TRUE);
this.postLogoutRedirectUrl = postLogoutRedirectUrl;
this.escapeHatchEnabled = Util.fixNull(escapeHatchEnabled, Boolean.FALSE);
this.escapeHatchUsername = Util.fixEmpty(escapeHatchUsername);
this.escapeHatchUsername = Util.fixEmptyAndTrim(escapeHatchUsername);
this.setEscapeHatchSecret(Secret.fromString(escapeHatchSecret));
this.escapeHatchGroup = Util.fixEmpty(escapeHatchGroup);
this.escapeHatchGroup = Util.fixEmptyAndTrim(escapeHatchGroup);
}

@DataBoundConstructor
Expand Down Expand Up @@ -534,7 +534,7 @@ private void loadWellKnownOpenIDConfigurationUrl() {
/** Parse headers to determine expiration date
*/
private void setWellKnownExpires(HttpHeaders headers) {
String expires = Util.fixEmpty(headers.getExpires());
String expires = Util.fixEmptyAndTrim(headers.getExpires());
// expires 0 means no cache
// we could (should?) have a look at Cache-Control header and max-age but for simplicity
// we can just leave it default TTL 1h refresh which sounds reasonable for such file
Expand Down Expand Up @@ -582,30 +582,30 @@ private void applyOverrideScopes() {

@DataBoundSetter
public void setUserNameField(String userNameField) {
this.userNameField = Util.fixEmpty(userNameField);
this.userNameField = Util.fixNull(Util.fixEmptyAndTrim(userNameField), "sub");
this.userNameFieldExpr = compileJMESPath(this.userNameField, "user name field");
}

@DataBoundSetter
public void setTokenFieldToCheckKey(String tokenFieldToCheckKey) {
this.tokenFieldToCheckKey = Util.fixEmpty(tokenFieldToCheckKey);
this.tokenFieldToCheckKey = Util.fixEmptyAndTrim(tokenFieldToCheckKey);
this.tokenFieldToCheckExpr = compileJMESPath(this.tokenFieldToCheckKey, "token field to check");
}

@DataBoundSetter
public void setTokenFieldToCheckValue(String tokenFieldToCheckValue) {
this.tokenFieldToCheckValue = Util.fixEmpty(tokenFieldToCheckValue);
this.tokenFieldToCheckValue = Util.fixEmptyAndTrim(tokenFieldToCheckValue);
}

@DataBoundSetter
public void setFullNameFieldName(String fullNameFieldName) {
this.fullNameFieldName = Util.fixEmpty(fullNameFieldName);
this.fullNameFieldName = Util.fixEmptyAndTrim(fullNameFieldName);
this.fullNameFieldExpr = compileJMESPath(this.fullNameFieldName, "full name field");
}

@DataBoundSetter
public void setEmailFieldName(String emailFieldName) {
this.emailFieldName = Util.fixEmpty(emailFieldName);
this.emailFieldName = Util.fixEmptyAndTrim(emailFieldName);
this.emailFieldExpr = compileJMESPath(this.emailFieldName, "email field");
}

Expand Down Expand Up @@ -634,7 +634,7 @@ private Object applyJMESPath(Expression<Object> expression, GenericJson json) {

@DataBoundSetter
public void setGroupsFieldName(String groupsFieldName) {
this.groupsFieldName = Util.fixEmpty(groupsFieldName);
this.groupsFieldName = Util.fixEmptyAndTrim(groupsFieldName);
this.groupsFieldExpr = this.compileJMESPath(groupsFieldName, "groups field");
}

Expand All @@ -650,7 +650,7 @@ public void setLogoutFromOpenidProvider(boolean logoutFromOpenidProvider) {

@DataBoundSetter
public void setPostLogoutRedirectUrl(String postLogoutRedirectUrl) {
this.postLogoutRedirectUrl = Util.fixEmpty(postLogoutRedirectUrl);
this.postLogoutRedirectUrl = Util.fixEmptyAndTrim(postLogoutRedirectUrl);

Check warning on line 653 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 653 is not covered by tests
}

@DataBoundSetter
Expand All @@ -660,7 +660,7 @@ public void setEscapeHatchEnabled(boolean escapeHatchEnabled) {

@DataBoundSetter
public void setEscapeHatchUsername(String escapeHatchUsername) {
this.escapeHatchUsername = Util.fixEmpty(escapeHatchUsername);
this.escapeHatchUsername = Util.fixEmptyAndTrim(escapeHatchUsername);
}

@DataBoundSetter
Expand All @@ -686,7 +686,7 @@ protected boolean checkEscapeHatch(String username, String password) {

@DataBoundSetter
public void setEscapeHatchGroup(String escapeHatchGroup) {
this.escapeHatchGroup = Util.fixEmpty(escapeHatchGroup);
this.escapeHatchGroup = Util.fixEmptyAndTrim(escapeHatchGroup);
}

@DataBoundSetter
Expand Down Expand Up @@ -872,7 +872,15 @@ public HttpResponse onSuccess(String authorizationCode, AuthorizationCodeFlow fl

OicTokenResponse response = (OicTokenResponse) tokenRequest.execute();

IdToken idToken = response.parseIdToken();
if (response.getIdToken() == null) {
return HttpResponses.errorWithoutStack(500, Messages.OicSecurityRealm_NoIdTokenInResponse());
}
IdToken idToken;
try {
idToken = response.parseIdToken();
} catch(IllegalArgumentException e) {
return HttpResponses.errorWithoutStack(403, Messages.OicSecurityRealm_IdTokenParseError());
}
if (!isNonceDisabled() && !validateNonce(idToken)) {
return HttpResponses.errorWithoutStack(401, "Unauthorized");
}
Expand All @@ -890,10 +898,7 @@ public HttpResponse onSuccess(String authorizationCode, AuthorizationCodeFlow fl

String username = determineStringField(userNameFieldExpr, idToken, userInfo);
if (username == null) {
return HttpResponses.error(
500,
"no field '" + userNameField
+ "' was supplied in the UserInfo or the IdToken payload to be used as the username");
return HttpResponses.error(500, Messages.OicSecurityRealm_UsernameNotFound(userNameField));
}

flow.createAndStoreCredential(response, null);
Expand All @@ -903,7 +908,7 @@ public HttpResponse onSuccess(String authorizationCode, AuthorizationCodeFlow fl
return new HttpRedirect(redirectOnFinish);

} catch (IOException e) {
return HttpResponses.error(500, e);
return HttpResponses.error(500, Messages.OicSecurityRealm_TokenRequestFailure(e));

Check warning on line 911 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 901-911 are not covered by tests
}
}
}.withNonceDisabled(isNonceDisabled())
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/org/jenkinsci/plugins/oic/OicTokenResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ public OicTokenResponse setIdToken(String str) {
}

public IdToken parseIdToken() throws IOException {
if (this.idToken == null) {

Check warning on line 53 in src/main/java/org/jenkinsci/plugins/oic/OicTokenResponse.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 53 is only partially covered, one branch is missing
return null;

Check warning on line 54 in src/main/java/org/jenkinsci/plugins/oic/OicTokenResponse.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 54 is not covered by tests
}
return IdToken.parse(getFactory(), this.idToken);
}

Expand Down
4 changes: 4 additions & 0 deletions src/main/resources/org/jenkinsci/plugins/oic/Messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,7 @@ OicSecurityRealm.UsingDefaultScopes = Using ''openid email''.
OicSecurityRealm.RUSureOpenIdNotInScope = Are you sure you don''t want to include ''openid'' as an scope?
OicSecurityRealm.EndSessionURLKeyRequired = End Session URL Key is required.
OicSecurityRealm.InvalidFieldName = Invalid field name - must be a valid JMESPath expression.
OicSecurityRealm.NoIdTokenInResponse = No idtoken was provided in response to token request.
OicSecurityRealm.IdTokenParseError = Idtoken could not be parsed.
OicSecurityRealm.UsernameNotFound = No field ''{0}'' was supplied in the UserInfo or the IdToken payload to be used as the username.
OicSecurityRealm.TokenRequestFailure = Token request failed: {0}"
69 changes: 69 additions & 0 deletions src/test/java/org/jenkinsci/plugins/oic/PluginTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;


/**
* goes trough a login scenario, the openid provider is mocked and always returns state. We aren't checking
* if if openid connect or if the openid connect implementation works. Rather we are only
Expand Down Expand Up @@ -1294,6 +1295,74 @@ private String createUserInfoJWT(PrivateKey privateKey, String userInfo) throws
return JsonWebSignature.signUsingRsaSha256(privateKey, GsonFactory.getDefaultInstance(), header, payload);
}

@Test
public void testLoginWithMisingIdTokenShouldBeRefused() 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<String, Object> 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("{"
+ "\"access_token\":\"AcCeSs_ToKeN\","
+ "\"token_type\":\"example\","
+ "\"expires_in\":3600,"
+ "\"refresh_token\":\"ReFrEsH_ToKeN\","
+ "\"example_parameter\":\"example_value\""
+ "}")));

jenkins.setSecurityRealm(new TestRealm(wireMockRule, null, null, null));

assertEquals("Shouldn't be authenticated", getAuthentication().getPrincipal(), Jenkins.ANONYMOUS.getPrincipal());

webClient.assertFails(jenkins.getSecurityRealm().getLoginUrl(), 500);
}

@Test
public void testLoginWithUnreadableIdTokenShouldBeRefused() 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<String, Object> 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\": \"Thi is not anb IdToken\","
+ "\"access_token\":\"AcCeSs_ToKeN\","
+ "\"token_type\":\"example\","
+ "\"expires_in\":3600,"
+ "\"refresh_token\":\"ReFrEsH_ToKeN\","
+ "\"example_parameter\":\"example_value\""
+ "}")));

jenkins.setSecurityRealm(new TestRealm(wireMockRule, null, null, null));

assertEquals("Shouldn't be authenticated", getAuthentication().getPrincipal(), Jenkins.ANONYMOUS.getPrincipal());

webClient.assertFails(jenkins.getSecurityRealm().getLoginUrl(), 403);
}

/**
* Gets the authentication object from the web client.
*
Expand Down

0 comments on commit 325750a

Please sign in to comment.