[glib-networking/mcatanzaro/#127] Fix peer-certificate[-errors] props set too soon

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 8ed9fc6e0071673487dafebab55718602d89297d
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Sat Feb 1 17:03:57 2020 -0600

    Fix peer-certificate[-errors] props set too soon
    
    Our documentation says these properties will not be set until *after* a
    *successful* handshake. Sadly, libsoup really depends on it being set
    after every handshake, but at least we can fix it to not be set until
    after accept-certificate. We can update the documentation to specify
    that these properties will be set after unsuccessful handshakes, since
    that has always been our behavior and we can't change that without
    breaking libsoup.
    
    Since OpenSSL allows its handshake to succeed before manually failing it
    afterwards, we need to manually add an error in that case. I haven't
    investigated far enough to know why, but it doesn't matter because I
    have a sidebranch where OpenSSL handshakes normally without this hack,
    which I hope to land soon.
    
    Fixes #127

 tls/base/gtlsconnection-base.c       | 64 ++++++++++++-------------
 tls/openssl/gtlsconnection-openssl.c |  6 ++-
 tls/tests/connection.c               | 90 ++++++++++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+), 35 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index 7f0e6fe..931e1cf 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -1280,51 +1280,28 @@ verify_peer_certificate (GTlsConnectionBase *tls,
   return errors;
 }
 
-static void
-update_peer_certificate_and_compute_errors (GTlsConnectionBase *tls)
+static gboolean
+accept_or_reject_peer_certificate (gpointer user_data)
 {
+  GTlsConnectionBase *tls = user_data;
   GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
   GTlsCertificate *peer_certificate = NULL;
   GTlsCertificateFlags peer_certificate_errors = 0;
+  gboolean accepted = FALSE;
 
   /* This function must be called from the handshake context thread
    * (probably the main thread, NOT the handshake thread) because
    * it emits notifies that are application-visible.
-   *
-   * verify_certificate_mutex should be locked.
    */
   g_assert (priv->handshake_context);
   g_assert (g_main_context_is_owner (priv->handshake_context));
 
   peer_certificate = G_TLS_CONNECTION_BASE_GET_CLASS (tls)->retrieve_peer_certificate (tls);
-  if (peer_certificate)
-    peer_certificate_errors = verify_peer_certificate (tls, peer_certificate);
-
-  g_clear_object (&priv->peer_certificate);
-  priv->peer_certificate = g_steal_pointer (&peer_certificate);
-  g_clear_object (&peer_certificate);
-
-  priv->peer_certificate_errors = peer_certificate_errors;
-
-  g_object_notify (G_OBJECT (tls), "peer-certificate");
-  g_object_notify (G_OBJECT (tls), "peer-certificate-errors");
-}
 
-static gboolean
-accept_or_reject_peer_certificate (gpointer user_data)
-{
-  GTlsConnectionBase *tls = user_data;
-  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
-  gboolean accepted = FALSE;
-
-  g_assert (g_main_context_is_owner (priv->handshake_context));
-
-  g_mutex_lock (&priv->verify_certificate_mutex);
-
-  update_peer_certificate_and_compute_errors (tls);
-
-  if (priv->peer_certificate)
+  if (peer_certificate)
     {
+      peer_certificate_errors = verify_peer_certificate (tls, peer_certificate);
+
       if (G_IS_TLS_CLIENT_CONNECTION (tls))
         {
           GTlsCertificateFlags validation_flags;
@@ -1336,7 +1313,7 @@ accept_or_reject_peer_certificate (gpointer user_data)
             validation_flags =
               g_dtls_client_connection_get_validation_flags (G_DTLS_CLIENT_CONNECTION (tls));
 
-          if ((priv->peer_certificate_errors & validation_flags) == 0)
+          if ((peer_certificate_errors & validation_flags) == 0)
             accepted = TRUE;
         }
 
@@ -1352,8 +1329,8 @@ accept_or_reject_peer_certificate (gpointer user_data)
             g_main_context_pop_thread_default (priv->handshake_context);
 
           accepted = g_tls_connection_emit_accept_certificate (G_TLS_CONNECTION (tls),
-                                                               priv->peer_certificate,
-                                                               priv->peer_certificate_errors);
+                                                               peer_certificate,
+                                                               peer_certificate_errors);
 
           if (sync_handshake_in_progress)
             g_main_context_push_thread_default (priv->handshake_context);
@@ -1371,8 +1348,20 @@ accept_or_reject_peer_certificate (gpointer user_data)
         accepted = TRUE;
     }
 
+  g_mutex_lock (&priv->verify_certificate_mutex);
+
   priv->peer_certificate_accepted = accepted;
 
+  /* Warning: the API documentation indicates that these properties are not
+   * set until *after* accept-certificate.
+   */
+  g_clear_object (&priv->peer_certificate);
+  priv->peer_certificate = g_steal_pointer (&peer_certificate);
+  priv->peer_certificate_errors = peer_certificate_errors;
+
+  g_object_notify (G_OBJECT (tls), "peer-certificate");
+  g_object_notify (G_OBJECT (tls), "peer-certificate-errors");
+
   /* This has to be the very last statement before signaling the
    * condition variable because otherwise the code could spuriously
    * wakeup and continue before we are done here.
@@ -1584,7 +1573,14 @@ finish_handshake (GTlsConnectionBase  *tls,
            * anything with the result here.
            */
           g_mutex_lock (&priv->verify_certificate_mutex);
-          update_peer_certificate_and_compute_errors (tls);
+
+          g_clear_object (&priv->peer_certificate);
+          priv->peer_certificate = G_TLS_CONNECTION_BASE_GET_CLASS (tls)->retrieve_peer_certificate (tls);
+          priv->peer_certificate_errors = verify_peer_certificate (tls, priv->peer_certificate);
+
+          g_object_notify (G_OBJECT (tls), "peer-certificate");
+          g_object_notify (G_OBJECT (tls), "peer-certificate-errors");
+
           priv->peer_certificate_examined = TRUE;
           priv->peer_certificate_accepted = TRUE;
           g_mutex_unlock (&priv->verify_certificate_mutex);
diff --git a/tls/openssl/gtlsconnection-openssl.c b/tls/openssl/gtlsconnection-openssl.c
index fe77300..c8e0f5b 100644
--- a/tls/openssl/gtlsconnection-openssl.c
+++ b/tls/openssl/gtlsconnection-openssl.c
@@ -303,7 +303,11 @@ g_tls_connection_openssl_handshake_thread_handshake (GTlsConnectionBase  *tls,
   if (ret > 0)
     {
       if (!g_tls_connection_base_handshake_thread_verify_certificate (G_TLS_CONNECTION_BASE (openssl)))
-        return G_TLS_CONNECTION_BASE_ERROR;
+        {
+          g_set_error_literal (error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
+                               _("Unacceptable TLS certificate"));
+          return G_TLS_CONNECTION_BASE_ERROR;
+        }
     }
 
   return status;
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index 936d1f9..7f119a9 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -2512,6 +2512,94 @@ test_connection_missing_server_identity (TestConnection *test,
   g_assert_no_error (test->server_error);
 }
 
+typedef struct {
+  TestConnection *test;
+  gboolean peer_certificate_notified;
+  gboolean peer_certificate_errors_notified;
+} NotifyTestData;
+
+static gboolean
+on_accept_certificate_peer_certificate_notify (GTlsConnection       *conn,
+                                               GTlsCertificate      *cert,
+                                               GTlsCertificateFlags  errors,
+                                               NotifyTestData       *data)
+{
+  TestConnection *test = data->test;
+
+  g_assert_true (G_IS_TLS_CERTIFICATE (cert));
+
+  /* We guarantee these props are not set until after the handshake. */
+  g_assert_null (g_tls_connection_get_peer_certificate (conn));
+  g_assert_cmpint (g_tls_connection_get_peer_certificate_errors (conn), ==, 0);
+
+  g_assert_false (data->peer_certificate_notified);
+  g_assert_false (data->peer_certificate_errors_notified);
+
+  return errors == test->accept_flags;
+}
+
+static void
+on_peer_certificate_notify (GTlsConnection *conn,
+                            GParamSpec     *pspec,
+                            gboolean       *notified)
+{
+  *notified = TRUE;
+}
+
+static void
+on_peer_certificate_errors_notify (GTlsConnection *conn,
+                                   GParamSpec     *pspec,
+                                   gboolean       *notified)
+{
+  *notified = TRUE;
+}
+
+static void
+test_peer_certificate_notify (TestConnection *test,
+                              gconstpointer   data)
+{
+  NotifyTestData notify_data = { test, FALSE, FALSE };
+  GIOStream *connection;
+  GError *error = NULL;
+
+  connection = start_async_server_and_connect_to_it (test, G_TLS_AUTHENTICATION_NONE);
+  test->client_connection = g_tls_client_connection_new (connection, test->identity, &error);
+  g_assert_no_error (error);
+  g_object_unref (connection);
+
+  /* For this test, we need validation to fail to ensure that the
+   * accept-certificate signal gets emitted.
+   */
+  g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection),
+                                                G_TLS_CERTIFICATE_VALIDATE_ALL);
+
+  g_signal_connect (test->client_connection, "accept-certificate",
+                    G_CALLBACK (on_accept_certificate_peer_certificate_notify), &notify_data);
+  g_signal_connect (test->client_connection, "notify::peer-certificate",
+                    G_CALLBACK (on_peer_certificate_notify), &notify_data.peer_certificate_notified);
+  g_signal_connect (test->client_connection, "notify::peer-certificate-errors",
+                    G_CALLBACK (on_peer_certificate_errors_notify), 
&notify_data.peer_certificate_errors_notified);
+
+  read_test_data_async (test);
+  g_main_loop_run (test->loop);
+  wait_until_server_finished (test);
+
+  g_assert_true (notify_data.peer_certificate_notified);
+  g_assert_true (notify_data.peer_certificate_errors_notified);
+
+  g_assert_true (G_IS_TLS_CERTIFICATE (g_tls_connection_get_peer_certificate (G_TLS_CONNECTION 
(test->client_connection))));
+  g_assert_cmpint (g_tls_connection_get_peer_certificate_errors (G_TLS_CONNECTION 
(test->client_connection)), !=, 0);
+
+  g_assert_error (test->read_error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE);
+#ifdef BACKEND_IS_GNUTLS
+  g_assert_error (test->server_error, G_TLS_ERROR, G_TLS_ERROR_NOT_TLS);
+#elif defined(BACKEND_IS_OPENSSL)
+  /* FIXME: This is not OK. There should be a NOT_TLS errors. But some times
+   * we either get no error or BROKEN_PIPE
+   */
+#endif
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -2600,6 +2688,8 @@ main (int   argc,
               setup_connection, test_socket_timeout, teardown_connection);
   g_test_add ("/tls/" BACKEND "/connection/missing-server-identity", TestConnection, NULL,
               setup_connection, test_connection_missing_server_identity, teardown_connection);
+  g_test_add ("/tls/" BACKEND "/connection/peer-certificate-notify", TestConnection, NULL,
+              setup_connection, test_peer_certificate_notify, teardown_connection);
 
   ret = g_test_run ();