Uploaded image for project: 'ActiveMQ'
  1. ActiveMQ
  2. AMQ-4249

Race condition in PropertiesLoginModule

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 5.5.1, 5.6.0, 5.7.0, 5.8.0
    • Fix Version/s: 5.8.0
    • Component/s: Broker
    • 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

    • Patch Info:
      Patch Available

      Description

      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:

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

      The broker has been set up using a very simplistic configuration:

      <?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>
      

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

      login.config

      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";
      };
      

      users.properties

      system=manager
      user=password
      guest=password
      

      groups.properties

      admins=system
      users=user
      guests=guest
      

      Cause

      After debugging the problem it seems as if the problem is caused by a race condition introduced in PropertiesLoginModule in revision 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:

              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);
                  }
              }
      

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

              String password = users.getProperty(user); 
      

      The final part of the puzzle occurs in the commit-method:

                  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;
                          }
                      }
                  }
      

      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.

      Proposed solution

      I've created a testcase that reproduces the problem and an additional patch that introduces a wrapper around the Properties-objects for the users- and groups-fields.

      The testcase and the proposed solution is available via https://github.com/lothor/activemq.

        Attachments

        1. AMQ-4249-testcase.diff
          7 kB
          Tarjei Skorgenes
        2. AMQ-4249-proposed-fix.diff
          7 kB
          Tarjei Skorgenes

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              lothor Tarjei Skorgenes
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: