From a15626193c8d3c099b0bcf1e605b892b533a4d3e Mon Sep 17 00:00:00 2001 From: gtully Date: Fri, 4 Aug 2017 13:46:16 +0100 Subject: [PATCH] [AMQ-6787] release securty context on failure to addConnection subsequent to auth, resolve leak. fix and test --- .../security/JaasAuthenticationBroker.java | 18 +++- .../apache/activemq/broker/StubBroker.java | 6 ++ .../JaasAuthenticationBrokerTest.java | 97 +++++++++++++++++++ 3 files changed, 116 insertions(+), 5 deletions(-) create mode 100644 activemq-unit-tests/src/test/java/org/apache/activemq/security/JaasAuthenticationBrokerTest.java diff --git a/activemq-broker/src/main/java/org/apache/activemq/security/JaasAuthenticationBroker.java b/activemq-broker/src/main/java/org/apache/activemq/security/JaasAuthenticationBroker.java index 148c2e2b76..6756027361 100644 --- a/activemq-broker/src/main/java/org/apache/activemq/security/JaasAuthenticationBroker.java +++ b/activemq-broker/src/main/java/org/apache/activemq/security/JaasAuthenticationBroker.java @@ -63,16 +63,24 @@ public class JaasAuthenticationBroker extends AbstractAuthenticationBroker { // Set the TCCL since it seems JAAS needs it to find the login module classes. ClassLoader original = Thread.currentThread().getContextClassLoader(); Thread.currentThread().setContextClassLoader(JaasAuthenticationBroker.class.getClassLoader()); - + SecurityContext securityContext = null; try { - SecurityContext s = authenticate(info.getUserName(), info.getPassword(), null); - context.setSecurityContext(s); - securityContexts.add(s); + securityContext = authenticate(info.getUserName(), info.getPassword(), null); + context.setSecurityContext(securityContext); + securityContexts.add(securityContext); + super.addConnection(context, info); + } catch (Exception error) { + if (securityContext != null) { + securityContexts.remove(securityContext); + } + context.setSecurityContext(null); + throw error; } finally { Thread.currentThread().setContextClassLoader(original); } + } else { + super.addConnection(context, info); } - super.addConnection(context, info); } @Override diff --git a/activemq-unit-tests/src/test/java/org/apache/activemq/broker/StubBroker.java b/activemq-unit-tests/src/test/java/org/apache/activemq/broker/StubBroker.java index 7b4fa1bbc2..af42eb0a52 100644 --- a/activemq-unit-tests/src/test/java/org/apache/activemq/broker/StubBroker.java +++ b/activemq-unit-tests/src/test/java/org/apache/activemq/broker/StubBroker.java @@ -17,6 +17,7 @@ package org.apache.activemq.broker; +import javax.jms.InvalidClientIDException; import java.util.LinkedList; import org.apache.activemq.command.ConnectionInfo; @@ -47,6 +48,11 @@ public class StubBroker extends EmptyBroker { } public void addConnection(ConnectionContext context, ConnectionInfo info) throws Exception { + for (AddConnectionData data : addConnectionData) { + if (data.connectionInfo.getClientId() != null && data.connectionInfo.getClientId().equals(info.getClientId())) { + throw new InvalidClientIDException("ClientID already exists"); + } + } addConnectionData.add(new AddConnectionData(context, info)); } diff --git a/activemq-unit-tests/src/test/java/org/apache/activemq/security/JaasAuthenticationBrokerTest.java b/activemq-unit-tests/src/test/java/org/apache/activemq/security/JaasAuthenticationBrokerTest.java new file mode 100644 index 0000000000..75f567e698 --- /dev/null +++ b/activemq-unit-tests/src/test/java/org/apache/activemq/security/JaasAuthenticationBrokerTest.java @@ -0,0 +1,97 @@ +/** + * 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.security; + +import javax.jms.InvalidClientIDException; +import javax.security.auth.login.AppConfigurationEntry; +import javax.security.auth.login.Configuration; +import java.util.HashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +import junit.framework.TestCase; + +import org.apache.activemq.broker.Broker; +import org.apache.activemq.broker.ConnectionContext; +import org.apache.activemq.broker.StubBroker; +import org.apache.activemq.command.ConnectionInfo; + +public class JaasAuthenticationBrokerTest extends TestCase { + StubBroker receiveBroker; + + JaasAuthenticationBroker authBroker; + + ConnectionContext connectionContext; + ConnectionInfo connectionInfo; + + CopyOnWriteArrayList visibleSecurityContexts; + + class JaasAuthenticationBrokerTester extends JaasAuthenticationBroker { + public JaasAuthenticationBrokerTester(Broker next, String jassConfiguration) { + super(next, jassConfiguration); + visibleSecurityContexts = securityContexts; + } + } + + @Override + protected void setUp() throws Exception { + receiveBroker = new StubBroker(); + + authBroker = new JaasAuthenticationBrokerTester(receiveBroker, ""); + + connectionContext = new ConnectionContext(); + connectionInfo = new ConnectionInfo(); + } + + @Override + protected void tearDown() throws Exception { + super.tearDown(); + } + + private void setConfiguration(boolean loginShouldSucceed) { + HashMap configOptions = new HashMap(); + + configOptions.put(StubLoginModule.ALLOW_LOGIN_PROPERTY, loginShouldSucceed ? "true" : "false"); + configOptions.put(StubLoginModule.USERS_PROPERTY, ""); + configOptions.put(StubLoginModule.GROUPS_PROPERTY, ""); + AppConfigurationEntry configEntry = new AppConfigurationEntry("org.apache.activemq.security.StubLoginModule", AppConfigurationEntry.LoginModuleControlFlag.REQUIRED, configOptions); + + StubJaasConfiguration jaasConfig = new StubJaasConfiguration(configEntry); + + Configuration.setConfiguration(jaasConfig); + } + + + public void testAddConnectionFailureOnDuplicateClientId() throws Exception { + setConfiguration(true); + + connectionInfo.setClientId("CliIdX"); + authBroker.addConnection(connectionContext, connectionInfo); + ConnectionContext secondContext = connectionContext.copy(); + secondContext.setSecurityContext(null); + ConnectionInfo secondInfo = connectionInfo.copy(); + try { + authBroker.addConnection(secondContext, secondInfo); + fail("Expect duplicate id"); + } catch (InvalidClientIDException expected) { + } + + assertEquals("one connection allowed.", 1, receiveBroker.addConnectionData.size()); + assertEquals("one context .", 1, visibleSecurityContexts.size()); + + } +}