[ 
https://issues.apache.org/jira/browse/AMQ-4249?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Tarjei Skorgenes updated AMQ-4249:
----------------------------------

    Description: 
h1. Problem

While setting up a connection pool towards ActiveMq 5.5.1 using Bitronix 2.1.3 
I've been having some issues related to authentication and authorization of the 
JMS connections.

When doing a clean restart of the JMS-clients JVM the connection pool has been 
unable to connect successfully with one or more of the 14 connections it's been 
set up to use.

The error messages I've been getting has usually been one of the following two:

# User system is not authorized to read from: 
ActiveMQ.Advisory.TempQueue,ActiveMQ.Advisory.TempTopic
# User name or password is invalid.

The broker has been set up using a very simplistic configuration:
{code:xml}
<?xml version="1.0" encoding="UTF-8"?>
<beans xmlns="http://www.springframework.org/schema/beans";
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; 
xmlns:context="http://www.springframework.org/schema/context";

        xsi:schemaLocation="http://activemq.apache.org/schema/core 
http://activemq.apache.org/schema/core/activemq-core-5.5.0.xsd
                http://www.springframework.org/schema/beans 
http://www.springframework.org/schema/beans/spring-beans.xsd
                http://www.springframework.org/schema/context 
http://www.springframework.org/schema/context/spring-context.xsd";>
        <context:property-placeholder />

        <broker brokerId="localhost" 
xmlns="http://activemq.apache.org/schema/core";
                dataDirectory="${datadir}" start="false">
                <managementContext>
                        <managementContext connectorHost="0.0.0.0"
                                connectorPort="14522" createConnector="true" />
                </managementContext>
                <plugins>
                        <jaasAuthenticationPlugin 
configuration="activemq-domain"
                                discoverLoginConfig="true" />
                        <authorizationPlugin>
                                <map>
                                        <authorizationMap>
                                                <authorizationEntries>
                                                        <authorizationEntry 
queue=">" read="admins"
                                                                write="admins" 
admin="admins" />

                                                        <authorizationEntry 
topic=">" read="admins"
                                                                write="admins" 
admin="admins" />

                                                        <authorizationEntry 
topic="ActiveMQ.Advisory.>"
                                                                
read="guests,users" write="guests,users" admin="guests,users" />
                                                </authorizationEntries>
                                        </authorizationMap>
                                </map>
                        </authorizationPlugin>
                </plugins>
                <transportConnectors>
                        <transportConnector id="openwire" 
uri="tcp://0.0.0.0:61616?trace=true" />
                </transportConnectors>
        </broker>
</beans>
{code}

The JAAS-configuration has been verified to match username and password used by 
the client when connecting (username = system):

h4. login.config

{code}
activemq-domain {
    org.apache.activemq.jaas.PropertiesLoginModule required
        debug=false
        org.apache.activemq.jaas.properties.user="users.properties"
        org.apache.activemq.jaas.properties.group="groups.properties";
};
{code}

h4.users.properties

{code}
system=manager
user=password
guest=password
{code}

h4. groups.properties
{code}
admins=system
users=user
guests=guest
{code}

h1. Cause

After debugging the problem it seems as if the problem is caused by a race 
condition introduced in PropertiesLoginModule in revision 
[1086219|https://fisheye6.atlassian.com/changelog/activemq?showid=1086219] 
(AMQ-3244). When the reload-support was added the users- and groups-fields were 
changed into static fields. But no additional synchronization was introduced, 
thereby introducing a race condition when several threads are entering the 
initialize- and commit-methods are the same time.

The following section of the initialize-method in PropertiesLoginModule 
contains the first part of the race condition. Please note the unsynchronized 
modification of both the users- and groups static fields:

{code:java}
        if (reload || users == null || uf.lastModified() > usersReloadTime) {
            if (debug) {
                LOG.debug("Reloading users from " + uf.getAbsolutePath());
            }
            try {
                users = new Properties(); // XXX Here be dragons
                java.io.FileInputStream in = new java.io.FileInputStream(uf);
                users.load(in);
                in.close();
                usersReloadTime = System.currentTimeMillis();
            } catch (IOException ioe) {
                LOG.warn("Unable to load user properties file " + uf);
            }
        }

        groupsFile = (String) options.get(GROUP_FILE) + "";
        File gf = baseDir != null ? new File(baseDir, groupsFile) : new 
File(groupsFile);
        if (reload || groups == null || gf.lastModified() > groupsReloadTime) {
            if (debug) {
                LOG.debug("Reloading groups from " + gf.getAbsolutePath());
            }
            try {
                groups = new Properties(); // XXX Here be dragons
                java.io.FileInputStream in = new java.io.FileInputStream(gf);
                groups.load(in);
                in.close();
                groupsReloadTime = System.currentTimeMillis();
            } catch (IOException ioe) {
                LOG.warn("Unable to load group properties file " + gf);
            }
        }
{code}

The next part comes in the login-method when the users password is retrieved:

{code:java}
        String password = users.getProperty(user); 
{code}

The final part of the puzzle occurs in the commit-method:
{code:java}
            for (Enumeration<?> enumeration = groups.keys(); 
enumeration.hasMoreElements();) {
                String name = (String)enumeration.nextElement();
                String[] userList = ((String)groups.getProperty(name) + 
"").split(",");
                for (int i = 0; i < userList.length; i++) {
                    if (user.equals(userList[i])) {
                        principals.add(new GroupPrincipal(name));
                        break;
                    }
                }
            }
{code}

The retrieval of the user password will fail if invoked by a thread immediately 
after a different thread has assigned an empty Properties-object to the users 
field in the initialize-method.

Similarly population of the GroupPrincipals into the Subject in the 
commit-method will silently fail if executed by a thread immediately after a 
different thread has assigned an empty Properties-object to the groups-field in 
the initialize-method.

h1. Proposed solution

I've created a testcase that illustrates the problem and an additional patch 
that introduces a wrapper around the Properties-objects for the users- and 
groups-fields. The wrapper uses a read-write lock to synchronize access and 
modification of the properties-objects containing the users and groups.

The testcase is available via 
https://github.com/lothor/activemq/commit/42f430c74a345729ca099eab2464d562ded62fbe
 and the proposed solution via 
https://github.com/lothor/activemq/commit/263cf14c796e993261351186084b6c3afd939afb.

  was:
h1. Problem

While setting up a connection pool towards ActiveMq 5.5.1 using Bitronix 2.1.3 
I've been having some issues related to authentication and authorization of the 
JMS connections.

When doing a clean restart of the JMS-clients JVM the connection pool has been 
unable to connect successfully with one or more of the 14 connections it's been 
set up to use.

The error messages I've been getting has usually been one of the following two:

# User system is not authorized to read from: 
ActiveMQ.Advisory.TempQueue,ActiveMQ.Advisory.TempTopic
# User name or password is invalid.

The broker has been set up using a very simplistic configuration:
{code:xml}
<?xml version="1.0" encoding="UTF-8"?>
<beans xmlns="http://www.springframework.org/schema/beans";
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; 
xmlns:context="http://www.springframework.org/schema/context";

        xsi:schemaLocation="http://activemq.apache.org/schema/core 
http://activemq.apache.org/schema/core/activemq-core-5.5.0.xsd
                http://www.springframework.org/schema/beans 
http://www.springframework.org/schema/beans/spring-beans.xsd
                http://www.springframework.org/schema/context 
http://www.springframework.org/schema/context/spring-context.xsd";>
        <context:property-placeholder />

        <broker brokerId="localhost" 
xmlns="http://activemq.apache.org/schema/core";
                dataDirectory="${datadir}" start="false">
                <managementContext>
                        <managementContext connectorHost="0.0.0.0"
                                connectorPort="14522" createConnector="true" />
                </managementContext>
                <plugins>
                        <jaasAuthenticationPlugin 
configuration="activemq-domain"
                                discoverLoginConfig="true" />
                        <authorizationPlugin>
                                <map>
                                        <authorizationMap>
                                                <authorizationEntries>
                                                        <authorizationEntry 
queue=">" read="admins"
                                                                write="admins" 
admin="admins" />

                                                        <authorizationEntry 
topic=">" read="admins"
                                                                write="admins" 
admin="admins" />

                                                        <authorizationEntry 
topic="ActiveMQ.Advisory.>"
                                                                
read="guests,users" write="guests,users" admin="guests,users" />
                                                </authorizationEntries>
                                        </authorizationMap>
                                </map>
                        </authorizationPlugin>
                </plugins>
                <transportConnectors>
                        <transportConnector id="openwire" 
uri="tcp://0.0.0.0:61616?trace=true" />
                </transportConnectors>
        </broker>
</beans>
{code}

The JAAS-configuration has been verified to match username and password used by 
the client when connecting (username = system):

h4. login.config

{code}
activemq-domain {
    org.apache.activemq.jaas.PropertiesLoginModule required
        debug=false
        org.apache.activemq.jaas.properties.user="users.properties"
        org.apache.activemq.jaas.properties.group="groups.properties";
};
{code}

h4.users.properties

{code}
system=manager
user=password
guest=password
{code}

h4. groups.properties
{code}
admins=system
users=user
guests=guest
{code}

h1. Cause

After debugging the problem it seems as if the problem is caused by a race 
condition introduced in PropertiesLoginModule in revision 
[1086219|https://fisheye6.atlassian.com/changelog/activemq?showid=1086219] 
(AMQ-3244). When the reload-support was added the users- and groups-fields were 
changed into static fields. But no additional synchronization was introduced, 
thereby introducing a race condition when several threads are entering the 
initialize- and commit-methods are the same time.

The following section of the initialize-method in PropertiesLoginModule 
contains the first part of the race condition. Please note the unsynchronized 
modification of both the users- and groups static fields:

{code:java}
        if (reload || users == null || uf.lastModified() > usersReloadTime) {
            if (debug) {
                LOG.debug("Reloading users from " + uf.getAbsolutePath());
            }
            try {
                users = new Properties(); // XXX Here be dragons
                java.io.FileInputStream in = new java.io.FileInputStream(uf);
                users.load(in);
                in.close();
                usersReloadTime = System.currentTimeMillis();
            } catch (IOException ioe) {
                LOG.warn("Unable to load user properties file " + uf);
            }
        }

        groupsFile = (String) options.get(GROUP_FILE) + "";
        File gf = baseDir != null ? new File(baseDir, groupsFile) : new 
File(groupsFile);
        if (reload || groups == null || gf.lastModified() > groupsReloadTime) {
            if (debug) {
                LOG.debug("Reloading groups from " + gf.getAbsolutePath());
            }
            try {
                groups = new Properties(); // XXX Here be dragons
                java.io.FileInputStream in = new java.io.FileInputStream(gf);
                groups.load(in);
                in.close();
                groupsReloadTime = System.currentTimeMillis();
            } catch (IOException ioe) {
                LOG.warn("Unable to load group properties file " + gf);
            }
        }
{code}

The next part comes in the login-method when the users password is retrieved:

{code:java}
        String password = users.getProperty(user); 
{code}

The final part of the puzzle occurs in the commit-method:
{code:java}
            for (Enumeration<?> enumeration = groups.keys(); 
enumeration.hasMoreElements();) {
                String name = (String)enumeration.nextElement();
                String[] userList = ((String)groups.getProperty(name) + 
"").split(",");
                for (int i = 0; i < userList.length; i++) {
                    if (user.equals(userList[i])) {
                        principals.add(new GroupPrincipal(name));
                        break;
                    }
                }
            }
{code}

The retrieval of the user password will fail if invoked by a thread immediately 
after a different thread has assigned an empty Properties-object to the users 
field in the initialize-method.

Similarly population of the GroupPrincipals into the Subject in the 
commit-method will silently fail if executed by a thread immediately after a 
different thread has assigned an empty Properties-object to the groups-field in 
the initialize-method.

h1. Proposed solution

I've created a testcase that illustrates the problem and an additional patch 
that introduces a wrapper around the Properties-objects for the users- and 
groups-fields. The wrapper uses a read-write lock to synchronize access and 
modification of the properties-objects containing the users and groups.

    
> Race condition in PropertiesLoginModule
> ---------------------------------------
>
>                 Key: AMQ-4249
>                 URL: https://issues.apache.org/jira/browse/AMQ-4249
>             Project: ActiveMQ
>          Issue Type: Bug
>          Components: Broker
>    Affects Versions: 5.5.1, 5.6.0, 5.7.0
>         Environment: ActiveMQ 5.5.1 with JAAS authentication via 
> PropertiesLoginModule configured, Spring JMS 3.0.5, 64-bit, Intel quad core 
> CPU (i7 950), Windows 7
>            Reporter: Tarjei Skorgenes
>            Priority: Minor
>              Labels: Authentication, JAAS, Security
>
> h1. Problem
> While setting up a connection pool towards ActiveMq 5.5.1 using Bitronix 
> 2.1.3 I've been having some issues related to authentication and 
> authorization of the JMS connections.
> When doing a clean restart of the JMS-clients JVM the connection pool has 
> been unable to connect successfully with one or more of the 14 connections 
> it's been set up to use.
> The error messages I've been getting has usually been one of the following 
> two:
> # User system is not authorized to read from: 
> ActiveMQ.Advisory.TempQueue,ActiveMQ.Advisory.TempTopic
> # User name or password is invalid.
> The broker has been set up using a very simplistic configuration:
> {code:xml}
> <?xml version="1.0" encoding="UTF-8"?>
> <beans xmlns="http://www.springframework.org/schema/beans";
>       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; 
> xmlns:context="http://www.springframework.org/schema/context";
>       xsi:schemaLocation="http://activemq.apache.org/schema/core 
> http://activemq.apache.org/schema/core/activemq-core-5.5.0.xsd
>               http://www.springframework.org/schema/beans 
> http://www.springframework.org/schema/beans/spring-beans.xsd
>               http://www.springframework.org/schema/context 
> http://www.springframework.org/schema/context/spring-context.xsd";>
>       <context:property-placeholder />
>       <broker brokerId="localhost" 
> xmlns="http://activemq.apache.org/schema/core";
>               dataDirectory="${datadir}" start="false">
>               <managementContext>
>                       <managementContext connectorHost="0.0.0.0"
>                               connectorPort="14522" createConnector="true" />
>               </managementContext>
>               <plugins>
>                       <jaasAuthenticationPlugin 
> configuration="activemq-domain"
>                               discoverLoginConfig="true" />
>                       <authorizationPlugin>
>                               <map>
>                                       <authorizationMap>
>                                               <authorizationEntries>
>                                                       <authorizationEntry 
> queue=">" read="admins"
>                                                               write="admins" 
> admin="admins" />
>                                                       <authorizationEntry 
> topic=">" read="admins"
>                                                               write="admins" 
> admin="admins" />
>                                                       <authorizationEntry 
> topic="ActiveMQ.Advisory.>"
>                                                               
> read="guests,users" write="guests,users" admin="guests,users" />
>                                               </authorizationEntries>
>                                       </authorizationMap>
>                               </map>
>                       </authorizationPlugin>
>               </plugins>
>               <transportConnectors>
>                       <transportConnector id="openwire" 
> uri="tcp://0.0.0.0:61616?trace=true" />
>               </transportConnectors>
>       </broker>
> </beans>
> {code}
> The JAAS-configuration has been verified to match username and password used 
> by the client when connecting (username = system):
> h4. login.config
> {code}
> activemq-domain {
>     org.apache.activemq.jaas.PropertiesLoginModule required
>         debug=false
>         org.apache.activemq.jaas.properties.user="users.properties"
>         org.apache.activemq.jaas.properties.group="groups.properties";
> };
> {code}
> h4.users.properties
> {code}
> system=manager
> user=password
> guest=password
> {code}
> h4. groups.properties
> {code}
> admins=system
> users=user
> guests=guest
> {code}
> h1. Cause
> After debugging the problem it seems as if the problem is caused by a race 
> condition introduced in PropertiesLoginModule in revision 
> [1086219|https://fisheye6.atlassian.com/changelog/activemq?showid=1086219] 
> (AMQ-3244). When the reload-support was added the users- and groups-fields 
> were changed into static fields. But no additional synchronization was 
> introduced, thereby introducing a race condition when several threads are 
> entering the initialize- and commit-methods are the same time.
> The following section of the initialize-method in PropertiesLoginModule 
> contains the first part of the race condition. Please note the unsynchronized 
> modification of both the users- and groups static fields:
> {code:java}
>         if (reload || users == null || uf.lastModified() > usersReloadTime) {
>             if (debug) {
>                 LOG.debug("Reloading users from " + uf.getAbsolutePath());
>             }
>             try {
>                 users = new Properties(); // XXX Here be dragons
>                 java.io.FileInputStream in = new java.io.FileInputStream(uf);
>                 users.load(in);
>                 in.close();
>                 usersReloadTime = System.currentTimeMillis();
>             } catch (IOException ioe) {
>                 LOG.warn("Unable to load user properties file " + uf);
>             }
>         }
>         groupsFile = (String) options.get(GROUP_FILE) + "";
>         File gf = baseDir != null ? new File(baseDir, groupsFile) : new 
> File(groupsFile);
>         if (reload || groups == null || gf.lastModified() > groupsReloadTime) 
> {
>             if (debug) {
>                 LOG.debug("Reloading groups from " + gf.getAbsolutePath());
>             }
>             try {
>                 groups = new Properties(); // XXX Here be dragons
>                 java.io.FileInputStream in = new java.io.FileInputStream(gf);
>                 groups.load(in);
>                 in.close();
>                 groupsReloadTime = System.currentTimeMillis();
>             } catch (IOException ioe) {
>                 LOG.warn("Unable to load group properties file " + gf);
>             }
>         }
> {code}
> The next part comes in the login-method when the users password is retrieved:
> {code:java}
>         String password = users.getProperty(user); 
> {code}
> The final part of the puzzle occurs in the commit-method:
> {code:java}
>             for (Enumeration<?> enumeration = groups.keys(); 
> enumeration.hasMoreElements();) {
>                 String name = (String)enumeration.nextElement();
>                 String[] userList = ((String)groups.getProperty(name) + 
> "").split(",");
>                 for (int i = 0; i < userList.length; i++) {
>                     if (user.equals(userList[i])) {
>                         principals.add(new GroupPrincipal(name));
>                         break;
>                     }
>                 }
>             }
> {code}
> The retrieval of the user password will fail if invoked by a thread 
> immediately after a different thread has assigned an empty Properties-object 
> to the users field in the initialize-method.
> Similarly population of the GroupPrincipals into the Subject in the 
> commit-method will silently fail if executed by a thread immediately after a 
> different thread has assigned an empty Properties-object to the groups-field 
> in the initialize-method.
> h1. Proposed solution
> I've created a testcase that illustrates the problem and an additional patch 
> that introduces a wrapper around the Properties-objects for the users- and 
> groups-fields. The wrapper uses a read-write lock to synchronize access and 
> modification of the properties-objects containing the users and groups.
> The testcase is available via 
> https://github.com/lothor/activemq/commit/42f430c74a345729ca099eab2464d562ded62fbe
>  and the proposed solution via 
> https://github.com/lothor/activemq/commit/263cf14c796e993261351186084b6c3afd939afb.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to