[glib-networking/mcatanzaro/#92] Don't hide errors from TLS interaction

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 6c3f9d1de121df134e13dbccf7477ab939051339
Author: Michael Catanzaro <mcatanzaro igalia com>
Date:   Mon Jul 22 14:30:29 2019 -0500

    Don't hide errors from TLS interaction
    
    The previous commit was arguably a bit too aggressive in that errors
    from the TLS interaction are no longer reported, so the application will
    never see the reason for why a client certificate was not provided. I'm
    not honestly sure what the desired behavior here is, but I suppose it's
    not great if the interaction errors are never used for anything. Also,
    this is consistent with our original behavior from glib-networking 2.54,
    back before I started messing with glib-networking, so that seems good.

 tls/base/gtlsconnection-base.c     | 62 ++++++++++++++++++--------------------
 tls/base/gtlsconnection-base.h     |  5 ---
 tls/gnutls/gtlsconnection-gnutls.c | 15 ---------
 tls/tests/connection.c             | 15 ++-------
 4 files changed, 31 insertions(+), 66 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index 804eea3..259cdf0 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -735,6 +735,7 @@ g_tls_connection_base_real_pop_io (GTlsConnectionBase  *tls,
       else
         g_clear_error (&priv->read_error);
     }
+
   if (direction & G_IO_OUT)
     {
       priv->write_cancellable = NULL;
@@ -755,13 +756,39 @@ g_tls_connection_base_real_pop_io (GTlsConnectionBase  *tls,
       g_propagate_error (error, my_error);
       return G_TLS_CONNECTION_BASE_WOULD_BLOCK;
     }
-  else if (g_error_matches (my_error, G_IO_ERROR, G_IO_ERROR_TIMED_OUT))
+
+  if (g_error_matches (my_error, G_IO_ERROR, G_IO_ERROR_TIMED_OUT))
     {
       g_propagate_error (error, my_error);
       return G_TLS_CONNECTION_BASE_TIMED_OUT;
     }
+
+  if (priv->missing_requested_client_certificate &&
+      !priv->successful_posthandshake_op)
+    {
+      /* Probably the server requires a client certificate, but we failed to
+       * provide one. With TLS 1.3 the server is no longer able to tell us
+       * this, so we just have to guess. This only applies to the small minority
+       * of connections where a client cert is requested but not provided, and
+       * then only if the client has seen a "successful" handshake but never a
+       * successful read or write.
+       */
+      if (priv->interaction_error)
+        {
+          g_propagate_error (error, priv->interaction_error);
+          priv->interaction_error = NULL;
+        }
+      else
+        {
+          g_clear_error (error);
+          g_set_error_literal (error, G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED,
+                               _("Server required TLS certificate"));
+        }
+    }
   else if (my_error)
-    g_propagate_error (error, my_error);
+    {
+      g_propagate_error (error, my_error);
+    }
 
   return G_TLS_CONNECTION_BASE_ERROR;
 }
@@ -1381,21 +1408,6 @@ handshake_thread (GTask        *task,
   tls_class->handshake_thread_handshake (tls, timeout, cancellable, &error);
   priv->need_handshake = FALSE;
 
-  if (error && priv->missing_requested_client_certificate)
-    {
-      g_clear_error (&error);
-      if (priv->interaction_error)
-        {
-          error = priv->interaction_error;
-          priv->interaction_error = NULL;
-        }
-      else
-        {
-          g_set_error_literal (&error, G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED,
-                               _("Server required TLS certificate"));
-        }
-    }
-
   if (error)
     {
       g_task_return_error (task, error);
@@ -2398,14 +2410,6 @@ g_tls_connection_base_get_base_ostream (GTlsConnectionBase *tls)
   return priv->base_ostream;
 }
 
-gboolean
-g_tls_connection_base_get_missing_requested_client_certificate (GTlsConnectionBase *tls)
-{
-  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
-
-  return priv->missing_requested_client_certificate;
-}
-
 void
 g_tls_connection_base_set_missing_requested_client_certificate (GTlsConnectionBase *tls)
 {
@@ -2519,14 +2523,6 @@ g_tls_connection_base_buffer_application_data (GTlsConnectionBase *tls,
   g_byte_array_append (priv->app_data_buf, data, length);
 }
 
-gboolean
-g_tls_connection_base_has_successful_posthandshake_op (GTlsConnectionBase *tls)
-{
-  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
-
-  return priv->successful_posthandshake_op;
-}
-
 static void
 g_tls_connection_base_class_init (GTlsConnectionBaseClass *klass)
 {
diff --git a/tls/base/gtlsconnection-base.h b/tls/base/gtlsconnection-base.h
index 483b41a..54ce895 100644
--- a/tls/base/gtlsconnection-base.h
+++ b/tls/base/gtlsconnection-base.h
@@ -175,8 +175,6 @@ GPollableOutputStream    *g_tls_connection_base_get_base_ostream        (GTlsCon
 
 void                      g_tls_connection_base_set_missing_requested_client_certificate
                                                                         (GTlsConnectionBase *tls);
-gboolean                  g_tls_connection_base_get_missing_requested_client_certificate
-                                                                        (GTlsConnectionBase *tls);
 
 GError                  **g_tls_connection_base_get_read_error          (GTlsConnectionBase *tls);
 GError                  **g_tls_connection_base_get_write_error         (GTlsConnectionBase *tls);
@@ -197,9 +195,6 @@ void                      g_tls_connection_base_buffer_application_data (GTlsCon
                                                                          guint8             *data,
                                                                          gsize               length);
 
-gboolean                  g_tls_connection_base_has_successful_posthandshake_op
-                                                                        (GTlsConnectionBase *tls);
-
 void                      GTLS_DEBUG                                    (gpointer    connection,
                                                                          const char *message,
                                                                          ...);
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index b504635..0990006 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -456,21 +456,6 @@ end_gnutls_io (GTlsConnectionGnutls  *gnutls,
       return G_TLS_CONNECTION_BASE_ERROR;
     }
 
-  if ((ret == GNUTLS_E_INVALID_SESSION || g_error_matches (my_error, G_IO_ERROR, 
G_IO_ERROR_CONNECTION_CLOSED)) &&
-      g_tls_connection_base_get_missing_requested_client_certificate (tls) &&
-      !g_tls_connection_base_has_successful_posthandshake_op (tls))
-    {
-      /* Probably the server requires a client certificate, but we failed to
-       * provide one. With TLS 1.3 the server is no longer able to tell us
-       * this so we just have to guess. This only applies to the small minority
-       * of connections where a client cert is requested but not provided.
-       */
-      g_clear_error (&my_error);
-      g_set_error_literal (error, G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED,
-                           _("Server required TLS certificate"));
-      return G_TLS_CONNECTION_BASE_ERROR;
-    }
-
   if (error && my_error)
     g_propagate_error (error, my_error);
 
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index 2da20ce..29fce2e 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -482,12 +482,7 @@ on_client_connection_close_finish (GObject        *object,
   g_io_stream_close_finish (G_IO_STREAM (object), res, &error);
 
   if (test->expected_client_close_error)
-#ifdef BACKEND_IS_GNUTLS
     g_assert_error (error, test->expected_client_close_error->domain, 
test->expected_client_close_error->code);
-#else
-    /* FIXME: Fix this for OpenSSL. */
-    ;
-#endif
   else
     g_assert_no_error (error);
 
@@ -1303,16 +1298,10 @@ test_client_auth_request_fail (TestConnection *test,
   g_main_loop_run (test->loop);
   wait_until_server_finished (test);
 
-#if BACKEND_IS_GNUTLS
-  g_assert_error (test->read_error, G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED);
-#elif BACKEND_IS_OPENSSL
-  /* FIXME: G_FILE_ERROR_ACCES is the error returned by our mock interaction
-   * object when the GTlsInteraction's certificate request fails. OpenSSL needs
-   * to fudge the error a bit, matching the GNUTLS_E_INVALID_SESSION case in
-   * GTlsConnectionGnutls's end_gnutls_io().
+  /* G_FILE_ERROR_ACCES is the error returned by our mock interaction object
+   * when the GTlsInteraction's certificate request fails.
    */
   g_assert_error (test->read_error, G_FILE_ERROR, G_FILE_ERROR_ACCES);
-#endif
   g_assert_error (test->server_error, G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED);
 
   g_io_stream_close (test->server_connection, NULL, NULL);