Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FIXED JENKINS-22830]
Providing the option for the user to pick the lesser of two evils. Performance vs Correctness.

Looking forward to collect more information in the future to improve this situation, but in the mean tim we need to make this plugin usable.
  • Loading branch information
kohsuke committed Jun 4, 2014
1 parent 8803815 commit ba12cf0
Show file tree
Hide file tree
Showing 6 changed files with 198 additions and 35 deletions.
Expand Up @@ -15,6 +15,8 @@
import hudson.security.SecurityRealm;
import hudson.security.TokenBasedRememberMeServices2;
import hudson.util.FormValidation;
import hudson.util.ListBoxModel;
import hudson.util.ListBoxModel.Option;
import hudson.util.Secret;
import hudson.util.spring.BeanBuilder;

Expand Down Expand Up @@ -107,12 +109,19 @@ public class ActiveDirectorySecurityRealm extends AbstractPasswordBasedSecurityR
*/
public final String server;

@DataBoundConstructor
private GroupLookupStrategy groupLookupStrategy;

public ActiveDirectorySecurityRealm(String domain, String site, String bindName, String bindPassword, String server) {
this(domain,site,bindName,bindPassword,server,GroupLookupStrategy.AUTO);
}

@DataBoundConstructor
public ActiveDirectorySecurityRealm(String domain, String site, String bindName, String bindPassword, String server, GroupLookupStrategy groupLookupStrategy) {
this.domain = fixEmpty(domain);
this.site = fixEmpty(site);
this.bindName = fixEmpty(bindName);
this.bindPassword = Secret.fromString(fixEmpty(bindPassword));
this.groupLookupStrategy = groupLookupStrategy;

// append default port if not specified
server = fixEmpty(server);
Expand All @@ -123,6 +132,11 @@ public ActiveDirectorySecurityRealm(String domain, String site, String bindName,
this.server = server;
}

public GroupLookupStrategy getGroupLookupStrategy() {
if (groupLookupStrategy==null) return GroupLookupStrategy.AUTO;
return groupLookupStrategy;
}

public SecurityComponents createSecurityComponents() {
BeanBuilder builder = new BeanBuilder(getClass().getClassLoader());
Binding binding = new Binding();
Expand Down Expand Up @@ -237,6 +251,14 @@ public boolean canDoNativeAuth() {
}
}

public ListBoxModel doFillGroupLookupStrategyItems() {
ListBoxModel model = new ListBoxModel();
for (GroupLookupStrategy e : GroupLookupStrategy.values()) {
model.add(e.getDisplayName(),e.name());
}
return model;
}

private static boolean WARNED = false;

public FormValidation doValidate(@QueryParameter(fixEmpty = true) String domain, @QueryParameter(fixEmpty = true) String site, @QueryParameter(fixEmpty = true) String bindName,
Expand Down
Expand Up @@ -6,6 +6,8 @@
import hudson.security.UserMayOrMayNotExistException;
import hudson.util.Secret;
import javax.naming.NameNotFoundException;

import hudson.util.TimeUnit2;
import org.acegisecurity.AuthenticationException;
import org.acegisecurity.AuthenticationServiceException;
import org.acegisecurity.BadCredentialsException;
Expand All @@ -32,8 +34,6 @@
import java.util.logging.Level;
import java.util.logging.Logger;

import org.apache.commons.lang.StringEscapeUtils;

/**
* {@link AuthenticationProvider} with Active Directory, through LDAP.
*
Expand All @@ -56,6 +56,8 @@ public class ActiveDirectoryUnixAuthenticationProvider extends AbstractActiveDir

private final ActiveDirectorySecurityRealm.DescriptorImpl descriptor;

private GroupLookupStrategy groupLookupStrategy;

/**
* {@link ActiveDirectoryGroupDetails} cache.
*/
Expand Down Expand Up @@ -121,6 +123,7 @@ public ActiveDirectoryUnixAuthenticationProvider(ActiveDirectorySecurityRealm re
this.bindName = realm.bindName;
this.server = realm.server;
this.bindPassword = Secret.toString(realm.bindPassword);
this.groupLookupStrategy = realm.getGroupLookupStrategy();
this.descriptor = realm.getDescriptor();
}

Expand Down Expand Up @@ -405,51 +408,103 @@ private Set<GrantedAuthority> resolveGroups(String domainDN, String userDN, DirC
*/
LOGGER.fine("Stage 2: looking up via memberOf");

// this Microsoft extension is explained in http://msdn.microsoft.com/en-us/library/aa746475(v=vs.85).aspx
renum = new LDAPSearchBuilder(context, domainDN).subTreeScope().returns("cn").search(
"(member:1.2.840.113556.1.4.1941:={0})", userDN);
while (true) {
switch (groupLookupStrategy) {
case AUTO:
// try the accurate one first, and if it's too slow fall back to recursive in the hope that it's faster
long start = System.currentTimeMillis();
boolean found = chainGroupLookup(domainDN, userDN, context, groups);
long duration = (System.currentTimeMillis() - start) / TimeUnit2.SECONDS.toMillis(1);
if (!found || duration >= 10) {
LOGGER.warning(String.format("AD chain lookup is taking too long (%dms). Falling back to recursive lookup", duration));
groupLookupStrategy = GroupLookupStrategy.RECURSIVE;
continue;
} else {
// it run fast enough, so let's stick to it
groupLookupStrategy = GroupLookupStrategy.CHAIN;
return groups;
}
case RECURSIVE:
recursiveGroupLookup(context, id, groups);
return groups;
case CHAIN:
chainGroupLookup(domainDN, userDN, context, groups);
return groups;
}
}
}
}

/**
* Performs AD-extension to LDAP query that performs recursive group lookup.
* This Microsoft extension is explained in http://msdn.microsoft.com/en-us/library/aa746475(v=vs.85).aspx
*
* @return
* false if it appears that this search failed.
* @see
*/
private boolean chainGroupLookup(String domainDN, String userDN, DirContext context, Set<GrantedAuthority> groups) throws NamingException {
NamingEnumeration<SearchResult> renum = new LDAPSearchBuilder(context, domainDN).subTreeScope().returns("cn").search(
"(member:1.2.840.113556.1.4.1941:={0})", userDN);
try {
if (renum.hasMore()) {
// http://ldapwiki.willeke.com/wiki/Active%20Directory%20Group%20Related%20Searches cites that
// this filter search extension requires at least Win2K3 SP2. So if this didn't find anything,
// fall back to the recursive search

// TODO: this search alone might be producing the super set of the tokenGroups/objectSid based search in the stage 1.
parseMembers(userDN, groups, renum);
return true;
} else {
Stack<Attributes> q = new Stack<Attributes>();
q.push(id);
while (!q.isEmpty()) {
Attributes identity = q.pop();
LOGGER.finer("Looking up group of " + identity);

Attribute memberOf = identity.get("memberOf");
if (memberOf == null)
continue;
return false;
}
} finally {
renum.close();
}
}

for (int i = 0; i < memberOf.size(); i++) {
try {
Attributes group = context.getAttributes(new LdapName(memberOf.get(i).toString()), new String[]{"CN", "memberOf"});
Attribute cn = group.get("CN");
if (cn == null) {
LOGGER.fine("Failed to obtain CN of " + memberOf.get(i));
continue;
}
if (LOGGER.isLoggable(Level.FINE))
LOGGER.fine(cn.get() + " is a member of " + memberOf.get(i));
/**
* Performs recursive group membership lookup.
*
* This was how we did the lookup traditionally until we discovered 1.2.840.113556.1.4.1941.
* But various people reported that it slows down the execution tremendously to the point that it is unusable,
* while others seem to report that it runs faster than recursive search (http://social.technet.microsoft.com/Forums/fr-FR/f238d2b0-a1d7-48e8-8a60-542e7ccfa2e8/recursive-retrieval-of-all-ad-group-memberships-of-a-user?forum=ITCG)
*
* This implementation is kept for Windows 2003 that doesn't support 1.2.840.113556.1.4.1941, but it can be also
* enabled for those who are seeing the performance problem.
*
* See JENKINS-22830
*/
private void recursiveGroupLookup(DirContext context, Attributes id, Set<GrantedAuthority> groups) throws NamingException {
Stack<Attributes> q = new Stack<Attributes>();
q.push(id);
while (!q.isEmpty()) {
Attributes identity = q.pop();
LOGGER.finer("Looking up group of " + identity);

Attribute memberOf = identity.get("memberOf");
if (memberOf == null)
continue;

for (int i = 0; i < memberOf.size(); i++) {
try {
Attributes group = context.getAttributes(new LdapName(memberOf.get(i).toString()), new String[]{"CN", "memberOf"});
Attribute cn = group.get("CN");
if (cn == null) {
LOGGER.fine("Failed to obtain CN of " + memberOf.get(i));
continue;
}
if (LOGGER.isLoggable(Level.FINE))
LOGGER.fine(cn.get() + " is a member of " + memberOf.get(i));

if (groups.add(new GrantedAuthorityImpl(cn.get().toString()))) {
q.add(group); // recursively look for groups that this group is a member of.
}
} catch (NameNotFoundException e) {
LOGGER.fine("Failed to obtain CN of " + memberOf.get(i));
}
if (groups.add(new GrantedAuthorityImpl(cn.get().toString()))) {
q.add(group); // recursively look for groups that this group is a member of.
}
} catch (NameNotFoundException e) {
LOGGER.fine("Failed to obtain CN of " + memberOf.get(i));
}
}
renum.close();
}

return groups;
}

private void parseMembers(String userDN, Set<GrantedAuthority> groups, NamingEnumeration<SearchResult> renum) throws NamingException {
Expand Down
@@ -0,0 +1,32 @@
package hudson.plugins.active_directory;

import org.jvnet.localizer.Localizable;

/**
* Hack to let people pick lesser of two evils: performance or completeness.
*
* See JENKINS-22830 for the context.
*
* Marking as package private because I hope to get rid of this switch one day by figuring out
* "the right way".
*
* See help-groupLookupStrategy.html for the detailed discussion.
*
* @author Kohsuke Kawaguchi
*/
/*hidden*/ enum GroupLookupStrategy {
AUTO (Messages._GroupLookupStrategy_Auto()),
RECURSIVE(Messages._GroupLookupStrategy_Recursive()),
CHAIN (Messages._GroupLookupStrategy_ChainMatch()),
;

public final Localizable msg;

GroupLookupStrategy(Localizable msg) {
this.msg = msg;
}

public String getDisplayName() {
return msg.toString();
}
}
@@ -1,6 +1,7 @@
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<j:choose>
<j:when test="${!descriptor.canDoNativeAuth()}">
<!-- code path for ActiveDirectoryUnixAuthenticationProvider -->
<f:entry title="${%Domain Name}" field="domain" help="/plugin/active-directory/help/domain-name-unix.html">
<f:textbox />
</f:entry>
Expand All @@ -17,12 +18,16 @@
<f:entry field="bindPassword" title="${%Bind Password}">
<f:password />
</f:entry>
<f:entry field="groupLookupStrategy" title="${%Group Membership Lookup Strategy}">
<f:select />
</f:entry>
</f:advanced>
<f:nested>
<f:validateButton with="domain,site,bindName,bindPassword,server" title="${%Test}" method="validate"/>
</f:nested>
</j:when>
<j:otherwise>
<!-- code path for ActiveDirectoryAuthenticationProvider -->
<f:nested>
<f:validateButton with="domain,server" title="${%Test}" method="validate"/>
</f:nested>
Expand Down
@@ -0,0 +1,45 @@
<div>
<p>
Determines how group membership lookup is performed.

<p>
This option is here because we the Jenkins developers are between a rock and a hard place when
it comes to determining groups that an user belongs to. We have several approaches implemented
over the time, but none of them seem to satisfy everyone. So we basically threw hands in the air
and asking you to find one that works for you.

<p>
The only people who should try to tweak this setting is (1) those who find login very slow or
(2) those who find that Jenkins isn't picking up all the groups that you belong to. The general rule
of thumb is that you try both options while paying attention to the time it takes to login and
the groups reported in the "/whoAmI" page, and pick one that works for you.

<dl>
<dt>Automatic
<dd>
Do the best to pick the "best" algorithm. Specifically, Jenkins tries LDAP_MATCHING_RULE_IN_CHAIN first,
and if it's going too slow, then stick to the "recursive queries".


<dt>Recursive queries
<dd>
For each group X that Jenkins discovers, recursively list up all the other groups that has X as a member,
by issuing a separate LDAP query. This increases the number of queries, but each query remains simple.
Note that several people reported that this approach fails to find some groups that the user is actually
a member of, but the developers of this plugin do not have access to such AD deployment to investigate
the claim.

<dt>LDAP_MATCHING_RULE_IN_CHAIN
<dd>
Use <a href="http://msdn.microsoft.com/en-us/library/aa746475(v=vs.85).aspx">Microsoft extension to LDAP</a>
that was specifically added to Windows 2003 to perform recursive group membership lookup.
The general consensus in the developer community appears to be that <a href="http://www.networksteve.com/forum/topic.php/?TopicId=43899">this is the way to go</a>,
but several Jenkins users have reported that <a href="https://issues.jenkins-ci.org/browse/JENKINS-22830">this approach makes lookup intolerably slow</a>.

</dl>

<p>
It is less than ideal to ask users to make a choice like this. If you have insights to share to improve
this situation, or even just data point,
<a href="https://issues.jenkins-ci.org/browse/JENKINS-22830">please add your thoughts to JENKINS-22830</a>.
</div>
@@ -1 +1,5 @@
DisplayName=Active Directory
DisplayName=Active Directory

GroupLookupStrategy.Auto=Automatic
GroupLookupStrategy.Recursive=Recursive group queries
GroupLookupStrategy.ChainMatch=LDAP_MATCHING_RULE_IN_CHAIN

0 comments on commit ba12cf0

Please sign in to comment.