QPID-3522: Distinguish between null and empty string for sasl response

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk/qpid@1182212 13f79535-47bb-0310-9956-ffa450edef68

10 files changed, 105 insertions, 53 deletions

diff --git a/cpp/src/qpid/Sasl.h b/cpp/src/qpid/Sasl.h
index 9a9d61b037..4d579fa051 100644
--- a/cpp/src/qpid/Sasl.h
+++ b/cpp/src/qpid/Sasl.h
@@ -47,8 +47,8 @@ class Sasl
      *             client supports.
      * @param externalSecuritySettings security related details from the underlying transport
      */
-    virtual std::string start(const std::string& mechanisms,
-                              const qpid::sys::SecuritySettings* externalSecuritySettings = 0) = 0;
+    virtual bool start(const std::string& mechanisms, std::string& response,
+                       const qpid::sys::SecuritySettings* externalSecuritySettings = 0) = 0;
     virtual std::string step(const std::string& challenge) = 0;
     virtual std::string getMechanism() = 0;
     virtual std::string getUserId() = 0;
diff --git a/cpp/src/qpid/SaslFactory.cpp b/cpp/src/qpid/SaslFactory.cpp
index f117404028..a8d1f94c1e 100644
--- a/cpp/src/qpid/SaslFactory.cpp
+++ b/cpp/src/qpid/SaslFactory.cpp
@@ -112,7 +112,7 @@ class CyrusSasl : public Sasl
   public:
     CyrusSasl(const std::string & username, const std::string & password, const std::string & serviceName, const std::string & hostName, int minSsf, int maxSsf, bool allowInteraction);
     ~CyrusSasl();
-    std::string start(const std::string& mechanisms, const SecuritySettings* externalSettings);
+    bool start(const std::string& mechanisms, std::string& response, const SecuritySettings* externalSettings);
     std::string step(const std::string& challenge);
     std::string getMechanism();
     std::string getUserId();
@@ -210,7 +210,7 @@ namespace {
     const std::string SSL("ssl");
 }
 
-std::string CyrusSasl::start(const std::string& mechanisms, const SecuritySettings* externalSettings)
+bool CyrusSasl::start(const std::string& mechanisms, std::string& response, const SecuritySettings* externalSettings)
 {
     QPID_LOG(debug, "CyrusSasl::start(" << mechanisms << ")");
     int result = sasl_client_new(settings.service.c_str(),
@@ -283,7 +283,12 @@ std::string CyrusSasl::start(const std::string& mechanisms, const SecuritySettin
     mechanism = std::string(chosenMechanism);
     QPID_LOG(debug, "CyrusSasl::start(" << mechanisms << "): selected "
              << mechanism << " response: '" << std::string(out, outlen) << "'");
-    return std::string(out, outlen);
+    if (out) {
+        response = std::string(out, outlen);
+        return true;
+    } else {
+        return false;
+    }
 }
 
 std::string CyrusSasl::step(const std::string& challenge)
diff --git a/cpp/src/qpid/broker/ConnectionHandler.cpp b/cpp/src/qpid/broker/ConnectionHandler.cpp
index 015002a70c..82b72d3f52 100644
--- a/cpp/src/qpid/broker/ConnectionHandler.cpp
+++ b/cpp/src/qpid/broker/ConnectionHandler.cpp
@@ -26,6 +26,7 @@
 #include "qpid/broker/SecureConnection.h"
 #include "qpid/Url.h"
 #include "qpid/framing/AllInvoker.h"
+#include "qpid/framing/ConnectionStartOkBody.h"
 #include "qpid/framing/enum.h"
 #include "qpid/log/Statement.h"
 #include "qpid/sys/SecurityLayer.h"
@@ -63,13 +64,24 @@ void ConnectionHandler::heartbeat()
     handler->proxy.heartbeat();
 }
 
+bool ConnectionHandler::handle(const framing::AMQMethodBody& method)
+{
+    //Need special handling for start-ok, in order to distinguish
+    //between null and empty response
+    if (method.isA<ConnectionStartOkBody>()) {
+        handler->startOk(dynamic_cast<const ConnectionStartOkBody&>(method));
+        return true;
+    } else {
+        return invoke(static_cast<AMQP_AllOperations::ConnectionHandler&>(*handler), method);
+    }
+}
+
 void ConnectionHandler::handle(framing::AMQFrame& frame)
 {
     AMQMethodBody* method=frame.getBody()->getMethod();
     Connection::ErrorListener* errorListener = handler->connection.getErrorListener();
     try{
-        if (method && invoke(
-                static_cast<AMQP_AllOperations::ConnectionHandler&>(*handler), *method)) {
+        if (method && handle(*method)) {
             // This is a connection control frame, nothing more to do.
         } else if (isOpen()) {
             handler->connection.getChannel(frame.getChannel()).in(frame);
@@ -125,13 +137,20 @@ ConnectionHandler::Handler::Handler(Connection& c, bool isClient, bool isShadow)
 ConnectionHandler::Handler::~Handler() {}
 
 
-void ConnectionHandler::Handler::startOk(const framing::FieldTable& clientProperties,
-                                         const string& mechanism,
-                                         const string& response,
+void ConnectionHandler::Handler::startOk(const framing::FieldTable& /*clientProperties*/,
+                                         const string& /*mechanism*/,
+                                         const string& /*response*/,
                                          const string& /*locale*/)
 {
+    //Need special handling for start-ok, in order to distinguish
+    //between null and empty response -> should never use this method
+    assert(false);
+}
+
+void ConnectionHandler::Handler::startOk(const ConnectionStartOkBody& body)
+{
     try {
-        authenticator->start(mechanism, response);
+        authenticator->start(body.getMechanism(), body.hasResponse() ? &body.getResponse() : 0);
     } catch (std::exception& /*e*/) {
         management::ManagementAgent* agent = connection.getAgent();
         if (agent) {
@@ -143,6 +162,7 @@ void ConnectionHandler::Handler::startOk(const framing::FieldTable& clientProper
         }
         throw;
     }
+    const framing::FieldTable& clientProperties = body.getClientProperties();
     connection.setFederationLink(clientProperties.get(QPID_FED_LINK));
     if (clientProperties.isSet(QPID_FED_TAG)) {
         connection.setFederationPeerTag(clientProperties.getAsString(QPID_FED_TAG));
@@ -308,11 +328,21 @@ void ConnectionHandler::Handler::start(const FieldTable& serverProperties,
     string response;
     if (sasl.get()) {
         const qpid::sys::SecuritySettings& ss = connection.getExternalSecuritySettings();
-        response = sasl->start ( requestedMechanism.empty()
-                                 ? supportedMechanismsList
-                                 : requestedMechanism,
-                                 & ss );
-        proxy.startOk ( ft, sasl->getMechanism(), response, en_US );
+        if (sasl->start ( requestedMechanism.empty()
+                          ? supportedMechanismsList
+                          : requestedMechanism,
+                          response,
+                          & ss )) {
+            proxy.startOk ( ft, sasl->getMechanism(), response, en_US );
+        } else {
+            //response was null
+            ConnectionStartOkBody body;
+            body.setClientProperties(ft);
+            body.setMechanism(sasl->getMechanism());
+            //Don't set response, as none was given
+            body.setLocale(en_US);
+            proxy.send(body);
+        }
     }
     else {
         response = ((char)0) + username + ((char)0) + password;
diff --git a/cpp/src/qpid/broker/ConnectionHandler.h b/cpp/src/qpid/broker/ConnectionHandler.h
index b32167669e..f3e7d7d21d 100644
--- a/cpp/src/qpid/broker/ConnectionHandler.h
+++ b/cpp/src/qpid/broker/ConnectionHandler.h
@@ -26,8 +26,10 @@
 #include "qpid/broker/SaslAuthenticator.h"
 #include "qpid/framing/amqp_types.h"
 #include "qpid/framing/AMQFrame.h"
+#include "qpid/framing/AMQMethodBody.h"
 #include "qpid/framing/AMQP_AllOperations.h"
 #include "qpid/framing/AMQP_AllProxy.h"
+#include "qpid/framing/ConnectionStartOkBody.h"
 #include "qpid/framing/enum.h"
 #include "qpid/framing/FrameHandler.h"
 #include "qpid/framing/ProtocolInitiation.h"
@@ -63,6 +65,7 @@ class ConnectionHandler : public framing::FrameHandler
 
         Handler(Connection& connection, bool isClient, bool isShadow=false);
         ~Handler();
+        void startOk(const qpid::framing::ConnectionStartOkBody& body);
         void startOk(const qpid::framing::FieldTable& clientProperties,
                      const std::string& mechanism, const std::string& response,
                      const std::string& locale);
@@ -96,7 +99,7 @@ class ConnectionHandler : public framing::FrameHandler
     };
     std::auto_ptr<Handler> handler;
 
-
+    bool handle(const qpid::framing::AMQMethodBody& method);
   public:
     ConnectionHandler(Connection& connection, bool isClient, bool isShadow=false );
     void close(framing::connection::CloseCode code, const std::string& text);
diff --git a/cpp/src/qpid/broker/SaslAuthenticator.cpp b/cpp/src/qpid/broker/SaslAuthenticator.cpp
index 12a13ccfe6..befe7eef25 100644
--- a/cpp/src/qpid/broker/SaslAuthenticator.cpp
+++ b/cpp/src/qpid/broker/SaslAuthenticator.cpp
@@ -57,7 +57,7 @@ public:
     NullAuthenticator(Connection& connection, bool encrypt);
     ~NullAuthenticator();
     void getMechanisms(framing::Array& mechanisms);
-    void start(const std::string& mechanism, const std::string& response);
+    void start(const std::string& mechanism, const std::string* response);
     void step(const std::string&) {}
     std::auto_ptr<SecurityLayer> getSecurityLayer(uint16_t maxFrameSize);
 };
@@ -81,7 +81,7 @@ public:
     ~CyrusAuthenticator();
     void init();
     void getMechanisms(framing::Array& mechanisms);
-    void start(const std::string& mechanism, const std::string& response);
+    void start(const std::string& mechanism, const std::string* response);
     void step(const std::string& response);
     void getError(std::string& error);
     void getUid(std::string& uid) { getUsername(uid); }
@@ -164,7 +164,7 @@ void NullAuthenticator::getMechanisms(Array& mechanisms)
     mechanisms.add(boost::shared_ptr<FieldValue>(new Str16Value("PLAIN")));//useful for testing
 }
 
-void NullAuthenticator::start(const string& mechanism, const string& response)
+void NullAuthenticator::start(const string& mechanism, const string* response)
 {
     if (encrypt) {
 #if HAVE_SASL
@@ -180,16 +180,16 @@ void NullAuthenticator::start(const string& mechanism, const string& response)
         }
     }
     if (mechanism == "PLAIN") { // Old behavior
-        if (response.size() > 0) {
+        if (response && response->size() > 0) {
             string uid;
-            string::size_type i = response.find((char)0);
-            if (i == 0 && response.size() > 1) {
+            string::size_type i = response->find((char)0);
+            if (i == 0 && response->size() > 1) {
                 //no authorization id; use authentication id
-                i = response.find((char)0, 1);
-                if (i != string::npos) uid = response.substr(1, i-1);
+                i = response->find((char)0, 1);
+                if (i != string::npos) uid = response->substr(1, i-1);
             } else if (i != string::npos) {
                 //authorization id is first null delimited field
-                uid = response.substr(0, i);
+                uid = response->substr(0, i);
             }//else not a valid SASL PLAIN response, throw error?            
             if (!uid.empty()) {
                 //append realm if it has not already been added
@@ -376,7 +376,7 @@ void CyrusAuthenticator::getMechanisms(Array& mechanisms)
     }
 }
 
-void CyrusAuthenticator::start(const string& mechanism, const string& response)
+void CyrusAuthenticator::start(const string& mechanism, const string* response)
 {
     const char *challenge;
     unsigned int challenge_len;
@@ -385,7 +385,7 @@ void CyrusAuthenticator::start(const string& mechanism, const string& response)
     QPID_LOG(info, "SASL: Starting authentication with mechanism: " << mechanism);
     int code = sasl_server_start(sasl_conn,
                                  mechanism.c_str(),
-                                 response.size() ? response.c_str() : 0, response.length(),
+                                 (response ? response->c_str() : 0), (response ? response->size() : 0),
                                  &challenge, &challenge_len);
     
     processAuthenticationStep(code, challenge, challenge_len);
diff --git a/cpp/src/qpid/broker/SaslAuthenticator.h b/cpp/src/qpid/broker/SaslAuthenticator.h
index cfbe1a0cd1..4e5d43214c 100644
--- a/cpp/src/qpid/broker/SaslAuthenticator.h
+++ b/cpp/src/qpid/broker/SaslAuthenticator.h
@@ -41,7 +41,7 @@ class SaslAuthenticator
 public:
     virtual ~SaslAuthenticator() {}
     virtual void getMechanisms(framing::Array& mechanisms) = 0;
-    virtual void start(const std::string& mechanism, const std::string& response) = 0;
+    virtual void start(const std::string& mechanism, const std::string* response) = 0;
     virtual void step(const std::string& response) = 0;
     virtual void getUid(std::string&) {}
     virtual bool getUsername(std::string&) { return false; };
diff --git a/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp b/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp
index d26b370632..2acc09cded 100644
--- a/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp
+++ b/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp
@@ -42,7 +42,7 @@ public:
     NullAuthenticator(Connection& connection);
     ~NullAuthenticator();
     void getMechanisms(framing::Array& mechanisms);
-    void start(const std::string& mechanism, const std::string& response);
+    void start(const std::string& mechanism, const std::string* response);
     void step(const std::string&) {}
     std::auto_ptr<SecurityLayer> getSecurityLayer(uint16_t maxFrameSize);
 };
@@ -57,7 +57,7 @@ public:
     SspiAuthenticator(Connection& connection);
     ~SspiAuthenticator();
     void getMechanisms(framing::Array& mechanisms);
-    void start(const std::string& mechanism, const std::string& response);
+    void start(const std::string& mechanism, const std::string* response);
     void step(const std::string& response);
     std::auto_ptr<SecurityLayer> getSecurityLayer(uint16_t maxFrameSize);
 };
@@ -96,12 +96,12 @@ void NullAuthenticator::getMechanisms(Array& mechanisms)
     mechanisms.add(boost::shared_ptr<FieldValue>(new Str16Value("PLAIN")));
 }
 
-void NullAuthenticator::start(const string& mechanism, const string& response)
+void NullAuthenticator::start(const string& mechanism, const string* response)
 {
     QPID_LOG(warning, "SASL: No Authentication Performed");
     if (mechanism == "PLAIN") { // Old behavior
-        if (response.size() > 0 && response[0] == (char) 0) {
-            string temp = response.substr(1);
+        if (response && response->size() > 0 && (*response).c_str()[0] == (char) 0) {
+            string temp = response->substr(1);
             string::size_type i = temp.find((char)0);
             string uid = temp.substr(0, i);
             string pwd = temp.substr(i + 1);
@@ -139,7 +139,7 @@ void SspiAuthenticator::getMechanisms(Array& mechanisms)
     QPID_LOG(info, "SASL: Mechanism list: ANONYMOUS PLAIN");
 }
 
-void SspiAuthenticator::start(const string& mechanism, const string& response)
+void SspiAuthenticator::start(const string& mechanism, const string* response)
 {
     QPID_LOG(info, "SASL: Starting authentication with mechanism: " << mechanism);
     if (mechanism == "ANONYMOUS") {
@@ -152,14 +152,14 @@ void SspiAuthenticator::start(const string& mechanism, const string& response)
 
     // PLAIN's response is composed of 3 strings separated by 0 bytes:
     // authorization id, authentication id (user), clear-text password.
-    if (response.size() == 0)
+    if (!response || response->size() == 0)
         throw ConnectionForcedException("Authentication failed");
 
-    string::size_type i = response.find((char)0);
-    string auth = response.substr(0, i);
-    string::size_type j = response.find((char)0, i+1);
-    string uid = response.substr(i+1, j-1);
-    string pwd = response.substr(j+1);
+    string::size_type i = response->find((char)0);
+    string auth = response->substr(0, i);
+    string::size_type j = response->find((char)0, i+1);
+    string uid = response->substr(i+1, j-1);
+    string pwd = response->substr(j+1);
     string dot(".");
     int error = 0;
     if (!LogonUser(const_cast<char*>(uid.c_str()),
diff --git a/cpp/src/qpid/client/ConnectionHandler.cpp b/cpp/src/qpid/client/ConnectionHandler.cpp
index 801fe38051..ab0d8e0700 100644
--- a/cpp/src/qpid/client/ConnectionHandler.cpp
+++ b/cpp/src/qpid/client/ConnectionHandler.cpp
@@ -254,8 +254,18 @@ void ConnectionHandler::start(const FieldTable& /*serverProps*/, const Array& me
     }
 
     if (sasl.get()) {
-        string response = sasl->start(join(mechlist), getSecuritySettings ? getSecuritySettings() : 0);
-        proxy.startOk(properties, sasl->getMechanism(), response, locale);
+        string response;
+        if (sasl->start(join(mechlist), response, getSecuritySettings ? getSecuritySettings() : 0)) {
+            proxy.startOk(properties, sasl->getMechanism(), response, locale);
+        } else {
+            //response was null
+            ConnectionStartOkBody body;
+            body.setClientProperties(properties);
+            body.setMechanism(sasl->getMechanism());
+            //Don't set response, as none was given
+            body.setLocale(locale);
+            proxy.send(body);
+        }
     } else {
         //TODO: verify that desired mechanism and locale are supported
         string response = ((char)0) + username + ((char)0) + password;
diff --git a/cpp/src/qpid/client/windows/SaslFactory.cpp b/cpp/src/qpid/client/windows/SaslFactory.cpp
index d1ae762f1b..53d825771b 100644
--- a/cpp/src/qpid/client/windows/SaslFactory.cpp
+++ b/cpp/src/qpid/client/windows/SaslFactory.cpp
@@ -71,7 +71,7 @@ class WindowsSasl : public Sasl
   public:
     WindowsSasl( const std::string &, const std::string &, const std::string &, const std::string &, int, int );
     ~WindowsSasl();
-    std::string start(const std::string& mechanisms, const SecuritySettings* externalSettings);
+    bool start(const std::string& mechanisms, std::string& response, const SecuritySettings* externalSettings);
     std::string step(const std::string& challenge);
     std::string getMechanism();
     std::string getUserId();
@@ -121,8 +121,8 @@ WindowsSasl::~WindowsSasl()
 {
 }
 
-std::string WindowsSasl::start(const std::string& mechanisms,
-                               const SecuritySettings* /*externalSettings*/)
+bool WindowsSasl::start(const std::string& mechanisms, std::string& response,
+                        const SecuritySettings* /*externalSettings*/)
 {
     QPID_LOG(debug, "WindowsSasl::start(" << mechanisms << ")");
 
@@ -142,15 +142,15 @@ std::string WindowsSasl::start(const std::string& mechanisms,
     if (!haveAnon && !havePlain)
         throw InternalErrorException(QPID_MSG("Sasl error: no common mechanism"));
 
-    std::string resp = "";
     if (havePlain) {
         mechanism = PLAIN;
-        resp = ((char)0) + settings.username + ((char)0) + settings.password;
+        response = ((char)0) + settings.username + ((char)0) + settings.password;
     }
     else {
         mechanism = ANONYMOUS;
+        response = "";
     }
-    return resp;
+    return true;
 }
 
 std::string WindowsSasl::step(const std::string& /*challenge*/)
diff --git a/cpp/src/tests/ssl_test b/cpp/src/tests/ssl_test
index cbf75eb237..2a56c0b80e 100755
--- a/cpp/src/tests/ssl_test
+++ b/cpp/src/tests/ssl_test
@@ -47,9 +47,13 @@ delete_certs() {
     fi
 }
 
-COMMON_OPTS="--daemon --no-data-dir --no-module-dir --auth no --config $CONFIG --load-module $SSL_LIB --ssl-cert-db $CERT_DIR --ssl-cert-password-file $CERT_PW_FILE --ssl-cert-name $TEST_HOSTNAME --require-encryption"
+COMMON_OPTS="--daemon --no-data-dir --no-module-dir --config $CONFIG --load-module $SSL_LIB --ssl-cert-db $CERT_DIR --ssl-cert-password-file $CERT_PW_FILE --ssl-cert-name $TEST_HOSTNAME --require-encryption"
 start_broker() { # $1 = extra opts
-    ../qpidd --transport ssl --port 0 --ssl-port 0 $COMMON_OPTS $1;
+    ../qpidd --transport ssl --port 0 --ssl-port 0 $COMMON_OPTS --auth no $1;
+}
+
+start_authenticating_broker() {
+    ../qpidd --transport ssl --port 0 --ssl-port 0 $COMMON_OPTS --ssl-sasl-no-dict --ssl-require-client-authentication --auth yes;
 }
 
 stop_brokers() {
@@ -93,7 +97,7 @@ test "$MSG" = "hello" || { echo "receive failed '$MSG' != 'hello'"; exit 1; }
 
 #### Client Authentication tests
 
-PORT2=`start_broker --ssl-require-client-authentication`  || error "Could not start broker"
+PORT2=`start_authenticating_broker`  || error "Could not start broker"
 echo "Running SSL client authentication test on port $PORT2"
 URL=amqp:ssl:$TEST_HOSTNAME:$PORT2
 
@@ -121,7 +125,7 @@ pick_port() {
     echo $PICK
 }
 ssl_cluster_broker() {		# $1 = port
-    ../qpidd $COMMON_OPTS --load-module  $CLUSTER_LIB --cluster-name ssl_test.$HOSTNAME.$$ --cluster-url amqp:ssl:$TEST_HOSTNAME:$1 --port 0 --ssl-port $1 --transport ssl > /dev/null
+    ../qpidd $COMMON_OPTS --auth no --load-module  $CLUSTER_LIB --cluster-name ssl_test.$HOSTNAME.$$ --cluster-url amqp:ssl:$TEST_HOSTNAME:$1 --port 0 --ssl-port $1 --transport ssl > /dev/null
     # Wait for broker to be ready
     qpid-ping -Pssl -b $TEST_HOSTNAME -qp $1 || { echo "Cannot connect to broker on $1"; exit 1; }
     echo "Running SSL cluster broker on port $1"