From 51c95babfeb8245b8d87d3cbbdfa73ae0bdf04e0 Mon Sep 17 00:00:00 2001 From: Timothy Bish Date: Wed, 21 Dec 2016 18:18:54 -0500 Subject: [PATCH] ARTEMIS-901 Account for the presence of authzid in sasl plan auth If the SASL plain mechanism arrives with the authzid value set the mechanism needs to account for its presence and use the correct fields of the exchange to get the username and password values. Adds some tests to validate this fix. --- .../protocol/amqp/sasl/ServerSASLPlain.java | 23 ++++--- .../transport/amqp/client/AmqpConnection.java | 30 ++++++--- .../integration/amqp/AmqpSecurityTest.java | 61 +++++++++++++++++-- 3 files changed, 86 insertions(+), 28 deletions(-) diff --git a/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/ServerSASLPlain.java b/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/ServerSASLPlain.java index da26d2efc6..177334ccb4 100644 --- a/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/ServerSASLPlain.java +++ b/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/ServerSASLPlain.java @@ -27,23 +27,22 @@ public class ServerSASLPlain implements ServerSASL { @Override public SASLResult processSASL(byte[] data) { - String username = null; String password = null; String bytes = new String(data); String[] credentials = bytes.split(Character.toString((char) 0)); - int offSet = 0; - if (credentials.length > 0) { - if (credentials[0].length() == 0) { - offSet = 1; - } - if (credentials.length >= offSet) { - username = credentials[offSet]; - } - if (credentials.length >= (offSet + 1)) { - password = credentials[offSet + 1]; - } + switch (credentials.length) { + case 2: + username = credentials[0]; + password = credentials[1]; + break; + case 3: + username = credentials[1]; + password = credentials[2]; + break; + default: + break; } boolean success = authenticate(username, password); diff --git a/tests/artemis-test-support/src/main/java/org/apache/activemq/transport/amqp/client/AmqpConnection.java b/tests/artemis-test-support/src/main/java/org/apache/activemq/transport/amqp/client/AmqpConnection.java index 723daef49c..fa44c02304 100644 --- a/tests/artemis-test-support/src/main/java/org/apache/activemq/transport/amqp/client/AmqpConnection.java +++ b/tests/artemis-test-support/src/main/java/org/apache/activemq/transport/amqp/client/AmqpConnection.java @@ -16,6 +16,8 @@ */ package org.apache.activemq.transport.amqp.client; +import static org.apache.activemq.transport.amqp.AmqpSupport.CONNECTION_OPEN_FAILED; + import java.io.IOException; import java.net.URI; import java.nio.ByteBuffer; @@ -30,9 +32,6 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; -import io.netty.buffer.ByteBuf; -import io.netty.buffer.Unpooled; -import io.netty.util.ReferenceCountUtil; import org.apache.activemq.transport.InactivityIOException; import org.apache.activemq.transport.amqp.client.sasl.SaslAuthenticator; import org.apache.activemq.transport.amqp.client.transport.NettyTransportListener; @@ -54,7 +53,9 @@ import org.apache.qpid.proton.engine.impl.TransportImpl; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.apache.activemq.transport.amqp.AmqpSupport.CONNECTION_OPEN_FAILED; +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; +import io.netty.util.ReferenceCountUtil; public class AmqpConnection extends AmqpAbstractResource implements NettyTransportListener { @@ -169,13 +170,22 @@ public class AmqpConnection extends AmqpAbstractResource implements } }); - if (connectTimeout <= 0) { - future.sync(); - } else { - future.sync(connectTimeout, TimeUnit.MILLISECONDS); - if (getEndpoint().getRemoteState() != EndpointState.ACTIVE) { - throw new IOException("Failed to connect after configured timeout."); + try { + if (connectTimeout <= 0) { + future.sync(); + } else { + future.sync(connectTimeout, TimeUnit.MILLISECONDS); + if (getEndpoint().getRemoteState() != EndpointState.ACTIVE) { + throw new IOException("Failed to connect after configured timeout."); + } } + } catch (Throwable e) { + try { + close(); + } catch (Throwable ignore) { + } + + throw e; } } } diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpSecurityTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpSecurityTest.java index 2c15c35d20..07b2ddc1fd 100644 --- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpSecurityTest.java +++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpSecurityTest.java @@ -16,6 +16,12 @@ */ package org.apache.activemq.artemis.tests.integration.amqp; +import java.io.IOException; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + import org.apache.activemq.artemis.api.core.SimpleString; import org.apache.activemq.artemis.core.config.Configuration; import org.apache.activemq.artemis.core.security.Role; @@ -33,12 +39,6 @@ import org.apache.activemq.transport.amqp.client.AmqpValidator; import org.apache.qpid.proton.engine.Delivery; import org.junit.Test; -import java.io.IOException; -import java.util.HashSet; -import java.util.Set; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; - public class AmqpSecurityTest extends AmqpClientTestSupport { @Override @@ -61,6 +61,55 @@ public class AmqpSecurityTest extends AmqpClientTestSupport { return server; } + @Test(timeout = 60000) + public void testSaslAuthWithInvalidCredentials() throws Exception { + AmqpConnection connection = null; + AmqpClient client = createAmqpClient("foo", "foo"); + + try { + connection = client.connect(); + fail("Should authenticate even with authzid set"); + } catch (Exception ex) { + } finally { + if (connection != null) { + connection.close(); + } + } + } + + @Test(timeout = 60000) + public void testSaslAuthWithAuthzid() throws Exception { + AmqpConnection connection = null; + AmqpClient client = createAmqpClient("foo", "bar"); + client.setAuthzid("foo"); + + try { + connection = client.connect(); + } catch (Exception ex) { + fail("Should authenticate even with authzid set"); + } finally { + if (connection != null) { + connection.close(); + } + } + } + + @Test(timeout = 60000) + public void testSaslAuthWithoutAuthzid() throws Exception { + AmqpConnection connection = null; + AmqpClient client = createAmqpClient("foo", "bar"); + + try { + connection = client.connect(); + } catch (Exception ex) { + fail("Should authenticate even with authzid set"); + } finally { + if (connection != null) { + connection.close(); + } + } + } + @Test(timeout = 60000) public void testSendAndRejected() throws Exception { AmqpConnection connection = null;