Skip to content

Commit

Permalink
Implement and enforce user account status (#9374)
Browse files Browse the repository at this point in the history
* Add AccountStatus field to User
* Try to cope with incomplete LDAP user entries.
 - If we can't get the userNameAttribute, simply use the username
 - Fill in dummy email address if none can't be found
* Prevent user logins for disabled accounts
* Add account_status to UserSummary
* Add endpoint to modify user acount status
* Add auditevent
* Double check that an account is enabled in AD
  This deliberately ignores the pre-auth flag set by the
  HTTPHeaderAuthRealm.
* Fix review comments

Fixes #9077
  • Loading branch information
mpfz0r committed Nov 9, 2020
1 parent 14a089d commit ae427f1
Show file tree
Hide file tree
Showing 20 changed files with 301 additions and 128 deletions.
Expand Up @@ -111,6 +111,7 @@ private User provisionUser(UserDetails userDetails) {

// Only set fields that are okay to override by the authentication service here!
user.setExternal(true);
user.setAccountStatus(userDetails.accountIsEnabled() ? User.AccountStatus.ENABLED : User.AccountStatus.DISABLED);
user.setAuthServiceId(userDetails.authServiceId());
user.setAuthServiceUid(userDetails.base64AuthServiceUid());
user.setName(userDetails.username());
Expand Down
Expand Up @@ -37,6 +37,8 @@ public abstract class UserDetails {

public abstract String username();

public abstract boolean accountIsEnabled();

public abstract String email();

public abstract String fullName();
Expand Down Expand Up @@ -71,6 +73,8 @@ public static Builder create() {

public abstract Builder username(String username);

public abstract Builder accountIsEnabled(boolean isEnabled);

public abstract Builder email(String email);

public abstract Builder fullName(String fullName);
Expand Down
Expand Up @@ -92,6 +92,10 @@ public Optional<UserDetails> authenticateAndProvision(AuthServiceCredentials aut

final LDAPUser userEntry = optionalUser.get();

if (!userEntry.accountIsEnabled()) {
LOG.warn("Account disabled within Active Directory for user <{}> (DN: {})", authCredentials.username(), userEntry.dn());
return Optional.empty();
}
if (!authCredentials.isAuthenticated()) {
if (!isAuthenticated(connection, userEntry, authCredentials)) {
LOG.debug("Invalid credentials for user <{}> (DN: {})", authCredentials.username(), userEntry.dn());
Expand All @@ -104,6 +108,7 @@ public Optional<UserDetails> authenticateAndProvision(AuthServiceCredentials aut
.authServiceId(backendId())
.base64AuthServiceUid(userEntry.base64UniqueId())
.username(userEntry.username())
.accountIsEnabled(userEntry.accountIsEnabled())
.fullName(userEntry.fullName())
.email(userEntry.email())
.defaultRoles(backend.defaultRoles())
Expand Down Expand Up @@ -257,13 +262,15 @@ private Map<String, Object> createTestResult(ADAuthServiceBackendConfig config,
.put("login_success", loginSuccess);

if (user != null) {
userDetails.put("user_details", ImmutableMap.of(
"dn", user.dn(),
AD_OBJECT_GUID, user.base64UniqueId(),
config.userNameAttribute(), user.username(),
config.userFullNameAttribute(), user.fullName(),
"email", user.email()
));
userDetails.put("user_details", ImmutableMap.<String, String>builder()
.put("dn", user.dn())
.put("account_enabled", String.valueOf(user.accountIsEnabled()))
.put(AD_OBJECT_GUID, user.base64UniqueId())
.put(config.userNameAttribute(), user.username())
.put(config.userFullNameAttribute(), user.fullName())
.put("email", user.email())
.build()
);
} else {
userDetails.put("user_details", ImmutableMap.of());
}
Expand Down
Expand Up @@ -87,6 +87,7 @@ public Optional<UserDetails> authenticateAndProvision(AuthServiceCredentials aut
final UserDetails userDetails = provisionerService.provision(provisionerService.newDetails(this)
.authServiceType(backendType())
.authServiceId(backendId())
.accountIsEnabled(true)
.base64AuthServiceUid(userEntry.base64UniqueId())
.username(userEntry.username())
.fullName(userEntry.fullName())
Expand Down Expand Up @@ -241,13 +242,14 @@ private Map<String, Object> createTestResult(LDAPAuthServiceBackendConfig config
.put("login_success", loginSuccess);

if (user != null) {
userDetails.put("user_details", ImmutableMap.of(
"dn", user.dn(),
config.userUniqueIdAttribute(), user.base64UniqueId(),
config.userNameAttribute(), user.username(),
config.userFullNameAttribute(), user.fullName(),
"email", user.email()
));
userDetails.put("user_details", ImmutableMap.<String, String>builder()
.put("dn", user.dn())
.put(config.userUniqueIdAttribute(), user.base64UniqueId())
.put(config.userNameAttribute(), user.username())
.put(config.userFullNameAttribute(), user.fullName())
.put("email", user.email())
.build()
);
} else {
userDetails.put("user_details", ImmutableMap.of());
}
Expand Down
Expand Up @@ -68,6 +68,10 @@ public Optional<UserDetails> authenticateAndProvision(AuthServiceCredentials aut
if (user.isLocalAdmin()) {
throw new IllegalStateException("Local admin user should have been handled earlier and not reach the authentication service authenticator");
}
if (!user.getAccountStatus().equals(User.AccountStatus.ENABLED)) {
LOG.warn("Account for user <{}> is disabled.", user.getName());
return Optional.empty();
}
if (user.isExternalUser()) {
// We don't store passwords for users synced from an authentication service, so we can't handle them here.
LOG.trace("Skipping mongodb-based password check for external user {}", authCredentials.username());
Expand All @@ -86,6 +90,7 @@ public Optional<UserDetails> authenticateAndProvision(AuthServiceCredentials aut
final UserDetails userDetails = provisionerService.provision(provisionerService.newDetails(this)
.databaseId(user.getId())
.username(user.getName())
.accountIsEnabled(user.getAccountStatus().equals(User.AccountStatus.ENABLED))
.email(user.getEmail())
.fullName(user.getFullName())
// No need to set default roles because MongoDB users will not be provisioned by the provisioner
Expand Down
@@ -0,0 +1,91 @@
/**
* This file is part of Graylog.
*
* Graylog is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* Graylog is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with Graylog. If not, see <http://www.gnu.org/licenses/>.
*/
package org.graylog.security.authservice.ldap;

import com.google.auto.value.AutoValue;

import java.util.EnumSet;
import java.util.stream.Collectors;

/**
* Active Directory UserAccountControl flags.
*
* See: https://docs.microsoft.com/en-us/troubleshoot/windows-server/identity/useraccountcontrol-manipulate-account-properties
*/

@AutoValue
public abstract class ADUserAccountControl {

public abstract EnumSet<Flags> flags();

public static ADUserAccountControl create(int userAccountControlField) {
EnumSet<Flags> set = EnumSet.noneOf(Flags.class);
for (Flags flag : Flags.values()) {
if (flag.isSetIn(userAccountControlField)) {
set.add(flag);
}
}
return new AutoValue_ADUserAccountControl(set);
}

public boolean accountIsDisabled() {
return flags().contains(Flags.ACCOUNTDISABLE);
}

public boolean isUserAccount() {
return flags().contains(Flags.NORMAL_ACCOUNT);
}

public boolean passwordExpired() {
return flags().contains(Flags.PASSWORD_EXPIRED);
}

public String printFlags() {
return flags().stream().sorted().map(Enum::toString).collect(Collectors.joining("|"));
}

public enum Flags {
/**
* The user account is disabled.
*/
ACCOUNTDISABLE(0x2),

/**
* This is a default account type that represents a typical user. (not a computer, for example)
*/
NORMAL_ACCOUNT(0x200),

/**
* The user password has expired. (This doesn't work with newer AD implementations)
*/
PASSWORD_EXPIRED(0x800000);

public int getFlagValue() {
return flagValue;
}

private final int flagValue;

Flags(int flagValue) {
this.flagValue = flagValue;
}

public boolean isSetIn(int userAccountControlValue) {
return (userAccountControlValue & flagValue) > 0;
}
}
}

This file was deleted.

Expand Up @@ -22,6 +22,8 @@
public abstract class LDAPUser {
public abstract String base64UniqueId();

public abstract boolean accountIsEnabled();

public abstract String username();

public abstract String fullName();
Expand All @@ -46,6 +48,8 @@ public static Builder create() {

public abstract Builder base64UniqueId(String base64UniqueId);

public abstract Builder accountIsEnabled(boolean isEnabled);

public abstract Builder username(String username);

public abstract Builder fullName(String fullName);
Expand Down
Expand Up @@ -18,6 +18,7 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.Ints;
import com.unboundid.ldap.sdk.Attribute;
import com.unboundid.ldap.sdk.BindRequest;
import com.unboundid.ldap.sdk.BindResult;
Expand Down Expand Up @@ -194,6 +195,7 @@ private Optional<LDAPUser> searchUser(LDAPConnection connection,
Filter filter) throws LDAPException {
final ImmutableSet<String> allAttributes = ImmutableSet.<String>builder()
.add("userPrincipalName") // TODO: This is ActiveDirectory specific - Do we need this here?
.add("userAccountControl")
.add("mail")
.add("rfc822Mailbox")
.add(config.userUniqueIdAttribute())
Expand Down Expand Up @@ -289,12 +291,29 @@ public LDAPUser createLDAPUser(UnboundLDAPConfig config, Entry entry) {
}

public LDAPUser createLDAPUser(UnboundLDAPConfig config, LDAPEntry ldapEntry) {
final String username = ldapEntry.nonBlankAttribute(config.userNameAttribute());
return LDAPUser.builder()
.base64UniqueId(ldapEntry.base64UniqueId())
.username(ldapEntry.nonBlankAttribute(config.userNameAttribute()))
.fullName(ldapEntry.nonBlankAttribute(config.userFullNameAttribute()))
.email(ldapEntry.firstAttributeValue("mail").orElse(ldapEntry.firstAttributeValue("rfc822Mailbox").orElse("")))
.accountIsEnabled(findAccountIsEnabled(ldapEntry))
.username(username)
.fullName(ldapEntry.firstAttributeValue(config.userFullNameAttribute()).orElse(username))
.email(ldapEntry.firstAttributeValue("mail").orElse(ldapEntry.firstAttributeValue("rfc822Mailbox").orElse("unknown@unknown.invalid")))
.entry(ldapEntry)
.build();
}

private boolean findAccountIsEnabled(LDAPEntry ldapEntry) {
final Optional<String> control = ldapEntry.firstAttributeValue("userAccountControl");

// No field present. Assume account is enabled
if (!control.isPresent()) {
return true;
}
final Integer userAccountControl = Ints.tryParse(control.get());
if (userAccountControl == null) {
LOG.warn("Ignoring non-parseable userAccountControl value");
return true;
}
return !ADUserAccountControl.create(userAccountControl).accountIsDisabled();
}
}
Expand Up @@ -16,6 +16,7 @@
*/
package org.graylog2.plugin.database.users;

import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.shiro.authz.Permission;
import org.graylog2.plugin.database.Persisted;
import org.graylog2.rest.models.users.requests.Startpage;
Expand Down Expand Up @@ -102,4 +103,17 @@ public interface User extends Persisted {
Set<String> getRoleIds();

void setRoleIds(Set<String> roles);

void setAccountStatus(AccountStatus status);

AccountStatus getAccountStatus();

enum AccountStatus {
@JsonProperty("enabled")
ENABLED,
@JsonProperty("disabled")
DISABLED,
@JsonProperty("deleted")
DELETED
}
}
Expand Up @@ -25,6 +25,7 @@
import org.apache.shiro.authz.permission.WildcardPermission;
import org.graylog.autovalue.WithBeanGetter;
import org.graylog.security.permissions.GRNPermission;
import org.graylog2.plugin.database.users.User;
import org.graylog2.rest.models.users.requests.Startpage;

import javax.annotation.Nullable;
Expand Down Expand Up @@ -95,6 +96,9 @@ public abstract class UserSummary {
@Nullable
public abstract String clientAddress();

@JsonProperty("account_status")
public abstract User.AccountStatus accountStatus();

@JsonCreator
public static UserSummary create(@JsonProperty("id") @Nullable String id,
@JsonProperty("username") String username,
Expand All @@ -111,7 +115,8 @@ public static UserSummary create(@JsonProperty("id") @Nullable String id,
@JsonProperty("roles") @Nullable Set<String> roles,
@JsonProperty("session_active") boolean sessionActive,
@JsonProperty("last_activity") @Nullable Date lastActivity,
@JsonProperty("client_address") @Nullable String clientAddress) {
@JsonProperty("client_address") @Nullable String clientAddress,
@JsonProperty("account_status") User.AccountStatus accountStatus) {
return new AutoValue_UserSummary(id,
username,
email,
Expand All @@ -127,6 +132,7 @@ public static UserSummary create(@JsonProperty("id") @Nullable String id,
roles,
sessionActive,
lastActivity,
clientAddress);
clientAddress,
accountStatus);
}
}

0 comments on commit ae427f1

Please sign in to comment.