[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...

RaiSaurabh
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 <howard.gao@...>
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.

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
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?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
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".


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
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).



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user gaohoward commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1771
 
    that's fine. I'll remove it.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
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. :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...

RaiSaurabh
In reply to this post by RaiSaurabh
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".


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user gaohoward commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1771
 
    @jbertram @clebertsuconic done!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1771: ARTEMIS-1600 Support masked passwords in boots...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user gaohoward commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1771
 
    OK it's done.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1771: ARTEMIS-1600 Support masked passwords i...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/1771


---