Return-Path: X-Original-To: apmail-activemq-commits-archive@www.apache.org Delivered-To: apmail-activemq-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id B52451821F for ; Fri, 20 Nov 2015 19:39:16 +0000 (UTC) Received: (qmail 90813 invoked by uid 500); 20 Nov 2015 19:39:16 -0000 Delivered-To: apmail-activemq-commits-archive@activemq.apache.org Received: (qmail 90779 invoked by uid 500); 20 Nov 2015 19:39:16 -0000 Mailing-List: contact commits-help@activemq.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@activemq.apache.org Delivered-To: mailing list commits@activemq.apache.org Received: (qmail 90770 invoked by uid 99); 20 Nov 2015 19:39:16 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 20 Nov 2015 19:39:16 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 63F9FE0440; Fri, 20 Nov 2015 19:39:16 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: tabish@apache.org To: commits@activemq.apache.org Message-Id: X-Mailer: ASF-Git Admin Mailer Subject: activemq git commit: https://issues.apache.org/jira/browse/AMQ-6055 Date: Fri, 20 Nov 2015 19:39:16 +0000 (UTC) Repository: activemq Updated Branches: refs/heads/master d7e4c6d96 -> b5dd0a16f https://issues.apache.org/jira/browse/AMQ-6055 Account for Authzid in SASL PLAIN mechanism and provide a means to fail the authorization if the challenge response is invalid. Update the client to properly exclude sasl mechanism that don't apply to it's configured credentials such as using only ANONYMOUS when no user or password is set. Project: http://git-wip-us.apache.org/repos/asf/activemq/repo Commit: http://git-wip-us.apache.org/repos/asf/activemq/commit/b5dd0a16 Tree: http://git-wip-us.apache.org/repos/asf/activemq/tree/b5dd0a16 Diff: http://git-wip-us.apache.org/repos/asf/activemq/diff/b5dd0a16 Branch: refs/heads/master Commit: b5dd0a16f4197cfab086b3139892a73b27c8ac74 Parents: d7e4c6d Author: Timothy Bish Authored: Fri Nov 20 14:39:03 2015 -0500 Committer: Timothy Bish Committed: Fri Nov 20 14:39:03 2015 -0500 ---------------------------------------------------------------------- .../amqp/sasl/AbstractSaslMechanism.java | 17 +++++++++++++ .../transport/amqp/sasl/AmqpAuthenticator.java | 17 ++++++++----- .../transport/amqp/sasl/AnonymousMechanism.java | 12 +-------- .../transport/amqp/sasl/PlainMechanism.java | 25 +++++++++++++------ .../transport/amqp/sasl/SaslMechanism.java | 10 ++++++++ .../amqp/client/sasl/AbstractMechanism.java | 5 ++++ .../amqp/client/sasl/CramMD5Mechanism.java | 5 ++++ .../transport/amqp/client/sasl/Mechanism.java | 25 +++++++++++++++++++ .../amqp/client/sasl/PlainMechanism.java | 5 ++++ .../amqp/client/sasl/SaslAuthenticator.java | 12 ++++++--- .../amqp/interop/AmqpSaslPlainTest.java | 26 +++++++++++++++++--- 11 files changed, 129 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AbstractSaslMechanism.java ---------------------------------------------------------------------- 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 6f2e97e..036b1c3 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; + } } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AmqpAuthenticator.java ---------------------------------------------------------------------- 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 b248fbf..5ae9488 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); http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/AnonymousMechanism.java ---------------------------------------------------------------------- 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 012e8fd..276b792 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; - } } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/PlainMechanism.java ---------------------------------------------------------------------- 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 cf2948f..0227c36 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(); + Buffer[] parts = new Buffer(data).split((byte) 0); + 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; } } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/sasl/SaslMechanism.java ---------------------------------------------------------------------- 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 95daa24..8fa55b7 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(); + } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/AbstractMechanism.java ---------------------------------------------------------------------- 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 4bff560..beccbb2 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; + } } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/CramMD5Mechanism.java ---------------------------------------------------------------------- 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 638fb2e..7ad14e4 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; + } } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/Mechanism.java ---------------------------------------------------------------------- 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 4b926b8..0e47b83 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); + } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/PlainMechanism.java ---------------------------------------------------------------------- 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 530c99c..bcfe56e 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; + } } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/client/sasl/SaslAuthenticator.java ---------------------------------------------------------------------- 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 88b53fa..d225943 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); } } http://git-wip-us.apache.org/repos/asf/activemq/blob/b5dd0a16/activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/interop/AmqpSaslPlainTest.java ---------------------------------------------------------------------- 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 2197bbc..5463601 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);