From db3b7f5233d28745ae706ffe00ff2c0ce0672b93 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 28 Sep 2022 22:42:17 +0000 Subject: [PATCH] Refactor and share use of deprecationn logger Signed-off-by: Peter Nied --- .../jwt/AbstractHTTPJwtAuthenticator.java | 2 + .../com/amazon/dlic/auth/ldap/LdapUser.java | 8 +-- .../backend/LDAPAuthenticationBackend.java | 16 +++-- .../dlic/auth/ldap/util/ConfigConstants.java | 1 + .../ldap2/LDAPConnectionFactoryFactory.java | 17 ++---- .../security/setting/DeprecatedSettings.java | 33 +++++++++++ .../setting/DeprecatedSettingsTest.java | 58 +++++++++++++++++++ 7 files changed, 112 insertions(+), 23 deletions(-) create mode 100644 src/main/java/org/opensearch/security/setting/DeprecatedSettings.java create mode 100644 src/test/java/org/opensearch/security/setting/DeprecatedSettingsTest.java diff --git a/src/main/java/com/amazon/dlic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java index d2e14d6145..2511014f50 100644 --- a/src/main/java/com/amazon/dlic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java @@ -42,6 +42,8 @@ import org.opensearch.security.auth.HTTPAuthenticator; import org.opensearch.security.user.AuthCredentials; +import static org.opensearch.security.setting.DeprecatedSettings.checkForDeprecatedSetting; + public abstract class AbstractHTTPJwtAuthenticator implements HTTPAuthenticator { private final static Logger log = LogManager.getLogger(AbstractHTTPJwtAuthenticator.class); diff --git a/src/main/java/com/amazon/dlic/auth/ldap/LdapUser.java b/src/main/java/com/amazon/dlic/auth/ldap/LdapUser.java index 37f16dd3b5..ab26a94402 100755 --- a/src/main/java/com/amazon/dlic/auth/ldap/LdapUser.java +++ b/src/main/java/com/amazon/dlic/auth/ldap/LdapUser.java @@ -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 attributes = getCustomAttributesMap(); - attributes.putAll(extractLdapAttributes(originalUsername, userEntry, customAttrMaxValueLen, whitelistedCustomLdapAttrMatcher)); + attributes.putAll(extractLdapAttributes(originalUsername, userEntry, customAttrMaxValueLen, allowlistedCustomLdapAttrMatcher)); } /** @@ -57,7 +57,7 @@ public String getOriginalUsername() { } public static Map extractLdapAttributes(String originalUsername, final LdapEntry userEntry, - int customAttrMaxValueLen, WildcardMatcher whitelistedCustomLdapAttrMatcher) { + int customAttrMaxValueLen, WildcardMatcher allowlistedCustomLdapAttrMatcher) { Map attributes = new HashMap<>(); attributes.put("ldap.original.username", originalUsername); attributes.put("ldap.dn", userEntry.getDn()); @@ -69,7 +69,7 @@ public static Map 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); } } diff --git a/src/main/java/com/amazon/dlic/auth/ldap/backend/LDAPAuthenticationBackend.java b/src/main/java/com/amazon/dlic/auth/ldap/backend/LDAPAuthenticationBackend.java index 00eb693e46..d9b723ee07 100755 --- a/src/main/java/com/amazon/dlic/auth/ldap/backend/LDAPAuthenticationBackend.java +++ b/src/main/java/com/amazon/dlic/auth/ldap/backend/LDAPAuthenticationBackend.java @@ -43,6 +43,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; @@ -55,7 +57,7 @@ public class LDAPAuthenticationBackend implements AuthenticationBackend { private final Path configPath; private final List> userBaseSettings; private final int customAttrMaxValueLen; - private final WildcardMatcher whitelistedCustomLdapAttrMatcher; + private final WildcardMatcher allowlistedCustomLdapAttrMatcher; public LDAPAuthenticationBackend(final Settings settings, final Path configPath) { this.settings = settings; @@ -63,8 +65,10 @@ public LDAPAuthenticationBackend(final Settings settings, final Path configPath) this.userBaseSettings = getUserBaseSettings(settings); 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 customAttrAllowList = settings.getAsList(ConfigConstants.LDAP_CUSTOM_ATTR_ALLOWLIST, settings.getAsList(ConfigConstants.LDAP_CUSTOM_ATTR_WHITELIST, Collections.singletonList("*"))); + allowlistedCustomLdapAttrMatcher = WildcardMatcher.from(customAttrAllowList); } @Override @@ -123,9 +127,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()) { @@ -160,7 +164,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; diff --git a/src/main/java/com/amazon/dlic/auth/ldap/util/ConfigConstants.java b/src/main/java/com/amazon/dlic/auth/ldap/util/ConfigConstants.java index 3e6fdd50f0..5cbbded73d 100755 --- a/src/main/java/com/amazon/dlic/auth/ldap/util/ConfigConstants.java +++ b/src/main/java/com/amazon/dlic/auth/ldap/util/ConfigConstants.java @@ -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_CONNECTION_STRATEGY = "connection_strategy"; diff --git a/src/main/java/com/amazon/dlic/auth/ldap2/LDAPConnectionFactoryFactory.java b/src/main/java/com/amazon/dlic/auth/ldap2/LDAPConnectionFactoryFactory.java index b61450359b..7a472237c1 100644 --- a/src/main/java/com/amazon/dlic/auth/ldap2/LDAPConnectionFactoryFactory.java +++ b/src/main/java/com/amazon/dlic/auth/ldap2/LDAPConnectionFactoryFactory.java @@ -60,13 +60,13 @@ import com.amazon.dlic.util.SettingsBasedSSLConfigurator; import com.amazon.dlic.util.SettingsBasedSSLConfigurator.SSLConfigException; -import org.opensearch.common.logging.DeprecationLogger; 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); - private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(LDAPConnectionFactoryFactory.class); private final Settings settings; @@ -131,8 +131,8 @@ public ConnectionPool createConnectionPool() { result.setValidator(getConnectionValidator()); - LDAPConnectionFactoryFactory.checkForDeprecatedSetting(settings, ConfigConstants.LDAP_LEGACY_POOL_PRUNING_PERIOD, ConfigConstants.LDAP_POOL_PRUNING_PERIOD); - LDAPConnectionFactoryFactory.checkForDeprecatedSetting(settings, ConfigConstants.LDAP_LEGACY_POOL_IDLE_TIME, ConfigConstants.LDAP_POOL_IDLE_TIME); + 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); result.setPruneStrategy(new IdlePruneStrategy( Duration.ofMinutes(this.settings.getAsLong(ConfigConstants.LDAP_POOL_PRUNING_PERIOD, this.settings.getAsLong(ConfigConstants.LDAP_LEGACY_POOL_PRUNING_PERIOD, 5l))), @@ -144,15 +144,6 @@ public ConnectionPool createConnectionPool() { return result; } - /** - * Checks for an deprecated key found in a setting, logs that it should be replaced with the another key - */ - private static void checkForDeprecatedSetting(final Settings settings, final String legacySettingKey, final String validSettingKey) { - if (settings.hasValue(legacySettingKey)) { - deprecationLogger.deprecate(legacySettingKey, "Found deprecated setting '{}', please replace with '{}'", legacySettingKey, validSettingKey); - } - } - private ConnectionConfig getConnectionConfig() { ConnectionConfig result = new ConnectionConfig(getLdapUrlString()); diff --git a/src/main/java/org/opensearch/security/setting/DeprecatedSettings.java b/src/main/java/org/opensearch/security/setting/DeprecatedSettings.java new file mode 100644 index 0000000000..22121faf73 --- /dev/null +++ b/src/main/java/org/opensearch/security/setting/DeprecatedSettings.java @@ -0,0 +1,33 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ +package org.opensearch.security.setting; + +import com.google.common.annotations.VisibleForTesting; + +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); + } + } +} diff --git a/src/test/java/org/opensearch/security/setting/DeprecatedSettingsTest.java b/src/test/java/org/opensearch/security/setting/DeprecatedSettingsTest.java new file mode 100644 index 0000000000..a83b825488 --- /dev/null +++ b/src/test/java/org/opensearch/security/setting/DeprecatedSettingsTest.java @@ -0,0 +1,58 @@ +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()); + } +}