From 549dde617dbcf533e6cabfe8cc148a250a398211 Mon Sep 17 00:00:00 2001 From: rsandell Date: Tue, 4 Jul 2023 12:57:36 +0200 Subject: [PATCH] [SECURITY-3059] --- pom.xml | 2 +- .../ActiveDirectoryDomain.java | 9 ++-- .../ActiveDirectorySecurityRealm.java | 45 ++++++++++++------- .../active_directory/TlsConfiguration.java | 2 +- .../ActiveDirectorySecurityRealm/config.jelly | 8 ++-- .../configAdvanced.jelly | 6 +-- .../message.jelly | 2 +- .../message.jelly | 2 +- .../WindowsAdsiModeUserCacheDisabledIT.java | 4 +- .../WindowsAdsiModeUserCacheEnabledIT.java | 4 +- .../ActiveDirectoryGenericContainer.java | 1 + .../docker/TheFlintstonesIT.java | 42 ++++++++++++----- 12 files changed, 82 insertions(+), 45 deletions(-) diff --git a/pom.xml b/pom.xml index af45b70d..be74d87a 100644 --- a/pom.xml +++ b/pom.xml @@ -34,7 +34,7 @@ - 2.31 + 2.30.1 -SNAPSHOT jenkinsci/${project.artifactId}-plugin 2.332.4 diff --git a/src/main/java/hudson/plugins/active_directory/ActiveDirectoryDomain.java b/src/main/java/hudson/plugins/active_directory/ActiveDirectoryDomain.java index fafb27a4..c788a7d2 100644 --- a/src/main/java/hudson/plugins/active_directory/ActiveDirectoryDomain.java +++ b/src/main/java/hudson/plugins/active_directory/ActiveDirectoryDomain.java @@ -268,14 +268,15 @@ public ListBoxModel doFillTlsConfigurationItems() { @RequirePOST public FormValidation doValidateTest(@QueryParameter(fixEmpty = true) String name, @QueryParameter(fixEmpty = true) String servers, @QueryParameter(fixEmpty = true) String site, @QueryParameter(fixEmpty = true) String bindName, - @QueryParameter(fixEmpty = true) String bindPassword, @QueryParameter(fixEmpty = true) TlsConfiguration tlsConfiguration, @QueryParameter(fixEmpty = true) boolean requireTLS) throws IOException, ServletException, NamingException { + @QueryParameter(fixEmpty = true) String bindPassword, @QueryParameter(fixEmpty = true) TlsConfiguration tlsConfiguration, @QueryParameter GroupLookupStrategy groupLookupStrategy, + @QueryParameter(fixEmpty = false) boolean removeIrrelevantGroups, @QueryParameter(fixEmpty = true) boolean startTls, @QueryParameter(fixEmpty = true) boolean requireTLS) throws NamingException { Jenkins.get().checkPermission(Jenkins.ADMINISTER); ActiveDirectoryDomain domain = new ActiveDirectoryDomain(name, servers, site, bindName, bindPassword, tlsConfiguration); List domains = new ArrayList<>(1); domains.add(domain); - ActiveDirectorySecurityRealm activeDirectorySecurityRealm = new ActiveDirectorySecurityRealm(null, domains, site, bindName, - bindPassword, null, GroupLookupStrategy.AUTO, false, true, null, false, (ActiveDirectoryInternalUsersDatabase) null, requireTLS); + ActiveDirectorySecurityRealm activeDirectorySecurityRealm = new ActiveDirectorySecurityRealm(name, domains, site, bindName, + bindPassword, null, groupLookupStrategy, removeIrrelevantGroups, true, null, startTls, null, requireTLS); ClassLoader ccl = Thread.currentThread().getContextClassLoader(); Thread.currentThread().setContextClassLoader(getClass().getClassLoader()); @@ -325,7 +326,7 @@ public FormValidation doValidateTest(@QueryParameter(fixEmpty = true) String nam // Make sure the bind actually works try { Hashtable props = new Hashtable<>(0); - DirContext context = activeDirectorySecurityRealm.getDescriptor().bind(bindName, Secret.toString(password), obtainerServers, props, tlsConfiguration, requireTLS); + DirContext context = activeDirectorySecurityRealm.getDescriptor().bind(bindName, Secret.toString(password), obtainerServers, props, tlsConfiguration, requireTLS, startTls); try { // Actually do a search to make sure the credential is valid Attributes userAttributes = new LDAPSearchBuilder(context, toDC(name)).subTreeScope().searchOne("(objectClass=user)"); diff --git a/src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java b/src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java index 72b80953..07c2c967 100644 --- a/src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java +++ b/src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java @@ -567,13 +567,18 @@ public DirContext bind(String principalName, String password, List l return bind(principalName, password, ldapServers, props, tlsConfiguration, isRequireTLS()); } - /** - * Binds to the server using the specified username/password. - *

- * In a real deployment, often there are servers that don't respond or - * otherwise broken, so try all the servers. - */ + @Deprecated public DirContext bind(String principalName, String password, List ldapServers, Hashtable props, TlsConfiguration tlsConfiguration, boolean requireTLS) throws NamingException { + return bind(principalName, password, ldapServers, props, tlsConfiguration, requireTLS, isStartTLS()); + } + + /** + * Binds to the server using the specified username/password. + *

+ * In a real deployment, often there are servers that don't respond or + * otherwise broken, so try all the servers. + */ + public DirContext bind(String principalName, String password, List ldapServers, Hashtable props, TlsConfiguration tlsConfiguration, boolean requireTLS, boolean startTls) throws NamingException { // in a AD forest, it'd be mighty nice to be able to login as "joe" // as opposed to "joe@europe", // but the bind operation doesn't appear to allow me to do so. @@ -599,7 +604,7 @@ public DirContext bind(String principalName, String password, List l for (SocketInfo ldapServer : ldapServers) { try { - LdapContext context = bind(principalName, password, ldapServer, newProps, tlsConfiguration, requireTLS); + LdapContext context = bind(principalName, password, ldapServer, newProps, tlsConfiguration, requireTLS, startTls); LOGGER.fine("Bound to " + ldapServer); return context; } catch (javax.naming.AuthenticationException e) { @@ -656,8 +661,15 @@ private LdapContext bind(String principalName, String password, SocketInfo serve return bind(principalName, password, server, props, null, isRequireTLS()); } + @SuppressFBWarnings(value = "UPM_UNCALLED_PRIVATE_METHOD", justification = "Deprecated method.It will removed at some point") @IgnoreJRERequirement + @Deprecated private LdapContext bind(String principalName, String password, SocketInfo server, Hashtable props, TlsConfiguration tlsConfiguration, boolean requireTLS) throws NamingException { + return bind(principalName, password, server, props, tlsConfiguration, requireTLS, isStartTLS()); + } + + @IgnoreJRERequirement + private LdapContext bind(String principalName, String password, SocketInfo server, Hashtable props, TlsConfiguration tlsConfiguration, boolean requireTLS, boolean startTLS) throws NamingException { String ldapUrl = (requireTLS?"ldaps://":"ldap://") + server + '/'; String oldName = Thread.currentThread().getName(); Thread.currentThread().setName("Connecting to "+ldapUrl+" : "+oldName); @@ -670,14 +682,7 @@ private LdapContext bind(String principalName, String password, SocketInfo serve LdapContext context = new InitialLdapContext(props, null); - boolean isStartTls = true; - SecurityRealm securityRealm = Jenkins.getActiveInstance().getSecurityRealm(); - if (securityRealm instanceof ActiveDirectorySecurityRealm) { - ActiveDirectorySecurityRealm activeDirectorySecurityRealm = (ActiveDirectorySecurityRealm) securityRealm; - isStartTls= activeDirectorySecurityRealm.isStartTls(); - } - - if (!requireTLS && isStartTls) { + if (!requireTLS && startTLS) { // try to upgrade to TLS if we can, but failing to do so isn't fatal // see http://download.oracle.com/javase/jndi/tutorial/ldap/ext/starttls.html StartTlsResponse rsp = null; @@ -858,6 +863,16 @@ private boolean isRequireTLS() { } return requireTLS; } + + @Deprecated // this is here purely to ease access to the variable from the descriptor. + private boolean isStartTLS() { + boolean startTLS = true; // secure by default + SecurityRealm securityRealm = Jenkins.get().getSecurityRealm(); + if (securityRealm instanceof ActiveDirectorySecurityRealm) { + startTLS = Boolean.TRUE.equals(((ActiveDirectorySecurityRealm)securityRealm).isStartTls()); + } + return startTLS; + } } @Override diff --git a/src/main/java/hudson/plugins/active_directory/TlsConfiguration.java b/src/main/java/hudson/plugins/active_directory/TlsConfiguration.java index 3f601c18..ed00334e 100644 --- a/src/main/java/hudson/plugins/active_directory/TlsConfiguration.java +++ b/src/main/java/hudson/plugins/active_directory/TlsConfiguration.java @@ -6,7 +6,7 @@ * Classification of all possible TLS configurations * */ -enum TlsConfiguration { +public enum TlsConfiguration { TRUST_ALL_CERTIFICATES (Messages._TlsConfiguration_TrustAllCertificates()), JDK_TRUSTSTORE (Messages._TlsConfiguration_JdkTrustStore()) ; diff --git a/src/main/resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/config.jelly b/src/main/resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/config.jelly index 5070fca8..a070e343 100644 --- a/src/main/resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/config.jelly +++ b/src/main/resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/config.jelly @@ -42,6 +42,9 @@ + + + @@ -65,7 +68,7 @@ - +

@@ -73,9 +76,6 @@
- - - diff --git a/src/main/resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/configAdvanced.jelly b/src/main/resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/configAdvanced.jelly index b2275d9e..bb01c529 100644 --- a/src/main/resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/configAdvanced.jelly +++ b/src/main/resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/configAdvanced.jelly @@ -1,12 +1,12 @@ - - - + + + diff --git a/src/main/resources/hudson/plugins/active_directory/Security1389AdministrativeMonitor/message.jelly b/src/main/resources/hudson/plugins/active_directory/Security1389AdministrativeMonitor/message.jelly index f63716f2..e39c7978 100644 --- a/src/main/resources/hudson/plugins/active_directory/Security1389AdministrativeMonitor/message.jelly +++ b/src/main/resources/hudson/plugins/active_directory/Security1389AdministrativeMonitor/message.jelly @@ -3,7 +3,7 @@
- +

${%blurb(rootURL + "/configureSecurity/")}

diff --git a/src/main/resources/hudson/plugins/active_directory/Security1389AdministrativeMonitorLegacySysProp/message.jelly b/src/main/resources/hudson/plugins/active_directory/Security1389AdministrativeMonitorLegacySysProp/message.jelly index c57d6ba5..adc9ea6a 100644 --- a/src/main/resources/hudson/plugins/active_directory/Security1389AdministrativeMonitorLegacySysProp/message.jelly +++ b/src/main/resources/hudson/plugins/active_directory/Security1389AdministrativeMonitorLegacySysProp/message.jelly @@ -3,7 +3,7 @@
- +

${%blurb}

diff --git a/src/test/java/hudson/plugins/active_directory/WindowsAdsiModeUserCacheDisabledIT.java b/src/test/java/hudson/plugins/active_directory/WindowsAdsiModeUserCacheDisabledIT.java index ddce1ddf..370d24f2 100644 --- a/src/test/java/hudson/plugins/active_directory/WindowsAdsiModeUserCacheDisabledIT.java +++ b/src/test/java/hudson/plugins/active_directory/WindowsAdsiModeUserCacheDisabledIT.java @@ -38,14 +38,14 @@ public static void setUp() { public void dynamicCacheEnableSetUp() throws Exception { CacheConfiguration cacheConfiguration = new CacheConfiguration(500,30); ActiveDirectorySecurityRealm activeDirectorySecurityRealm = new ActiveDirectorySecurityRealm(null, null, null, null, - null, null, null, false, null, cacheConfiguration, null, (ActiveDirectoryInternalUsersDatabase) null); + null, null, null, false, null, cacheConfiguration, null, (ActiveDirectoryInternalUsersDatabase) null, false); j.jenkins.setSecurityRealm(activeDirectorySecurityRealm); } public void dynamicCacheDisabledSetUp() throws Exception { ActiveDirectorySecurityRealm activeDirectorySecurityRealm = new ActiveDirectorySecurityRealm(null, null, null, null, - null, null, null, false, null, null, null, (ActiveDirectoryInternalUsersDatabase) null); + null, null, null, false, null, null, null, (ActiveDirectoryInternalUsersDatabase) null, false); j.jenkins.setSecurityRealm(activeDirectorySecurityRealm); } diff --git a/src/test/java/hudson/plugins/active_directory/WindowsAdsiModeUserCacheEnabledIT.java b/src/test/java/hudson/plugins/active_directory/WindowsAdsiModeUserCacheEnabledIT.java index e6e13e23..0114d013 100644 --- a/src/test/java/hudson/plugins/active_directory/WindowsAdsiModeUserCacheEnabledIT.java +++ b/src/test/java/hudson/plugins/active_directory/WindowsAdsiModeUserCacheEnabledIT.java @@ -58,14 +58,14 @@ public static void disableHealthMetrics() { public void dynamicCacheEnableSetUp() throws Exception { CacheConfiguration cacheConfiguration = new CacheConfiguration(500,30); ActiveDirectorySecurityRealm activeDirectorySecurityRealm = new ActiveDirectorySecurityRealm(null, null, null, null, - null, null, null, false, null, cacheConfiguration, null, (ActiveDirectoryInternalUsersDatabase) null); + null, null, null, false, null, cacheConfiguration, null, (ActiveDirectoryInternalUsersDatabase) null, false); j.jenkins.setSecurityRealm(activeDirectorySecurityRealm); } public void dynamicCacheDisabledSetUp() throws Exception { ActiveDirectorySecurityRealm activeDirectorySecurityRealm = new ActiveDirectorySecurityRealm(null, null, null, null, - null, null, null, false, null, null, null, (ActiveDirectoryInternalUsersDatabase) null); + null, null, null, false, null, null, null, (ActiveDirectoryInternalUsersDatabase) null, false); j.jenkins.setSecurityRealm(activeDirectorySecurityRealm); } diff --git a/src/test/java/hudson/plugins/active_directory/docker/ActiveDirectoryGenericContainer.java b/src/test/java/hudson/plugins/active_directory/docker/ActiveDirectoryGenericContainer.java index f286ca53..720e5bcd 100644 --- a/src/test/java/hudson/plugins/active_directory/docker/ActiveDirectoryGenericContainer.java +++ b/src/test/java/hudson/plugins/active_directory/docker/ActiveDirectoryGenericContainer.java @@ -34,6 +34,7 @@ public ActiveDirectoryGenericContainer() { */ public ActiveDirectoryGenericContainer withStaticPorts() { addFixedExposedPort(3268, 3268); // global catalog + addFixedExposedPort(3269, 3269); // global catalog over tls addFixedExposedPort(53, 53, InternetProtocol.TCP); // DNS over TCP addFixedExposedPort(53, 53, InternetProtocol.UDP); // DNS over UDP return this; diff --git a/src/test/java/hudson/plugins/active_directory/docker/TheFlintstonesIT.java b/src/test/java/hudson/plugins/active_directory/docker/TheFlintstonesIT.java index 26c9eaac..bfe4f966 100644 --- a/src/test/java/hudson/plugins/active_directory/docker/TheFlintstonesIT.java +++ b/src/test/java/hudson/plugins/active_directory/docker/TheFlintstonesIT.java @@ -30,20 +30,18 @@ import java.util.List; import javax.naming.NamingException; import javax.servlet.ServletException; + +import hudson.plugins.active_directory.TlsConfiguration; import org.acegisecurity.AuthenticationServiceException; import org.acegisecurity.userdetails.UserDetails; -import org.junit.Assume; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.LoggerRule; -import org.testcontainers.DockerClientFactory; import hudson.plugins.active_directory.ActiveDirectoryDomain; import hudson.plugins.active_directory.ActiveDirectorySecurityRealm; import hudson.plugins.active_directory.GroupLookupStrategy; -import hudson.util.Secret; /** * Integration tests with Docker and requiring custom DNS in the target env with fixed ports. @@ -66,16 +64,22 @@ public class TheFlintstonesIT { public final static String AD_MANAGER_DN = "CN=Administrator,CN=Users,DC=SAMDOM,DC=EXAMPLE,DC=COM"; public final static String AD_MANAGER_DN_PASSWORD = "ia4uV1EeKait"; public final static int MAX_RETRIES = 30; + public final static int GLOBAL_CATALOG_PLAIN_TEXT = 3268; + public final static int GLOBAL_CATALOG_TLS = 3269; public String dockerIp; public int dockerPort; public void dynamicSetUp() throws Exception { - dockerIp = docker.getHost(); - dockerPort = docker.getMappedPort(3268); + dynamicSetUp(false); + } + + public void dynamicSetUp(boolean requireTLS) throws Exception { + dockerIp = requireTLS ? docker.getHost() : "dc1.samdom.example.com"; + dockerPort = docker.getMappedPort(requireTLS ? GLOBAL_CATALOG_TLS : GLOBAL_CATALOG_PLAIN_TEXT); ActiveDirectoryDomain activeDirectoryDomain = new ActiveDirectoryDomain(AD_DOMAIN, dockerIp + ":" + dockerPort , null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD); List domains = new ArrayList<>(1); domains.add(activeDirectoryDomain); - ActiveDirectorySecurityRealm activeDirectorySecurityRealm = new ActiveDirectorySecurityRealm(null, domains, null, null, null, null, GroupLookupStrategy.RECURSIVE, false, true, null, false, null, false); + ActiveDirectorySecurityRealm activeDirectorySecurityRealm = new ActiveDirectorySecurityRealm(null, domains, null, null, null, null, GroupLookupStrategy.RECURSIVE, false, true, null, false, null, requireTLS); j.getInstance().setSecurityRealm(activeDirectorySecurityRealm); while(!docker.getLogs().contains("custom (exit status 0; expected)")) { Thread.sleep(1000); @@ -97,7 +101,7 @@ public void dynamicSetUp() throws Exception { public void validateCustomDomainController() throws ServletException, NamingException, IOException, Exception { dynamicSetUp(); ActiveDirectoryDomain.DescriptorImpl adDescriptor = new ActiveDirectoryDomain.DescriptorImpl(); - assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, dockerIp + ":" + dockerPort, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, null, false).toString().trim()); + assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, dockerIp + ":" + dockerPort, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, null, null, false, false, false).toString().trim()); } @Issue("JENKINS-36148") @@ -105,7 +109,7 @@ public void validateCustomDomainController() throws ServletException, NamingExce public void validateDomain() throws ServletException, NamingException, IOException, Exception { dynamicSetUp(); ActiveDirectoryDomain.DescriptorImpl adDescriptor = new ActiveDirectoryDomain.DescriptorImpl(); - assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, null, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, null, false).toString().trim()); + assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, null, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, null, null, false, false, false).toString().trim()); } @@ -114,7 +118,7 @@ public void validateDomain() throws ServletException, NamingException, IOExcepti public void validateTestDomainRequireTLSDisabled() throws Exception { dynamicSetUp(); ActiveDirectoryDomain.DescriptorImpl adDescriptor = new ActiveDirectoryDomain.DescriptorImpl(); - assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, null, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, null, false).toString().trim()); + assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, null, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, null, null, false, false, false).toString().trim()); } @Issue("JENKINS-69683") @@ -122,7 +126,23 @@ public void validateTestDomainRequireTLSDisabled() throws Exception { public void validateTestDomainServerRequireTLSDisabled() throws Exception { dynamicSetUp(); ActiveDirectoryDomain.DescriptorImpl adDescriptor = new ActiveDirectoryDomain.DescriptorImpl(); - assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, dockerIp + ":" + dockerPort, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, null, false).toString().trim()); + assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, dockerIp + ":" + dockerPort, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, null, null, false, false, false).toString().trim()); + } + + @Issue("SECURITY-3059") + @Test + public void validateTestDomainServerRequireTLSEnabled() throws Exception { + dynamicSetUp(true); + ActiveDirectoryDomain.DescriptorImpl adDescriptor = new ActiveDirectoryDomain.DescriptorImpl(); + assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, "dc1.samdom.example.com" + ":" + dockerPort, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, TlsConfiguration.TRUST_ALL_CERTIFICATES, GroupLookupStrategy.TOKENGROUPS, false, false, true).toString().trim()); + } + + @Issue("SECURITY-3059") + @Test + public void validateTestDomainServerRequireStartTLSEnabled() throws Exception { + dynamicSetUp(); + ActiveDirectoryDomain.DescriptorImpl adDescriptor = new ActiveDirectoryDomain.DescriptorImpl(); + assertEquals("OK: Success", adDescriptor.doValidateTest(AD_DOMAIN, "dc1.samdom.example.com" + ":" + dockerPort, null, AD_MANAGER_DN, AD_MANAGER_DN_PASSWORD, TlsConfiguration.TRUST_ALL_CERTIFICATES, GroupLookupStrategy.TOKENGROUPS, false, true, false).toString().trim()); } }