Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-55813] Improve analysis of AD attributes #96

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,12 @@ If you are not sure what the notation for a group name is, try the following pro
4. Add the relevant groups found to the security matrix with appropriate permissions
5. Do not forget to withdraw permissions from the anonymous user, taking into consideration the Overall:Read permission (hover over the column header for detail)

#### Account Validity Attributes

When a user account is disabled, expired, or locked, it is usually the responsibility of the Active Directory server to enforce such states when logging in as the affected user.
After a user has logged in to Jenkins successfully, these user states may change.
In order to ensure a user account is still valid when being used in other contexts where their password is not known (such as API tokens), additional checks are performed on user accounts to ensure the account is still allowed to be used.

## Troubleshooting

#### Create/Update a dedicated Logs Recorder
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,33 +43,49 @@
import hudson.security.GroupDetails;
import hudson.security.SecurityRealm;
import hudson.security.UserMayOrMayNotExistException;
import org.acegisecurity.AccountExpiredException;
import org.acegisecurity.AuthenticationException;
import org.acegisecurity.BadCredentialsException;
import org.acegisecurity.DisabledException;
import org.acegisecurity.GrantedAuthority;
import org.acegisecurity.GrantedAuthorityImpl;
import org.acegisecurity.LockedException;
import org.acegisecurity.providers.AuthenticationProvider;
import org.acegisecurity.providers.UsernamePasswordAuthenticationToken;
import org.acegisecurity.userdetails.UserDetails;
import org.acegisecurity.userdetails.UserDetailsService;
import org.acegisecurity.userdetails.UsernameNotFoundException;
import org.apache.commons.lang.StringUtils;
import org.springframework.dao.DataAccessException;
import org.springframework.dao.DataAccessResourceFailureException;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Date;
import java.util.GregorianCalendar;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.logging.Level;
import java.util.logging.Logger;

import org.apache.commons.lang.StringUtils;
import org.springframework.dao.DataAccessException;
import org.springframework.dao.DataAccessResourceFailureException;

/**
* {@link AuthenticationProvider} with Active Directory, plus {@link UserDetailsService}
*
* @author Kohsuke Kawaguchi
*/
public class ActiveDirectoryAuthenticationProvider extends AbstractActiveDirectoryAuthenticationProvider {

/**
* See https://docs.microsoft.com/en-us/windows/desktop/adsi/example-code-for-reading-a-constructed-attribute
* And https://issues.jenkins-ci.org/browse/JENKINS-10086
*/
private static final int E_ADS_PROPERTY_NOT_FOUND = 0x8000_500D;

// https://docs.microsoft.com/en-us/windows/win32/adschema/a-accountexpires
private static final GregorianCalendar ACCOUNT_DOES_NOT_EXPIRE =
new GregorianCalendar(1601, Calendar.JANUARY, 1);

@SuppressFBWarnings("MS_SHOULD_BE_FINAL")
private static /* non-final for Groovy */ boolean ALLOW_EMPTY_PASSWORD = Boolean.getBoolean(ActiveDirectoryAuthenticationProvider.class.getName() + ".ALLOW_EMPTY_PASSWORD");

Expand Down Expand Up @@ -201,12 +217,15 @@ public UserDetails call() {
}
groups.add(SecurityRealm.AUTHENTICATED_AUTHORITY);

checkIfAccountDisabled(usr);
checkIfAccountExpired(usr);
checkIfAccountLocked(usr);

LOGGER.log(Level.FINE, "Login successful: {0} dn={1}", new Object[] {username, dn});

return new ActiveDirectoryUserDetail(
username, "redacted",
!isAccountDisabled(usr),
true, true, true,
true, true, true, true,
groups.toArray(new GrantedAuthority[0]),
getFullName(usr), getEmailAddress(usr), getTelephoneNumber(usr)
).updateUserInfo();
Expand Down Expand Up @@ -239,8 +258,9 @@ private String getTelephoneNumber(IADsUser usr) {
Object t = usr.telephoneNumber();
return t==null ? null : t.toString();
} catch (ComException e) {
if (e.getHRESULT()==0x8000500D) // see http://support.microsoft.com/kb/243440
if (e.getHRESULT() == E_ADS_PROPERTY_NOT_FOUND) {
return null;
}
throw e;
}
}
Expand All @@ -249,8 +269,9 @@ private String getEmailAddress(IADsUser usr) {
try {
return usr.emailAddress();
} catch (ComException e) {
if (e.getHRESULT()==0x8000500D) // see http://support.microsoft.com/kb/243440
if (e.getHRESULT() == E_ADS_PROPERTY_NOT_FOUND){
return null;
}
throw e;
}
}
Expand All @@ -259,24 +280,51 @@ private String getFullName(IADsUser usr) {
try {
return usr.fullName();
} catch (ComException e) {
if (e.getHRESULT()==0x8000500D) // see http://support.microsoft.com/kb/243440
if (e.getHRESULT() == E_ADS_PROPERTY_NOT_FOUND) {
return null;
}
throw e;
}
}

private boolean isAccountDisabled(IADsUser usr) {
private void checkIfAccountDisabled(IADsUser usr) {
try {
if (usr.accountDisabled()) {
throw new DisabledException(Messages.UserDetails_Disabled(usr.name()));
}
} catch (ComException e) {
if (e.getHRESULT() != E_ADS_PROPERTY_NOT_FOUND) {
throw e;
}
}
}

private void checkIfAccountExpired(IADsUser usr) {
try {
return usr.accountDisabled();
Date expirationDate = usr.accountExpirationDate();
if (expirationDate.equals(ACCOUNT_DOES_NOT_EXPIRE.getTime())) {
return;
}
Date now = new Date();
if (now.after(expirationDate)) {
throw new AccountExpiredException(Messages.UserDetails_Expired(usr.name(), expirationDate));
}
} catch (ComException e) {
if (e.getHRESULT()==0x8000500D)
/*
See http://support.microsoft.com/kb/243440 and JENKINS-10086
We suspect this to be caused by old directory items that do not have this value,
so assume this account is enabled.
*/
return false;
throw e;
if (e.getHRESULT() != E_ADS_PROPERTY_NOT_FOUND) {
throw e;
}
}
}

private void checkIfAccountLocked(IADsUser usr) {
try {
if (usr.isAccountLocked()) {
throw new LockedException(Messages.UserDetails_Locked(usr.name()));
}
} catch (ComException e) {
if (e.getHRESULT() != E_ADS_PROPERTY_NOT_FOUND) {
throw e;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -615,10 +615,9 @@ private LdapContext bind(String principalName, String password, SocketInfo serve
LdapContext context = (LdapContext)LdapCtxFactory.getLdapCtxInstance(ldapUrl, props);

boolean isStartTls = true;
SecurityRealm securityRealm = Jenkins.getActiveInstance().getSecurityRealm();
SecurityRealm securityRealm = Jenkins.get().getSecurityRealm();
if (securityRealm instanceof ActiveDirectorySecurityRealm) {
ActiveDirectorySecurityRealm activeDirectorySecurityRealm = (ActiveDirectorySecurityRealm) securityRealm;
isStartTls= activeDirectorySecurityRealm.isStartTls();
isStartTls = Boolean.TRUE.equals(((ActiveDirectorySecurityRealm) securityRealm).isStartTls());
}

if (!FORCE_LDAPS && isStartTls) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@
import javax.naming.directory.SearchResult;
import javax.naming.ldap.LdapName;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Date;
import java.util.GregorianCalendar;
import java.util.HashSet;
import java.util.Hashtable;
import java.util.List;
Expand Down Expand Up @@ -382,12 +384,14 @@ public UserDetails call() throws AuthenticationException, NamingException {
// locate this user's record
final String domainDN = toDC(domain.getName());

Attributes user = new LDAPSearchBuilder(context, domainDN).subTreeScope().searchOne("(& (userPrincipalName={0})(objectCategory=user))", userPrincipalName);
Attributes user = new LDAPSearchBuilder(context, domainDN).subTreeScope().returns("*", "+")
.searchOne("(& (userPrincipalName={0})(objectCategory=user))", userPrincipalName);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no AD admin, but theoretically, these two LDAP filters can be combined into one like so:

(&(objectCategory=user)(|(userPrincipalName={0})(sAMAccountName={0})))

No idea how that would affect or potentially destroy performance of LDAP indices that may be set up.

if (user == null) {
// failed to find it. Fall back to sAMAccountName.
// see http://www.nabble.com/Re%3A-Hudson-AD-plug-in-td21428668.html
LOGGER.log(Level.FINE, "Failed to find {0} in userPrincipalName. Trying sAMAccountName", userPrincipalName);
user = new LDAPSearchBuilder(context, domainDN).subTreeScope().searchOne("(& (sAMAccountName={0})(objectCategory=user))", samAccountName);
user = new LDAPSearchBuilder(context, domainDN).subTreeScope().returns("*", "+")
.searchOne("(& (sAMAccountName={0})(objectCategory=user))", samAccountName);
if (user == null) {
throw new UsernameNotFoundException("Authentication was successful but cannot locate the user information for " + username);
}
Expand Down Expand Up @@ -420,10 +424,17 @@ public UserDetails call() throws AuthenticationException, NamingException {
Set<GrantedAuthority> groups = resolveGroups(domainDN, dnFormatted, context);
groups.add(SecurityRealm.AUTHENTICATED_AUTHORITY);

cacheMiss[0] = new ActiveDirectoryUserDetail(username, "redacted", true, true, true, true, groups.toArray(new GrantedAuthority[0]),
getStringAttribute(user, "displayName"),
getStringAttribute(user, "mail"),
getStringAttribute(user, "telephoneNumber")
UserAttributesHelper.checkIfUserIsEnabled(user);
UserAttributesHelper.checkIfAccountNonExpired(user);
UserAttributesHelper.checkIfCredentialsAreNonExpired(user);
UserAttributesHelper.checkIfAccountNonLocked(user);

cacheMiss[0] = new ActiveDirectoryUserDetail(username, "redacted",
true, true, true, true,
groups.toArray(new GrantedAuthority[0]),
UserAttributesHelper.getStringAttribute(user, "displayName"),
UserAttributesHelper.getStringAttribute(user, "mail"),
UserAttributesHelper.getStringAttribute(user, "telephoneNumber")
);
return cacheMiss[0];
} catch (NamingException e) {
Expand Down Expand Up @@ -599,15 +610,6 @@ private void closeQuietly(DirContext context) {
}
}


private String getStringAttribute(Attributes user, String name) throws NamingException {
Attribute a = user.get(name);
if (a==null) return null;
Object v = a.get();
if (v==null) return null;
return v.toString();
}

/**
* Returns the full user principal name of the form "joe@europe.contoso.com".
*
Expand Down