Merge lp:~myers-1/pyopenssl/npn into lp:~exarkun/pyopenssl/trunk

[Myers Carpenter]

This adds support for Next Protocol Negotiation found in OpenSSL 1.0.1. It includes a new test that exercises the functionality.

[lp:~myers-1/pyopenssl/npn]

165. By Myers Carpenter on 2012-02-10

add ifdefs so this will build with older versions of openssl

[Myers Carpenter (myers-1)]

OpenSSL 1.0.1 is out with NPN built it.

[Jean-Paul Calderone (exarkun)]

Thanks! A few comments:

  1. Where `global_next_protos_advertised_callback` invokes application code, `next_protos_advertised_callback`, it needs to check for an exception being raised and propagate it. It also needs to make sure the return value is a sequence and raise its own exception (likely a `TypeError`) if something else is returned. Notice the "XXX" near the end of `global_next_protos_advertised_callback`: that's the code doing the exception check. It doesn't do any good down there, after `ret` has been used. :)
  1. `global_next_proto_select_callback` needs to check the return value of `PyList_New`. It may return NULL to signal error.
  1. Is the reference counting for `conn->proto_selected` / `ret` correct? Does it need to be incref'd after `ret` is assigned to `conn->proto_selected`? I don't know who owns the reference returned by `PyEval_CallObject`.
  1. The `PyCallback_Check` in `ssl_Context_set_next_protos_advertised_callback` isn't necessary. Just try calling the object. If it fails, the exception handling code at the call site (still needs to be added :) will handle it.
  1. Same comment about `ssl_Context_set_next_proto_select_callback`.
  1. The unit tests should exercise as many of the error cases as possible.
  1. Avoid using assertEqual inside a callback in the unit test. There's no guarantee that the exception it raises will propagate to the test runner and be reported (and indeed, the current implementation will crash instead). Record the arguments and make assertions about them afterwards.

Thanks again! Looking forward to the next iteration.

review: Needs Fixing

Unmerged revisions

165. By Myers Carpenter on 2012-02-10

add ifdefs so this will build with older versions of openssl

164. By Myers Carpenter on 2012-02-10

add support for next protocol neg added in OpenSSL 1.0.1.

=== modified file 'OpenSSL/ssl/connection.c'
--- OpenSSL/ssl/connection.c	2011-07-16 05:14:58 +0000
+++ OpenSSL/ssl/connection.c	2012-02-10 13:47:29 +0000
@@ -327,6 +327,29 @@
     }
 }
 
+#if OPENSSL_VERSION_NUMBER >= 0x10001000L && !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_NEXTPROTONEG)
+static char ssl_Connection_get_next_proto_negotiated_doc[] = "\n\
+Get protocol that the client selected\n\
+\n\
+:return: A string with the protocol the client selected\n\
+";
+static PyObject *
+ssl_Connection_get_next_proto_negotiated(ssl_ConnectionObj *self, PyObject *args)
+{
+    const unsigned char *data;
+    unsigned len;
+
+    if (!PyArg_ParseTuple(args, ":get_next_proto_negotiated"))
+        return NULL;
+
+    SSL_get0_next_proto_negotiated(self->ssl, &data, &len);
+    if (len == 0) {
+        Py_INCREF(Py_None);
+        return Py_None;
+    }    
+    return PyBytes_FromStringAndSize((const char *)data, (Py_ssize_t)len);
+}
+#endif
 
 static char ssl_Connection_set_tlsext_host_name_doc[] = "\n\
 Set the value of the servername extension to send in the client hello.\n\
@@ -1271,6 +1294,9 @@
     ADD_METHOD(get_context),
     ADD_METHOD(set_context),
     ADD_METHOD(get_servername),
+#if OPENSSL_VERSION_NUMBER >= 0x10001000L && !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_NEXTPROTONEG)
+    ADD_METHOD(get_next_proto_negotiated),
+#endif
     ADD_METHOD(set_tlsext_host_name),
     ADD_METHOD(pending),
     ADD_METHOD(send),
@@ -1339,6 +1365,9 @@
     Py_INCREF(sock);
     self->socket = sock;
 
+    Py_INCREF(Py_None);
+    self->proto_selected = Py_None;
+
     self->ssl = NULL;
     self->from_ssl = NULL;
     self->into_ssl = NULL;
@@ -1466,6 +1495,8 @@
         ret = visit((PyObject *)self->context, arg);
     if (ret == 0 && self->socket != NULL)
         ret = visit(self->socket, arg);
+    if (ret == 0 && self->proto_selected != NULL)
+        ret = visit((PyObject *)self->proto_selected, arg);
     if (ret == 0 && self->app_data != NULL)
         ret = visit(self->app_data, arg);
     return ret;
@@ -1484,6 +1515,8 @@
     self->context = NULL;
     Py_XDECREF(self->socket);
     self->socket = NULL;
+    Py_XDECREF(self->proto_selected);
+    self->proto_selected = NULL;
     Py_XDECREF(self->app_data);
     self->app_data = NULL;
     self->into_ssl = NULL; /* was cleaned up by SSL_free() */

=== modified file 'OpenSSL/ssl/connection.h'
--- OpenSSL/ssl/connection.h	2011-03-03 00:26:20 +0000
+++ OpenSSL/ssl/connection.h	2012-02-10 13:47:29 +0000
@@ -42,6 +42,7 @@
     SSL                 *ssl;
     ssl_ContextObj      *context;
     PyObject            *socket;
+    PyObject            *proto_selected;
     PyThreadState       *tstate; /* This field is no longer used. */
     PyObject            *app_data;
     BIO                 *into_ssl, *from_ssl;  /* for connections without file descriptors */

=== modified file 'OpenSSL/ssl/context.c'
--- OpenSSL/ssl/context.c	2011-09-11 13:35:32 +0000
+++ OpenSSL/ssl/context.c	2012-02-10 13:47:29 +0000
@@ -237,6 +237,140 @@
     return;
 }
 
+#if OPENSSL_VERSION_NUMBER >= 0x10001000L && !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_NEXTPROTONEG)
+/*
+ * Globally defined next proto callback.  This is called from OpenSSL internally.
+ * The GIL will not be held when this function is invoked.  It must not be held
+ * when the function returns.
+ *
+ * Arguments: ssl   - The Connection
+ *            **out - handle to where we can put a new string to be returned
+ *            *outlen - pointer to where we can put the len of the string we want to return
+ * Returns:   SSL_TLSEXT_ERR_OK
+ */
+static int
+global_next_protos_advertised_callback(SSL *ssl, const unsigned char **out, unsigned int *outlen, void *arg)
+{
+    ssl_ConnectionObj *conn = (ssl_ConnectionObj *)SSL_get_app_data(ssl);
+    PyObject *argv, *ret, *item;
+    Py_ssize_t length, i, strlength;
+    unsigned char *outptr;
+
+    *outlen = 0;
+    
+    /*
+     * GIL isn't held yet.  First things first - acquire it, or any Python API
+     * we invoke might segfault or blow up the sun.  The reverse will be done
+     * before returning.
+     */
+    MY_END_ALLOW_THREADS(conn->tstate);
+
+    argv = Py_BuildValue("(O)", (PyObject *)conn);
+    ret = PyEval_CallObject(conn->context->next_protos_advertised_callback, argv);
+    Py_DECREF(argv);
+
+    length = PySequence_Size(ret);
+    for (i = 0; i < length; i++) {
+        strlength = PyBytes_Size(PySequence_GetItem(ret, i));
+        if (strlength >= 254) {
+            PyErr_SetString(PyExc_TypeError, "string returned by next_protos_advertised_callback was greater than 254 characters");
+            goto out;
+        }
+        *outlen += 1 + strlength;
+    }
+    *out = outptr = OPENSSL_malloc(*outlen + 1);
+    for (i = 0; i < length; i++) {
+        item = PySequence_GetItem(ret, i);
+        strlength = PyBytes_Size(item);
+        outptr[0] = Py_SAFE_DOWNCAST(strlength, Py_ssize_t, unsigned char);
+        outptr++;
+        memcpy(outptr, PyBytes_AsString(item), strlength);
+        outptr += strlength;
+    }
+
+    if (ret == NULL) {
+        /*
+         * XXX - This should be reported somehow. -exarkun
+         */
+        PyErr_Clear();
+    } else {
+        Py_DECREF(ret);
+    }
+
+  out:
+    /*
+     * This function is returning into OpenSSL.  Release the GIL again.
+     */
+    MY_BEGIN_ALLOW_THREADS(conn->tstate);
+    return SSL_TLSEXT_ERR_OK;
+}
+
+/*
+ * Globally defined next proto select callback.  This is called from OpenSSL internally.
+ * The GIL will not be held when this function is invoked.  It must not be held
+ * when the function returns.
+ *
+ * Arguments: ssl   - The Connection
+ *            **out - handle to where we can put the proto we want to pick
+ *            *outlen - pointer to where we can put the len of the proto we pick
+ *            *in - handle to where the list of protocols the server is advertising
+ *            inlen - pointer to where the lenght of protocols we can select
+ * Returns:   SSL_TLSEXT_ERR_OK
+ */
+static int
+global_next_proto_select_callback(SSL *ssl, unsigned char **out, unsigned char *outlen, const unsigned char *in, unsigned int inlen, void *arg)
+{
+    ssl_ConnectionObj *conn = (ssl_ConnectionObj *)SSL_get_app_data(ssl);
+    PyObject *argv, *proto_list, *str, *ret;
+    int retval = SSL_TLSEXT_ERR_ALERT_FATAL;
+    int i;
+
+    *outlen = 0;
+    /*
+     * GIL isn't held yet.  First things first - acquire it, or any Python API
+     * we invoke might segfault or blow up the sun.  The reverse will be done
+     * before returning.
+     */
+    MY_END_ALLOW_THREADS(conn->tstate);
+
+    proto_list = PyList_New(0);
+    for (i = 0; i < inlen; i++) {
+        str = PyBytes_FromStringAndSize((const char *)in+i+1, (Py_ssize_t)in[i]);
+        PyList_Append(proto_list, str);
+        Py_DECREF(str);
+        i += in[i];
+    }
+    
+    argv = Py_BuildValue("(OO)", (PyObject *)conn, proto_list);
+    ret = PyEval_CallObject(conn->context->next_proto_select_callback, argv);
+    Py_DECREF(argv);
+    Py_DECREF(proto_list);
+
+    if (ret == NULL) {
+        goto out;
+    }
+    if (!PyBytes_Check(ret)) {
+        /*
+         * XXX Returned something that wasn't a string.  This is bogus.  We'll
+         * return 0 and OpenSSL will treat it as an error, resulting in an
+         * exception from whatever Python API triggered this callback.
+         */
+        Py_DECREF(ret);
+        goto out;
+    }
+    Py_DECREF(conn->proto_selected);
+    conn->proto_selected = ret;
+    PyBytes_AsStringAndSize(ret, (char **)out, (Py_ssize_t *)outlen);
+    retval = SSL_TLSEXT_ERR_OK;
+  out:
+    /*
+     * This function is returning into OpenSSL.  Release the GIL again.
+     */
+    MY_BEGIN_ALLOW_THREADS(conn->tstate);
+    return retval;
+}
+#endif
+
 /*
  * Globally defined TLS extension server name callback.  This is called from
  * OpenSSL internally.  The GIL will not be held when this function is invoked.
@@ -1030,6 +1164,66 @@
     return Py_None;
 }
 
+#if OPENSSL_VERSION_NUMBER >= 0x10001000L && !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_NEXTPROTONEG)
+static char ssl_Context_set_next_protos_advertised_callback_doc[] = "\n\
+Set the next protos advertised callback\n\
+\n\
+:param callback: The Python callback to use\n\
+:return: None\n\
+";
+static PyObject *
+ssl_Context_set_next_protos_advertised_callback(ssl_ContextObj *self, PyObject *args)
+{
+    PyObject *callback;
+
+    if (!PyArg_ParseTuple(args, "O:set_next_protos_advertised_callback", &callback))
+        return NULL;
+
+    if (!PyCallable_Check(callback))
+    {
+        PyErr_SetString(PyExc_TypeError, "expected PyCallable");
+        return NULL;
+    }
+
+    Py_DECREF(self->next_protos_advertised_callback);
+    Py_INCREF(callback);
+    self->next_protos_advertised_callback = callback;
+    SSL_CTX_set_next_protos_advertised_cb(self->ctx, global_next_protos_advertised_callback, NULL);
+
+    Py_INCREF(Py_None);
+    return Py_None;
+}
+
+static char ssl_Context_set_next_proto_select_callback_doc[] = "\n\
+Set the next protos select callback\n\
+\n\
+:param callback: The Python callback to use.  This will take a list as an arg and return a str\n\
+:return: None\n\
+";
+static PyObject *
+ssl_Context_set_next_proto_select_callback(ssl_ContextObj *self, PyObject *args)
+{
+    PyObject *callback;
+
+    if (!PyArg_ParseTuple(args, "O:set_next_proto_select_callback", &callback))
+        return NULL;
+
+    if (!PyCallable_Check(callback))
+    {
+        PyErr_SetString(PyExc_TypeError, "expected PyCallable");
+        return NULL;
+    }
+
+    Py_DECREF(self->next_proto_select_callback);
+    Py_INCREF(callback);
+    self->next_proto_select_callback = callback;
+    SSL_CTX_set_next_proto_select_cb(self->ctx, global_next_proto_select_callback, NULL);
+
+    Py_INCREF(Py_None);
+    return Py_None;
+}
+#endif
+
 static char ssl_Context_get_app_data_doc[] = "\n\
 Get the application data (supplied via set_app_data())\n\
 \n\
@@ -1187,6 +1381,10 @@
     ADD_METHOD(set_timeout),
     ADD_METHOD(get_timeout),
     ADD_METHOD(set_info_callback),
+#if OPENSSL_VERSION_NUMBER >= 0x10001000L && !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_NEXTPROTONEG)
+    ADD_METHOD(set_next_protos_advertised_callback),
+    ADD_METHOD(set_next_proto_select_callback),
+#endif
     ADD_METHOD(get_app_data),
     ADD_METHOD(set_app_data),
     ADD_METHOD(get_cert_store),
@@ -1241,6 +1439,12 @@
     self->info_callback = Py_None;
 
     Py_INCREF(Py_None);
+    self->next_protos_advertised_callback = Py_None;
+
+    Py_INCREF(Py_None);
+    self->next_proto_select_callback = Py_None;
+
+    Py_INCREF(Py_None);
     self->tlsext_servername_callback = Py_None;
 
     Py_INCREF(Py_None);
@@ -1320,6 +1524,10 @@
         ret = visit((PyObject *)self->verify_callback, arg);
     if (ret == 0 && self->info_callback != NULL)
         ret = visit((PyObject *)self->info_callback, arg);
+    if (ret == 0 && self->next_protos_advertised_callback != NULL)
+        ret = visit((PyObject *)self->next_protos_advertised_callback, arg);
+    if (ret == 0 && self->next_proto_select_callback != NULL)
+        ret = visit((PyObject *)self->next_proto_select_callback, arg);
     if (ret == 0 && self->app_data != NULL)
         ret = visit(self->app_data, arg);
     return ret;
@@ -1342,6 +1550,10 @@
     self->verify_callback = NULL;
     Py_XDECREF(self->info_callback);
     self->info_callback = NULL;
+    Py_XDECREF(self->next_protos_advertised_callback);
+    self->next_protos_advertised_callback = NULL;
+    Py_XDECREF(self->next_proto_select_callback);
+    self->next_proto_select_callback = NULL;
     Py_XDECREF(self->app_data);
     self->app_data = NULL;
     return 0;

=== modified file 'OpenSSL/ssl/context.h'
--- OpenSSL/ssl/context.h	2011-05-26 22:47:00 +0000
+++ OpenSSL/ssl/context.h	2012-02-10 13:47:29 +0000
@@ -29,6 +29,8 @@
                         *passphrase_userdata,
                         *verify_callback,
                         *info_callback,
+                        *next_protos_advertised_callback,
+                        *next_proto_select_callback,
                         *tlsext_servername_callback,
                         *app_data;
     PyThreadState       *tstate;

=== modified file 'OpenSSL/test/test_ssl.py'
--- OpenSSL/test/test_ssl.py	2011-09-11 14:01:31 +0000
+++ OpenSSL/test/test_ssl.py	2012-02-10 13:47:29 +0000
@@ -587,6 +587,50 @@
         # Kind of lame.  Just make sure it got called somehow.
         self.assertTrue(called)
 
+    def test_next_proto(self):
+        if not hasattr(Context, 'set_next_proto_select_callback'):
+            return
+        
+        (server, client) = socket_pair()
+
+        server_protos = ['spdy/2', 'http/1.1',]
+        
+        clientContext = Context(TLSv1_METHOD)
+        select_called = []
+        def next_proto_select(conn, protos):
+            self.assertEqual(server_protos, protos) 
+            select_called.append(conn)
+            return protos[0]
+        clientContext.set_next_proto_select_callback(next_proto_select)
+        clientSSL = Connection(clientContext, client)
+        clientSSL.set_connect_state()
+
+        serverContext = Context(TLSv1_METHOD)
+        advertised_called = []
+        def next_protos(conn):
+            advertised_called.append(conn)
+            return server_protos
+        serverContext.set_next_protos_advertised_callback(next_protos)
+        serverContext.use_certificate(
+            load_certificate(FILETYPE_PEM, cleartextCertificatePEM))
+        serverContext.use_privatekey(
+            load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM))
+
+        serverSSL = Connection(serverContext, server)
+        serverSSL.set_accept_state()
+
+        while not (advertised_called and select_called):
+            for ssl in clientSSL, serverSSL:
+                try:
+                    ssl.do_handshake()
+                except WantReadError:
+                    pass
+
+        # Kind of lame.  Just make sure it got called somehow.
+        self.assertTrue(select_called)
+        self.assertTrue(advertised_called)
+        self.assertEqual(server_protos[0], serverSSL.get_next_proto_negotiated())
+
 
     def _load_verify_locations_test(self, *args):
         """