qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rob...@apache.org
Subject qpid-jms git commit: QPIDJMS-294: allow the SCRAM mechanisms to verify the server final message if it is sent in the additional-data field of the sasl-outcome frame, and ensure that SASL mechanism has completed before allowing authentication to complete
Date Wed, 02 Aug 2017 16:40:51 GMT
Repository: qpid-jms
Updated Branches:
  refs/heads/master 655277b5a -> 53d96e8a5


QPIDJMS-294: allow the SCRAM mechanisms to verify the server final message if it is sent in
the additional-data field of the sasl-outcome frame, and ensure that SASL mechanism has completed
before allowing authentication to complete successfully.

This closes #9.


Project: http://git-wip-us.apache.org/repos/asf/qpid-jms/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-jms/commit/53d96e8a
Tree: http://git-wip-us.apache.org/repos/asf/qpid-jms/tree/53d96e8a
Diff: http://git-wip-us.apache.org/repos/asf/qpid-jms/diff/53d96e8a

Branch: refs/heads/master
Commit: 53d96e8a5162257894aaaf3951b4ce1d77e641ed
Parents: 655277b
Author: Keith Wall <keith.wall@gmail.com>
Authored: Wed Jul 12 15:55:37 2017 +0100
Committer: Robert Gemmell <robbie@apache.org>
Committed: Wed Aug 2 17:22:52 2017 +0100

----------------------------------------------------------------------
 .../provider/amqp/AmqpSaslAuthenticator.java    |   5 +
 .../jms/sasl/AbstractScramSHAMechanism.java     |  10 +
 .../apache/qpid/jms/sasl/CramMD5Mechanism.java  |  12 +-
 .../amqp/AmqpSaslAuthenticatorTest.java         | 257 +++++++++++++++++++
 .../sasl/AbstractScramSHAMechanismTestBase.java |  20 ++
 .../qpid/jms/sasl/CramMD5MechanismTest.java     |  43 ++++
 6 files changed, 344 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/53d96e8a/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 584dc6d..deb67e9 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
@@ -138,6 +138,11 @@ public class AmqpSaslAuthenticator {
 
     private void handleSaslCompletion() {
         try {
+            if (sasl.pending() != 0) {
+                byte[] additionalData = new byte[sasl.pending()];
+                sasl.recv(additionalData, 0, additionalData.length);
+                mechanism.getChallengeResponse(additionalData);
+            }
             mechanism.verifyCompletion();
             complete = true;
         } catch (Throwable error) {

http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/53d96e8a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanism.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanism.java
b/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanism.java
index 60cbf6f..5f616fc 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanism.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanism.java
@@ -96,6 +96,16 @@ abstract class AbstractScramSHAMechanism extends AbstractMechanism {
         return response;
     }
 
+    @Override
+    public void verifyCompletion() throws SaslException {
+        super.verifyCompletion();
+        if (state != State.COMPLETE) {
+            throw new SaslException(String.format("SASL exchange was not fully completed."
+
+                                                  " Expected state %s but actual state %s",
+                                                  State.COMPLETE, state));
+        }
+    }
+
     private byte[] calculateClientProof(final byte[] challenge) throws SaslException {
         try {
             String serverFirstMessage = new String(challenge, StandardCharsets.US_ASCII);

http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/53d96e8a/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 8620aba..a96e909 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
@@ -26,9 +26,7 @@ import javax.crypto.spec.SecretKeySpec;
 import javax.security.sasl.SaslException;
 
 /**
- * Implements the SASL PLAIN authentication Mechanism.
- *
- * User name and Password values are sent without being encrypted.
+ * Implements the SASL CRAM-MD5 authentication Mechanism.
  */
 public class CramMD5Mechanism extends AbstractMechanism {
 
@@ -86,6 +84,14 @@ public class CramMD5Mechanism extends AbstractMechanism {
     }
 
     @Override
+    public void verifyCompletion() throws SaslException {
+        super.verifyCompletion();
+        if (!sentResponse) {
+            throw new SaslException("SASL exchange was not fully completed.");
+        }
+    }
+
+    @Override
     public boolean isApplicable(String username, String password, Principal localPrincipal)
{
         return username != null && username.length() > 0 && password !=
null && password.length() > 0;
     }

http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/53d96e8a/qpid-jms-client/src/test/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticatorTest.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/test/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticatorTest.java
b/qpid-jms-client/src/test/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticatorTest.java
new file mode 100644
index 0000000..c034728
--- /dev/null
+++ b/qpid-jms-client/src/test/java/org/apache/qpid/jms/provider/amqp/AmqpSaslAuthenticatorTest.java
@@ -0,0 +1,257 @@
+/*
+ * 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.qpid.jms.provider.amqp;
+
+import static org.apache.qpid.proton.engine.Sasl.SaslState;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.security.Principal;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Deque;
+import java.util.Map;
+import java.util.function.Function;
+
+import javax.security.sasl.SaslException;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.stubbing.Answer;
+
+import org.apache.qpid.jms.provider.AsyncResult;
+import org.apache.qpid.jms.sasl.Mechanism;
+import org.apache.qpid.proton.engine.Sasl;
+
+public class AmqpSaslAuthenticatorTest {
+
+    private static final String MECHANISM_NAME = "MY_MECHANISM";
+    private static final byte[] INITIAL_RESPONSE = "initial".getBytes();
+    private static final byte[] RESPONSE = "response".getBytes();
+    private static final byte[] EXPECTED_CHALLENGE = "expectedChallenge".getBytes();
+    private static final byte[] EMPTY_BYTES = {};
+
+    private final Sasl sasl = mock(Sasl.class);
+
+    @Before
+    public void setUp() {
+        when(sasl.getState()).thenReturn(SaslState.PN_SASL_IDLE);
+        when(sasl.getRemoteMechanisms()).thenReturn(new String[]{MECHANISM_NAME});
+    }
+
+    @Test
+    public void testNoSaslMechanismAgreed() throws Exception {
+        Function<String[], Mechanism> mechanismFunction = mechanismName -> null;
+
+        AmqpSaslAuthenticator authenticator = new AmqpSaslAuthenticator(sasl, mechanismFunction);
+        authenticator.tryAuthenticate();
+
+        assertTrue(authenticator.isComplete());
+        assertFalse(authenticator.wasSuccessful());
+        assertNotNull(authenticator.getFailureCause());
+        assertTrue(authenticator.getFailureCause().getMessage().contains("Could not find
a suitable SASL mechanism"));
+    }
+
+    @Test
+    public void testAuthenticationSuccess() throws Exception {
+        Mechanism mechanism = new TestSaslMechanism(INITIAL_RESPONSE, EXPECTED_CHALLENGE,
RESPONSE);
+        AmqpSaslAuthenticator authenticator = new AmqpSaslAuthenticator(sasl, mechanismName
-> mechanism);
+
+        authenticator.tryAuthenticate();
+        verify(sasl).setMechanisms(mechanism.getName());
+        verifySaslMockReceived(sasl, INITIAL_RESPONSE);
+
+        when(sasl.getState()).thenReturn(SaslState.PN_SASL_STEP);
+        configureSaslMockToProduce(sasl, EXPECTED_CHALLENGE);
+        authenticator.tryAuthenticate();
+        verifySaslMockReceived(sasl, RESPONSE);
+
+        when(sasl.getState()).thenReturn(SaslState.PN_SASL_PASS);
+        configureSaslMockToProduce(sasl, EMPTY_BYTES);
+        authenticator.tryAuthenticate();
+
+        assertTrue(authenticator.isComplete());
+        assertTrue(authenticator.wasSuccessful());
+        assertNull(authenticator.getFailureCause());
+    }
+
+    @Test
+    public void testAuthenticationSuccessWithoutChallengeStep() throws Exception {
+        Mechanism mechanism = new TestSaslMechanism(INITIAL_RESPONSE);
+        AmqpSaslAuthenticator authenticator = new AmqpSaslAuthenticator(sasl, mechanismName
-> mechanism);
+
+        authenticator.tryAuthenticate();
+        verifySaslMockReceived(sasl, INITIAL_RESPONSE);
+
+        when(sasl.getState()).thenReturn(SaslState.PN_SASL_PASS);
+        configureSaslMockToProduce(sasl, EMPTY_BYTES);
+        authenticator.tryAuthenticate();
+
+        assertTrue(authenticator.isComplete());
+        assertTrue(authenticator.wasSuccessful());
+    }
+
+    @Test
+    public void testPeerSignalsAuthenticationFail() throws Exception {
+        Mechanism mechanism = new TestSaslMechanism(INITIAL_RESPONSE);
+        AmqpSaslAuthenticator authenticator = new AmqpSaslAuthenticator(sasl, mechanismName
-> mechanism);
+
+        authenticator.tryAuthenticate();
+        verifySaslMockReceived(sasl, INITIAL_RESPONSE);
+
+        when(sasl.getState()).thenReturn(SaslState.PN_SASL_FAIL);
+        authenticator.tryAuthenticate();
+
+        assertTrue(authenticator.isComplete());
+        assertFalse(authenticator.wasSuccessful());
+        assertNotNull(authenticator.getFailureCause());
+        assertTrue(authenticator.getFailureCause().getMessage().contains("Client failed to
authenticate"));
+    }
+
+    @Test
+    public void testMechanismSignalsNotComplete() throws Exception {
+        Mechanism mechanism = new TestSaslMechanism(INITIAL_RESPONSE,
+                                                    EXPECTED_CHALLENGE, RESPONSE,
+                                                    EMPTY_BYTES, EMPTY_BYTES);
+        AmqpSaslAuthenticator authenticator = new AmqpSaslAuthenticator(sasl, mechanismName
-> mechanism);
+
+        when(sasl.getState()).thenReturn(SaslState.PN_SASL_IDLE);
+        authenticator.tryAuthenticate();
+        verifySaslMockReceived(sasl, INITIAL_RESPONSE);
+
+        when(sasl.getState()).thenReturn(SaslState.PN_SASL_STEP);
+        configureSaslMockToProduce(sasl, EXPECTED_CHALLENGE);
+        authenticator.tryAuthenticate();
+        verifySaslMockReceived(sasl, RESPONSE);
+
+        when(sasl.getState()).thenReturn(SaslState.PN_SASL_PASS);
+        configureSaslMockToProduce(sasl, EMPTY_BYTES);
+        authenticator.tryAuthenticate();
+
+        assertTrue(authenticator.isComplete());
+        assertFalse(authenticator.wasSuccessful());
+        assertNotNull(authenticator.getFailureCause());
+        assertTrue(authenticator.getFailureCause().getMessage().contains("SASL exchange not
completed"));
+    }
+
+    private void verifySaslMockReceived(final Sasl sasl, final byte[] response) {
+        verify(sasl).send(response, 0, response.length);
+    }
+
+    private void configureSaslMockToProduce(final Sasl sasl, final byte[] challenge) {
+        when(sasl.pending()).thenReturn(challenge.length);
+        Answer<Void> answer = invocationOnMock -> {
+            byte[] buf = invocationOnMock.getArgumentAt(0, byte[].class);
+            int offset = invocationOnMock.getArgumentAt(1, int.class);
+            int length = invocationOnMock.getArgumentAt(2, int.class);
+            System.arraycopy(challenge, 0, buf, offset, Math.min(length, challenge.length));
+            return null;
+        };
+        doAnswer(answer).when(sasl).recv(any(byte[].class), any(int.class), any(int.class));
+    }
+
+    private class TestSaslMechanism implements Mechanism {
+
+        private final byte[] initialResponse;
+        private final Deque<byte[]> challengeExpectationResponseSequence;
+
+        TestSaslMechanism(final byte[] initialResponse, final byte[]... challengeExpectationResponseSequence)
{
+            if (challengeExpectationResponseSequence.length % 2 != 0) {
+                throw new IllegalStateException("Number of items in the challenge expectation
/ response array must be even");
+            }
+            this.initialResponse = initialResponse;
+            this.challengeExpectationResponseSequence = new ArrayDeque<>(Arrays.asList(challengeExpectationResponseSequence));
+        }
+
+        @Override
+        public int getPriority() {
+            return 0;
+        }
+
+        @Override
+        public String getName() {
+            return MECHANISM_NAME;
+        }
+
+        @Override
+        public void init(final Map<String, String> options) {
+        }
+
+        @Override
+        public byte[] getInitialResponse() {
+            return initialResponse;
+        }
+
+        @Override
+        public byte[] getChallengeResponse(final byte[] challenge) throws SaslException {
+            final byte[] expectedChallenge = challengeExpectationResponseSequence.removeFirst();
+            final byte[] response = challengeExpectationResponseSequence.removeFirst();
+
+            if (!Arrays.equals(expectedChallenge, challenge)) {
+                throw new SaslException("Challenge does not meet the challenge expectation");
+            }
+            return response;
+        }
+
+        @Override
+        public void verifyCompletion() throws SaslException {
+            if (!challengeExpectationResponseSequence.isEmpty()) {
+                throw new SaslException("SASL exchange not completed");
+            }
+        }
+
+        @Override
+        public void setUsername(final String username) {
+        }
+
+        @Override
+        public String getUsername() {
+            return null;
+        }
+
+        @Override
+        public void setPassword(final String username) {
+        }
+
+        @Override
+        public String getPassword() {
+            return null;
+        }
+
+        @Override
+        public boolean isApplicable(final String username, final String password, final Principal
localPrincipal) {
+            return true;
+        }
+
+        @Override
+        public int compareTo(final Mechanism o) {
+            return 0;
+        }
+
+        @Override
+        public boolean isEnabledByDefault() {
+            return true;
+        }
+    }
+}

http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/53d96e8a/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanismTestBase.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanismTestBase.java
b/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanismTestBase.java
index 4b39ea6..07913cc 100644
--- a/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanismTestBase.java
+++ b/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanismTestBase.java
@@ -56,6 +56,8 @@ public abstract class AbstractScramSHAMechanismTestBase {
 
         byte[] expectedFinalChallengeResponse = "".getBytes();
         assertArrayEquals(expectedFinalChallengeResponse, mechanism.getChallengeResponse(serverFinalMessage));
+
+        mechanism.verifyCompletion();
     }
 
     @Test
@@ -137,4 +139,22 @@ public abstract class AbstractScramSHAMechanismTestBase {
             // PASS
         }
     }
+
+    @Test
+    public void testIncompleteExchange() throws Exception {
+        Mechanism mechanism = getConfiguredMechanism();
+
+        byte[] clientInitialResponse = mechanism.getInitialResponse();
+        assertArrayEquals(expectedClientInitialResponse, clientInitialResponse);
+
+        byte[] clientFinalMessage = mechanism.getChallengeResponse(serverFirstMessage);
+        assertArrayEquals(expectedClientFinalMessage, clientFinalMessage);
+
+        try {
+            mechanism.verifyCompletion();
+            fail("Exception not thrown");
+        } catch (SaslException e) {
+            // PASS
+        }
+    }
 }
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/53d96e8a/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/CramMD5MechanismTest.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/CramMD5MechanismTest.java
b/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/CramMD5MechanismTest.java
index 3110147..bfd6f01 100644
--- a/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/CramMD5MechanismTest.java
+++ b/qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/CramMD5MechanismTest.java
@@ -18,13 +18,43 @@ package org.apache.qpid.jms.sasl;
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.fail;
 
 import java.security.Principal;
+import java.util.Base64;
 
 import org.junit.Test;
 
+import javax.security.sasl.SaslException;
+
+/**
+ * The known good used by these tests is taken from the example in RFC 2195 section 2.
+ */
+
 public class CramMD5MechanismTest {
 
+    private final byte[] SERVER_FIRST_MESSAGE = Base64.getDecoder().decode("PDE4OTYuNjk3MTcwOTUyQHBvc3RvZmZpY2UucmVzdG9uLm1jaS5uZXQ+");
+    private final byte[] EXPECTED_CLIENT_FINAL_MESSAGE = Base64.getDecoder().decode("dGltIGI5MTNhNjAyYzdlZGE3YTQ5NWI0ZTZlNzMzNGQzODkw");
+    private static final String USERNAME = "tim";
+    private static final String PASSWORD = "tanstaaftanstaaf";
+
+    @Test
+    public void testSuccessfulAuthentication() throws Exception {
+        Mechanism mechanism = new CramMD5Mechanism();
+        mechanism.setUsername(USERNAME);
+        mechanism.setPassword(PASSWORD);
+
+        byte[] clientInitialResponse = mechanism.getInitialResponse();
+        assertNull(clientInitialResponse);
+
+        byte[] clientFinalResponse = mechanism.getChallengeResponse(SERVER_FIRST_MESSAGE);
+        assertArrayEquals(EXPECTED_CLIENT_FINAL_MESSAGE, clientFinalResponse);
+
+        mechanism.verifyCompletion();
+    }
+
     @Test
     public void testIsNotApplicableWithNoCredentials() {
         CramMD5Mechanism mech = new CramMD5Mechanism();
@@ -92,4 +122,17 @@ public class CramMD5MechanismTest {
 
         assertTrue("Should be enabled by default", mech.isEnabledByDefault());
     }
+
+    public void testIncompleteExchange() throws Exception {
+        Mechanism mechanism = new CramMD5Mechanism();
+
+        mechanism.getInitialResponse();
+
+        try {
+            mechanism.verifyCompletion();
+            fail("Exception not thrown");
+        } catch (SaslException e) {
+            // PASS
+        }
+    }
 }


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message