From fb67e160a11db104f89043c702000b7c974d7537 Mon Sep 17 00:00:00 2001
From: Pieter Hintjens <ph@imatix.com>
Date: Mon, 2 Sep 2013 18:21:36 +0200
Subject: [PATCH] Fixed ZAP authentication

- if ZAP server returns anything except 200, connection is closed
- all security tests now pass correctly
- test_security_curve now does proper client key authentication using test key
- test_security_plain now does proper password authentication
---
 src/curve_server.cpp          |  4 +++
 src/plain_mechanism.cpp       |  4 +++
 tests/test_security_curve.cpp | 46 ++++++++++++++++++++++-------------
 tests/test_security_plain.cpp | 27 ++++++++++----------
 tests/testutil.hpp            | 11 +++++----
 5 files changed, 56 insertions(+), 36 deletions(-)

diff --git a/src/curve_server.cpp b/src/curve_server.cpp
index 19aba7a8..56e4cdf0 100644
--- a/src/curve_server.cpp
+++ b/src/curve_server.cpp
@@ -588,24 +588,28 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
 
     //  Address delimiter frame
     if (msg [0].size () > 0) {
+        rc = -1;
         errno = EPROTO;
         goto error;
     }
 
     //  Version frame
     if (msg [1].size () != 3 || memcmp (msg [1].data (), "1.0", 3)) {
+        rc = -1;
         errno = EPROTO;
         goto error;
     }
 
     //  Request id frame
     if (msg [2].size () != 1 || memcmp (msg [2].data (), "1", 1)) {
+        rc = -1;
         errno = EPROTO;
         goto error;
     }
 
     //  Status code frame
     if (msg [3].size () != 3 || memcmp (msg [3].data (), "200", 3)) {
+        rc = -1;
         errno = EACCES;
         goto error;
     }
diff --git a/src/plain_mechanism.cpp b/src/plain_mechanism.cpp
index 4ba58c40..002adc37 100644
--- a/src/plain_mechanism.cpp
+++ b/src/plain_mechanism.cpp
@@ -439,24 +439,28 @@ int zmq::plain_mechanism_t::receive_and_process_zap_reply ()
 
     //  Address delimiter frame
     if (msg [0].size () > 0) {
+        rc = -1;
         errno = EPROTO;
         goto error;
     }
 
     //  Version frame
     if (msg [1].size () != 3 || memcmp (msg [1].data (), "1.0", 3)) {
+        rc = -1;
         errno = EPROTO;
         goto error;
     }
 
     //  Request id frame
     if (msg [2].size () != 1 || memcmp (msg [2].data (), "1", 1)) {
+        rc = -1;
         errno = EPROTO;
         goto error;
     }
 
     //  Status code frame
     if (msg [3].size () != 3 || memcmp (msg [3].data (), "200", 3)) {
+        rc = -1;
         errno = EACCES;
         goto error;
     }
diff --git a/tests/test_security_curve.cpp b/tests/test_security_curve.cpp
index 464f1267..752b3f1c 100644
--- a/tests/test_security_curve.cpp
+++ b/tests/test_security_curve.cpp
@@ -18,10 +18,17 @@
 */
 
 #include "platform.hpp"
-#include "../include/zmq_utils.h"
 #include <string.h>
 #include <stdlib.h>
 #include "testutil.hpp"
+#include "../include/zmq_utils.h"
+#include "../src/z85_codec.hpp"
+
+//  Test keys from the zmq_curve man page
+static char client_public [] = "Yne@$w-vo<fVvi]a<NY6T1ed:M$fCG*[IaLV{hID";
+static char client_secret [] = "D:)Q[IlAW!ahhC2ac:9*A}h:p?([4%wOTJ%JR%cs";
+static char server_public [] = "rq:rM>}U?@Lns47E1%kR.o@n%FcmmsL/@{H8]yf7";
+static char server_secret [] = "JTKVSB%%)wK0E.X)V>+}o?pNmC{O&4W4b!Ni{Lh6";
 
 static void zap_handler (void *ctx)
 {
@@ -30,7 +37,7 @@ static void zap_handler (void *ctx)
     assert (zap);
     int rc = zmq_bind (zap, "inproc://zeromq.zap.01");
     assert (rc == 0);
-
+    
     //  Process ZAP requests forever
     while (true) {
         char *version = s_recv (zap);
@@ -42,26 +49,38 @@ static void zap_handler (void *ctx)
         char *address = s_recv (zap);
         char *identity = s_recv (zap);
         char *mechanism = s_recv (zap);
-        char *client_key = s_recv (zap);
-        
+        uint8_t client_key [32];
+        int size = zmq_recv (zap, client_key, 32, 0);
+        assert (size == 32);
+
+        char client_key_text [40];
+        Z85_encode (client_key_text, client_key, 32);
+
         assert (streq (version, "1.0"));
         assert (streq (mechanism, "CURVE"));
         assert (streq (identity, "IDENT"));
 
         s_sendmore (zap, version);
         s_sendmore (zap, sequence);
-        s_sendmore (zap, "200");
-        s_sendmore (zap, "OK");
-        s_sendmore (zap, "anonymous");
-        s_send (zap, "");
         
+        if (streq (client_key_text, client_public)) {
+            s_sendmore (zap, "200");
+            s_sendmore (zap, "OK");
+            s_sendmore (zap, "anonymous");
+            s_send (zap, "");
+        }
+        else {
+            s_sendmore (zap, "400");
+            s_sendmore (zap, "Invalid username or password");
+            s_sendmore (zap, "");
+            s_send (zap, "");
+        }
         free (version);
         free (sequence);
         free (domain);
         free (address);
         free (identity);
         free (mechanism);
-        free (client_key);
     }
     rc = zmq_close (zap);
     assert (rc == 0);
@@ -81,12 +100,6 @@ int main (void)
     //  Spawn ZAP handler
     void *zap_thread = zmq_threadstart (&zap_handler, ctx);
 
-    //  Test keys from the zmq_curve man page
-    char client_public [] = "Yne@$w-vo<fVvi]a<NY6T1ed:M$fCG*[IaLV{hID";
-    char client_secret [] = "D:)Q[IlAW!ahhC2ac:9*A}h:p?([4%wOTJ%JR%cs";
-    char server_public [] = "rq:rM>}U?@Lns47E1%kR.o@n%FcmmsL/@{H8]yf7";
-    char server_secret [] = "JTKVSB%%)wK0E.X)V>+}o?pNmC{O&4W4b!Ni{Lh6";
-
     //  Server socket will accept connections
     void *server = zmq_socket (ctx, ZMQ_DEALER);
     assert (server);
@@ -176,8 +189,7 @@ int main (void)
     assert (rc == 0);
     rc = zmq_connect (client, "tcp://localhost:9998");
     assert (rc == 0);
-    //  TODO: does not handle ZAP failures properly
-    //     expect_bounce_fail (server, client);
+    expect_bounce_fail (server, client);
     close_zero_linger (client);
     
     //  Shutdown
diff --git a/tests/test_security_plain.cpp b/tests/test_security_plain.cpp
index b1f6e42c..7a55d946 100644
--- a/tests/test_security_plain.cpp
+++ b/tests/test_security_plain.cpp
@@ -114,6 +114,18 @@ int main (void)
     rc = zmq_close (client);
     assert (rc == 0);
 
+    //  Check PLAIN security with badly configured client (as_server)
+    //  This will be caught by the plain_server class, not passed to ZAP
+    client = zmq_socket (ctx, ZMQ_DEALER);
+    assert (client);
+    as_server = 1;
+    rc = zmq_setsockopt (client, ZMQ_PLAIN_SERVER, &as_server, sizeof (int));
+    assert (rc == 0);
+    rc = zmq_connect (client, "tcp://localhost:9998");
+    assert (rc == 0);
+    expect_bounce_fail (server, client);
+    close_zero_linger (client);
+    
     //  Check PLAIN security -- failed authentication
     client = zmq_socket (ctx, ZMQ_DEALER);
     assert (client);
@@ -125,22 +137,9 @@ int main (void)
     assert (rc == 0);
     rc = zmq_connect (client, "tcp://localhost:9998");
     assert (rc == 0);
-    //  TODO: this does not fail as it should
-    //     expect_bounce_fail (server, client);
+    expect_bounce_fail (server, client);
     close_zero_linger (client);
 
-    //  Check PLAIN security with badly configured client (as_server)
-    client = zmq_socket (ctx, ZMQ_DEALER);
-    assert (client);
-    as_server = 1;
-    rc = zmq_setsockopt (client, ZMQ_PLAIN_SERVER, &as_server, sizeof (int));
-    assert (rc == 0);
-    rc = zmq_connect (client, "tcp://localhost:9998");
-    assert (rc == 0);
-    //  TODO: this does not fail as it should
-    //     expect_bounce_fail (server, client);
-    close_zero_linger (client);
-    
     //  Shutdown
     rc = zmq_close (server);
     assert (rc == 0);
diff --git a/tests/testutil.hpp b/tests/testutil.hpp
index ff6aff42..dd368d83 100644
--- a/tests/testutil.hpp
+++ b/tests/testutil.hpp
@@ -102,21 +102,22 @@ expect_bounce_fail (void *server, void *client)
     int timeout = 150;
     rc = zmq_setsockopt (server, ZMQ_RCVTIMEO, &timeout, sizeof (int));
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_RCVTIMEO, &timeout, sizeof (int));
-    assert (rc == 0);
-    
     rc = zmq_recv (server, buffer, 32, 0);
     assert (rc == -1);
-    assert (zmq_errno() == EAGAIN);
+    assert (zmq_errno () == EAGAIN);
 
+    //  Send message from server to client to test other direction
     rc = zmq_send (server, content, 32, ZMQ_SNDMORE);
     assert (rc == 32);
     rc = zmq_send (server, content, 32, 0);
     assert (rc == 32);
 
+    //  Receive message at client side (should not succeed)
+    rc = zmq_setsockopt (client, ZMQ_RCVTIMEO, &timeout, sizeof (int));
+    assert (rc == 0);
     rc = zmq_recv (client, buffer, 32, 0);
     assert (rc == -1);
-    assert (zmq_errno() == EAGAIN);
+    assert (zmq_errno () == EAGAIN);
 }
 
 //  Receive 0MQ string from socket and convert into C string