[glib-networking] Revert "Fix peer-certificate[-errors] props set too soon"

[Michael Catanzaro <mcatanzaro src gnome org>]

commit b56ba8c6d214453b2394096926bdc916156b0ab9
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Thu Feb 27 19:06:25 2020 -0600

    Revert "Fix peer-certificate[-errors] props set too soon"
    
    This reverts commit 52038963fc9f3c0ffbaf1ef8cb51a5774d3d924a.
    
    It broke libsoup. Fixes #129

 tls/base/gtlsconnection-base.c       |  72 +++++++++----------
 tls/openssl/gtlsconnection-openssl.c |   6 +-
 tls/tests/connection.c               | 131 -----------------------------------
 3 files changed, 35 insertions(+), 174 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index b17cc8d..d6c4e37 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -1272,28 +1272,51 @@ verify_peer_certificate (GTlsConnectionBase *tls,
   return errors;
 }
 
-static gboolean
-accept_or_reject_peer_certificate (gpointer user_data)
+static void
+update_peer_certificate_and_compute_errors (GTlsConnectionBase *tls)
 {
-  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);
+    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 (G_IS_TLS_CLIENT_CONNECTION (tls))
         {
           GTlsCertificateFlags validation_flags;
@@ -1305,7 +1328,7 @@ accept_or_reject_peer_certificate (gpointer user_data)
             validation_flags =
               g_dtls_client_connection_get_validation_flags (G_DTLS_CLIENT_CONNECTION (tls));
 
-          if ((peer_certificate_errors & validation_flags) == 0)
+          if ((priv->peer_certificate_errors & validation_flags) == 0)
             accepted = TRUE;
         }
 
@@ -1321,8 +1344,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),
-                                                               peer_certificate,
-                                                               peer_certificate_errors);
+                                                               priv->peer_certificate,
+                                                               priv->peer_certificate_errors);
 
           if (sync_handshake_in_progress)
             g_main_context_push_thread_default (priv->handshake_context);
@@ -1340,28 +1363,8 @@ accept_or_reject_peer_certificate (gpointer user_data)
         accepted = TRUE;
     }
 
-  g_mutex_lock (&priv->verify_certificate_mutex);
-
   priv->peer_certificate_accepted = accepted;
 
-  if (accepted)
-    {
-      /* The API documentation indicates that these properties are not
-       * set until after a successful handshake.
-       */
-      g_clear_object (&priv->peer_certificate);
-      priv->peer_certificate = g_steal_pointer (&peer_certificate);
-      priv->peer_certificate_errors = peer_certificate_errors;
-    }
-  else
-    {
-      g_clear_object (&peer_certificate);
-      g_clear_object (&priv->peer_certificate);
-      priv->peer_certificate_errors = 0;
-    }
-  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.
@@ -1573,14 +1576,7 @@ finish_handshake (GTlsConnectionBase  *tls,
            * anything with the result here.
            */
           g_mutex_lock (&priv->verify_certificate_mutex);
-
-          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");
-
+          update_peer_certificate_and_compute_errors (tls);
           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 a78309d..d751344 100644
--- a/tls/openssl/gtlsconnection-openssl.c
+++ b/tls/openssl/gtlsconnection-openssl.c
@@ -304,11 +304,7 @@ 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)))
-        {
-          g_set_error_literal (error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
-                               _("Unacceptable TLS certificate"));
-          return G_TLS_CONNECTION_BASE_ERROR;
-        }
+        return G_TLS_CONNECTION_BASE_ERROR;
     }
 
   return status;
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index 1b1583b..21a150a 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -2440,135 +2440,6 @@ test_socket_timeout (TestConnection *test,
   g_assert_error (test->server_error, G_TLS_ERROR, G_TLS_ERROR_NOT_TLS);
 }
 
-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);
-
-  /* These properties should only be set after a *successful* handshake. */
-  g_assert_null (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 an error here. */
-  g_assert_no_error (test->server_error);
-#endif
-
-  /* Now do it all again, this time with a successful connection. */
-
-  g_clear_object (&test->service);
-  g_clear_object (&test->server_connection);
-  g_clear_object (&test->client_connection);
-
-  g_clear_error (&test->read_error);
-  g_clear_error (&test->server_error);
-
-  notify_data.peer_certificate_notified = FALSE;
-  notify_data.peer_certificate_errors_notified = FALSE;
-
-  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);
-
-  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);
-
-  /* Disable validation so we have a successful handshake. */
-  g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection),
-                                                0);
-  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_no_error (test->read_error);
-  g_assert_no_error (test->server_error);
-}
-
 int
 main (int   argc,
       char *argv[])
@@ -2655,8 +2526,6 @@ main (int   argc,
               setup_connection, test_sync_op_during_handshake, teardown_connection);
   g_test_add ("/tls/" BACKEND "/connection/socket-timeout", TestConnection, NULL,
               setup_connection, test_socket_timeout, 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 ();