Problem: duplicated code & inconsistent behaviour between

mechanisms

Solution: uniformly require a ZAP domain to be set to activate ZAP
handling, clarify comment on Stonehouse pattern

src/curve_server.cpp

@@ -390,14 +390,17 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
     rc = crypto_box_beforenm (cn_precom, cn_client, cn_secret);
     zmq_assert (rc == 0);
 
-    //  Use ZAP protocol (RFC 27) to authenticate the user.
-    //  Note that rc will be -1 only if ZAP is not set up (Stonehouse pattern -
-    //  encryption without authentication), but if it was requested and it does
-    //  not work properly the program will abort.
     if (zap_required ()) {
+        //  Use ZAP protocol (RFC 27) to authenticate the user.
         rc = session->zap_connect ();
         if (rc == 0) {
             send_zap_request (client_key);
+            state = waiting_for_zap_reply;
+
+            //  TODO actually, it is quite unlikely that we can read the ZAP 
+            //  reply already, but removing this has some strange side-effect
+            //  (probably because the pipe's in_active flag is true until a read 
+            //  is attempted)
             rc = receive_and_process_zap_reply ();
             if (rc == -1)
                 return -1;
@@ -406,8 +409,10 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
               session->get_endpoint (), EFAULT);
             return -1;
         }
-    } else
+    } else {
+        //  This supports the Stonehouse pattern (encryption without authentication).
         state = sending_ready;
+    }
 
     return parse_metadata (initiate_plaintext + crypto_box_ZEROBYTES + 128,
                            clen - crypto_box_ZEROBYTES - 128);
@@ -478,10 +483,4 @@ void zmq::curve_server_t::send_zap_request (const uint8_t *key)
     zap_client_t::send_zap_request ("CURVE", 5, key, crypto_box_PUBLICKEYBYTES);
 }
 
-bool zmq::curve_server_t::zap_required () const
-{
-    // TODO: make this explicit by a separate option zap_required (uniformly across all mechanisms)
-    return !options.zap_domain.empty();
-}
-
 #endif

src/curve_server.hpp

@@ -82,7 +82,6 @@ namespace zmq
         int produce_error (msg_t *msg_) const;
 
         void send_zap_request (const uint8_t *key);
-        bool zap_required () const;
     };
 #ifdef _MSC_VER
 #pragma warning (pop)

src/mechanism_base.cpp

@@ -63,3 +63,8 @@ void zmq::mechanism_base_t::handle_error_reason (const char *error_reason,
           session->get_endpoint (), (error_reason[0] - '0') * 100);
     }
 }
+
+bool zmq::mechanism_base_t::zap_required() const
+{
+    return !options.zap_domain.empty ();
+}

src/mechanism_base.hpp

@@ -45,6 +45,8 @@ class mechanism_base_t : public mechanism_t
     int check_basic_command_structure (msg_t *msg_);
 
     void handle_error_reason (const char *error_reason, size_t error_reason_len);
+
+    bool zap_required() const;
 };
 }
 

src/null_mechanism.cpp

@@ -79,9 +79,15 @@ int zmq::null_mechanism_t::next_handshake_command (msg_t *msg_)
         }
         send_zap_request ();
         zap_request_sent = true;
+
+        //  TODO actually, it is quite unlikely that we can read the ZAP 
+        //  reply already, but removing this has some strange side-effect
+        //  (probably because the pipe's in_active flag is true until a read 
+        //  is attempted)
         rc = receive_and_process_zap_reply ();
-        if (rc == -1 || rc == 1)
+        if (rc != 0)
             return -1;
+
         zap_reply_received = true;
     }
 
@@ -208,13 +214,6 @@ zmq::mechanism_t::status_t zmq::null_mechanism_t::status () const
         return handshaking;
 }
 
-bool zmq::null_mechanism_t::zap_required() const
-{
-    //  NULL mechanism only uses ZAP if there's a domain defined
-    //  This prevents ZAP requests on naive sockets
-    return options.zap_domain.size() > 0;
-}
-
 void zmq::null_mechanism_t::send_zap_request ()
 {
     zap_client_t::send_zap_request ("NULL", 4, NULL, NULL, 0);

src/null_mechanism.hpp

@@ -55,8 +55,6 @@ namespace zmq
         virtual int zap_msg_available ();
         virtual status_t status () const;
 
-        bool zap_required () const;
-
     private:
 
         bool ready_command_sent;

src/plain_server.cpp

@@ -44,6 +44,10 @@ zmq::plain_server_t::plain_server_t (session_base_t *session_,
     zap_client_common_handshake_t (
       session_, peer_address_, options_, sending_welcome)
 {
+    //  Note that there is no point to PLAIN if ZAP is not set up to handle the
+    //  username and password, so if ZAP is not configured it is considered a
+    //  failure.
+    zmq_assert (zap_required());
 }
 
 zmq::plain_server_t::~plain_server_t ()
@@ -173,17 +177,20 @@ int zmq::plain_server_t::process_hello (msg_t *msg_)
     }
 
     //  Use ZAP protocol (RFC 27) to authenticate the user.
-    //  Note that there is no point to PLAIN if ZAP is not set up to handle the
-    //  username and password, so if ZAP is not configured it is considered a
-    //  failure.
     rc = session->zap_connect ();
-    if (rc != 0)
-    {
-        session->get_socket()->event_handshake_failed_no_detail(
-            session->get_endpoint(), EFAULT);
+    if (rc != 0) {
+        session->get_socket ()->event_handshake_failed_no_detail (
+          session->get_endpoint (), EFAULT);
         return -1;
     }
+
     send_zap_request (username, password);
+    state = waiting_for_zap_reply;
+
+    //  TODO actually, it is quite unlikely that we can read the ZAP 
+    //  reply already, but removing this has some strange side-effect
+    //  (probably because the pipe's in_active flag is true until a read 
+    //  is attempted)
     return receive_and_process_zap_reply () == -1 ? -1 : 0;
 }
 

src/session_base.cpp

@@ -286,8 +286,10 @@ void zmq::session_base_t::read_activated (pipe_t *pipe_)
 
     if (likely (pipe_ == pipe))
         engine->restart_output ();
-    else
+    else {
+        // i.e. pipe_ == zap_pipe
         engine->zap_msg_available ();
+    }
 }
 
 void zmq::session_base_t::write_activated (pipe_t *pipe_)

src/zap_client.cpp

@@ -299,10 +299,7 @@ void zap_client_common_handshake_t::handle_zap_status_code ()
 
 int zap_client_common_handshake_t::receive_and_process_zap_reply ()
 {
-    int rc = zap_client_t::receive_and_process_zap_reply ();
-    if (rc == 1)
-        // TODO shouldn't the state already be this?
-        state = waiting_for_zap_reply;
-    return rc;
+    zmq_assert (state == waiting_for_zap_reply);
+    return zap_client_t::receive_and_process_zap_reply ();
 }
 }

tests/test_security_plain.cpp

@@ -109,6 +109,9 @@ int main (void)
     void *server = zmq_socket (ctx, ZMQ_DEALER);
     assert (server);
     int rc = zmq_setsockopt (server, ZMQ_IDENTITY, "IDENT", 6);
+    const char domain[] = "test";
+    assert (rc == 0);
+    rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, domain, strlen (domain));
     assert (rc == 0);
     int as_server = 1;
     rc = zmq_setsockopt (server, ZMQ_PLAIN_SERVER, &as_server, sizeof (int));
@@ -141,6 +144,8 @@ int main (void)
     client = zmq_socket (ctx, ZMQ_DEALER);
     assert (client);
     as_server = 1;
+    rc = zmq_setsockopt(client, ZMQ_ZAP_DOMAIN, domain, strlen (domain));
+    assert (rc == 0);
     rc = zmq_setsockopt (client, ZMQ_PLAIN_SERVER, &as_server, sizeof (int));
     assert (rc == 0);
     rc = zmq_connect (client, my_endpoint);