ARTEMIS-5101 Deprecate default codec two-way algorithm

Throughout the years, the standard mechanism for storing passwords has evolved.
In the beginning, passwords were stored in plaintext. Developers are now
encouraged to leverage adaptive one-way functions to store a password. Using a
two-way function by default for storing passwords without a warning could lead
users to a false sense of security.

artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Mask.java

@@ -64,8 +64,7 @@ public class Mask extends ActionAbstract {
          Configuration brokerConfiguration = getBrokerConfiguration();
          codec = PasswordMaskingUtil.getCodec(brokerConfiguration.getPasswordCodec());
       } else {
-         codec = PasswordMaskingUtil.getDefaultCodec();
+         codec = PasswordMaskingUtil.getDefaultCodec(params);
-         codec.init(params);
       }
 
       String masked = codec.encode(password);

artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/log4j2-utility.properties

@@ -18,7 +18,7 @@
 # Monitor config file every X seconds for updates
 monitorInterval = 5
 
-rootLogger.level = ERROR
+rootLogger.level = WARN
 rootLogger.appenderRef.console.ref = console
 
 # Console appender

artemis-commons/src/main/java/org/apache/activemq/artemis/logs/ActiveMQUtilLogger.java

@@ -83,4 +83,7 @@ public interface ActiveMQUtilLogger {
 
    @LogMessage(id = 202016, value = "Could not list files to clean up in {}", level = LogMessage.Level.WARN)
    void failedListFilesToCleanup(String path);
+
+   @LogMessage(id = 202017, value = "Algorithm two-way is deprecated and will be removed from the default codec in a future version. Use a custom codec instead. Consult the manual for details.", level = LogMessage.Level.WARN)
+   void deprecatedDefaultCodecTwoWayAlgorithm();
 }

artemis-commons/src/main/java/org/apache/activemq/artemis/utils/DefaultSensitiveStringCodec.java

@@ -26,13 +26,13 @@ import java.security.NoSuchAlgorithmException;
 import java.security.SecureRandom;
 import java.security.spec.InvalidKeySpecException;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Properties;
 
+import org.apache.activemq.artemis.logs.ActiveMQUtilLogger;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import java.lang.invoke.MethodHandles;
@@ -59,15 +59,23 @@ public class DefaultSensitiveStringCodec implements SensitiveDataCodec<String> {
    public static final String TWO_WAY = "two-way";
    public static final String KEY_SYSTEM_PROPERTY = "artemis.default.sensitive.string.codec.key";
 
-   private CodecAlgorithm algorithm = new BlowfishAlgorithm(Collections.EMPTY_MAP);
+   private CodecAlgorithm algorithm;
 
    @Override
    public String decode(Object secret) throws Exception {
+      if (algorithm == null) {
+         throw new IllegalStateException("Algorithm not initialized");
+      }
+
       return algorithm.decode((String) secret);
    }
 
    @Override
    public String encode(Object secret) throws Exception {
+      if (algorithm == null) {
+         throw new IllegalStateException("Algorithm not initialized");
+      }
+
       return algorithm.encode((String) secret);
    }
 
@@ -76,6 +84,7 @@ public class DefaultSensitiveStringCodec implements SensitiveDataCodec<String> {
       String algorithm = params.get(ALGORITHM);
       if (algorithm == null || algorithm.equals(TWO_WAY)) {
          //two way
+         ActiveMQUtilLogger.LOGGER.deprecatedDefaultCodecTwoWayAlgorithm();
          this.algorithm = new BlowfishAlgorithm(params);
       } else if (algorithm.equals(ONE_WAY)) {
          this.algorithm = new PBKDF2Algorithm(params);
@@ -131,6 +140,7 @@ public class DefaultSensitiveStringCodec implements SensitiveDataCodec<String> {
       }
    }
 
+   @Deprecated
    private class BlowfishAlgorithm extends CodecAlgorithm {
 
       private byte[] internalKey = "clusterpassword".getBytes();

artemis-commons/src/main/java/org/apache/activemq/artemis/utils/PasswordMaskingUtil.java

@@ -18,6 +18,7 @@ package org.apache.activemq.artemis.utils;
 
 import java.security.AccessController;
 import java.security.PrivilegedAction;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.ServiceLoader;
@@ -203,27 +204,36 @@ public final class PasswordMaskingUtil {
          }
       });
 
-      if (parts.length > 1) {
       Map<String, String> props = new HashMap<>();
-
+      if (parts.length > 1) {
          for (int i = 1; i < parts.length; i++) {
             String[] keyVal = parts[i].split("=");
             if (keyVal.length != 2)
                throw ActiveMQUtilBundle.BUNDLE.invalidProperty(parts[i]);
             props.put(keyVal[0], keyVal[1]);
          }
+      }
       try {
          codecInstance.init(props);
       } catch (Exception e) {
          throw new ActiveMQException("Fail to init codec", e, ActiveMQExceptionType.SECURITY_EXCEPTION);
       }
-      }
 
       return codecInstance;
    }
 
    public static DefaultSensitiveStringCodec getDefaultCodec() {
-      return new DefaultSensitiveStringCodec();
+      return getDefaultCodec(Collections.emptyMap());
+   }
+
+   public static DefaultSensitiveStringCodec getDefaultCodec(Map<String, String> params) {
+      DefaultSensitiveStringCodec defaultCodec = new DefaultSensitiveStringCodec();
+      try {
+         defaultCodec.init(params);
+      } catch (Exception e) {
+         throw ActiveMQUtilBundle.BUNDLE.errorCreatingCodec(DefaultSensitiveStringCodec.class.getName(), e);
+      }
+      return defaultCodec;
    }
 
 }

artemis-commons/src/test/java/org/apache/activemq/artemis/utils/DefaultSensitiveStringCodecTest.java

@@ -22,11 +22,13 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
+import org.apache.activemq.artemis.logs.AssertionLoggerHandler;
 import org.junit.jupiter.api.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import java.lang.invoke.MethodHandles;
 
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -35,29 +37,53 @@ public class DefaultSensitiveStringCodecTest {
    private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
    @Test
-   public void testDefaultAlgorithm() throws Exception {
+   public void testDefaultCodec() {
       SensitiveDataCodec<String> codec = PasswordMaskingUtil.getDefaultCodec();
       assertTrue(codec instanceof DefaultSensitiveStringCodec);
    }
 
    @Test
-   public void testOnewayAlgorithm() throws Exception {
+   public void testDefaultAlgorithm() throws Exception {
-      testAlgorithm(DefaultSensitiveStringCodec.ONE_WAY);
+      testAlgorithm(Collections.emptyMap());
+
+      testAlgorithm(Map.of(DefaultSensitiveStringCodec.BLOWFISH_KEY, "my-key"));
+
+      System.setProperty(DefaultSensitiveStringCodec.KEY_SYSTEM_PROPERTY, "my-key");
+      try {
+         testAlgorithm(Collections.emptyMap());
+      } finally {
+         System.clearProperty(DefaultSensitiveStringCodec.KEY_SYSTEM_PROPERTY);
+      }
    }
 
    @Test
-   public void testTwowayAlgorithm() throws Exception {
+   public void testOneWayAlgorithm() throws Exception {
-      testAlgorithm(DefaultSensitiveStringCodec.TWO_WAY);
+      testAlgorithm(Map.of(DefaultSensitiveStringCodec.ALGORITHM, DefaultSensitiveStringCodec.ONE_WAY));
    }
 
-   private void testAlgorithm(String algorithm) throws Exception {
+   @Test
-      DefaultSensitiveStringCodec codec = getDefaultSensitiveStringCodec(algorithm);
+   public void testTwoWayAlgorithm() throws Exception {
+      try (AssertionLoggerHandler loggerHandler = new AssertionLoggerHandler()) {
+         testAlgorithm(Map.of(DefaultSensitiveStringCodec.ALGORITHM,
+            DefaultSensitiveStringCodec.TWO_WAY));
+         assertTrue(loggerHandler.findText("AMQ202017"));
+      }
+
+      try (AssertionLoggerHandler loggerHandler = new AssertionLoggerHandler()) {
+         testAlgorithm(Map.of(DefaultSensitiveStringCodec.ALGORITHM, DefaultSensitiveStringCodec.TWO_WAY,
+            DefaultSensitiveStringCodec.BLOWFISH_KEY, "my-key"));
+         assertTrue(loggerHandler.findText("AMQ202017"));
+      }
+   }
+
+   private void testAlgorithm(Map<String, String> params) throws Exception {
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec(params);
 
       String plainText = "some_password";
       String maskedText = codec.encode(plainText);
       logger.debug("encoded value: {}", maskedText);
 
-      if (algorithm.equals(DefaultSensitiveStringCodec.ONE_WAY)) {
+      if (DefaultSensitiveStringCodec.ONE_WAY.equals(params.get(DefaultSensitiveStringCodec.ALGORITHM))) {
          //one way can't decode
          try {
             codec.decode(maskedText);
@@ -92,21 +118,23 @@ public class DefaultSensitiveStringCodecTest {
       Map<String, String> params = new HashMap<>();
       codecFromEnvVarConfig.init(params);
       String blaVersion = codecFromEnvVarConfig.encode(someString);
-      assertNotEquals(blaVersion,  getDefaultSensitiveStringCodec(DefaultSensitiveStringCodec.TWO_WAY).encode(someString));
+      Map<String, String> twoWayParams = Map.of(DefaultSensitiveStringCodec.ALGORITHM, DefaultSensitiveStringCodec.TWO_WAY);
+      assertNotEquals(blaVersion,  PasswordMaskingUtil.getDefaultCodec(twoWayParams).encode(someString));
    }
 
    @Test
-   public void testCompareWithOnewayAlgorithm() throws Exception {
+   public void testCompareWithOneWayAlgorithm() throws Exception {
-      testCompareWithAlgorithm(DefaultSensitiveStringCodec.ONE_WAY);
+      testCompareWithAlgorithm(Map.of(DefaultSensitiveStringCodec.ALGORITHM, DefaultSensitiveStringCodec.ONE_WAY));
    }
 
    @Test
-   public void testCompareWithTwowayAlgorithm() throws Exception {
+   public void testCompareWithTwoWayAlgorithm() throws Exception {
-      testCompareWithAlgorithm(DefaultSensitiveStringCodec.TWO_WAY);
+      testCompareWithAlgorithm(Map.of(DefaultSensitiveStringCodec.ALGORITHM, DefaultSensitiveStringCodec.TWO_WAY));
    }
 
-   private void testCompareWithAlgorithm(String algorithm) throws Exception {
+
-      DefaultSensitiveStringCodec codec = getDefaultSensitiveStringCodec(algorithm);
+   private void testCompareWithAlgorithm(Map<String, String> params) throws Exception {
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec(params);
 
       String plainText = "some_password";
       String maskedText = codec.encode(plainText);
@@ -117,13 +145,4 @@ public class DefaultSensitiveStringCodecTest {
       String otherPassword = "some_other_password";
       assertFalse(codec.verify(otherPassword.toCharArray(), maskedText));
    }
-
-   private DefaultSensitiveStringCodec getDefaultSensitiveStringCodec(String algorithm) throws Exception {
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
-      Map<String, String> params = new HashMap<>();
-      params.put(DefaultSensitiveStringCodec.ALGORITHM, algorithm);
-      codec.init(params);
-
-      return codec;
-   }
 }

artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/FileConfigurationParserTest.java

@@ -211,7 +211,7 @@ public class FileConfigurationParserTest extends ServerTestBase {
       assertEquals("helloworld", config.getClusterPassword());
 
       //if we add mask, it should be able to decode correctly
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
       String mask = codec.encode("helloworld");
 
       String maskPasswordPart = "<mask-password>true</mask-password>";
@@ -266,7 +266,7 @@ public class FileConfigurationParserTest extends ServerTestBase {
       assertEquals("helloworld", config.getClusterPassword());
 
       //if we add mask, it should be able to decode correctly
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
       String mask = codec.encode("helloworld");
 
       clusterPasswordPart = "<cluster-password>" + PasswordMaskingUtil.wrap(mask) + "</cluster-password>";

docs/user-manual/masking-passwords.adoc

@@ -43,7 +43,10 @@ Newer configurations may not support it.
 To mask a password use the `mask` command from the `bin` directory of your Artemis _instance_.
 This command will not work from the Artemis home.
 
-The `mask` command uses <<the-default-codec,the default codec>> unless <<using-a-custom-codec,a custom codec>> is defined in `broker.xml` and the `--password-codec` option is `true`.
+By default, the `mask` command uses the legacy two-way algorithm of <<the-default-codec,the default codec>>
+unless <<using-a-custom-codec,a custom codec>> is defined in `broker.xml` and the `--password-codec` option is `true`.
+The legacy `two-way` algorithm is now *deprecated* and exists only to maintain backward-compatibility,
+use <<using-a-custom-codec,a custom codec>> instead.
 Here's a simple example:
 
 [,sh]
@@ -104,7 +107,7 @@ For example:
 ./artemis user add --user-command-user guest --user-command-password guest --role admin
 ----
 
-This will use the default codec to perform a "one-way" hash of the password and alter both the `artemis-users.properties` and `artemis-roles.properties` files with the specified values.
+This will use the default codec to perform a `one-way` hash of the password and alter both the `artemis-users.properties` and `artemis-roles.properties` files with the specified values.
 
 Passwords in `artemis-users.properties` are automatically detected as hashed or not by looking for the syntax `ENC(<hash>)`.
 The `mask-password` parameter does not need to be `true` to use hashed passwords here.
@@ -366,13 +369,16 @@ However, a user can implement their own if they wish.
 
 Whenever no codec is specified in the configuration, the default codec is used.
 The class name for the default codec is `org.apache.activemq.artemis.utils.DefaultSensitiveStringCodec`.
-It has hashing, encoding, and decoding capabilities.
+It provides 2 algorithms: `one-way` and `two-way`.
-It uses `java.crypto.Cipher` utilities to hash or encode a plaintext password and also to decode a masked string using the same algorithm and "key."
+The `one-way` algorithm can hash a string and is the default algorithm used by <<propertiesloginmodule,PropertiesLoginModule>>.
-
+The `two-way` algorithm can encode/decode a string by using a `key`.
-The "key" used here is important since the _same_ key *must* be used to both mask and unmask the password.
+The `two-way` algorithm has a default key in `org.apache.activemq.artemis.utils.DefaultSensitiveStringCodec`,
-The key is just a string of characters which the codec feeds to the underlying algorithm.
+but using the default key leaves open the possibility that nefarious actors could also use that key to unmask the password(s).
-There is a default key in `org.apache.activemq.artemis.utils.DefaultSensitiveStringCodec`, but using the default key leaves open the possibility that nefarious actors could also use that key to unmask the password(s).
+It is now *deprecated* and exists only to maintain backward-compatibility,
-Therefore, it is possible to supply your _own_ key, and there are a few ways to do this.
+use <<using-a-custom-codec,a custom codec>> instead.
+The `key` used here is important since the _same_ key *must* be used to both mask and unmask the password.
+It is just a string of characters which the codec feeds to the underlying algorithm.
+There are a few ways to to supply your _own_ key:
 
 . Specify the key in the codec configuration using the `key=value` syntax.
 Depending on which password you're trying to mask the configuration specifics will differ slightly, but this can be done, for example, in `broker.xml` with `<password-codec>`:

docs/user-manual/versions.adoc

@@ -12,6 +12,14 @@ NOTE: If the upgrade spans multiple versions then the steps from *each* version
 
 NOTE: Follow the general upgrade procedure outlined in the xref:upgrading.adoc#upgrading-the-broker[Upgrading the Broker]  chapter in addition to any version-specific upgrade instructions outlined here.
 
+== 2.38.0
+
+https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12315920&version=12355013[Full release notes]
+
+=== Highlights
+
+* The `two-way` algorithm of the default codec is deprecated, use a custom codec instead.
+
 == 2.37.0
 
 https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12315920&version=12354977[Full release notes]

tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/ra/ResourceAdapterTest.java

@@ -694,7 +694,7 @@ public class ResourceAdapterTest extends ActiveMQRATestBase {
       qResourceAdapter.setConnectorClassName(INVM_CONNECTOR_FACTORY);
       ActiveMQRATestBase.MyBootstrapContext ctx = new ActiveMQRATestBase.MyBootstrapContext();
 
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
       String mask = codec.encode("helloworld");
 
       qResourceAdapter.setUseMaskedPassword(true);
@@ -730,7 +730,7 @@ public class ResourceAdapterTest extends ActiveMQRATestBase {
       qResourceAdapter.setConnectorClassName(INVM_CONNECTOR_FACTORY);
       ActiveMQRATestBase.MyBootstrapContext ctx = new ActiveMQRATestBase.MyBootstrapContext();
 
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
       String mask = codec.encode("helloworld");
 
       qResourceAdapter.setPassword(PasswordMaskingUtil.wrap(mask));

tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/ssl/CoreClientOverOneWaySSLTest.java

@@ -355,8 +355,6 @@ public class CoreClientOverOneWaySSLTest extends ActiveMQTestBase {
       String text = RandomUtil.randomString();
 
       DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
-      Map<String, String> params = new HashMap<>();
-      codec.init(params);
 
       String masked = codec.encode(PASSWORD);
       String url = "tcp://127.0.0.1:61616?sslEnabled=true;trustStorePath=" + CLIENT_SIDE_TRUSTSTORE + ";trustStorePassword=" + masked + ";activemq.usemaskedpassword=true";
@@ -389,8 +387,6 @@ public class CoreClientOverOneWaySSLTest extends ActiveMQTestBase {
       String text = RandomUtil.randomString();
 
       DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
-      Map<String, String> params = new HashMap<>();
-      codec.init(params);
 
       String masked = codec.encode(PASSWORD);
 

tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/remoting/impl/netty/NettyConnectorTest.java

@@ -179,7 +179,7 @@ public class NettyConnectorTest extends ActiveMQTestBase {
       BufferHandler handler = (connectionID, buffer) -> {
       };
 
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
 
       System.setProperty(NettyConnector.JAVAX_KEYSTORE_PATH_PROP_NAME, "client-keystore.jks");
       System.setProperty(NettyConnector.JAVAX_KEYSTORE_PASSWORD_PROP_NAME, PasswordMaskingUtil.wrap(codec.encode("securepass")));
@@ -209,7 +209,7 @@ public class NettyConnectorTest extends ActiveMQTestBase {
       BufferHandler handler = (connectionID, buffer) -> {
       };
 
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
 
       System.setProperty(NettyConnector.JAVAX_KEYSTORE_PATH_PROP_NAME, "client-keystore.jks");
       System.setProperty(NettyConnector.JAVAX_KEYSTORE_PASSWORD_PROP_NAME, PasswordMaskingUtil.wrap(codec.encode("bad password")));
@@ -386,7 +386,7 @@ public class NettyConnectorTest extends ActiveMQTestBase {
 
       NettyConnector connector = new NettyConnector(params, handler, listener, executorService, Executors.newCachedThreadPool(ActiveMQThreadFactory.defaultThreadFactory(getClass().getName())), Executors.newScheduledThreadPool(5, ActiveMQThreadFactory.defaultThreadFactory(getClass().getName())));
 
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
 
       System.setProperty(NettyConnector.ACTIVEMQ_KEYSTORE_PATH_PROP_NAME, "client-keystore.jks");
       System.setProperty(NettyConnector.ACTIVEMQ_KEYSTORE_PASSWORD_PROP_NAME, PasswordMaskingUtil.wrap(codec.encode("securepass")));
@@ -410,7 +410,7 @@ public class NettyConnectorTest extends ActiveMQTestBase {
 
       NettyConnector connector = new NettyConnector(params, handler, listener, executorService, Executors.newCachedThreadPool(ActiveMQThreadFactory.defaultThreadFactory(getClass().getName())), Executors.newScheduledThreadPool(5, ActiveMQThreadFactory.defaultThreadFactory(getClass().getName())));
 
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
 
       System.setProperty(NettyConnector.ACTIVEMQ_KEYSTORE_PATH_PROP_NAME, "client-keystore.jks");
       System.setProperty(NettyConnector.ACTIVEMQ_KEYSTORE_PASSWORD_PROP_NAME, PasswordMaskingUtil.wrap(codec.encode("bad password")));
@@ -503,7 +503,7 @@ public class NettyConnectorTest extends ActiveMQTestBase {
 
       NettyConnector connector = new NettyConnector(params, handler, listener, executorService, Executors.newCachedThreadPool(ActiveMQThreadFactory.defaultThreadFactory(getClass().getName())), Executors.newScheduledThreadPool(5, ActiveMQThreadFactory.defaultThreadFactory(getClass().getName())));
 
-      DefaultSensitiveStringCodec codec = new DefaultSensitiveStringCodec();
+      DefaultSensitiveStringCodec codec = PasswordMaskingUtil.getDefaultCodec();
 
       System.setProperty(NettyConnector.ACTIVEMQ_SSL_PASSWORD_CODEC_CLASS_PROP_NAME, NettyConnectorTestPasswordCodec.class.getName());
       System.setProperty(NettyConnector.ACTIVEMQ_KEYSTORE_PATH_PROP_NAME, "client-keystore.jks");