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

Wrong with role-based authorization when using right permission

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 5.10.0, 5.10.2, 5.11.0, 5.11.1
    • Fix Version/s: 5.11.2, 5.12.0
    • Component/s: Broker, MQTT
    • Labels:
      None
    • Environment:

      Unix, Windows

    • Regression:
      Unit Test Broken

      Description

      Since version 5.10, the problem has been caused by broker. It was behaving wrong with role-based authorization. I have configured right SimpleAuthorization plugin but no luck.

      I guess it has problem from AMQ-5160

      Here my test cases on github, please review and let me know if you have any questions: https://github.com/hongphu8790/activemq/tree/master/mqtt-authorization-test

      Project test-case descriptions:

      • Using debug mode for broker to view detail
        problem.
      • Repeat with pom.xml file
        + With AMQ 5.9.1 (it will pass all test cases)
        + With AMQ >= 5.10.0 (it will pass only test cases with using publisher have super permission

      Here my log occurred when the test case failed:

      Log debug mode - activemq.log
      2015-05-29 10:30:24,746 | DEBUG | AbstractRegion                 | Subscription denied for TopicSubscription: consumer=ID:XXXXXX-50828-1432870224218-2:2:-1:1, destinations=0, dispatched=0, delivered=0, matched=0, discarded=0 to destination topic://dcu.id: User publisher is not authorized to read from: topic://dcu.id
      

        Issue Links

          Activity

          Hide
          tabish121 Timothy Bish added a comment -

          Have you tried against a 5.12-SNAPSHOT, there have been some fixes in this area I think.

          Show
          tabish121 Timothy Bish added a comment - Have you tried against a 5.12-SNAPSHOT, there have been some fixes in this area I think.
          Hide
          hongphu8790 PhuNH5-VTICT added a comment -

          Hi Timothy Bish,

          I was tested again with 5.12-snapshot, and no luck. Please view my attachment: activemq-snapshot-5.12-20150529.223833.txt

          Thanks!

          Show
          hongphu8790 PhuNH5-VTICT added a comment - Hi Timothy Bish, I was tested again with 5.12-snapshot, and no luck. Please view my attachment: activemq-snapshot-5.12-20150529.223833.txt Thanks!
          Hide
          hongphu8790 PhuNH5-VTICT added a comment -

          Hi Timothy Bish,

          This was a big problem for my production. I couldn't use AMQ version greater than version 5.9.1.

          Thanks!

          Show
          hongphu8790 PhuNH5-VTICT added a comment - Hi Timothy Bish, This was a big problem for my production. I couldn't use AMQ version greater than version 5.9.1. Thanks!
          Hide
          hongphu8790 PhuNH5-VTICT added a comment -

          Hi Timothy Bish,

          Please help me to reproduce this error, we couldn't use AMQ version greater than 5.9.1 with SimpleAuthorization support.

          Thanks!

          Show
          hongphu8790 PhuNH5-VTICT added a comment - Hi Timothy Bish , Please help me to reproduce this error, we couldn't use AMQ version greater than 5.9.1 with SimpleAuthorization support. Thanks!
          Hide
          tabish121 Timothy Bish added a comment -

          I wrote a little test that doesn't requite MQTT to reproduce, the difference from previous behaviour seems to have come from the changes made for AMQ-5160. Some further investigation is needed.

          Show
          tabish121 Timothy Bish added a comment - I wrote a little test that doesn't requite MQTT to reproduce, the difference from previous behaviour seems to have come from the changes made for AMQ-5160 . Some further investigation is needed.
          Hide
          tabish121 Timothy Bish added a comment -

          Changes here seem to have introduced this behavior

          Show
          tabish121 Timothy Bish added a comment - Changes here seem to have introduced this behavior
          Hide
          cshannon Christopher L. Shannon added a comment - - edited

          I looked at this a little yesterday. When the producer sends a message it causes a new destination to get created because the subscriber is listening on a wildcard destination. The issue seems to be that during destination creation, the ConnectionContext of the producer is used to create the destination, instead of the consumer. (Happens in addSubscriptionsForDestination method of TopicRegion). The end result is the security check fails in the addSubscription method of AuthorizationDestinationFilter because it uses the producer's ACLs instead of the consumer's ACLs to determine if the subscription is allowed.

          Show
          cshannon Christopher L. Shannon added a comment - - edited I looked at this a little yesterday. When the producer sends a message it causes a new destination to get created because the subscriber is listening on a wildcard destination. The issue seems to be that during destination creation, the ConnectionContext of the producer is used to create the destination, instead of the consumer. (Happens in addSubscriptionsForDestination method of TopicRegion). The end result is the security check fails in the addSubscription method of AuthorizationDestinationFilter because it uses the producer's ACLs instead of the consumer's ACLs to determine if the subscription is allowed.
          Hide
          hongphu8790 PhuNH5-VTICT added a comment -

          This is a big bug, i think so. Could you tell me how to bypass or fix this problem. We have waited a long time since first release AMQ 5.9.1.

          Thanks!

          Show
          hongphu8790 PhuNH5-VTICT added a comment - This is a big bug, i think so. Could you tell me how to bypass or fix this problem. We have waited a long time since first release AMQ 5.9.1. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user cshannon opened a pull request:

          https://github.com/apache/activemq/pull/125

          https://issues.apache.org/jira/browse/AMQ-5814

          This fixes an issue where a producer's connection context was being
          used to create a subscription instead of the consumer's context
          which could lead to permission check failures.

          Thanks to hongphu8790@gmail.com for providing the test case.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/cshannon/activemq AMQ-5814

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/activemq/pull/125.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 #125


          commit ef552fcc8bb14eea33170019aca2850e3c03467d
          Author: Christopher L. Shannon (cshannon) <christopher.l.shannon@gmail.com>
          Date: 2015-07-02T12:33:16Z

          https://issues.apache.org/jira/browse/AMQ-5814

          This fixes an issue where a producer's connection context was being
          used to create a subscription instead of the consumer's context
          which could lead to permission check failures.

          Thanks to hongphu8790@gmail.com for providing the test case.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user cshannon opened a pull request: https://github.com/apache/activemq/pull/125 https://issues.apache.org/jira/browse/AMQ-5814 This fixes an issue where a producer's connection context was being used to create a subscription instead of the consumer's context which could lead to permission check failures. Thanks to hongphu8790@gmail.com for providing the test case. You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq AMQ-5814 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq/pull/125.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 #125 commit ef552fcc8bb14eea33170019aca2850e3c03467d Author: Christopher L. Shannon (cshannon) <christopher.l.shannon@gmail.com> Date: 2015-07-02T12:33:16Z https://issues.apache.org/jira/browse/AMQ-5814 This fixes an issue where a producer's connection context was being used to create a subscription instead of the consumer's context which could lead to permission check failures. Thanks to hongphu8790@gmail.com for providing the test case.
          Hide
          cshannon Christopher L. Shannon added a comment -

          PhuNH5-VTICT,

          I have pushed up a pull request to fix this issue and included your test case in the patch to make sure that the issue stays fixed.

          Timothy Bish,

          Can you take a look and make sure it looks ok? It seemed like a very simple fix but I just want to make sure what I changed wouldn't have any unintended consequences. I ran through some of the tests relating to AMQ-5160 and they all still seemed to pass. I'm running through all the tests right now but that will take a while to finish.

          Show
          cshannon Christopher L. Shannon added a comment - PhuNH5-VTICT , I have pushed up a pull request to fix this issue and included your test case in the patch to make sure that the issue stays fixed. Timothy Bish , Can you take a look and make sure it looks ok? It seemed like a very simple fix but I just want to make sure what I changed wouldn't have any unintended consequences. I ran through some of the tests relating to AMQ-5160 and they all still seemed to pass. I'm running through all the tests right now but that will take a while to finish.
          Hide
          tabish121 Timothy Bish added a comment -

          I was testing a similar fix last night, guess I should have assigned this one to myself to avoid duplication of effort, oops. So I made basically the same change although I tried to ensure that a valid context was always passed along by checking that the context in the sub was non-null and using the pass in context if none was supplied in the subscription.

                  ConnectionContext originalContext = sub.getContext() != null ? sub.getContext() : context;
          

          From a run of the tests with this change I didn't see any new failures so I don't think there are any hidden surprises. If you want to update your PR and do some additional sanity checks we can just push that.

          I also created a simpler OpenWire only test since this actually affects any client connection which I will show below:

          /**
           * 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
           *
           *      http://www.apache.org/licenses/LICENSE-2.0
           *
           * 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.bugs;
          
          import static org.junit.Assert.assertNotNull;
          
          import java.net.URI;
          import java.util.ArrayList;
          import java.util.List;
          
          import javax.jms.Connection;
          import javax.jms.ConnectionFactory;
          import javax.jms.MessageConsumer;
          import javax.jms.MessageProducer;
          import javax.jms.Session;
          import javax.jms.Topic;
          
          import org.apache.activemq.ActiveMQConnectionFactory;
          import org.apache.activemq.broker.BrokerPlugin;
          import org.apache.activemq.broker.BrokerService;
          import org.apache.activemq.broker.TransportConnector;
          import org.apache.activemq.filter.DestinationMapEntry;
          import org.apache.activemq.security.AuthenticationUser;
          import org.apache.activemq.security.AuthorizationEntry;
          import org.apache.activemq.security.AuthorizationPlugin;
          import org.apache.activemq.security.DefaultAuthorizationMap;
          import org.apache.activemq.security.SimpleAuthenticationPlugin;
          import org.junit.After;
          import org.junit.Before;
          import org.junit.Test;
          
          public class AMQ5814Test {
          
              private BrokerService brokerService;
              private String openwireClientUrl;
          
              public BrokerPlugin configureAuthentication() throws Exception {
                  List<AuthenticationUser> users = new ArrayList<>();
                  users.add(new AuthenticationUser("publisher", "123", "publisher"));
                  users.add(new AuthenticationUser("subscriber", "123", "subscriber"));
                  users.add(new AuthenticationUser("admin", "123", "publisher,subscriber"));
          
                  SimpleAuthenticationPlugin authenticationPlugin = new SimpleAuthenticationPlugin(users);
          
                  return authenticationPlugin;
              }
          
              public BrokerPlugin configureAuthorization() throws Exception {
          
                  @SuppressWarnings("rawtypes")
                  List<DestinationMapEntry> authorizationEntries = new ArrayList<>();
          
                  AuthorizationEntry entry = new AuthorizationEntry();
                  entry.setTopic("dcu.>");
                  entry.setRead("subscriber");
                  entry.setWrite("publisher");
                  entry.setAdmin("publisher,subscriber");
                  authorizationEntries.add(entry);
          
                  entry = new AuthorizationEntry();
                  entry.setTopic("ActiveMQ.Advisory.>");
                  entry.setRead("publisher,subscriber");
                  entry.setWrite("publisher,subscriber");
                  entry.setAdmin("publisher,subscriber");
                  authorizationEntries.add(entry);
          
                  DefaultAuthorizationMap authorizationMap = new DefaultAuthorizationMap(authorizationEntries);
                  AuthorizationPlugin authorizationPlugin = new AuthorizationPlugin(authorizationMap);
          
                  return authorizationPlugin;
              }
          
              @Before
              public void setup() throws Exception {
          
                  TransportConnector openwireConnector = new TransportConnector();
                  openwireConnector.setUri(new URI("tcp://localhost:0"));
                  openwireConnector.setName("openwire");
          
                  ArrayList<BrokerPlugin> plugins = new ArrayList<>();
                  plugins.add(configureAuthentication());
                  plugins.add(configureAuthorization());
          
                  brokerService = new BrokerService();
                  brokerService.setPersistent(false);
                  brokerService.addConnector(openwireConnector);
                  if (!plugins.isEmpty()) {
                      BrokerPlugin[] array = new BrokerPlugin[plugins.size()];
                      brokerService.setPlugins(plugins.toArray(array));
                  }
                  brokerService.start();
                  brokerService.waitUntilStarted();
          
                  openwireClientUrl = openwireConnector.getPublishableConnectString();
              }
          
              @After
              public void shutdown() throws Exception {
                  if (brokerService != null) {
                      brokerService.stop();
                      brokerService.waitUntilStopped();
                      brokerService = null;
                  }
              }
          
              @Test(timeout=30000)
              public void testProduceConsumeWithAuthorization() throws Exception {
                  ConnectionFactory factory = new ActiveMQConnectionFactory(openwireClientUrl);
                  Connection connection1 = factory.createConnection("subscriber", "123");
                  Session session1 = connection1.createSession(false, Session.AUTO_ACKNOWLEDGE);
                  Topic wildCarded = session1.createTopic("dcu.>");
                  MessageConsumer consumer = session1.createConsumer(wildCarded);
                  connection1.start();
          
                  Connection connection2 = factory.createConnection("publisher", "123");
                  Session session2 = connection2.createSession(false, Session.AUTO_ACKNOWLEDGE);
                  Topic named = session2.createTopic("dcu.id");
                  MessageProducer producer = session2.createProducer(named);
                  producer.send(session2.createTextMessage("test"));
          
                  assertNotNull(consumer.receive(2000));
          
                  connection1.close();
                  connection2.close();
              }
          }
          
          Show
          tabish121 Timothy Bish added a comment - I was testing a similar fix last night, guess I should have assigned this one to myself to avoid duplication of effort, oops. So I made basically the same change although I tried to ensure that a valid context was always passed along by checking that the context in the sub was non-null and using the pass in context if none was supplied in the subscription. ConnectionContext originalContext = sub.getContext() != null ? sub.getContext() : context; From a run of the tests with this change I didn't see any new failures so I don't think there are any hidden surprises. If you want to update your PR and do some additional sanity checks we can just push that. I also created a simpler OpenWire only test since this actually affects any client connection which I will show below: /** * 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 * * http: //www.apache.org/licenses/LICENSE-2.0 * * 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.bugs; import static org.junit.Assert.assertNotNull; import java.net.URI; import java.util.ArrayList; import java.util.List; import javax.jms.Connection; import javax.jms.ConnectionFactory; import javax.jms.MessageConsumer; import javax.jms.MessageProducer; import javax.jms.Session; import javax.jms.Topic; import org.apache.activemq.ActiveMQConnectionFactory; import org.apache.activemq.broker.BrokerPlugin; import org.apache.activemq.broker.BrokerService; import org.apache.activemq.broker.TransportConnector; import org.apache.activemq.filter.DestinationMapEntry; import org.apache.activemq.security.AuthenticationUser; import org.apache.activemq.security.AuthorizationEntry; import org.apache.activemq.security.AuthorizationPlugin; import org.apache.activemq.security.DefaultAuthorizationMap; import org.apache.activemq.security.SimpleAuthenticationPlugin; import org.junit.After; import org.junit.Before; import org.junit.Test; public class AMQ5814Test { private BrokerService brokerService; private String openwireClientUrl; public BrokerPlugin configureAuthentication() throws Exception { List<AuthenticationUser> users = new ArrayList<>(); users.add( new AuthenticationUser( "publisher" , "123" , "publisher" )); users.add( new AuthenticationUser( "subscriber" , "123" , "subscriber" )); users.add( new AuthenticationUser( "admin" , "123" , "publisher,subscriber" )); SimpleAuthenticationPlugin authenticationPlugin = new SimpleAuthenticationPlugin(users); return authenticationPlugin; } public BrokerPlugin configureAuthorization() throws Exception { @SuppressWarnings( "rawtypes" ) List<DestinationMapEntry> authorizationEntries = new ArrayList<>(); AuthorizationEntry entry = new AuthorizationEntry(); entry.setTopic( "dcu.>" ); entry.setRead( "subscriber" ); entry.setWrite( "publisher" ); entry.setAdmin( "publisher,subscriber" ); authorizationEntries.add(entry); entry = new AuthorizationEntry(); entry.setTopic( "ActiveMQ.Advisory.>" ); entry.setRead( "publisher,subscriber" ); entry.setWrite( "publisher,subscriber" ); entry.setAdmin( "publisher,subscriber" ); authorizationEntries.add(entry); DefaultAuthorizationMap authorizationMap = new DefaultAuthorizationMap(authorizationEntries); AuthorizationPlugin authorizationPlugin = new AuthorizationPlugin(authorizationMap); return authorizationPlugin; } @Before public void setup() throws Exception { TransportConnector openwireConnector = new TransportConnector(); openwireConnector.setUri( new URI( "tcp: //localhost:0" )); openwireConnector.setName( "openwire" ); ArrayList<BrokerPlugin> plugins = new ArrayList<>(); plugins.add(configureAuthentication()); plugins.add(configureAuthorization()); brokerService = new BrokerService(); brokerService.setPersistent( false ); brokerService.addConnector(openwireConnector); if (!plugins.isEmpty()) { BrokerPlugin[] array = new BrokerPlugin[plugins.size()]; brokerService.setPlugins(plugins.toArray(array)); } brokerService.start(); brokerService.waitUntilStarted(); openwireClientUrl = openwireConnector.getPublishableConnectString(); } @After public void shutdown() throws Exception { if (brokerService != null ) { brokerService.stop(); brokerService.waitUntilStopped(); brokerService = null ; } } @Test(timeout=30000) public void testProduceConsumeWithAuthorization() throws Exception { ConnectionFactory factory = new ActiveMQConnectionFactory(openwireClientUrl); Connection connection1 = factory.createConnection( "subscriber" , "123" ); Session session1 = connection1.createSession( false , Session.AUTO_ACKNOWLEDGE); Topic wildCarded = session1.createTopic( "dcu.>" ); MessageConsumer consumer = session1.createConsumer(wildCarded); connection1.start(); Connection connection2 = factory.createConnection( "publisher" , "123" ); Session session2 = connection2.createSession( false , Session.AUTO_ACKNOWLEDGE); Topic named = session2.createTopic( "dcu.id" ); MessageProducer producer = session2.createProducer(named); producer.send(session2.createTextMessage( "test" )); assertNotNull(consumer.receive(2000)); connection1.close(); connection2.close(); } }
          Hide
          cshannon Christopher L. Shannon added a comment -

          Not a problem, I only spent a few minutes on this as it was only a one line fix. Since you created the OpenWire test I'll let you go ahead and push up the fix and I'll close my PR.

          Show
          cshannon Christopher L. Shannon added a comment - Not a problem, I only spent a few minutes on this as it was only a one line fix. Since you created the OpenWire test I'll let you go ahead and push up the fix and I'll close my PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user cshannon closed the pull request at:

          https://github.com/apache/activemq/pull/125

          Show
          githubbot ASF GitHub Bot added a comment - Github user cshannon closed the pull request at: https://github.com/apache/activemq/pull/125
          Hide
          tabish121 Timothy Bish added a comment -

          Fix and test added on master

          Show
          tabish121 Timothy Bish added a comment - Fix and test added on master
          Hide
          hongphu8790 PhuNH5-VTICT added a comment -

          Hey Timothy Bish, [~christopher.l.shannon],

          It's working fine now. When is it released or a nightly build version? I'm looking forward to it.
          Thanks!

          Show
          hongphu8790 PhuNH5-VTICT added a comment - Hey Timothy Bish , [~christopher.l.shannon] , It's working fine now. When is it released or a nightly build version? I'm looking forward to it. Thanks!

            People

            • Assignee:
              tabish121 Timothy Bish
              Reporter:
              hongphu8790 PhuNH5-VTICT
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development