You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by ta...@apache.org on 2017/07/20 21:54:04 UTC
qpid-jms git commit: QPIDJMS-305 Refactor the SASL Authentication
process to remove race
Repository: qpid-jms
Updated Branches:
refs/heads/master bafac0c5b -> f5f287ec0
QPIDJMS-305 Refactor the SASL Authentication process to remove race
Refactor of the SASL Authenticator and the way it handles failures and
how they are then reported. Removes a race where an async provider
exception and the exception returned from the connect call can chase
each other and the wrong one gets tracked as the failure cuase.
Project: http://git-wip-us.apache.org/repos/asf/qpid-jms/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-jms/commit/f5f287ec
Tree: http://git-wip-us.apache.org/repos/asf/qpid-jms/tree/f5f287ec
Diff: http://git-wip-us.apache.org/repos/asf/qpid-jms/diff/f5f287ec
Branch: refs/heads/master
Commit: f5f287ec021e2bd881b3992b328c78f48d8397eb
Parents: bafac0c
Author: Timothy Bish <ta...@gmail.com>
Authored: Thu Jul 20 17:53:57 2017 -0400
Committer: Timothy Bish <ta...@gmail.com>
Committed: Thu Jul 20 17:53:57 2017 -0400
----------------------------------------------------------------------
.../qpid/jms/provider/amqp/AmqpProvider.java | 13 ++-
.../provider/amqp/AmqpSaslAuthenticator.java | 85 +++++++++++---------
.../apache/qpid/jms/sasl/AbstractMechanism.java | 5 +-
.../apache/qpid/jms/sasl/CramMD5Mechanism.java | 6 +-
.../org/apache/qpid/jms/sasl/Mechanism.java | 23 ++++++
.../jms/integration/SaslIntegrationTest.java | 6 ++
6 files changed, 93 insertions(+), 45 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/f5f287ec/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpProvider.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpProvider.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpProvider.java
index 262c6b8..4a4178e 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpProvider.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpProvider.java
@@ -947,12 +947,21 @@ public class AmqpProvider implements Provider, TransportListener , AmqpResourceP
}
try {
- if (authenticator.authenticate()) {
+ authenticator.tryAuthenticate();
+
+ if (authenticator.isComplete()) {
if (!authenticator.wasSuccessful()) {
- // Close the transport to avoid emitting any additional frames.
+ // Close the transport to avoid emitting any additional frames if the
+ // authentication process was unsuccessful, then signal the completion
+ // to avoid any race with the caller triggering any other traffic.
+ // Don't release the authenticator as we need it on close to know what
+ // the state of authentication was.
org.apache.qpid.proton.engine.Transport t = protonConnection.getTransport();
t.close_head();
+ authenticator.signalCompletion();
} else {
+ // Signal completion and release the authenticator we won't use it again.
+ authenticator.signalCompletion();
authenticator = null;
}
}
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/f5f287ec/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticator.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticator.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticator.java
index 8839bae..3b986a8 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticator.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticator.java
@@ -19,7 +19,6 @@ package org.apache.qpid.jms.provider.amqp;
import java.util.function.Function;
import javax.jms.JMSSecurityException;
-import javax.security.sasl.SaslException;
import org.apache.qpid.jms.provider.AsyncResult;
import org.apache.qpid.jms.sasl.Mechanism;
@@ -36,6 +35,7 @@ public class AmqpSaslAuthenticator {
private Mechanism mechanism;
private boolean complete;
+ private JMSSecurityException failureCause;
/**
* Create the authenticator and initialize it.
@@ -58,10 +58,8 @@ public class AmqpSaslAuthenticator {
* method must be called by the managing entity until the return value is true indicating a
* successful authentication or a JMSSecurityException is thrown indicating that the
* handshake failed.
- *
- * @return true if the authentication process completed.
*/
- public boolean authenticate() {
+ public void tryAuthenticate() {
try {
switch (sasl.getState()) {
case PN_SASL_IDLE:
@@ -74,17 +72,14 @@ public class AmqpSaslAuthenticator {
handleSaslFail();
break;
case PN_SASL_PASS:
- authenticationRequest.onSuccess();
+ handleSaslCompletion();
+ break;
default:
break;
}
- } catch (JMSSecurityException result) {
- authenticationRequest.onFailure(result);
+ } catch (Throwable error) {
+ recordFailure(error.getMessage(), error);
}
-
- complete = authenticationRequest.isComplete();
-
- return complete;
}
public boolean isComplete() {
@@ -92,23 +87,22 @@ public class AmqpSaslAuthenticator {
}
public boolean wasSuccessful() throws IllegalStateException {
- switch (sasl.getState()) {
- case PN_SASL_CONF:
- case PN_SASL_IDLE:
- case PN_SASL_STEP:
- break;
- case PN_SASL_FAIL:
- return false;
- case PN_SASL_PASS:
- return true;
- default:
- break;
+ if (complete) {
+ return failureCause == null;
+ } else {
+ throw new IllegalStateException("Authentication has not completed yet.");
}
+ }
- throw new IllegalStateException("Authentication has not completed yet.");
+ public void signalCompletion() {
+ if (failureCause != null) {
+ authenticationRequest.onFailure(failureCause);
+ } else {
+ authenticationRequest.onSuccess();
+ }
}
- private void handleSaslInit() throws JMSSecurityException {
+ private void handleSaslInit() {
try {
String[] remoteMechanisms = sasl.getRemoteMechanisms();
if (remoteMechanisms != null && remoteMechanisms.length != 0) {
@@ -120,18 +114,15 @@ public class AmqpSaslAuthenticator {
sasl.send(response, 0, response.length);
}
} else {
- throw new JMSSecurityException("Could not find a suitable SASL mechanism for the remote peer using the available credentials.");
+ recordFailure("Could not find a suitable SASL mechanism for the remote peer using the available credentials.", null);
}
}
- } catch (SaslException se) {
- JMSSecurityException jmsse = new JMSSecurityException("Exception while processing SASL init: " + se.getMessage());
- jmsse.setLinkedException(se);
- jmsse.initCause(se);
- throw jmsse;
+ } catch (Throwable error) {
+ recordFailure("Exception while processing SASL init: " + error.getMessage(), error);
}
}
- private void handleSaslStep() throws JMSSecurityException {
+ private void handleSaslStep() {
try {
if (sasl.pending() != 0) {
byte[] challenge = new byte[sasl.pending()];
@@ -139,19 +130,35 @@ public class AmqpSaslAuthenticator {
byte[] response = mechanism.getChallengeResponse(challenge);
sasl.send(response, 0, response.length);
}
- } catch (SaslException se) {
- JMSSecurityException jmsse = new JMSSecurityException("Exception while processing SASL step: " + se.getMessage());
- jmsse.setLinkedException(se);
- jmsse.initCause(se);
- throw jmsse;
+ } catch (Throwable error) {
+ recordFailure("Exception while processing SASL step: " + error.getMessage(), error);
}
}
- private void handleSaslFail() throws JMSSecurityException {
+ private void handleSaslFail() {
if (mechanism != null) {
- throw new JMSSecurityException("Client failed to authenticate using SASL: " + mechanism.getName());
+ recordFailure("Client failed to authenticate using SASL: " + mechanism.getName(), null);
} else {
- throw new JMSSecurityException("Client failed to authenticate");
+ recordFailure("Client failed to authenticate", null);
}
}
+
+ private void handleSaslCompletion() {
+ try {
+ mechanism.verifyCompletion();
+ complete = true;
+ } catch (Throwable error) {
+ recordFailure("Exception while processing SASL exchange completion: " + error.getMessage(), error);
+ }
+ }
+
+ private void recordFailure(String message, Throwable cause) {
+ failureCause = new JMSSecurityException(message);
+ if (cause instanceof Exception) {
+ failureCause.setLinkedException((Exception) cause);
+ }
+ failureCause.initCause(cause);
+
+ complete = true;
+ }
}
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/f5f287ec/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractMechanism.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractMechanism.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractMechanism.java
index c2b5855..40efa6a 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractMechanism.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractMechanism.java
@@ -28,8 +28,11 @@ public abstract class AbstractMechanism implements Mechanism {
private String password;
@Override
- public int compareTo(Mechanism other) {
+ public void verifyCompletion() {
+ }
+ @Override
+ public int compareTo(Mechanism other) {
if (getPriority() < other.getPriority()) {
return -1;
} else if (getPriority() > other.getPriority()) {
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/f5f287ec/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/CramMD5Mechanism.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/CramMD5Mechanism.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/CramMD5Mechanism.java
index 8d74f33..8620aba 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/CramMD5Mechanism.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/CramMD5Mechanism.java
@@ -34,7 +34,7 @@ public class CramMD5Mechanism extends AbstractMechanism {
private static final String ASCII = "ASCII";
private static final String HMACMD5 = "HMACMD5";
- private boolean _sentResponse;
+ private boolean sentResponse;
@Override
public int getPriority() {
@@ -53,7 +53,7 @@ public class CramMD5Mechanism extends AbstractMechanism {
@Override
public byte[] getChallengeResponse(byte[] challenge) throws SaslException {
- if (!_sentResponse && challenge != null && challenge.length != 0) {
+ if (!sentResponse && challenge != null && challenge.length != 0) {
try {
SecretKeySpec key = new SecretKeySpec(getPassword().getBytes(ASCII), HMACMD5);
Mac mac = Mac.getInstance(HMACMD5);
@@ -71,7 +71,7 @@ public class CramMD5Mechanism extends AbstractMechanism {
hash.append(hex);
}
- _sentResponse = true;
+ sentResponse = true;
return hash.toString().getBytes(ASCII);
} catch (UnsupportedEncodingException e) {
throw new SaslException("Unable to utilise required encoding", e);
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/f5f287ec/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/Mechanism.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/Mechanism.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/Mechanism.java
index 2478820..ca8225d 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/Mechanism.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/Mechanism.java
@@ -83,6 +83,16 @@ public interface Mechanism extends Comparable<Mechanism> {
byte[] getChallengeResponse(byte[] challenge) throws SaslException;
/**
+ * Verifies that the SASL exchange has completed successfully. This is
+ * an opportunity for the mechanism to ensure that all mandatory
+ * steps have been completed successfully and to cleanup and resources
+ * that are held by this Mechanism.
+ *
+ * @throws SaslException if the outcome of the SASL exchange is not valid for this Mechanism
+ */
+ void verifyCompletion() throws SaslException;
+
+ /**
* Sets the user name value for this Mechanism. The Mechanism can ignore this
* value if it does not utilize user name in it's authentication processing.
*
@@ -114,6 +124,19 @@ public interface Mechanism extends Comparable<Mechanism> {
*/
String getPassword();
+ /**
+ * Allows the mechanism to determine if it can be used given the authentication
+ * provided.
+ *
+ * @param username
+ * The user name given to the client for authentication.
+ * @param password
+ * The password given to the client for authentication.
+ * @param localPrincipal
+ * The local Principal configured for the client for authentication.
+ *
+ * @return if this Mechanism is able to validate using the given credentials.
+ */
boolean isApplicable(String username, String password, Principal localPrincipal);
}
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/f5f287ec/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/SaslIntegrationTest.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/SaslIntegrationTest.java b/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/SaslIntegrationTest.java
index 8279316..8cb12f0 100644
--- a/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/SaslIntegrationTest.java
+++ b/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/SaslIntegrationTest.java
@@ -41,9 +41,13 @@ import org.apache.qpid.jms.transports.TransportSslOptions;
import org.apache.qpid.jms.transports.TransportSupport;
import org.apache.qpid.proton.amqp.Symbol;
import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class SaslIntegrationTest extends QpidJmsTestCase {
+ private static final Logger LOG = LoggerFactory.getLogger(SaslIntegrationTest.class);
+
private static final Symbol ANONYMOUS = Symbol.valueOf("ANONYMOUS");
private static final Symbol PLAIN = Symbol.valueOf("PLAIN");
private static final Symbol CRAM_MD5 = Symbol.valueOf("CRAM-MD5");
@@ -235,6 +239,8 @@ public class SaslIntegrationTest extends QpidJmsTestCase {
// Expected, we deliberately failed the SASL process,
// we only wanted to verify the correct mechanism
// was selected, other tests verify the remainder.
+
+ LOG.info("Caught expected security exception: {}", jmsse.getMessage());
}
if (wait) {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org