Problem: missing tests and inconsistent behaviour for ZAP corner cases

src/curve_server.cpp

@@ -390,18 +390,29 @@ 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.
-    rc = session->zap_connect ();
-    if (rc == 0) {
-        send_zap_request (client_key);
-        rc = receive_and_process_zap_reply ();
-        if (rc == -1)
+    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;
+        } else {
+            session->get_socket ()->event_handshake_failed_no_detail (
+              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);

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

@@ -48,15 +48,9 @@ zmq::null_mechanism_t::null_mechanism_t (session_base_t *session_,
     error_command_sent (false),
     ready_command_received (false),
     error_command_received (false),
-    zap_connected (false),
     zap_request_sent (false),
     zap_reply_received (false)
 {
-    //  NULL mechanism only uses ZAP if there's a domain defined
-    //  This prevents ZAP requests on naive sockets
-    if (options.zap_domain.size () > 0
-    &&  session->zap_connect () == 0)
-        zap_connected = true;
 }
 
 zmq::null_mechanism_t::~null_mechanism_t ()
@@ -69,16 +63,31 @@ int zmq::null_mechanism_t::next_handshake_command (msg_t *msg_)
         errno = EAGAIN;
         return -1;
     }
-    if (zap_connected && !zap_reply_received) {
+
+    if (zap_required() && !zap_reply_received) {
         if (zap_request_sent) {
             errno = EAGAIN;
             return -1;
         }
+        int rc = session->zap_connect();
+        if (rc == -1)
+        {
+            session->get_socket()->event_handshake_failed_no_detail (
+                session->get_endpoint(),
+                EFAULT);
+            return -1;
+        }
         send_zap_request ();
         zap_request_sent = true;
-        int rc = receive_and_process_zap_reply ();
-        if (rc == -1 || rc == 1)
+
+        //  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 != 0)
             return -1;
+
         zap_reply_received = true;
     }
 

src/null_mechanism.hpp

@@ -61,7 +61,6 @@ namespace zmq
         bool error_command_sent;
         bool ready_command_received;
         bool error_command_received;
-        bool zap_connected;
         bool zap_request_sent;
         bool zap_reply_received;
 

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,13 +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)
+    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);

tests/test_security_zap.cpp

@@ -59,6 +59,44 @@ static void zap_handler_too_many_parts (void *ctx)
     zap_handler_generic (ctx, zap_too_many_parts);
 }
 
+static void zap_handler_disconnect (void *ctx)
+{
+    zap_handler_generic (ctx, zap_disconnect);
+}
+
+static void zap_handler_do_not_recv (void *ctx)
+{
+    zap_handler_generic (ctx, zap_do_not_recv);
+}
+
+static void zap_handler_do_not_send (void *ctx)
+{
+    zap_handler_generic (ctx, zap_do_not_send);
+}
+
+int expect_new_client_bounce_fail_and_count_monitor_events (
+  void *ctx,
+  char *my_endpoint,
+  void *server,
+  socket_config_fn socket_config_,
+  void *socket_config_data_,
+  void **client_mon,
+  void *server_mon,
+  int expected_event,
+  int expected_err)
+{
+    expect_new_client_bounce_fail (ctx, my_endpoint, server, socket_config_,
+                                   socket_config_data_, client_mon);
+
+    int events_received = 0;
+#ifdef ZMQ_BUILD_DRAFT_API
+    events_received =
+      expect_monitor_event_multiple (server_mon, expected_event, expected_err);
+#endif
+
+    return events_received;
+}
+
 void test_zap_unsuccessful (void *ctx,
                             char *my_endpoint,
                             void *server,
@@ -69,21 +107,41 @@ void test_zap_unsuccessful (void *ctx,
                             void *socket_config_data_,
                             void **client_mon = NULL)
 {
-    expect_new_client_bounce_fail (ctx, my_endpoint, server, socket_config_,
-                                   socket_config_data_, client_mon);
-
-    int events_received = 0;
-#ifdef ZMQ_BUILD_DRAFT_API
-    events_received =
-      expect_monitor_event_multiple (server_mon, expected_event, expected_err);
-#endif
+    int events_received =
+      expect_new_client_bounce_fail_and_count_monitor_events (
+        ctx, my_endpoint, server, socket_config_, socket_config_data_,
+        client_mon, server_mon, expected_event, expected_err);
 
-    //  there may be more than one ZAP request due to repeated attempts by the 
+    //  there may be more than one ZAP request due to repeated attempts by the
     //  client (actually only in case if ZAP status code 300)
     assert (events_received == 0
             || 1 <= zmq_atomic_counter_value (zap_requests_handled));
 }
 
+void test_zap_unsuccessful_no_handler (void *ctx,
+                                       char *my_endpoint,
+                                       void *server,
+                                       void *server_mon,
+                                       int expected_event,
+                                       int expected_err,
+                                       socket_config_fn socket_config_,
+                                       void *socket_config_data_,
+                                       void **client_mon = NULL)
+{
+    int events_received =
+      expect_new_client_bounce_fail_and_count_monitor_events (
+        ctx, my_endpoint, server, socket_config_, socket_config_data_,
+        client_mon, server_mon, expected_event, expected_err);
+
+#ifdef ZMQ_BUILD_DRAFT_API
+    //  there may be more than one ZAP request due to repeated attempts by the
+    //  client
+    assert (events_received > 0);
+#else
+    LIBZMQ_UNUSED (events_received);
+#endif
+}
+
 void test_zap_protocol_error (void *ctx,
                               char *my_endpoint,
                               void *server,
@@ -147,7 +205,7 @@ void test_zap_unsuccessful_status_500 (void *ctx,
     int events_received = 0;
     events_received = expect_monitor_event_multiple (
       client_mon, ZMQ_EVENT_HANDSHAKE_FAILED_AUTH, 500, true);
-    
+
     // this should actually be events_received == 1, but this is not always
     // true, see https://github.com/zeromq/libzmq/issues/2705
     assert (events_received <= 1);
@@ -266,6 +324,72 @@ void test_zap_errors (socket_config_fn server_socket_config_,
                                       client_socket_config_data_);
     shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
                                       handler);
+
+    //  no ZAP handler
+    fprintf (stderr, "test_zap_unsuccessful no ZAP handler started\n");
+    setup_context_and_server_side (&ctx, &handler, &zap_thread, &server,
+                                   &server_mon, my_endpoint, NULL,
+                                   server_socket_config_);
+    test_zap_unsuccessful_no_handler (
+      ctx, my_endpoint, server, server_mon,
+#ifdef ZMQ_BUILD_DRAFT_API
+      ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL, EFAULT,
+#else
+      0, 0,
+#endif
+      client_socket_config_, client_socket_config_data_);
+    shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
+                                      handler);
+
+    //  ZAP handler disconnecting on first message
+    fprintf(stderr, "test_zap_unsuccessful ZAP handler disconnects\n");
+    setup_context_and_server_side(&ctx, &handler, &zap_thread, &server,
+        &server_mon, my_endpoint, &zap_handler_disconnect,
+        server_socket_config_);
+    test_zap_unsuccessful_no_handler (
+      ctx, my_endpoint, server, server_mon,
+#ifdef ZMQ_BUILD_DRAFT_API
+      ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL, EPIPE,
+#else
+      0, 0,
+#endif
+      client_socket_config_, client_socket_config_data_);
+    shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
+                                      handler, true);
+
+    //  ZAP handler does not read request
+    fprintf (stderr,
+             "test_zap_unsuccessful ZAP handler does not read request\n");
+    setup_context_and_server_side (&ctx, &handler, &zap_thread, &server,
+        &server_mon, my_endpoint, &zap_handler_do_not_recv,
+        server_socket_config_);
+    test_zap_unsuccessful_no_handler (
+      ctx, my_endpoint, server, server_mon,
+#ifdef ZMQ_BUILD_DRAFT_API
+      ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL, EPIPE,
+#else
+      0, 0,
+#endif
+      client_socket_config_, client_socket_config_data_);
+    shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
+                                      handler);
+
+    //  ZAP handler does not send reply
+    fprintf (stderr,
+             "test_zap_unsuccessful ZAP handler does not write reply\n");
+    setup_context_and_server_side (
+      &ctx, &handler, &zap_thread, &server, &server_mon, my_endpoint,
+      &zap_handler_do_not_send, server_socket_config_);
+    test_zap_unsuccessful_no_handler (
+      ctx, my_endpoint, server, server_mon,
+#ifdef ZMQ_BUILD_DRAFT_API
+      ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL, EPIPE,
+#else
+      0, 0,
+#endif
+      client_socket_config_, client_socket_config_data_);
+    shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
+                                      handler);
 }
 
 int main (void)