Skip to content

Commit

Permalink
SECURITY-3461
Browse files Browse the repository at this point in the history
  • Loading branch information
rsandell authored and jgreffe committed Jan 13, 2025
1 parent 2849bd3 commit 4d7765c
Show file tree
Hide file tree
Showing 15 changed files with 341 additions and 7 deletions.
55 changes: 54 additions & 1 deletion src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.Util;
import hudson.model.Descriptor;
import hudson.model.Descriptor.FormException;
Expand Down Expand Up @@ -88,6 +89,8 @@
import java.util.logging.Logger;
import java.util.regex.Pattern;
import javax.annotation.PostConstruct;
import jenkins.model.IdStrategy;
import jenkins.model.IdStrategyDescriptor;
import jenkins.model.Jenkins;
import jenkins.security.ApiTokenProperty;
import jenkins.security.FIPS140;
Expand Down Expand Up @@ -151,6 +154,8 @@ public class OicSecurityRealm extends SecurityRealm implements Serializable {
private static final long serialVersionUID = 1L;

private static final Logger LOGGER = Logger.getLogger(OicSecurityRealm.class.getName());
private IdStrategy userIdStrategy;
private IdStrategy groupIdStrategy;

public static enum TokenAuthMethod {
client_secret_basic(ClientAuthenticationMethod.CLIENT_SECRET_BASIC),
Expand Down Expand Up @@ -316,7 +321,9 @@ public OicSecurityRealm(
String clientId,
Secret clientSecret,
OicServerConfiguration serverConfiguration,
Boolean disableSslVerification)
Boolean disableSslVerification,
IdStrategy userIdStrategy,
IdStrategy groupIdStrategy)
throws IOException, FormException {
// Needed in DataBoundSetter
this.disableSslVerification = Util.fixNull(disableSslVerification, Boolean.FALSE);
Expand All @@ -327,6 +334,8 @@ public OicSecurityRealm(
this.clientId = clientId;
this.clientSecret = clientSecret;
this.serverConfiguration = serverConfiguration;
this.userIdStrategy = userIdStrategy;
this.groupIdStrategy = groupIdStrategy;
}

@SuppressWarnings("deprecated")
Expand Down Expand Up @@ -420,6 +429,20 @@ public String getUserNameField() {
return userNameField;
}

@Restricted(NoExternalUse.class)
public boolean isMissingIdStrategy() {
return userIdStrategy == null || groupIdStrategy == null;

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

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 434 is only partially covered, one branch is missing
}

@Override
public IdStrategy getUserIdStrategy() {
if (userIdStrategy != null) {
return userIdStrategy;
} else {
return IdStrategy.CASE_INSENSITIVE;
}
}

public String getTokenFieldToCheckKey() {
return tokenFieldToCheckKey;
}
Expand All @@ -440,6 +463,15 @@ public String getGroupsFieldName() {
return groupsFieldName;
}

@Override
public IdStrategy getGroupIdStrategy() {
if (groupIdStrategy != null) {
return groupIdStrategy;
} else {
return IdStrategy.CASE_INSENSITIVE;
}
}

public boolean isDisableSslVerification() {
return disableSslVerification;
}
Expand Down Expand Up @@ -1628,5 +1660,26 @@ public Descriptor<OicServerConfiguration> getDefaultServerConfigurationType() {
public boolean isFipsEnabled() {
return FIPS140.useCompliantAlgorithms();
}

@Restricted(NoExternalUse.class)
public List<IdStrategyDescriptor> getIdStrategyDescriptors() {
return ExtensionList.lookup(IdStrategyDescriptor.class);
}

/**
* The default username strategy for new OicSecurityRealm
*/
@Restricted(NoExternalUse.class)
public IdStrategy defaultUsernameIdStrategy() {
return new IdStrategy.CaseSensitive();
}

/**
* The default group strategy for new OicSecurityRealm
*/
@Restricted(NoExternalUse.class)
public IdStrategy defaultGroupIdStrategy() {
return new IdStrategy.CaseSensitive();

Check warning on line 1682 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 1666-1682 are not covered by tests
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package org.jenkinsci.plugins.oic.monitor;

Check warning on line 1 in src/main/java/org/jenkinsci/plugins/oic/monitor/OicIdStrategyMonitor.java

View check run for this annotation

ci.jenkins.io / Java Compiler

checkstyle:check

ERROR: (misc) NewlineAtEndOfFile: Expected line ending for file is LF(\n), but CRLF(\r\n) is detected.

import com.google.common.annotations.VisibleForTesting;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.model.AdministrativeMonitor;
import hudson.security.SecurityRealm;
import java.io.IOException;
import jenkins.model.Jenkins;
import org.jenkinsci.plugins.oic.Messages;
import org.jenkinsci.plugins.oic.OicSecurityRealm;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.HttpResponses;
import org.kohsuke.stapler.interceptor.RequirePOST;

@Extension
@Restricted(NoExternalUse.class)
public class OicIdStrategyMonitor extends AdministrativeMonitor {

// if null, means not evaluated yet
Boolean missingIdStrategy;

public OicIdStrategyMonitor() {}

@VisibleForTesting
protected static OicIdStrategyMonitor get() {
return ExtensionList.lookupSingleton(OicIdStrategyMonitor.class);
}

@Override
public String getDisplayName() {
return Messages.OicSecurityRealm_monitor_DisplayName();

Check warning on line 34 in src/main/java/org/jenkinsci/plugins/oic/monitor/OicIdStrategyMonitor.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 34 is not covered by tests
}

@Override
public boolean isActivated() {
if (!Boolean.FALSE.equals(missingIdStrategy)) {

Check warning on line 39 in src/main/java/org/jenkinsci/plugins/oic/monitor/OicIdStrategyMonitor.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 39 is only partially covered, one branch is missing
SecurityRealm securityRealm = Jenkins.get().getSecurityRealm();
if (securityRealm instanceof OicSecurityRealm) {
missingIdStrategy = ((OicSecurityRealm) securityRealm).isMissingIdStrategy();
} else {
missingIdStrategy = Boolean.FALSE;
}
}
return missingIdStrategy;
}

@RequirePOST
public HttpResponse doForward() throws IOException {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
return HttpResponses.redirectViaContextPath("configureSecurity");

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

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 52-53 are not covered by tests
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,4 @@ OicSecurityRealm.DisableSslVerificationFipsMode = SSL verification can not be di
OicSecurityRealm.DisableTokenVerificationFipsMode = Token verification can not be disabled in FIPS mode
OicServerWellKnownConfiguration.DisplayName = Discovery via well-known endpoint
OicServerManualConfiguration.DisplayName = Manual entry
OicSecurityRealm.monitor.DisplayName= Openid Connect Id Strategy Configuration
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
<f:entry title="${%UsernameFieldName}" field="userNameField">
<f:textbox/>
</f:entry>
<f:entry>
<f:dropdownDescriptorSelector title="${%UsernameIdStrategy}" field="userIdStrategy" default="${descriptor.defaultUsernameIdStrategy()}" descriptors="${descriptor.idStrategyDescriptors}"/>
</f:entry>
<f:entry title="${%FullnameFieldName}" field="fullNameFieldName">
<f:textbox/>
</f:entry>
Expand All @@ -25,6 +28,9 @@
<f:entry title="${%GroupsFieldName}" field="groupsFieldName">
<f:textbox/>
</f:entry>
<f:entry>
<f:dropdownDescriptorSelector title="${%GroupIdStrategy}" field="groupIdStrategy" default="${descriptor.defaultGroupIdStrategy()}" descriptors="${descriptor.idStrategyDescriptors}"/>
</f:entry>
</f:advanced>
<f:entry title="${%LogoutFromOpenIDProvider}" field="logoutFromOpenidProvider">
<f:checkbox id="logoutFromIDP"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,5 @@ UserFields=User fields
Username=Username
UsernameFieldName=User name field name
WellknownConfigurationEndpoint=Well-known configuration endpoint
UsernameIdStrategy=Username case sensitivity
GroupIdStrategy=Group name case sensitivity
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout" xmlns:f="/lib/form" xmlns:i="jelly:fmt" xmlns:st="jelly:stapler">
${%blurb}
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
blurb=The OpenId Connect Security Realm's "Username case sensitivity" and "Group name case sensitivity" options have not been configured.
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:l="/lib/layout">
<div class="alert alert-warning">
<l:isAdmin>
<form method="post" action="${rootURL}/${it.url}/forward">
<f:submit value="${%configureSecurityRealm}"/>
</form>
</l:isAdmin>
<j:set var="actionAnchor">
<a href="${rootURL}/configure">${%actionUrlContent}</a>
</j:set>
${%blurb}
</div>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
blurb=\
<p>The Openid Connect Security Realm was configured before the introduction of the "Username case sensitivity" and "Group name case sensitivity" options.</p> \
<p>As a result, the current configuration treats these values as case-insensitive, whereas the default for new configurations is case-sensitive.</p> \
<p>This difference could introduce a security vulnerability depending on your specific use case. For further information, refer to the <a href="https://www.jenkins.io/security/advisory/2025-01-22/#SECURITY-3461">security advisory</a>. Please review and select the appropriate case sensitivity settings for your environment, then save the updated security realm configuration.</p> \
<p>Warning: Switching from case-insensitive to case-sensitive behavior can be a lossy operation if there are mixed-case usernames or group names. Users with externally-defined mixed-case names will effectively be treated as new users they next time they log in, and will lose their existing user preferences. Group-related configurations in Jenkins for externally-defined groups with mixed case names will no longer apply to members of those external groups. Users with mixed-case names previously defined in groups in Jenkins will also no longer be considered members of those groups after the switch.</p>
configureSecurityRealm=Configure the Security Realm
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ public class OicSecurityRealmFipsTest {
@Test
@WithoutJenkins
public void settingNonCompliantValuesNotAllowedTest() throws IOException, Descriptor.FormException {
OicSecurityRealm realm = new OicSecurityRealm("clientId", Secret.fromString("secret"), null, false);
OicSecurityRealm realm = new OicSecurityRealm("clientId", Secret.fromString("secret"), null, false, null, null);
Descriptor.FormException ex = assertThrows(
Descriptor.FormException.class,
() -> new OicSecurityRealm("clientId", Secret.fromString("secret"), null, true));
() -> new OicSecurityRealm("clientId", Secret.fromString("secret"), null, true, null, null));
assertThat(
"Exception contains the reason",
ex.getMessage(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
package org.jenkinsci.plugins.oic;

Check warning on line 1 in src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmIdStrategyTest.java

View check run for this annotation

ci.jenkins.io / Java Compiler

checkstyle:check

ERROR: (misc) NewlineAtEndOfFile: Expected line ending for file is LF(\n), but CRLF(\r\n) is detected.

import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import hudson.model.User;
import hudson.security.SecurityRealm;
import jenkins.model.IdStrategy;
import jenkins.model.Jenkins;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsSessionRule;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsInstanceOf.instanceOf;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

public class OicSecurityRealmIdStrategyTest {

@Rule
public JenkinsSessionRule sessions = new JenkinsSessionRule();

@Rule
public WireMockRule wireMockRule = new WireMockRule(new WireMockConfiguration().dynamicPort(), true);

@Test
@Issue("SECURITY-3461")
public void testUserIdStrategy_caseInsensitive() throws Throwable {
sessions.then(r -> {
TestRealm realm = new TestRealm(new TestRealm.Builder(wireMockRule)
.WithMinimalDefaults().WithUserIdStrategy(IdStrategy.CASE_INSENSITIVE));
Jenkins.get().setSecurityRealm(realm);
User testuser = User.getById("testuser", true);
assertNotNull(testuser);
assertEquals("testuser", testuser.getDisplayName());
testuser.save();

User testUSER = User.getById("testUSER", true);
assertNotNull(testUSER);
assertEquals("testuser", testUSER.getDisplayName());
testUSER.save();

assertEquals(testuser, testUSER);
});
sessions.then(r -> {
User testuser = User.getById("testuser", false);
assertNotNull(testuser);
assertEquals("testuser", testuser.getDisplayName());

User testUSER = User.getById("testUSER", false);
assertNotNull(testUSER);
assertEquals("testuser", testUSER.getDisplayName());
assertEquals(testuser, testUSER);
});
}

@Test
@Issue("SECURITY-3461")
public void testUserIdStrategy_caseSensitive() throws Throwable {
sessions.then(r -> {
TestRealm realm = new TestRealm(new TestRealm.Builder(wireMockRule)
.WithMinimalDefaults().WithUserIdStrategy(new IdStrategy.CaseSensitive()));
Jenkins.get().setSecurityRealm(realm);
User testuser = User.getById("testuser", true);
assertNotNull(testuser);
assertEquals("testuser", testuser.getDisplayName());
testuser.save();

User testUSER = User.getById("testUSER", true);
assertNotNull(testUSER);
assertEquals("testUSER", testUSER.getDisplayName());
testUSER.save();

assertNotEquals(testuser, testUSER);
});
sessions.then(r -> {
User testuser = User.getById("testuser", false);
assertNotNull(testuser);
assertEquals("testuser", testuser.getDisplayName());

User testUSER = User.getById("testUSER", false);
assertNotNull(testUSER);
assertEquals("testUSER", testUSER.getDisplayName());

assertNotEquals(testuser, testUSER);
});
}

@Test
@Issue("SECURITY-3461")
public void testUserIdStrategy_default() throws Throwable {
sessions.then(r -> {
TestRealm realm = new TestRealm(wireMockRule);
Jenkins.get().setSecurityRealm(realm);
});
sessions.then(r -> {
// when restarting, ensure the default case-insensitive is used
SecurityRealm securityRealm = Jenkins.get().getSecurityRealm();
assertThat(securityRealm, instanceOf(OicSecurityRealm.class));
OicSecurityRealm oicSecurityRealm = (OicSecurityRealm) securityRealm;
assertTrue(oicSecurityRealm.isMissingIdStrategy());
assertEquals(IdStrategy.CASE_INSENSITIVE, securityRealm.getUserIdStrategy());
assertEquals(IdStrategy.CASE_INSENSITIVE, securityRealm.getGroupIdStrategy());

TestRealm realm = new TestRealm(new TestRealm.Builder(wireMockRule)
.WithMinimalDefaults()
.WithUserIdStrategy(IdStrategy.CASE_INSENSITIVE)
.WithGroupIdStrategy(IdStrategy.CASE_INSENSITIVE));
Jenkins.get().setSecurityRealm(realm);
assertFalse(realm.isMissingIdStrategy());
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@ public class SecurityRealmConfigurationFIPSTest {

@Test(expected = Descriptor.FormException.class)
public void escapeHatchThrowsException() throws Exception {
new OicSecurityRealm("clientId", null, null, null).setEscapeHatchEnabled(true);
new OicSecurityRealm("clientId", null, null, null, null, null).setEscapeHatchEnabled(true);
}

@Test
public void escapeHatchToFalse() throws Exception {
OicSecurityRealm oicSecurityRealm = new OicSecurityRealm("clientId", null, null, null);
OicSecurityRealm oicSecurityRealm = new OicSecurityRealm("clientId", null, null, null, null, null);
oicSecurityRealm.setEscapeHatchEnabled(false);
assertThat(oicSecurityRealm.isEscapeHatchEnabled(), is(false));
}

@Test
public void readresolve() throws Exception {
OicSecurityRealm oicSecurityRealm = new OicSecurityRealm("clientId", null, null, null);
OicSecurityRealm oicSecurityRealm = new OicSecurityRealm("clientId", null, null, null, null, null);
oicSecurityRealm.setEscapeHatchEnabled(false);
assertThat(oicSecurityRealm.isEscapeHatchEnabled(), is(false));
oicSecurityRealm.readResolve();
Expand Down
Loading

0 comments on commit 4d7765c

Please # to comment.