Problem: tests without ZAP handler are failing

Solution: emit events as expected by tests, and refuse connections when
ZAP is required but no handler started

Addresses #2711 partially

src/curve_server.cpp

@@ -394,12 +394,18 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
     //  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 (zap_required ()) {
-    if (rc == 0) {
+        rc = session->zap_connect ();
-        send_zap_request (client_key);
+        if (rc == 0) {
-        rc = receive_and_process_zap_reply ();
+            send_zap_request (client_key);
-        if (rc == -1)
+            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
         state = sending_ready;
 
@@ -472,4 +478,10 @@ 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,6 +82,7 @@ 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/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,14 +63,23 @@ 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 ();
+        rc = receive_and_process_zap_reply ();
         if (rc == -1 || rc == 1)
             return -1;
         zap_reply_received = true;
@@ -205,6 +208,13 @@ 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,13 +55,14 @@ namespace zmq
         virtual int zap_msg_available ();
         virtual status_t status () const;
 
+        bool zap_required () const;
+
     private:
 
         bool ready_command_sent;
         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

@@ -178,7 +178,11 @@ int zmq::plain_server_t::process_hello (msg_t *msg_)
     //  failure.
     rc = session->zap_connect ();
     if (rc != 0)
+    {
+        session->get_socket()->event_handshake_failed_no_detail(
+            session->get_endpoint(), EFAULT);
         return -1;
+    }
     send_zap_request (username, password);
     return receive_and_process_zap_reply () == -1 ? -1 : 0;
 }

tests/test_security_zap.cpp

@@ -59,15 +59,16 @@ static void zap_handler_too_many_parts (void *ctx)
     zap_handler_generic (ctx, zap_too_many_parts);
 }
 
-void test_zap_unsuccessful (void *ctx,
+int expect_new_client_bounce_fail_and_count_monitor_events (
-                            char *my_endpoint,
+  void *ctx,
-                            void *server,
+  char *my_endpoint,
-                            void *server_mon,
+  void *server,
-                            int expected_event,
+  socket_config_fn socket_config_,
-                            int expected_err,
+  void *socket_config_data_,
-                            socket_config_fn socket_config_,
+  void **client_mon,
-                            void *socket_config_data_,
+  void *server_mon,
-                            void **client_mon = NULL)
+  int expected_event,
+  int expected_err)
 {
     expect_new_client_bounce_fail (ctx, my_endpoint, server, socket_config_,
                                    socket_config_data_, client_mon);
@@ -78,12 +79,54 @@ void test_zap_unsuccessful (void *ctx,
       expect_monitor_event_multiple (server_mon, expected_event, expected_err);
 #endif
 
-    //  there may be more than one ZAP request due to repeated attempts by the 
+    return events_received;
+}
+
+void test_zap_unsuccessful (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);
+
+    //  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 +190,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,18 +309,20 @@ 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 (
+    setup_context_and_server_side (&ctx, &handler, &zap_thread, &server,
-      &ctx, &handler, &zap_thread, &server, &server_mon, my_endpoint,
+                                   &server_mon, my_endpoint, NULL,
-      NULL, server_socket_config_);
+                                   server_socket_config_);
-    test_zap_unsuccessful (ctx, my_endpoint, server, server_mon,
+    test_zap_unsuccessful_no_handler (
+      ctx, my_endpoint, server, server_mon,
 #ifdef ZMQ_BUILD_DRAFT_API
-                           ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL, EFAULT,
+      ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL, EFAULT,
 #else
-                           0, 0,
+      0, 0,
 #endif
-                           client_socket_config_, client_socket_config_data_);
+      client_socket_config_, client_socket_config_data_);
     shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
                                       handler);
 }

tests/testutil_security.hpp

@@ -49,7 +49,8 @@ void socket_config_null_server (void *server, void *server_secret)
 {
     LIBZMQ_UNUSED (server_secret);
 
-    int rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, test_zap_domain, 7);
+    int rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, test_zap_domain,
+                             strlen (test_zap_domain));
     assert (rc == 0);
 }
 
@@ -61,7 +62,8 @@ void socket_config_plain_client (void *server, void *server_secret)
 {
     LIBZMQ_UNUSED (server_secret);
 
-    int rc = zmq_setsockopt (server, ZMQ_PLAIN_PASSWORD, test_plain_password, 8);
+    int rc =
+      zmq_setsockopt (server, ZMQ_PLAIN_PASSWORD, test_plain_password, 8);
     assert (rc == 0);
 
     rc = zmq_setsockopt (server, ZMQ_PLAIN_USERNAME, test_plain_username, 8);
@@ -73,8 +75,13 @@ void socket_config_plain_server (void *server, void *server_secret)
     LIBZMQ_UNUSED (server_secret);
 
     int as_server = 1;
-    int rc = zmq_setsockopt (server, ZMQ_PLAIN_SERVER, &as_server, sizeof (int));
+    int rc =
+      zmq_setsockopt (server, ZMQ_PLAIN_SERVER, &as_server, sizeof (int));
     assert (rc == 0);
+
+    rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, test_zap_domain,
+                         strlen (test_zap_domain));
+    assert(rc == 0);
 }
 
 //  CURVE specific functions
@@ -97,11 +104,16 @@ void setup_testutil_security_curve ()
 void socket_config_curve_server (void *server, void *server_secret)
 {
     int as_server = 1;
-    int rc = zmq_setsockopt (server, ZMQ_CURVE_SERVER, &as_server, sizeof (int));
+    int rc =
+      zmq_setsockopt (server, ZMQ_CURVE_SERVER, &as_server, sizeof (int));
     assert (rc == 0);
 
     rc = zmq_setsockopt (server, ZMQ_CURVE_SECRETKEY, server_secret, 41);
     assert (rc == 0);
+
+    rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, test_zap_domain,
+                         strlen (test_zap_domain));
+    assert(rc == 0);
 }
 
 struct curve_client_data_t
@@ -133,15 +145,15 @@ void socket_config_curve_client (void *client, void *data)
 
 enum zap_protocol_t
 {
-  zap_ok,
+    zap_ok,
-  // ZAP-compliant non-standard cases
+    // ZAP-compliant non-standard cases
-  zap_status_temporary_failure,
+    zap_status_temporary_failure,
-  zap_status_internal_error,
+    zap_status_internal_error,
-  // ZAP protocol errors
+    // ZAP protocol errors
-  zap_wrong_version,
+    zap_wrong_version,
-  zap_wrong_request_id,
+    zap_wrong_request_id,
-  zap_status_invalid,
+    zap_status_invalid,
-  zap_too_many_parts
+    zap_too_many_parts
 };
 
 void *zap_requests_handled;
@@ -165,7 +177,8 @@ void zap_handler_generic (void *ctx,
     assert (rc == 2);
 
     zmq_pollitem_t items[] = {
-      {control, 0, ZMQ_POLLIN, 0}, {handler, 0, ZMQ_POLLIN, 0},
+      {control, 0, ZMQ_POLLIN, 0},
+      {handler, 0, ZMQ_POLLIN, 0},
     };
 
     //  Process ZAP requests forever
@@ -200,15 +213,13 @@ void zap_handler_generic (void *ctx,
 
             authentication_succeeded =
               streq (client_key_text, valid_client_public);
-        }
+        } else if (streq (mechanism, "PLAIN")) {
-        else if (streq(mechanism, "PLAIN"))
+            char client_username [32];
-        {
-            char client_username[32];
             int size = zmq_recv (handler, client_username, 32, 0);
             assert (size > 0);
             client_username [size] = 0;
-            
+
-            char client_password[32];
+            char client_password [32];
             size = zmq_recv (handler, client_password, 32, 0);
             assert (size > 0);
             client_password [size] = 0;
@@ -216,13 +227,9 @@ void zap_handler_generic (void *ctx,
             authentication_succeeded =
               streq (test_plain_username, client_username)
               && streq (test_plain_password, client_password);
-        }
+        } else if (streq (mechanism, "NULL")) {
-        else if (streq(mechanism, "NULL"))
-        {
             authentication_succeeded = true;
-        }
+        } else {
-        else
-        {
             fprintf (stderr, "Unsupported mechanism: %s\n", mechanism);
             assert (false);
         }
@@ -352,7 +359,7 @@ void setup_context_and_server_side (
 
     socket_config_ (*server, socket_config_data_);
 
-    rc = zmq_setsockopt (*server, ZMQ_IDENTITY, identity, strlen(identity));
+    rc = zmq_setsockopt (*server, ZMQ_IDENTITY, identity, strlen (identity));
     assert (rc == 0);
 
     rc = zmq_bind (*server, "tcp://127.0.0.1:*");
@@ -367,21 +374,20 @@ void setup_context_and_server_side (
                                     server_monitor_endpoint);
 }
 
-void shutdown_context_and_server_side (void *ctx,
+void shutdown_context_and_server_side (
-                                       void *zap_thread,
+  void *ctx, void *zap_thread, void *server, void *server_mon, void *handler)
-                                       void *server,
-                                       void *server_mon,
-                                       void *handler)
 {
-    int rc = s_send (handler, "STOP");
+    if (zap_thread) {
-    assert (rc == 4);
+        int rc = s_send (handler, "STOP");
-    char *buf = s_recv (handler);
+        assert (rc == 4);
-    assert (buf);
+        char *buf = s_recv (handler);
-    assert (streq (buf, "STOPPED"));
+        assert (buf);
-    free (buf);
+        assert (streq (buf, "STOPPED"));
-    rc = zmq_unbind (handler, "inproc://handler-control");
+        free (buf);
-    assert (rc == 0);
+        rc = zmq_unbind (handler, "inproc://handler-control");
-    close_zero_linger (handler);
+        assert (rc == 0);
+    }
+    close_zero_linger(handler);
 
 #ifdef ZMQ_BUILD_DRAFT_API
     close_zero_linger (server_mon);
@@ -390,9 +396,9 @@ void shutdown_context_and_server_side (void *ctx,
 
     //  Wait until ZAP handler terminates
     if (zap_thread)
-      zmq_threadclose (zap_thread);
+        zmq_threadclose (zap_thread);
 
-    rc = zmq_ctx_term (ctx);
+    int rc = zmq_ctx_term (ctx);
     assert (rc == 0);
 
     zmq_atomic_counter_destroy (&zap_requests_handled);
@@ -412,8 +418,7 @@ void *create_and_connect_client (void *ctx,
     int rc = zmq_connect (client, my_endpoint);
     assert (rc == 0);
 
-    if (client_mon)
+    if (client_mon) {
-    {
         setup_handshake_socket_monitor (ctx, client, client_mon,
                                         "inproc://client-monitor");
     }
@@ -440,8 +445,10 @@ void expect_new_client_bounce_fail (void *ctx,
 //  by reference, if not null, and event number by value. Returns -1
 //  in case of error.
 
-static int
+static int get_monitor_event_internal (void *monitor,
-get_monitor_event_internal (void *monitor, int *value, char **address, int recv_flag)
+                                       int *value,
+                                       char **address,
+                                       int recv_flag)
 {
     //  First frame in message contains event number and value
     zmq_msg_t msg;
@@ -511,8 +518,7 @@ int get_monitor_event (void *monitor, int *value, char **address)
 void expect_monitor_event (void *monitor, int expected_event)
 {
     int event = get_monitor_event (monitor, NULL, NULL);
-    if (event != expected_event)
+    if (event != expected_event) {
-    {
         fprintf (stderr, "Expected monitor event %x but received %x\n",
                  expected_event, event);
         assert (event == expected_event);
@@ -526,19 +532,18 @@ void print_unexpected_event (int event,
                              int expected_event,
                              int expected_err)
 {
-  fprintf(
+    fprintf (stderr,
-    stderr,
+             "Unexpected event: 0x%x, value = %i/0x%x (expected: 0x%x, value "
-    "Unexpected event: 0x%x, value = %i/0x%x (expected: 0x%x, value "
+             "= %i/0x%x)\n",
-    "= %i/0x%x)\n",
+             event, err, err, expected_event, expected_err, expected_err);
-    event, err, err, expected_event, expected_err, expected_err);
 }
 
-//  expects that one or more occurrences of the expected event are received 
+//  expects that one or more occurrences of the expected event are received
 //  via the specified socket monitor
 //  returns the number of occurrences of the expected event
 //  interrupts, if a ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL with EPIPE, ECONNRESET
 //  or ECONNABORTED occurs; in this case, 0 is returned
-//  this should be investigated further, see 
+//  this should be investigated further, see
 //  https://github.com/zeromq/libzmq/issues/2644
 int expect_monitor_event_multiple (void *server_mon,
                                    int expected_event,
@@ -554,14 +559,15 @@ int expect_monitor_event_multiple (void *server_mon,
     int err;
     while (
       (event = get_monitor_event_with_timeout (server_mon, &err, NULL, timeout))
-      != -1 || !count_of_expected_events) {
+        != -1
+      || !count_of_expected_events) {
         if (event == -1) {
             if (optional)
                 break;
             wait_time += timeout;
             fprintf (stderr,
                      "Still waiting for first event after %ims (expected event "
-                     "%x (value %i/%x))\n",
+                     "%x (value %i/0x%x))\n",
                      wait_time, expected_event, expected_err, expected_err);
             continue;
         }
@@ -569,12 +575,12 @@ int expect_monitor_event_multiple (void *server_mon,
         // ECONNRESET can happen on very slow machines, when the engine writes
         // to the peer and then tries to read the socket before the peer reads
         // ECONNABORTED happens when a client aborts a connection via RST/timeout
-        if (event == ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL &&
+        if (event == ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL
-                (err == EPIPE || err == ECONNRESET || err == ECONNABORTED)) {
+            && (err == EPIPE || err == ECONNRESET || err == ECONNABORTED)) {
-            fprintf (
+            fprintf (stderr,
-              stderr,
+                     "Ignored event (skipping any further events): %x (err = "
-              "Ignored event (skipping any further events): %x (err = %i)\n",
+                     "%i == %s)\n",
-              event, err);
+                     event, err, zmq_strerror (err));
             client_closed_connection = 1;
             break;
         }
@@ -585,7 +591,8 @@ int expect_monitor_event_multiple (void *server_mon,
         }
         ++count_of_expected_events;
     }
-    assert (optional || count_of_expected_events > 0 || client_closed_connection);
+    assert (optional || count_of_expected_events > 0
+            || client_closed_connection);
 
     return count_of_expected_events;
 }
@@ -604,8 +611,8 @@ int expect_monitor_event_multiple (void *server_mon,
                     || err == ECONNABORTED)) {                                 \
                 fprintf (stderr,                                               \
                          "Ignored event (skipping any further events): %x "    \
-                         "(err = %i)\n",                                       \
+                         "(err = %i == %s)\n",                                 \
-                         event, err);                                          \
+                         event, err, zmq_strerror (err));                      \
                 continue;                                                      \
             }                                                                  \
             ++event_count;                                                     \