From a5f94cb610f0a66046a8e749fb08f1e673c8be0e Mon Sep 17 00:00:00 2001 From: sigiesec Date: Mon, 18 Sep 2017 11:48:14 +0200 Subject: [PATCH] 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 | 22 ++++-- src/curve_server.hpp | 1 + src/null_mechanism.cpp | 26 +++++-- src/null_mechanism.hpp | 3 +- src/plain_server.cpp | 4 + tests/test_security_zap.cpp | 81 ++++++++++++++++----- tests/testutil_security.hpp | 141 +++++++++++++++++++----------------- 7 files changed, 179 insertions(+), 99 deletions(-) diff --git a/src/curve_server.cpp b/src/curve_server.cpp index bbcec99b..0ad43c57 100644 --- a/src/curve_server.cpp +++ b/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 (rc == 0) { - send_zap_request (client_key); - rc = receive_and_process_zap_reply (); - if (rc == -1) + if (zap_required ()) { + rc = session->zap_connect (); + if (rc == 0) { + send_zap_request (client_key); + 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 diff --git a/src/curve_server.hpp b/src/curve_server.hpp index 30efda5d..13c1d510 100644 --- a/src/curve_server.hpp +++ b/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) diff --git a/src/null_mechanism.cpp b/src/null_mechanism.cpp index c8192bf3..112cdb32 100644 --- a/src/null_mechanism.cpp +++ b/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); diff --git a/src/null_mechanism.hpp b/src/null_mechanism.hpp index b9b5ea0d..d2050efa 100644 --- a/src/null_mechanism.hpp +++ b/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; diff --git a/src/plain_server.cpp b/src/plain_server.cpp index 421d0e5a..a085be37 100644 --- a/src/plain_server.cpp +++ b/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; } diff --git a/tests/test_security_zap.cpp b/tests/test_security_zap.cpp index 213d3939..c40b9a7c 100644 --- a/tests/test_security_zap.cpp +++ b/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, - 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 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); @@ -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 ( - &ctx, &handler, &zap_thread, &server, &server_mon, my_endpoint, - NULL, server_socket_config_); - test_zap_unsuccessful (ctx, my_endpoint, server, server_mon, + 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, + 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); } diff --git a/tests/testutil_security.hpp b/tests/testutil_security.hpp index 288783db..04c7449c 100644 --- a/tests/testutil_security.hpp +++ b/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-compliant non-standard cases - zap_status_temporary_failure, - zap_status_internal_error, - // ZAP protocol errors - zap_wrong_version, - zap_wrong_request_id, - zap_status_invalid, - zap_too_many_parts + zap_ok, + // ZAP-compliant non-standard cases + zap_status_temporary_failure, + zap_status_internal_error, + // ZAP protocol errors + zap_wrong_version, + zap_wrong_request_id, + zap_status_invalid, + 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")) - { - char client_username[32]; + } else if (streq (mechanism, "PLAIN")) { + 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 *zap_thread, - void *server, - void *server_mon, - void *handler) +void shutdown_context_and_server_side ( + void *ctx, void *zap_thread, void *server, void *server_mon, void *handler) { - int rc = s_send (handler, "STOP"); - assert (rc == 4); - char *buf = s_recv (handler); - assert (buf); - assert (streq (buf, "STOPPED")); - free (buf); - rc = zmq_unbind (handler, "inproc://handler-control"); - assert (rc == 0); - close_zero_linger (handler); + if (zap_thread) { + int rc = s_send (handler, "STOP"); + assert (rc == 4); + char *buf = s_recv (handler); + assert (buf); + assert (streq (buf, "STOPPED")); + free (buf); + rc = zmq_unbind (handler, "inproc://handler-control"); + 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 -get_monitor_event_internal (void *monitor, int *value, char **address, int recv_flag) +static int get_monitor_event_internal (void *monitor, + 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( - stderr, - "Unexpected event: 0x%x, value = %i/0x%x (expected: 0x%x, value " - "= %i/0x%x)\n", - event, err, err, expected_event, expected_err, expected_err); + fprintf (stderr, + "Unexpected event: 0x%x, value = %i/0x%x (expected: 0x%x, value " + "= %i/0x%x)\n", + 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 && - (err == EPIPE || err == ECONNRESET || err == ECONNABORTED)) { - fprintf ( - stderr, - "Ignored event (skipping any further events): %x (err = %i)\n", - event, err); + if (event == ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL + && (err == EPIPE || err == ECONNRESET || err == ECONNABORTED)) { + fprintf (stderr, + "Ignored event (skipping any further events): %x (err = " + "%i == %s)\n", + 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", \ - event, err); \ + "(err = %i == %s)\n", \ + event, err, zmq_strerror (err)); \ continue; \ } \ ++event_count; \