Problem: ZAP status codes != 200 do not result in an appropriate monitor event (#2665)

* Problem: missing test for status code 300, inadequate assertion for status code 500

Solution: add test, change assertion (currently test fails)

* Problem: gcc compiler error deprecated conversion from string constant

Solution: declare variable as const

* Problem: in case of ZAP handler returning a status code other than 200, no appropriate event is emitted

Solution: immediately emit event after receiving reply from ZAP handler

* Problem: endpoint address is not included in zap-reply monitor event

Solution: added functions to retrieve endpoint address in zmq::i_engine and zmq::session_base_t
removed unused code block in zmq::stream_engine_t::next_handshake_command

* Problem: wrong formatting

Solution: fix formatting

* Problem: test fails because of EPIPE

Solution: add EPIPE/ECONNRESET/ECONNAGAIN handling for more test cases

src/curve_server.cpp

@@ -248,9 +248,7 @@ int zmq::curve_server_t::zap_msg_available ()
     }
     const int rc = receive_and_process_zap_reply ();
     if (rc == 0)
-        state = status_code == "200"
-            ? send_ready
-            : send_error;
+        handle_zap_status_code ();
     return rc;
 }
 
@@ -506,9 +504,7 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
             return -1;
         rc = receive_and_process_zap_reply ();
         if (rc == 0)
-            state = status_code == "200"
-                ? send_ready
-                : send_error;
+            handle_zap_status_code ();
         else
         if (errno == EAGAIN)
             state = expect_zap_reply;
@@ -708,8 +704,11 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
         return close_and_return (msg, -1);
     }
 
-    //  Status code frame
-    if (msg [3].size () != 3) {
+    //  Status code frame, only 200, 300, 400 and 500 are valid status codes
+    char *status_code_data = static_cast <char*> (msg [3].data());
+    if (msg [3].size () != 3 || status_code_data [0] < '2'
+          || status_code_data [0] > '5' || status_code_data [1] != '0'
+          || status_code_data [2] != '0') {
         // CURVE I: ZAP handler sent invalid status code
         current_error_detail = zap;
         errno = EPROTO;
@@ -738,4 +737,33 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
     return 0;
 }
 
+void zmq::curve_server_t::handle_zap_status_code ()
+{
+    //  we can assume here that status_code is a valid ZAP status code, 
+    //  i.e. 200, 300, 400 or 500
+    if (status_code [0] == '2') {
+        state = send_ready;
+    } else {
+        state = send_error;
+
+        int err = 0;
+        switch (status_code [0]) {
+            case '3':
+                err = EAGAIN;
+                break;
+            case '4':
+                err = EACCES;
+                break;
+            case '5':
+                err = EFAULT;
+                break;
+        }
+        //  TODO use event_handshake_failed_zap here? but this is not a ZAP 
+        //  protocol error
+        
+        session->get_socket ()->event_handshake_failed_no_detail (
+          session->get_endpoint (), err);
+    }
+}
+
 #endif

src/curve_server.hpp

@@ -131,6 +131,7 @@ namespace zmq
 
         int send_zap_request (const uint8_t *key);
         int receive_and_process_zap_reply ();
+        void handle_zap_status_code ();
     };
 
 }

src/i_engine.hpp

@@ -58,6 +58,9 @@ namespace zmq
         virtual void restart_output () = 0;
 
         virtual void zap_msg_available () = 0;
+
+        virtual const char * get_endpoint() const = 0;
+
     };
 
 }

src/session_base.cpp

@@ -113,6 +113,11 @@ zmq::session_base_t::session_base_t (class io_thread_t *io_thread_,
 {
 }
 
+const char *zmq::session_base_t::get_endpoint () const
+{
+    return engine->get_endpoint ();
+}
+
 zmq::session_base_t::~session_base_t ()
 {
     zmq_assert (!pipe);

src/session_base.hpp

@@ -97,6 +97,7 @@ namespace zmq
         int write_zap_msg (msg_t *msg_);
 
         socket_base_t *get_socket ();
+        const char *get_endpoint () const;
 
     protected:
 

src/stream_engine.cpp

@@ -784,19 +784,6 @@ int zmq::stream_engine_t::next_handshake_command (msg_t *msg_)
 
         if (rc == 0)
             msg_->set_flags (msg_t::command);
-#ifdef ZMQ_BUILD_DRAFT_API
-        if (mechanism->status () == mechanism_t::error) {
-            int err = errno;
-            if (mechanism->error_detail () == mechanism_t::zmtp)
-                socket->event_handshake_failed_zmtp (endpoint, err);
-            else if (mechanism->error_detail () == mechanism_t::zap)
-                socket->event_handshake_failed_zap (endpoint, err);
-            else if (mechanism->error_detail () == mechanism_t::encryption)
-                socket->event_handshake_failed_encryption (endpoint, err);
-            else
-                socket->event_handshake_failed_no_detail (endpoint, err);
-        }
-#endif
 
         return rc;
     }
@@ -836,6 +823,11 @@ void zmq::stream_engine_t::zap_msg_available ()
         restart_output ();
 }
 
+const char *zmq::stream_engine_t::get_endpoint () const
+{
+    return endpoint.c_str ();
+}
+
 void zmq::stream_engine_t::mechanism_ready ()
 {
     if (options.heartbeat_interval > 0) {

src/stream_engine.hpp

@@ -79,6 +79,7 @@ namespace zmq
         void restart_input ();
         void restart_output ();
         void zap_msg_available ();
+        const char *get_endpoint () const;
 
         //  i_poll_events interface implementation.
         void in_event ();

src/udp_engine.cpp

@@ -289,6 +289,11 @@ void zmq::udp_engine_t::out_event()
        reset_pollout (handle);
 }
 
+const char *zmq::udp_engine_t::get_endpoint () const
+{
+    return "";
+}
+
 void zmq::udp_engine_t::restart_output()
 {
     //  If we don't support send we just drop all messages

src/udp_engine.hpp

@@ -44,8 +44,9 @@ namespace zmq
             void in_event ();
             void out_event ();
 
-        private:
+            const char *get_endpoint () const;
 
+        private:
             int resolve_raw_address (char *addr_, size_t length_);
             void sockaddr_to_msg (zmq::msg_t *msg, sockaddr_in* addr);
 

tests/test_security_curve.cpp

@@ -141,6 +141,7 @@ enum zap_protocol_t
 {
   zap_ok,
   // ZAP-compliant non-standard cases
+  zap_status_temporary_failure,
   zap_status_internal_error,
   // ZAP protocol errors
   zap_wrong_version,
@@ -210,11 +211,21 @@ static void zap_handler_generic (void *ctx, zap_protocol_t zap_protocol)
                                : sequence);
 
         if (streq (client_key_text, valid_client_public)) {
-            s_sendmore (handler, zap_protocol == zap_status_internal_error
-                                   ? "500"
-                                   : (zap_protocol == zap_status_invalid
-                                        ? "invalid_status"
-                                        : "200"));
+            const char *status_code;
+            switch (zap_protocol) {
+                case zap_status_internal_error:
+                    status_code = "500";
+                    break;
+                case zap_status_temporary_failure:
+                    status_code = "300";
+                    break;
+                case zap_status_invalid:
+                    status_code = "invalid_status";
+                    break;
+                default:
+                    status_code = "200";
+            }
+            s_sendmore (handler, status_code);
             s_sendmore (handler, "OK");
             s_sendmore (handler, "anonymous");
             if (zap_protocol == zap_too_many_parts) {
@@ -265,6 +276,11 @@ static void zap_handler_wrong_status_invalid (void *ctx)
     zap_handler_generic (ctx, zap_status_invalid);
 }
 
+static void zap_handler_wrong_status_temporary_failure (void *ctx)
+{
+    zap_handler_generic (ctx, zap_status_temporary_failure);
+}
+
 static void zap_handler_wrong_status_internal_error (void *ctx)
 {
     zap_handler_generic (ctx, zap_status_internal_error);
@@ -322,6 +338,7 @@ int expect_monitor_event_multiple (void *server_mon,
 {
     int count_of_expected_events = 0;
     int client_closed_connection = 0;
+    //  infinite timeout at the start
     int timeout = -1;
 
     int event;
@@ -512,18 +529,17 @@ void test_curve_security_with_bogus_client_credentials (
     expect_new_client_curve_bounce_fail (ctx, valid_server_public, bogus_public,
                                          bogus_secret, my_endpoint, server);
 
+    int event_count = 0;
 #ifdef ZMQ_BUILD_DRAFT_API
-    int event = get_monitor_event_with_timeout (server_mon, NULL, NULL, -1);
     // TODO add another event type ZMQ_EVENT_HANDSHAKE_FAILED_AUTH for this case?
-    assert (
-      event
-      == ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL); // ZAP handle the error,  not curve_server
-
-    assert_no_more_monitor_events_with_timeout (server_mon, timeout);
+    event_count = expect_monitor_event_multiple (
+      server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL, EACCES);
+    assert (event_count <= 1);
 #endif
 
     // there may be more than one ZAP request due to repeated attempts by the client
-    assert (1 <= zmq_atomic_counter_value (zap_requests_handled));
+    assert (0 == event_count
+            || 1 <= zmq_atomic_counter_value (zap_requests_handled));
 }
 
 void expect_zmtp_failure (void *client, char *my_endpoint, void *server, void *server_mon)
@@ -534,12 +550,9 @@ void expect_zmtp_failure (void *client, char *my_endpoint, void *server, void *s
     expect_bounce_fail (server, client);
     close_zero_linger (client);
 
-#ifdef ZMQ_BUILD_DRAFT_API
-    int err;
-    int event = get_monitor_event_with_timeout (server_mon, &err, NULL, -1);
 
-    assert (event == ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP
-            || (event == ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL && err == EPIPE));
+#ifdef ZMQ_BUILD_DRAFT_API
+    expect_monitor_event_multiple (server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP);
 #endif
 
     assert (0 == zmq_atomic_counter_value (zap_requests_handled));
@@ -622,7 +635,7 @@ void test_curve_security_zap_unsuccessful (void *ctx,
     int events_received = 0;
 #ifdef ZMQ_BUILD_DRAFT_API
     events_received =
-    expect_monitor_event_multiple (server_mon, expected_event, expected_err);
+      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 client
@@ -801,17 +814,34 @@ int main (void)
 
     //  ZAP non-standard cases
 
+    //  TODO make these observable on the client side as well (they are 
+    //  transmitted as an ERROR message)
+
+    //  status 300 temporary failure
+    fprintf (stderr, "test_curve_security_zap_unsuccessful status 300\n");
+    setup_context_and_server_side (&ctx, &handler, &zap_thread, &server,
+                                   &server_mon, my_endpoint,
+                                   &zap_handler_wrong_status_temporary_failure);
+    test_curve_security_zap_unsuccessful (ctx, my_endpoint, server, server_mon,
+#ifdef ZMQ_BUILD_DRAFT_API
+                                          ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL,
+                                          EAGAIN
+#else
+                                          0, 0
+#endif
+    );
+    shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
+            handler);
+
     //  status 500 internal error
+    fprintf (stderr, "test_curve_security_zap_unsuccessful status 500\n");
     setup_context_and_server_side (&ctx, &handler, &zap_thread, &server,
                                    &server_mon, my_endpoint,
                                    &zap_handler_wrong_status_internal_error);
-
-    //  TODO is this usable? EAGAIN does not appear to be an appropriate error 
-    //  code, and the status text is completely lost
     test_curve_security_zap_unsuccessful (ctx, my_endpoint, server, server_mon,
 #ifdef ZMQ_BUILD_DRAFT_API
                                           ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL,
-                                          EAGAIN
+                                          EFAULT
 #else
                                           0, 0
 #endif