qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From acon...@apache.org
Subject qpid-proton git commit: PROTON-1571: c++: ssl example is leaky
Date Fri, 06 Oct 2017 20:11:44 GMT
Repository: qpid-proton
Updated Branches:
  refs/heads/master ace1970ef -> 3e4b963a6


PROTON-1571: c++: ssl example is leaky

Nothing to do with SSL, the problem was the example code throwing from an event
handling function. We should not leak even in this case, but I will raise a
separate issue.

Made the following positive  changes while investigating:
- Added trival copy/assign/dtor to proton::listener as future-proofing.
- Added C TLS tests to proactor.c


Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/3e4b963a
Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/3e4b963a
Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/3e4b963a

Branch: refs/heads/master
Commit: 3e4b963a6c77723dd1796fe64af970d87b239d36
Parents: ace1970
Author: Alan Conway <aconway@redhat.com>
Authored: Fri Oct 6 10:25:18 2017 -0400
Committer: Alan Conway <aconway@redhat.com>
Committed: Fri Oct 6 15:55:53 2017 -0400

----------------------------------------------------------------------
 examples/cpp/ssl.cpp                            |  24 +---
 .../bindings/cpp/include/proton/listener.hpp    |   3 +
 proton-c/bindings/cpp/src/listener.cpp          |   8 +-
 proton-c/src/tests/proactor.c                   | 119 ++++++++++++++-----
 proton-c/src/tests/test_handler.h               |   2 +
 5 files changed, 103 insertions(+), 53 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/3e4b963a/examples/cpp/ssl.cpp
----------------------------------------------------------------------
diff --git a/examples/cpp/ssl.cpp b/examples/cpp/ssl.cpp
index 388f05b..d93454f 100644
--- a/examples/cpp/ssl.cpp
+++ b/examples/cpp/ssl.cpp
@@ -52,15 +52,7 @@ namespace {
     const std::string verify_noname("noname"); // Skip matching host name against the certificate
     const std::string verify_fail("fail");  // Force name mismatch failure
     std::string verify(verify_full);  // Default for example
-    bool verify_failed(false);
     std::string cert_directory;
-
-    class example_cert_error : public std::runtime_error
-    {
-    public:
-        explicit example_cert_error(const std::string& s) : std::runtime_error(s) {}
-    };
-
 }
 
 
@@ -136,9 +128,11 @@ class hello_world_direct : public proton::messaging_handler {
 
     void on_transport_error(proton::transport &t) OVERRIDE {
         std::string err = t.error().what();
-        if (err.find("certificate") != std::string::npos) {
-            verify_failed = true;
-            throw example_cert_error(err);
+        if (verify == verify_fail && err.find("certificate") != std::string::npos)
{
+            std::cout << "Expected failure of connection with wrong peer name: " <<
err
+                      << std::endl;
+        } else {
+            std::cout << "Unexpected transport error: " << err << std::endl;
         }
     }
 
@@ -181,14 +175,6 @@ int main(int argc, char **argv) {
         proton::container(hwd).run();
         return 0;
     } catch (const std::exception& e) {
-        if (verify_failed) {
-            if (verify == verify_fail) {
-                std::cout << "Expected failure of connection with wrong peer name:
" << e.what() << std::endl;
-                return 0;
-            } else {
-                std::cerr << "unexpected internal certificate failure: ";
-            }
-        }
         std::cerr << e.what() << std::endl;
     }
     return 1;

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/3e4b963a/proton-c/bindings/cpp/include/proton/listener.hpp
----------------------------------------------------------------------
diff --git a/proton-c/bindings/cpp/include/proton/listener.hpp b/proton-c/bindings/cpp/include/proton/listener.hpp
index 5f6ec6c..d8421be 100644
--- a/proton-c/bindings/cpp/include/proton/listener.hpp
+++ b/proton-c/bindings/cpp/include/proton/listener.hpp
@@ -38,6 +38,9 @@ class PN_CPP_CLASS_EXTERN listener {
   public:
     /// Create an empty listener.
     PN_CPP_EXTERN listener();
+    PN_CPP_EXTERN listener(const listener&);
+    PN_CPP_EXTERN ~listener();
+    PN_CPP_EXTERN listener& operator=(const listener&);
 
     /// Stop listening on the address provided to the call to
     /// container::listen that returned this listener.

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/3e4b963a/proton-c/bindings/cpp/src/listener.cpp
----------------------------------------------------------------------
diff --git a/proton-c/bindings/cpp/src/listener.cpp b/proton-c/bindings/cpp/src/listener.cpp
index a9ca53d..2e38076 100644
--- a/proton-c/bindings/cpp/src/listener.cpp
+++ b/proton-c/bindings/cpp/src/listener.cpp
@@ -26,7 +26,13 @@
 namespace proton {
 
 listener::listener(): listener_(0) {}
-listener::listener(pn_listener_t* l) : listener_(l) {}
+listener::listener(pn_listener_t* l): listener_(l) {}
+// Out-of-line big-3 with trivial implementations, in case we need them in future. 
+listener::listener(const listener& l) : listener_(l.listener_) {}
+listener::~listener() {}
+listener& listener::operator=(const listener& l) { listener_ = l.listener_; return
*this; }
+
+// FIXME aconway 2017-10-06: should be a no-op if already closed
 void listener::stop() { if (listener_) pn_listener_close(listener_); }
 
 }

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/3e4b963a/proton-c/src/tests/proactor.c
----------------------------------------------------------------------
diff --git a/proton-c/src/tests/proactor.c b/proton-c/src/tests/proactor.c
index c34e0ed..38ee46a 100644
--- a/proton-c/src/tests/proactor.c
+++ b/proton-c/src/tests/proactor.c
@@ -185,7 +185,7 @@ static pn_event_type_t test_proactors_run(test_proactor_t *tps, size_t
n) {
   return e;
 }
 
-/* Run an array of proactors till a handler returns an event. */
+/* Run an array of proactors till a handler returns the desired event. */
 void test_proactors_run_until(test_proactor_t *tps, size_t n, pn_event_type_t want) {
   while (test_proactors_get(tps, n) != want)
          ;
@@ -324,17 +324,19 @@ static pn_event_type_t common_handler(test_handler_t *th, pn_event_t
*e) {
   }
 }
 
-/* Like common_handler but does not auto-close the listener after one accept */
+/* Like common_handler but does not auto-close the listener after one accept,
+   and returns on LISTENER_CLOSE
+*/
 static pn_event_type_t listen_handler(test_handler_t *th, pn_event_t *e) {
   switch (pn_event_type(e)) {
    case PN_LISTENER_ACCEPT:
     /* No automatic listener close/free for tests that accept multiple connections */
     last_accepted = pn_connection();
     pn_listener_accept(pn_event_listener(e), last_accepted);
+    /* No automatic close */
     return PN_EVENT_NONE;
 
    case PN_LISTENER_CLOSE:
-    /* No automatic free */
     return PN_LISTENER_CLOSE;
 
    default:
@@ -747,60 +749,111 @@ static void test_release_free(test_t *t) {
   pn_connection_free(pn_connection());
 }
 
-/* TODO aconway 2017-03-27: need windows version with .p12 certs */
-#define CERTFILE(NAME) CMAKE_CURRENT_SOURCE_DIR "/ssl_certs/" NAME ".pem"
+#define SSL_FILE(NAME) CMAKE_CURRENT_SOURCE_DIR "/ssl_certs/" NAME
+#define SSL_PW "tserverpw"
+/* Windows vs. OpenSSL certificates */
+#if defined(_WIN32)
+#  define CERTIFICATE(NAME) SSL_FILE(NAME "-certificate.p12")
+#  define SET_CREDENTIALS(DOMAIN, NAME)                                 \
+  pn_ssl_domain_set_credentials(DOMAIN, SSL_FILE(NAME "-full.p12"), "", SSL_PW)
+
+#else
+#  define CERTIFICATE(NAME) SSL_FILE(NAME "-certificate.pem")
+#  define SET_CREDENTIALS(DOMAIN, NAME)                                 \
+  pn_ssl_domain_set_credentials(DOMAIN, CERTIFICATE(NAME), SSL_FILE(NAME "-private-key.pem"),
SSL_PW)
+#endif
 
-static pn_event_type_t ssl_handler(test_handler_t *th, pn_event_t *e) {
-  pn_connection_t *c = pn_event_connection(e);
+static pn_event_type_t ssl_handler(test_handler_t *h, pn_event_t *e) {
   switch (pn_event_type(e)) {
 
-   case PN_CONNECTION_BOUND: {
-     bool incoming = (pn_connection_state(c) & PN_LOCAL_UNINIT);
-     pn_ssl_domain_t *ssld = pn_ssl_domain(incoming ? PN_SSL_MODE_SERVER : PN_SSL_MODE_CLIENT);
-     TEST_CHECK(th->t, 0 == pn_ssl_domain_set_credentials(
-                  ssld, CERTFILE("tserver-certificate"), CERTFILE("tserver-private-key"),
"tserverpw"));
-     TEST_CHECK(th->t, 0 == pn_ssl_init(pn_ssl(pn_event_transport(e)), ssld, NULL));
-     pn_ssl_domain_free(ssld);
-     return PN_EVENT_NONE;
+   case PN_CONNECTION_BOUND:
+    TEST_CHECK(h->t, 0 == pn_ssl_init(pn_ssl(pn_event_transport(e)), h->ssl_domain,
NULL));
+    return PN_EVENT_NONE;
+
+   case PN_CONNECTION_REMOTE_OPEN: {
+     pn_ssl_t *ssl = pn_ssl(pn_event_transport(e));
+     TEST_CHECK(h->t, ssl);
+     TEST_CHECK(h->t, pn_ssl_get_protocol_name(ssl, NULL, 0));
+     return PN_CONNECTION_REMOTE_OPEN;
    }
+   default:
+    return PN_EVENT_NONE;
+  }
+}
 
+static pn_event_type_t ssl_server_handler(test_handler_t *h, pn_event_t *e) {
+  switch (pn_event_type(e)) {
+   case PN_CONNECTION_BOUND:
    case PN_CONNECTION_REMOTE_OPEN: {
-     if (pn_connection_state(c) & PN_LOCAL_ACTIVE) {
-       /* Outgoing connection is complete, close it */
-       pn_connection_close(c);
-     } else {
-       /* Incoming connection, check for SSL */
-       pn_ssl_t *ssl = pn_ssl(pn_event_transport(e));
-       TEST_CHECK(th->t, ssl);
-       TEST_CHECK(th->t, pn_ssl_get_protocol_name(ssl, NULL, 0));
-       pn_connection_open(c);      /* Return the open (no-op if already open) */
-     }
-    return PN_CONNECTION_REMOTE_OPEN;
+     pn_event_type_t et = ssl_handler(h, e);
+     pn_connection_open(pn_event_connection(e));
+     return et;
    }
+   default:
+    return listen_handler(h, e);
+  }
+}
 
+static pn_event_type_t ssl_client_handler(test_handler_t *h, pn_event_t *e) {
+  switch (pn_event_type(e)) {
+   case PN_CONNECTION_BOUND:
+   case PN_CONNECTION_REMOTE_OPEN: {
+     pn_event_type_t et = ssl_handler(h, e);
+     pn_connection_close(pn_event_connection(e));
+     return et;
+   }
+    break;
    default:
-    return common_handler(th, e);
+    return common_handler(h, e);
   }
 }
 
-/* Establish an SSL connection between proactors*/
+/* Test various SSL connections between proactors*/
 static void test_ssl(test_t *t) {
   if (!pn_ssl_present()) {
     TEST_LOGF(t, "Skip SSL test, no support");
     return;
   }
 
-  test_proactor_t tps[] ={ test_proactor(t, ssl_handler), test_proactor(t, ssl_handler) };
-  pn_proactor_t *client = tps[0].proactor;
-  test_listener_t l = test_listen(&tps[1], localhost);
-  pn_connection_t *c = pn_connection();
-  pn_proactor_connect(client, c, l.port.host_port);
+  test_proactor_t tps[] ={ test_proactor(t, ssl_client_handler), test_proactor(t, ssl_server_handler)
};
+  test_proactor_t *client = &tps[0], *server = &tps[1];
+  pn_ssl_domain_t *cd = client->handler.ssl_domain = pn_ssl_domain(PN_SSL_MODE_CLIENT);
+  pn_ssl_domain_t *sd =  server->handler.ssl_domain = pn_ssl_domain(PN_SSL_MODE_SERVER);
+  TEST_CHECK(t, 0 == SET_CREDENTIALS(sd, "tserver"));
+  test_listener_t l = test_listen(server, localhost);
+
+  /* Basic SSL connection */
+  pn_proactor_connect(client->proactor, pn_connection(), l.port.host_port);
   /* Open ok at both ends */
   TEST_ETYPE_EQUAL(t, PN_CONNECTION_REMOTE_OPEN, TEST_PROACTORS_RUN(tps));
   TEST_COND_EMPTY(t, last_condition);
   TEST_ETYPE_EQUAL(t, PN_CONNECTION_REMOTE_OPEN, TEST_PROACTORS_RUN(tps));
   TEST_COND_EMPTY(t, last_condition);
+  TEST_PROACTORS_DRAIN(tps);
+
+  /* Verify peer with good hostname */
+  TEST_INT_EQUAL(t, 0, pn_ssl_domain_set_trusted_ca_db(cd, CERTIFICATE("tserver")));
+  TEST_INT_EQUAL(t, 0, pn_ssl_domain_set_peer_authentication(cd, PN_SSL_VERIFY_PEER_NAME,
NULL));
+  pn_connection_t *c = pn_connection();
+  pn_connection_set_hostname(c, "test_server");
+  pn_proactor_connect(client->proactor, c, l.port.host_port);
+  TEST_ETYPE_EQUAL(t, PN_CONNECTION_REMOTE_OPEN, TEST_PROACTORS_RUN(tps));
+  TEST_COND_EMPTY(t, last_condition);
+  TEST_ETYPE_EQUAL(t, PN_CONNECTION_REMOTE_OPEN, TEST_PROACTORS_RUN(tps));
+  TEST_COND_EMPTY(t, last_condition);
+  TEST_PROACTORS_DRAIN(tps);
+
+  /* Verify peer with bad hostname */
+  c = pn_connection();
+  pn_connection_set_hostname(c, "wrongname");
+  pn_proactor_connect(client->proactor, c, l.port.host_port);
+  TEST_ETYPE_EQUAL(t, PN_TRANSPORT_CLOSED, TEST_PROACTORS_RUN(tps));
+  TEST_COND_NAME(t, "amqp:connection:framing-error",  last_condition);
+  TEST_COND_DESC(t, "SSL",  last_condition);
+  TEST_PROACTORS_DRAIN(tps);
 
+  pn_ssl_domain_free(cd);
+  pn_ssl_domain_free(sd);
   TEST_PROACTORS_DESTROY(tps);
 }
 

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/3e4b963a/proton-c/src/tests/test_handler.h
----------------------------------------------------------------------
diff --git a/proton-c/src/tests/test_handler.h b/proton-c/src/tests/test_handler.h
index 6eb9079..108d0d9 100644
--- a/proton-c/src/tests/test_handler.h
+++ b/proton-c/src/tests/test_handler.h
@@ -21,6 +21,7 @@
  */
 
 #include "test_tools.h"
+#include <proton/ssl.h>
 
 /* C event handlers for tests */
 
@@ -45,6 +46,7 @@ typedef struct test_handler_t {
   pn_link_t *sender;
   pn_link_t *receiver;
   pn_delivery_t *delivery;
+  pn_ssl_domain_t *ssl_domain;
 } test_handler_t;
 
 void test_handler_init(test_handler_t *th, test_t *t, test_handler_fn f) {


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message