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

Log deprecation message on legacy ldap pool settings #2099

Merged
merged 7 commits into from
Oct 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/main/java/com/amazon/dlic/auth/ldap/LdapUser.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ public class LdapUser extends User {
private final String originalUsername;

public LdapUser(final String name, String originalUsername, final LdapEntry userEntry,
final AuthCredentials credentials, int customAttrMaxValueLen, WildcardMatcher whitelistedCustomLdapAttrMatcher) {
final AuthCredentials credentials, int customAttrMaxValueLen, WildcardMatcher allowlistedCustomLdapAttrMatcher) {
super(name, null, credentials);
this.originalUsername = originalUsername;
this.userEntry = userEntry;
Map<String, String> attributes = getCustomAttributesMap();
attributes.putAll(extractLdapAttributes(originalUsername, userEntry, customAttrMaxValueLen, whitelistedCustomLdapAttrMatcher));
attributes.putAll(extractLdapAttributes(originalUsername, userEntry, customAttrMaxValueLen, allowlistedCustomLdapAttrMatcher));
}

/**
Expand All @@ -57,7 +57,7 @@ public String getOriginalUsername() {
}

public static Map<String, String> extractLdapAttributes(String originalUsername, final LdapEntry userEntry,
int customAttrMaxValueLen, WildcardMatcher whitelistedCustomLdapAttrMatcher) {
int customAttrMaxValueLen, WildcardMatcher allowlistedCustomLdapAttrMatcher) {
Map<String, String> attributes = new HashMap<>();
attributes.put("ldap.original.username", originalUsername);
attributes.put("ldap.dn", userEntry.getDn());
Expand All @@ -69,7 +69,7 @@ public static Map<String, String> extractLdapAttributes(String originalUsername,
// only consider attributes which are not binary and where its value is not
// longer than customAttrMaxValueLen characters
if (val != null && val.length() > 0 && val.length() <= customAttrMaxValueLen) {
if (whitelistedCustomLdapAttrMatcher.test(attr.getName())) {
if (allowlistedCustomLdapAttrMatcher.test(attr.getName())) {
attributes.put("attr.ldap." + attr.getName(), val);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
import org.opensearch.security.user.AuthCredentials;
import org.opensearch.security.user.User;

import static org.opensearch.security.setting.DeprecatedSettings.checkForDeprecatedSetting;

public class LDAPAuthenticationBackend implements AuthenticationBackend {

static final int ZERO_PLACEHOLDER = 0;
Expand All @@ -56,7 +58,7 @@ public class LDAPAuthenticationBackend implements AuthenticationBackend {
private final Path configPath;
private final List<Map.Entry<String, Settings>> userBaseSettings;
private final int customAttrMaxValueLen;
private final WildcardMatcher whitelistedCustomLdapAttrMatcher;
private final WildcardMatcher allowlistedCustomLdapAttrMatcher;

private final String[] returnAttributes;

Expand All @@ -67,8 +69,10 @@ public LDAPAuthenticationBackend(final Settings settings, final Path configPath)
this.returnAttributes = settings.getAsList(ConfigConstants.LDAP_RETURN_ATTRIBUTES, Arrays.asList(ReturnAttributes.ALL.value())).toArray(new String[0]);

customAttrMaxValueLen = settings.getAsInt(ConfigConstants.LDAP_CUSTOM_ATTR_MAXVAL_LEN, 36);
whitelistedCustomLdapAttrMatcher = WildcardMatcher.from(settings.getAsList(ConfigConstants.LDAP_CUSTOM_ATTR_WHITELIST,
checkForDeprecatedSetting(settings, ConfigConstants.LDAP_CUSTOM_ATTR_WHITELIST, ConfigConstants.LDAP_CUSTOM_ATTR_ALLOWLIST);
final List<String> customAttrAllowList = settings.getAsList(ConfigConstants.LDAP_CUSTOM_ATTR_ALLOWLIST, settings.getAsList(ConfigConstants.LDAP_CUSTOM_ATTR_WHITELIST,
Collections.singletonList("*")));
allowlistedCustomLdapAttrMatcher = WildcardMatcher.from(customAttrAllowList);
}

@Override
Expand Down Expand Up @@ -127,9 +131,9 @@ public User authenticate(final AuthCredentials credentials) throws OpenSearchSec

// by default all ldap attributes which are not binary and with a max value
// length of 36 are included in the user object
// if the whitelist contains at least one value then all attributes will be
// additional check if whitelisted (whitelist can contain wildcard and regex)
return new LdapUser(username, user, entry, credentials, customAttrMaxValueLen, whitelistedCustomLdapAttrMatcher);
// if the allowlist contains at least one value then all attributes will be
// additional check if allowlisted (allowlist can contain wildcard and regex)
return new LdapUser(username, user, entry, credentials, customAttrMaxValueLen, allowlistedCustomLdapAttrMatcher);

} catch (final Exception e) {
if (log.isDebugEnabled()) {
Expand Down Expand Up @@ -164,7 +168,7 @@ public boolean exists(final User user) {
boolean exists = userEntry != null;

if(exists) {
user.addAttributes(LdapUser.extractLdapAttributes(userName, userEntry, customAttrMaxValueLen, whitelistedCustomLdapAttrMatcher));
user.addAttributes(LdapUser.extractLdapAttributes(userName, userEntry, customAttrMaxValueLen, allowlistedCustomLdapAttrMatcher));
}

return exists;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public final class ConfigConstants {
// custom attributes
public static final String LDAP_CUSTOM_ATTR_MAXVAL_LEN = "custom_attr_maxval_len";
public static final String LDAP_CUSTOM_ATTR_WHITELIST = "custom_attr_whitelist";
public static final String LDAP_CUSTOM_ATTR_ALLOWLIST = "custom_attr_allowlist";
public static final String LDAP_RETURN_ATTRIBUTES = "custom_return_attributes";

public static final String LDAP_CONNECTION_STRATEGY = "connection_strategy";
Expand All @@ -83,6 +84,9 @@ public final class ConfigConstants {

public static final String LDAP_POOL_TYPE = "pool.type";

public static final String LDAP_LEGACY_POOL_PRUNING_PERIOD = "pruning.period";
public static final String LDAP_LEGACY_POOL_IDLE_TIME = "pruning.idleTime";

public static final String LDAP_POOL_PRUNING_PERIOD = "pool.pruning_period";
public static final String LDAP_POOL_IDLE_TIME = "pool.idle_time";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@

import org.opensearch.common.settings.Settings;

import static org.opensearch.security.setting.DeprecatedSettings.checkForDeprecatedSetting;

public class LDAPConnectionFactoryFactory {

private static final Logger log = LogManager.getLogger(LDAPConnectionFactoryFactory.class);
Expand Down Expand Up @@ -127,10 +129,13 @@ public ConnectionPool createConnectionPool() {
}

result.setValidator(getConnectionValidator());

checkForDeprecatedSetting(settings, ConfigConstants.LDAP_LEGACY_POOL_PRUNING_PERIOD, ConfigConstants.LDAP_POOL_PRUNING_PERIOD);
checkForDeprecatedSetting(settings, ConfigConstants.LDAP_LEGACY_POOL_IDLE_TIME, ConfigConstants.LDAP_POOL_IDLE_TIME);
peternied marked this conversation as resolved.
Show resolved Hide resolved

result.setPruneStrategy(new IdlePruneStrategy(
Duration.ofMinutes(this.settings.getAsLong(ConfigConstants.LDAP_POOL_PRUNING_PERIOD, this.settings.getAsLong("pruning.period", 5l))),
Duration.ofMinutes(this.settings.getAsLong(ConfigConstants.LDAP_POOL_IDLE_TIME, this.settings.getAsLong("pruning.idleTime", 10l))))
Duration.ofMinutes(this.settings.getAsLong(ConfigConstants.LDAP_POOL_PRUNING_PERIOD, this.settings.getAsLong(ConfigConstants.LDAP_LEGACY_POOL_PRUNING_PERIOD, 5l))),
Duration.ofMinutes(this.settings.getAsLong(ConfigConstants.LDAP_POOL_IDLE_TIME, this.settings.getAsLong(ConfigConstants.LDAP_LEGACY_POOL_IDLE_TIME, 10l))))
);

result.initialize();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/
peternied marked this conversation as resolved.
Show resolved Hide resolved

package org.opensearch.security.setting;

import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.settings.Settings;

/**
* Functionality around settings that have been deprecated
*/
public class DeprecatedSettings {

static DeprecationLogger DEPRECATION_LOGGER = DeprecationLogger.getLogger(DeprecatedSettings.class);

/**
* Checks for an deprecated key found in a setting, logs that it should be replaced with the another key
*/
public static void checkForDeprecatedSetting(final Settings settings, final String legacySettingKey, final String validSettingKey) {
if (settings.hasValue(legacySettingKey)) {
DEPRECATION_LOGGER.deprecate(legacySettingKey, "Found deprecated setting '{}', please replace with '{}'", legacySettingKey, validSettingKey);
}
}
peternied marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/
peternied marked this conversation as resolved.
Show resolved Hide resolved

package org.opensearch.security.setting;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.settings.Settings;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.opensearch.security.setting.DeprecatedSettings.checkForDeprecatedSetting;

@RunWith(MockitoJUnitRunner.class)
public class DeprecatedSettingsTest {

@Mock
private DeprecationLogger logger;

private DeprecationLogger original;

@Before
public void before() {
original = DeprecatedSettings.DEPRECATION_LOGGER;
DeprecatedSettings.DEPRECATION_LOGGER = logger;
}

@After
public void after() {
DeprecatedSettings.DEPRECATION_LOGGER = original;
verifyNoMoreInteractions(logger);
}

@Test
public void testCheckForDeprecatedSettingNoLegacy() {
final Settings settings = Settings.builder().put("properKey", "value").build();

checkForDeprecatedSetting(settings, "legacyKey", "properKey");

verifyNoInteractions(logger);
}

@Test
public void testCheckForDeprecatedSettingFoundLegacy() {
final Settings settings = Settings.builder().put("legacyKey", "value").build();

checkForDeprecatedSetting(settings, "legacyKey", "properKey");

verify(logger).deprecate(eq("legacyKey"), anyString(), any());
}
}