You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gaohoward <gi...@git.apache.org> on 2018/01/11 05:05:51 UTC
[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...
GitHub user gaohoward opened a pull request:
https://github.com/apache/activemq-artemis/pull/1771
ARTEMIS-1600 Support masked passwords in bootstrap.xm and login.config
We provide a feature to mask passwords in the configuration files.
However, passwords in the bootstrap.xml (when the console is
secured with HTTPS) cannot be masked. This enhancement has
been opened to allow passwords in the bootstrap.xml to be masked
using the built-in masking feature provided by the broker.
Also the LDAPLoginModule configuration (in login.config) has a
connection password attribute that also needs this mask support.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaohoward/activemq-artemis fent908
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1771.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1771
----
commit f9f6d907bd9aa23dc4ab93ee209deb80d9a72749
Author: Howard Gao <ho...@...>
Date: 2018-01-11T04:07:31Z
ARTEMIS-1600 Support masked passwords in bootstrap.xm and login.config
We provide a feature to mask passwords in the configuration files.
However, passwords in the bootstrap.xml (when the console is
secured with HTTPS) cannot be masked. This enhancement has
been opened to allow passwords in the bootstrap.xml to be masked
using the built-in masking feature provided by the broker.
Also the LDAPLoginModule configuration (in login.config) has a
connection password attribute that also needs this mask support.
----
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
well, almost done. Just found a unused var. I'll delete it right away. Sorry about that. :)
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
We should keep mask-password config support where it exists already, but don't add any new features that use it. Instead we can rely on the ENC() syntax.
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
Aside from the failing tests this looks OK.
That said, I would love to see us move all our password masking to use the "ENC(<hash>)" syntax instead of using boolean "mask-password" attributes everywhere if possible. This is done for the properties login module and it's very clean. If the text follows the pattern then it should be treated as masked otherwise it shouldn't.
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
@jbertram @clebertsuconic done!
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
OK it's done.
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
@jbertram Hi Justin, I think it's done. Can you take a look again?
Thanks
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
@jbertram Please hold for a moment. I just found a change that may be wrong.
---
[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1771#discussion_r162146958
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/config/ActiveMQDefaultConfiguration.java ---
@@ -622,8 +622,8 @@ public static String getDefaultClusterPassword() {
/**
* This option controls whether passwords in server configuration need be masked. If set to "true" the passwords are masked.
*/
- public static boolean isDefaultMaskPassword() {
- return DEFAULT_MASK_PASSWORD;
+ public static Boolean isDefaultMaskPassword() {
--- End diff --
@clebertsuconic, check out org.apache.activemq.artemis.utils.PasswordMaskingUtil.resolveMask. I believe the way that method works is why he needs 3 values.
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
In my opinion, you should remove the mask-password config property from this PR as it will require more code/documentation changes later when it's deprecated and eventually removed.
---
[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1771
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
OK, so I'd add ENC() syntax and keeps the mask-password as an option. (I mean I won't remove this from this PR, but won't add any more in the future if there is new password mask requirements).
---
[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1771#discussion_r162233948
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/config/ActiveMQDefaultConfiguration.java ---
@@ -622,8 +622,8 @@ public static String getDefaultClusterPassword() {
/**
* This option controls whether passwords in server configuration need be masked. If set to "true" the passwords are masked.
*/
- public static boolean isDefaultMaskPassword() {
- return DEFAULT_MASK_PASSWORD;
+ public static Boolean isDefaultMaskPassword() {
--- End diff --
@clebertsuconic yes that's the purpose as @jbertram pointed out. I need 'null' to represent the case where the mask-password is not specified at all.
---
[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1771#discussion_r162151931
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/LDAPLoginModule.java ---
@@ -105,12 +110,39 @@ public void initialize(Subject subject,
this.subject = subject;
this.handler = callbackHandler;
- config = new LDAPLoginProperty[]{new LDAPLoginProperty(INITIAL_CONTEXT_FACTORY, (String) options.get(INITIAL_CONTEXT_FACTORY)), new LDAPLoginProperty(CONNECTION_URL, (String) options.get(CONNECTION_URL)), new LDAPLoginProperty(CONNECTION_USERNAME, (String) options.get(CONNECTION_USERNAME)), new LDAPLoginProperty(CONNECTION_PASSWORD, (String) options.get(CONNECTION_PASSWORD)), new LDAPLoginProperty(CONNECTION_PROTOCOL, (String) options.get(CONNECTION_PROTOCOL)), new LDAPLoginProperty(AUTHENTICATION, (String) options.get(AUTHENTICATION)), new LDAPLoginProperty(USER_BASE, (String) options.get(USER_BASE)), new LDAPLoginProperty(USER_SEARCH_MATCHING, (String) options.get(USER_SEARCH_MATCHING)), new LDAPLoginProperty(USER_SEARCH_SUBTREE, (String) options.get(USER_SEARCH_SUBTREE)), new LDAPLoginProperty(ROLE_BASE, (String) options.get(ROLE_BASE)), new LDAPLoginProperty(ROLE_NAME, (String) options.get(ROLE_NAME)), new LDAPLoginProperty(ROLE_SEARCH_MATCHING, (String) options.get(RO
LE_SEARCH_MATCHING)), new LDAPLoginProperty(ROLE_SEARCH_SUBTREE, (String) options.get(ROLE_SEARCH_SUBTREE)), new LDAPLoginProperty(USER_ROLE_NAME, (String) options.get(USER_ROLE_NAME)), new LDAPLoginProperty(EXPAND_ROLES, (String) options.get(EXPAND_ROLES)), new LDAPLoginProperty(EXPAND_ROLES_MATCHING, (String) options.get(EXPAND_ROLES_MATCHING)), new LDAPLoginProperty(REFERRAL, (String) options.get(REFERRAL))};
+ config = new LDAPLoginProperty[]{new LDAPLoginProperty(INITIAL_CONTEXT_FACTORY, (String) options.get(INITIAL_CONTEXT_FACTORY)),
+ new LDAPLoginProperty(CONNECTION_URL, (String) options.get(CONNECTION_URL)),
+ new LDAPLoginProperty(CONNECTION_USERNAME, (String) options.get(CONNECTION_USERNAME)),
+ new LDAPLoginProperty(CONNECTION_PASSWORD, (String) options.get(CONNECTION_PASSWORD)),
+ new LDAPLoginProperty(CONNECTION_PROTOCOL, (String) options.get(CONNECTION_PROTOCOL)),
+ new LDAPLoginProperty(AUTHENTICATION, (String) options.get(AUTHENTICATION)),
+ new LDAPLoginProperty(USER_BASE, (String) options.get(USER_BASE)),
+ new LDAPLoginProperty(USER_SEARCH_MATCHING, (String) options.get(USER_SEARCH_MATCHING)),
+ new LDAPLoginProperty(USER_SEARCH_SUBTREE, (String) options.get(USER_SEARCH_SUBTREE)),
+ new LDAPLoginProperty(ROLE_BASE, (String) options.get(ROLE_BASE)),
+ new LDAPLoginProperty(ROLE_NAME, (String) options.get(ROLE_NAME)),
+ new LDAPLoginProperty(ROLE_SEARCH_MATCHING, (String) options.get(ROLE_SEARCH_MATCHING)),
+ new LDAPLoginProperty(ROLE_SEARCH_SUBTREE, (String) options.get(ROLE_SEARCH_SUBTREE)),
+ new LDAPLoginProperty(USER_ROLE_NAME, (String) options.get(USER_ROLE_NAME)),
+ new LDAPLoginProperty(EXPAND_ROLES, (String) options.get(EXPAND_ROLES)),
+ new LDAPLoginProperty(EXPAND_ROLES_MATCHING, (String) options.get(EXPAND_ROLES_MATCHING)),
+ new LDAPLoginProperty(REFERRAL, (String) options.get(REFERRAL))};
+
if (isLoginPropertySet(AUTHENTICATE_USER)) {
authenticateUser = Boolean.valueOf(getLDAPPropertyValue(AUTHENTICATE_USER));
}
isRoleAttributeSet = isLoginPropertySet(ROLE_NAME);
roleAttributeName = getLDAPPropertyValue(ROLE_NAME);
+ String isMask = (String) options.get(MASK_PASSWORD);
--- End diff --
This isn't used anywhere so it can be removed.
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
@jbertram ok I think we can support ENC() as well as "mask-password".
---
[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1771#discussion_r162122543
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/config/ActiveMQDefaultConfiguration.java ---
@@ -622,8 +622,8 @@ public static String getDefaultClusterPassword() {
/**
* This option controls whether passwords in server configuration need be masked. If set to "true" the passwords are masked.
*/
- public static boolean isDefaultMaskPassword() {
- return DEFAULT_MASK_PASSWORD;
+ public static Boolean isDefaultMaskPassword() {
--- End diff --
Why this?
Why you need a third state? true / false / undefined?
Why not just keep it boolean.. either mask it or not...
Besides.. if you really need the third option.. I would make DEFAULT_MASK_PASSWORD = null; instead of returning null here.
I couldn't understand why you need it.
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
@jbertram good point. I think I can do that. Do you think we still need to support "mask-password" for backward compatibility?
---
[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1771#discussion_r162153980
--- Diff: docs/user-manual/en/masking-passwords.md ---
@@ -18,12 +18,33 @@ Apache ActiveMQ Artemis provides a default password encoder and decoder. Optiona
users can use or implement their own encoder and decoder for masking the
passwords.
+In general, a masked password can be identified using one of two ways. The first one
+iS the ENC() syntax, i.e. any string value wrapped in ENC() is to be treated as
--- End diff --
Capitalization error on "iS".
---
[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1771
that's fine. I'll remove it.
---