[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3851: ARTEMIS-3573 Support PropertiesLoginModule custom password codecs

[GitBox <gi...@apache.org>]

gemmellr commented on a change in pull request #3851:
URL: https://github.com/apache/activemq-artemis/pull/3851#discussion_r749250265



##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/security/MD5SensitiveDataCodec.java
##########
@@ -0,0 +1,38 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.integration.security;
+
+import org.apache.activemq.artemis.utils.SensitiveDataCodec;
+import org.apache.commons.codec.digest.DigestUtils;
+
+public class MD5SensitiveDataCodec implements SensitiveDataCodec<String> {

Review comment:
       I know its only a test impl, but at this point this seems like we should do better than MD5, the MD5SensitiveDataCodec name alone is a bit of an oxymoron at this stage, and it seems like the digest utils being used has far better available.

##########
File path: docs/user-manual/en/masking-passwords.md
##########
@@ -257,7 +257,26 @@ codec other than the default one. For example
 With this configuration, both passwords in ra.xml and all of its MDBs will have
 to be in masked form.
 
-### login.config
+### PropertiesLoginModule
+Artemis supports Properties login module to be configured in JAAS configuration file
+(default name is `login.config`). By default, the passwords of the users are in plain text
+or masked with the DefaultSensitiveStringCodec.
+
+Set the `org.apache.activemq.jaas.properties.password.codec` property to set a custom codec,
+i.e. to use the `org.apache.activemq.artemis.tests.integration.security.MD5SensitiveDataCodec` codec

Review comment:
       ```suggestion
   To use a custom codec class, set the `org.apache.activemq.jaas.properties.password.codec` property to the class name
   e.g. to use the `com.example.MySensitiveDataCodecImpl` codec class:
   ```
   Related to other comment, it definitely seems like we shouldnt be adding docs pointing to a class using MD5. Both since there seems little need using a real class (which is only part of the tests) at all, and since inevitably someone might actually go find it and just use what its doing.
   
   Might also be worth linking to the section on custom codecs, since it wasnt covered yet and comes later.

##########
File path: docs/user-manual/en/masking-passwords.md
##########
@@ -462,6 +485,12 @@ public class MyCodec implements SensitiveDataCodec<String> {
    public void init(Map<String, String> params) {
       // Initialization done here. It is called right after the codec has been created.
    }
+
+   @Override
+   public boolean compare(Object value, Object encodedValue) {
+      // Return true if the value matches the encodedValue.
+      return true;

Review comment:
       ```suggestion
         // Return true if the value matches the encodedValue.
         return checkValueMatchesEncoding(value, encodedValue);
   ```
   
   Just setting return true seems like a bad example, this at least spells it out.

##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/DefaultSensitiveStringCodec.java
##########
@@ -79,6 +79,21 @@ public void init(Map<String, String> params) throws Exception {
       }
    }
 
+   @Override
+   public boolean compare(Object value, Object encodedValue) {
+      if (encodedValue instanceof String == false) {
+         throw new IllegalArgumentException("Not supported encodedValue type: " + encodedValue.getClass().getName());
+      }

Review comment:
       doing '== boolean' in an if is pretty ugly. Seems like either using !(encodedValue instanceof String), or changing the if to wrap the overall comparison and throw at the end would be nicer.
   
   This could also NPE generating the exception, on encodedValue.getClass().
   
   "Unsupported" would seem a better fit rather than "Not supported".

##########
File path: docs/user-manual/en/masking-passwords.md
##########
@@ -257,7 +257,26 @@ codec other than the default one. For example
 With this configuration, both passwords in ra.xml and all of its MDBs will have
 to be in masked form.
 
-### login.config
+### PropertiesLoginModule
+Artemis supports Properties login module to be configured in JAAS configuration file
+(default name is `login.config`). By default, the passwords of the users are in plain text
+or masked with the DefaultSensitiveStringCodec.
+
+Set the `org.apache.activemq.jaas.properties.password.codec` property to set a custom codec,
+i.e. to use the `org.apache.activemq.artemis.tests.integration.security.MD5SensitiveDataCodec` codec
+
+```
+PropertiesLoginWithPasswordCodec {
+    org.apache.activemq.artemis.spi.core.security.jaas.PropertiesLoginModule required
+        debug=true
+        org.apache.activemq.jaas.properties.user="users.properties"
+        org.apache.activemq.jaas.properties.role="roles.properties"
+        org.apache.activemq.jaas.properties.password.codec="org.apache.activemq.artemis.tests.integration.security.MD5SensitiveDataCodec";

Review comment:
       ```suggestion
           org.apache.activemq.jaas.properties.password.codec="com.example.MySensitiveDataCodecImpl";
   ```

##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/DefaultSensitiveStringCodec.java
##########
@@ -79,6 +79,21 @@ public void init(Map<String, String> params) throws Exception {
       }
    }
 
+   @Override
+   public boolean compare(Object value, Object encodedValue) {
+      if (encodedValue instanceof String == false) {
+         throw new IllegalArgumentException("Not supported encodedValue type: " + encodedValue.getClass().getName());
+      }
+
+      if (value instanceof String) {
+         return verify(((String)value).toCharArray(), (String)encodedValue);
+      } else if (value instanceof char[]) {
+         return verify((char[])value, (String)encodedValue);
+      } else {
+         throw new IllegalArgumentException("Not supported value type: " + value.getClass().getName());

Review comment:
       Could NPE on the value.getClass()




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org