From 13b972b226d65a0cc30fe09f49d43aebbea08446 Mon Sep 17 00:00:00 2001 From: sigiesec Date: Thu, 17 Aug 2017 15:09:17 +0200 Subject: [PATCH 1/5] Problem: no tests without ZAP handler where one is expected Solution: added test case for NULL/PLAIN/CURVE --- tests/test_security_zap.cpp | 14 ++++++++++++++ tests/testutil_security.hpp | 16 ++++++++++------ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/tests/test_security_zap.cpp b/tests/test_security_zap.cpp index ffdd0999..213d3939 100644 --- a/tests/test_security_zap.cpp +++ b/tests/test_security_zap.cpp @@ -266,6 +266,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, +#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); } int main (void) diff --git a/tests/testutil_security.hpp b/tests/testutil_security.hpp index ea27d724..288783db 100644 --- a/tests/testutil_security.hpp +++ b/tests/testutil_security.hpp @@ -336,12 +336,15 @@ void setup_context_and_server_side ( int rc = zmq_bind (*handler, "inproc://handler-control"); assert (rc == 0); - *zap_thread = zmq_threadstart (zap_handler_, *ctx); + if (zap_handler_) { + *zap_thread = zmq_threadstart (zap_handler_, *ctx); - char *buf = s_recv (*handler); - assert (buf); - assert (streq (buf, "GO")); - free (buf); + char *buf = s_recv (*handler); + assert (buf); + assert (streq (buf, "GO")); + free (buf); + } else + *zap_thread = NULL; // Server socket will accept connections *server = zmq_socket (*ctx, ZMQ_DEALER); @@ -386,7 +389,8 @@ void shutdown_context_and_server_side (void *ctx, close_zero_linger (server); // Wait until ZAP handler terminates - zmq_threadclose (zap_thread); + if (zap_thread) + zmq_threadclose (zap_thread); rc = zmq_ctx_term (ctx); assert (rc == 0); From a5f94cb610f0a66046a8e749fb08f1e673c8be0e Mon Sep 17 00:00:00 2001 From: sigiesec Date: Mon, 18 Sep 2017 11:48:14 +0200 Subject: [PATCH 2/5] 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; \ From ee8b8bd29cdcfbff1ad3fd342f9d9c8fd991f418 Mon Sep 17 00:00:00 2001 From: sigiesec Date: Mon, 18 Sep 2017 15:11:51 +0200 Subject: [PATCH 3/5] Problem: no test for ZAP handler terminating in flight Solution: added test & some improvements of test utils --- tests/test_security_zap.cpp | 21 ++++++++++++++++++ tests/testutil_security.hpp | 43 +++++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/tests/test_security_zap.cpp b/tests/test_security_zap.cpp index c40b9a7c..92987629 100644 --- a/tests/test_security_zap.cpp +++ b/tests/test_security_zap.cpp @@ -59,6 +59,11 @@ 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); +} + int expect_new_client_bounce_fail_and_count_monitor_events ( void *ctx, char *my_endpoint, @@ -325,6 +330,22 @@ void test_zap_errors (socket_config_fn server_socket_config_, 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); } int main (void) diff --git a/tests/testutil_security.hpp b/tests/testutil_security.hpp index 04c7449c..4586f208 100644 --- a/tests/testutil_security.hpp +++ b/tests/testutil_security.hpp @@ -153,7 +153,8 @@ enum zap_protocol_t zap_wrong_version, zap_wrong_request_id, zap_status_invalid, - zap_too_many_parts + zap_too_many_parts, + zap_disconnect }; void *zap_requests_handled; @@ -196,6 +197,8 @@ void zap_handler_generic (void *ctx, char *version = s_recv (handler); if (!version) break; // Terminating - peer's socket closed + if (zap_protocol == zap_disconnect) + break; char *sequence = s_recv (handler); char *domain = s_recv (handler); @@ -285,8 +288,11 @@ void zap_handler_generic (void *ctx, assert (rc == 0); close_zero_linger (handler); - rc = s_send (control, "STOPPED"); - assert (rc == 7); + if (zap_protocol != zap_disconnect) + { + rc = s_send(control, "STOPPED"); + assert(rc == 7); + } close_zero_linger (control); } @@ -321,7 +327,7 @@ void setup_handshake_socket_monitor (void *ctx, void setup_context_and_server_side ( void **ctx, - void **handler, + void **zap_control, void **zap_thread, void **server, void **server_mon, @@ -338,15 +344,15 @@ void setup_context_and_server_side ( zap_requests_handled = zmq_atomic_counter_new (); assert (zap_requests_handled != NULL); - *handler = zmq_socket (*ctx, ZMQ_REP); - assert (*handler); - int rc = zmq_bind (*handler, "inproc://handler-control"); + *zap_control = zmq_socket (*ctx, ZMQ_REP); + assert (*zap_control); + int rc = zmq_bind (*zap_control, "inproc://handler-control"); assert (rc == 0); if (zap_handler_) { *zap_thread = zmq_threadstart (zap_handler_, *ctx); - char *buf = s_recv (*handler); + char *buf = s_recv (*zap_control); assert (buf); assert (streq (buf, "GO")); free (buf); @@ -374,20 +380,24 @@ 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 *zap_control, + bool zap_handler_stopped = false) { - if (zap_thread) { - int rc = s_send (handler, "STOP"); + if (zap_thread && !zap_handler_stopped) { + int rc = s_send (zap_control, "STOP"); assert (rc == 4); - char *buf = s_recv (handler); + char *buf = s_recv (zap_control); assert (buf); assert (streq (buf, "STOPPED")); free (buf); - rc = zmq_unbind (handler, "inproc://handler-control"); + rc = zmq_unbind (zap_control, "inproc://handler-control"); assert (rc == 0); } - close_zero_linger(handler); + close_zero_linger(zap_control); #ifdef ZMQ_BUILD_DRAFT_API close_zero_linger (server_mon); @@ -576,7 +586,8 @@ int expect_monitor_event_multiple (void *server_mon, // 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)) { + && ((err == EPIPE && expected_err != EPIPE) || err == ECONNRESET + || err == ECONNABORTED)) { fprintf (stderr, "Ignored event (skipping any further events): %x (err = " "%i == %s)\n", From e546f9296e6002c0b3311c98cd5e308d3757807f Mon Sep 17 00:00:00 2001 From: sigiesec Date: Mon, 18 Sep 2017 15:24:10 +0200 Subject: [PATCH 4/5] 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 | 21 ++++++++++----------- src/curve_server.hpp | 1 - src/mechanism_base.cpp | 5 +++++ src/mechanism_base.hpp | 2 ++ src/null_mechanism.cpp | 15 +++++++-------- src/null_mechanism.hpp | 2 -- src/plain_server.cpp | 21 ++++++++++++++------- src/session_base.cpp | 4 +++- src/zap_client.cpp | 7 ++----- tests/test_security_plain.cpp | 5 +++++ 10 files changed, 48 insertions(+), 35 deletions(-) diff --git a/src/curve_server.cpp b/src/curve_server.cpp index 0ad43c57..23173045 100644 --- a/src/curve_server.cpp +++ b/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 diff --git a/src/curve_server.hpp b/src/curve_server.hpp index 13c1d510..30efda5d 100644 --- a/src/curve_server.hpp +++ b/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) diff --git a/src/mechanism_base.cpp b/src/mechanism_base.cpp index 93a6f4f9..216261a4 100644 --- a/src/mechanism_base.cpp +++ b/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 (); +} diff --git a/src/mechanism_base.hpp b/src/mechanism_base.hpp index e09a3a1a..c6e76eea 100644 --- a/src/mechanism_base.hpp +++ b/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; }; } diff --git a/src/null_mechanism.cpp b/src/null_mechanism.cpp index 112cdb32..560aec3c 100644 --- a/src/null_mechanism.cpp +++ b/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); diff --git a/src/null_mechanism.hpp b/src/null_mechanism.hpp index d2050efa..4fca49df 100644 --- a/src/null_mechanism.hpp +++ b/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; diff --git a/src/plain_server.cpp b/src/plain_server.cpp index a085be37..5fabb1ca 100644 --- a/src/plain_server.cpp +++ b/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; } diff --git a/src/session_base.cpp b/src/session_base.cpp index 368598b8..7316dfff 100644 --- a/src/session_base.cpp +++ b/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_) diff --git a/src/zap_client.cpp b/src/zap_client.cpp index c1d719be..fb14b0f2 100644 --- a/src/zap_client.cpp +++ b/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 (); } } diff --git a/tests/test_security_plain.cpp b/tests/test_security_plain.cpp index c587cf2b..7e306c65 100644 --- a/tests/test_security_plain.cpp +++ b/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); From 77f76a49b27d9d91b02069d4e52f376670adf1c7 Mon Sep 17 00:00:00 2001 From: sigiesec Date: Mon, 18 Sep 2017 17:19:36 +0200 Subject: [PATCH 5/5] Problem: no tests for cases 5 and 6 of #2711 Solution: added tests --- tests/test_security_zap.cpp | 48 +++++++++++++++++++++++++++++++++++-- tests/testutil_security.hpp | 18 ++++++++++---- 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/tests/test_security_zap.cpp b/tests/test_security_zap.cpp index 92987629..49486680 100644 --- a/tests/test_security_zap.cpp +++ b/tests/test_security_zap.cpp @@ -64,6 +64,16 @@ 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, @@ -344,8 +354,42 @@ void test_zap_errors (socket_config_fn server_socket_config_, 0, 0, #endif client_socket_config_, client_socket_config_data_); - shutdown_context_and_server_side(ctx, zap_thread, server, server_mon, - handler, true); + 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) diff --git a/tests/testutil_security.hpp b/tests/testutil_security.hpp index 4586f208..74564ba0 100644 --- a/tests/testutil_security.hpp +++ b/tests/testutil_security.hpp @@ -154,7 +154,9 @@ enum zap_protocol_t zap_wrong_request_id, zap_status_invalid, zap_too_many_parts, - zap_disconnect + zap_disconnect, + zap_do_not_recv, + zap_do_not_send }; void *zap_requests_handled; @@ -182,8 +184,11 @@ void zap_handler_generic (void *ctx, {handler, 0, ZMQ_POLLIN, 0}, }; + // if ordered not to receive the request, ignore the second poll item + const int numitems = (zap_protocol == zap_do_not_recv) ? 1 : 2; + // Process ZAP requests forever - while (zmq_poll (items, 2, -1) >= 0) { + while (zmq_poll (items, numitems, -1) >= 0) { if (items[0].revents & ZMQ_POLLIN) { char *buf = s_recv (control); assert (buf); @@ -198,7 +203,10 @@ void zap_handler_generic (void *ctx, if (!version) break; // Terminating - peer's socket closed if (zap_protocol == zap_disconnect) + { + free (version); break; + } char *sequence = s_recv (handler); char *domain = s_recv (handler); @@ -268,12 +276,14 @@ void zap_handler_generic (void *ctx, if (zap_protocol == zap_too_many_parts) { s_sendmore (handler, ""); } - s_send (handler, ""); + if (zap_protocol != zap_do_not_send) + s_send (handler, ""); } else { s_sendmore (handler, "400"); s_sendmore (handler, "Invalid client public key"); s_sendmore (handler, ""); - s_send (handler, ""); + if (zap_protocol != zap_do_not_send) + s_send(handler, ""); } free (version); free (sequence);