diff --git a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AbstractSaslMechanism.java b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AbstractSaslMechanism.java index 6f2e97ecf0..036b1c36dc 100644 --- a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AbstractSaslMechanism.java +++ b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AbstractSaslMechanism.java @@ -23,6 +23,8 @@ public abstract class AbstractSaslMechanism implements SaslMechanism { protected String username; protected String password; + protected boolean failed; + protected String failureReason; @Override public String getUsername() { @@ -33,4 +35,19 @@ public abstract class AbstractSaslMechanism implements SaslMechanism { public String getPassword() { return password; } + + @Override + public boolean isFailed() { + return failed; + } + + @Override + public String getFailureReason() { + return failureReason; + } + + void setFailed(String failureReason) { + this.failed = true; + this.failureReason = failureReason; + } } diff --git a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AmqpAuthenticator.java b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AmqpAuthenticator.java index b248fbf6a5..5ae94888b4 100644 --- a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AmqpAuthenticator.java +++ b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AmqpAuthenticator.java @@ -75,17 +75,22 @@ public class AmqpAuthenticator { LOG.debug("SASL [{}} Handshake started.", mechanism.getMechanismName()); mechanism.processSaslStep(sasl); + if (!mechanism.isFailed()) { - connectionInfo.setUserName(mechanism.getUsername()); - connectionInfo.setPassword(mechanism.getPassword()); + connectionInfo.setUserName(mechanism.getUsername()); + connectionInfo.setPassword(mechanism.getPassword()); - if (tryAuthenticate(connectionInfo, transport.getPeerCertificates())) { - sasl.done(Sasl.SaslOutcome.PN_SASL_OK); + if (tryAuthenticate(connectionInfo, transport.getPeerCertificates())) { + sasl.done(Sasl.SaslOutcome.PN_SASL_OK); + } else { + sasl.done(Sasl.SaslOutcome.PN_SASL_AUTH); + } + + LOG.debug("SASL [{}} Handshake complete.", mechanism.getMechanismName()); } else { + LOG.debug("SASL [{}} Handshake failed: {}", mechanism.getMechanismName(), mechanism.getFailureReason()); sasl.done(Sasl.SaslOutcome.PN_SASL_AUTH); } - - LOG.debug("SASL [{}} Handshake complete.", mechanism.getMechanismName()); } else { LOG.info("SASL: could not find supported mechanism"); sasl.done(Sasl.SaslOutcome.PN_SASL_PERM); diff --git a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AnonymousMechanism.java b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AnonymousMechanism.java index 012e8fd187..276b7925bf 100644 --- a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AnonymousMechanism.java +++ b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AnonymousMechanism.java @@ -21,7 +21,7 @@ import org.apache.qpid.proton.engine.Sasl; /** * SASL Anonymous mechanism implementation. */ -public class AnonymousMechanism implements SaslMechanism { +public class AnonymousMechanism extends AbstractSaslMechanism { @Override public void processSaslStep(Sasl sasl) { @@ -31,14 +31,4 @@ public class AnonymousMechanism implements SaslMechanism { public String getMechanismName() { return "ANONYMOUS"; } - - @Override - public String getUsername() { - return null; - } - - @Override - public String getPassword() { - return null; - } } diff --git a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/PlainMechanism.java b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/PlainMechanism.java index cf2948fde5..0227c3614b 100644 --- a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/PlainMechanism.java +++ b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/PlainMechanism.java @@ -28,14 +28,25 @@ public class PlainMechanism extends AbstractSaslMechanism { public void processSaslStep(Sasl sasl) { byte[] data = new byte[sasl.pending()]; sasl.recv(data, 0, data.length); + Buffer[] parts = new Buffer(data).split((byte) 0); - - if (parts.length > 0) { - username = parts[0].utf8().toString(); - } - - if (parts.length > 1) { - password = parts[1].utf8().toString(); + switch (parts.length) { + case 0: + // Treat this as anonymous connect to support legacy behavior + // which allowed this. Connection will fail if broker is not + // configured to allow anonymous connections. + break; + case 2: + username = parts[0].utf8().toString(); + password = parts[1].utf8().toString(); + break; + case 3: + username = parts[1].utf8().toString(); + password = parts[2].utf8().toString(); + break; + default: + setFailed("Invalid encoding of Authentication credentials"); + break; } } diff --git a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/SaslMechanism.java b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/SaslMechanism.java index 95daa24291..8fa55b75f8 100644 --- a/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/SaslMechanism.java +++ b/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/SaslMechanism.java @@ -48,4 +48,14 @@ public interface SaslMechanism { */ String getMechanismName(); + /** + * @return true if the SASL processing failed during a step. + */ + boolean isFailed(); + + /** + * @return a failure error to explain why the mechanism failed. + */ + String getFailureReason(); + } diff --git a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/AbstractMechanism.java b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/AbstractMechanism.java index 4bff5602fb..beccbb2e9f 100644 --- a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/AbstractMechanism.java +++ b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/AbstractMechanism.java @@ -88,4 +88,9 @@ public abstract class AbstractMechanism implements Mechanism { public void setAuthzid(String authzid) { this.authzid = authzid; } + + @Override + public boolean isApplicable(String username, String password) { + return true; + } } diff --git a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/CramMD5Mechanism.java b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/CramMD5Mechanism.java index 638fb2e3da..7ad14e434d 100644 --- a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/CramMD5Mechanism.java +++ b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/CramMD5Mechanism.java @@ -83,4 +83,9 @@ public class CramMD5Mechanism extends AbstractMechanism { return EMPTY; } } + + @Override + public boolean isApplicable(String username, String password) { + return username != null && username.length() > 0 && password != null && password.length() > 0; + } } diff --git a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/Mechanism.java b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/Mechanism.java index 4b926b8341..0e47b83ca5 100644 --- a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/Mechanism.java +++ b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/Mechanism.java @@ -122,7 +122,32 @@ public interface Mechanism extends Comparable { */ Map getProperties(); + /** + * Using the configured credentials, check if the mechanism applies or not. + * + * @param username + * The user name that will be used with this mechanism + * @param password + * The password that will be used with this mechanism + * + * @return true if the mechanism works with the provided credentials or not. + */ + boolean isApplicable(String username, String password); + + /** + * Get the currently configured Authentication ID. + * + * @return the currently set Authentication ID. + */ String getAuthzid(); + /** + * Sets an Authentication ID that some mechanism can use during the + * challenge response phase. + * + * @param authzid + * The Authentication ID to use. + */ void setAuthzid(String authzid); + } diff --git a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/PlainMechanism.java b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/PlainMechanism.java index 530c99c823..bcfe56e7cf 100644 --- a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/PlainMechanism.java +++ b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/PlainMechanism.java @@ -68,4 +68,9 @@ public class PlainMechanism extends AbstractMechanism { public byte[] getChallengeResponse(byte[] challenge) { return EMPTY; } + + @Override + public boolean isApplicable(String username, String password) { + return username != null && username.length() > 0 && password != null && password.length() > 0; + } } diff --git a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/SaslAuthenticator.java b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/SaslAuthenticator.java index 88b53faff8..d225943eab 100644 --- a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/SaslAuthenticator.java +++ b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/SaslAuthenticator.java @@ -131,14 +131,20 @@ public class SaslAuthenticator { continue; } + Mechanism mechanism = null; if (remoteMechanism.equalsIgnoreCase("PLAIN")) { - found.add(new PlainMechanism()); + mechanism = new PlainMechanism(); } else if (remoteMechanism.equalsIgnoreCase("ANONYMOUS")) { - found.add(new AnonymousMechanism()); + mechanism = new AnonymousMechanism(); } else if (remoteMechanism.equalsIgnoreCase("CRAM-MD5")) { - found.add(new CramMD5Mechanism()); + mechanism = new CramMD5Mechanism(); } else { LOG.debug("Unknown remote mechanism {}, skipping", remoteMechanism); + continue; + } + + if (mechanism.isApplicable(username, password)) { + found.add(mechanism); } } diff --git a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/interop/AmqpSaslPlainTest.java b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/interop/AmqpSaslPlainTest.java index 2197bbc88c..5463601774 100644 --- a/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/interop/AmqpSaslPlainTest.java +++ b/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/interop/AmqpSaslPlainTest.java @@ -33,7 +33,6 @@ import org.apache.activemq.transport.amqp.client.AmqpConnection; import org.apache.activemq.transport.amqp.client.AmqpSender; import org.apache.activemq.transport.amqp.client.AmqpSession; import org.apache.activemq.transport.amqp.client.sasl.PlainMechanism; -import org.junit.Ignore; import org.junit.Test; /** @@ -63,15 +62,22 @@ public class AmqpSaslPlainTest extends AmqpClientTestSupport { doSucessfullConnectionTestImpl(client); } - @Ignore //TODO: fix broker to handle authzid @Test(timeout = 30000) - public void testSaslPlainWithValidUsernameAndPasswordAndAuthzid() throws Exception { + public void testSaslPlainWithValidUsernameAndPasswordAndAuthzidAsUser() throws Exception { AmqpClient client = createAmqpClient(USER, USER_PASSWORD); client.setAuthzid(USER); doSucessfullConnectionTestImpl(client); } + @Test(timeout = 30000) + public void testSaslPlainWithValidUsernameAndPasswordAndAuthzidAsUnkown() throws Exception { + AmqpClient client = createAmqpClient(USER, USER_PASSWORD); + client.setAuthzid("unknown"); + + doSucessfullConnectionTestImpl(client); + } + private void doSucessfullConnectionTestImpl(AmqpClient client) throws Exception { client.setMechanismRestriction(PlainMechanism.MECH_NAME); @@ -110,6 +116,20 @@ public class AmqpSaslPlainTest extends AmqpClientTestSupport { doFailedConnectionTestImpl(client); } + @Test(timeout = 30000) + public void testSaslPlainWithInvalidUsernameAndAuthzid() throws Exception { + AmqpClient client = createAmqpClient("not-user", USER_PASSWORD); + client.setAuthzid(USER); + doFailedConnectionTestImpl(client); + } + + @Test(timeout = 30000) + public void testSaslPlainWithInvalidPasswordAndAuthzid() throws Exception { + AmqpClient client = createAmqpClient(USER, "not-user-password"); + client.setAuthzid(USER); + doFailedConnectionTestImpl(client); + } + private void doFailedConnectionTestImpl(AmqpClient client) throws Exception { client.setMechanismRestriction(PlainMechanism.MECH_NAME);