GNOME.org
 

[glib-networking/mcatanzaro/#92: 1/2] Fix G_TLS_AUTHENTICATION_REQUESTED

commit fee869e20934d9957c784d9e6c0f577637a30c35
Author: Michael Catanzaro <mcatanzaro igalia com>
Date:   Fri Jul 12 11:59:00 2019 -0500

    Fix G_TLS_AUTHENTICATION_REQUESTED
    
    I accidentally broke G_TLS_AUTHENTICATION_REQUESTED when converting
    GnuTLS to use the base class code. It currently functions the same as
    G_TLS_AUTHENTICATION_REQUIRED, where we fail the connection if the
    client does not provide a client certificate. This is not correct: the
    client certificate is supposed to be optional.
    
    Fixing this means that the test server will now see broken pipe errors
    when trying to close its connection to the client during our two client
    auth failure tests once again. I don't know why, exactly, but it doesn't
    seem terrible, so let's expect it. I had previously added code to deal
    with this, but removed it once I discovered that it was mysteriously no
    longer required (because I had broken G_TLS_AUTHENTICATION_REQUIRED).
    
    This fix on its own would mean that the client will start to receive
    G_TLS_ERROR_MISC instead of G_TLS_ERROR_CERTIFICATE_REQUIRED if TLS 1.3
    is in use. I used to have a bunch of code in the testsuite to handle
    this condition to ensure the tests expect different results depending on
    which version of TLS is being used, but it would be much better to
    present the same errors. Unfortunately, the client cannot know for sure
    that the client certificate was required -- I'm not sure if this is a
    limitation of the TLS protocol, or just the GnuTLS API -- but we can
    reasonably guess that if we failed to provide a requested client cert,
    and we see the error GNUTLS_E_INVALID_SESSION or
    G_IO_ERROR_CONNECTION_CLOSED, it's very likely that the certificate was
    required required. This way, we don't have to change the testsuite to
    expect different errors depending on TLS version, and we can provide the
    same consistent errors to API users. We fudge the error here only if the
    handshake has completed successfully but no read or write has ever
    succeeded, corresponding to the case where the TLS 1.3 client is unable
    to realize that it has failed authentication.
    
    Finally, note that the previous commit, by Martin Pitt, adds a test to
    ensure this doesn't break again.
    
    Fixes #92

 tls/base/gtlsconnection-base.c           | 65 ++++++++++++++++++++++++--------
 tls/base/gtlsconnection-base.h           |  5 +++
 tls/gnutls/gtlsclientconnection-gnutls.c |  4 +-
 tls/gnutls/gtlsconnection-gnutls.c       | 15 ++++++++
 tls/tests/connection.c                   | 34 +++++++++++++++--
 5 files changed, 102 insertions(+), 21 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index 8bb5f7f..0c5868f 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -147,6 +147,8 @@ typedef struct
   GError        *write_error;
   GCancellable  *write_cancellable;
 
+  gboolean       successful_posthandshake_op;
+
   gboolean       is_system_certdb;
   gboolean       database_is_unset;
 
@@ -1379,7 +1381,7 @@ handshake_thread (GTask        *task,
   tls_class->handshake_thread_handshake (tls, timeout, cancellable, &error);
   priv->need_handshake = FALSE;
 
-  if (priv->missing_requested_client_certificate)
+  if (error && priv->missing_requested_client_certificate)
     {
       g_clear_error (&error);
       if (priv->certificate_error)
@@ -1829,9 +1831,12 @@ g_tls_connection_base_read (GTlsConnectionBase  *tls,
   while (status == G_TLS_CONNECTION_BASE_REHANDSHAKE);
 
   if (status == G_TLS_CONNECTION_BASE_OK)
-    return nread;
-  else
-    return -1;
+    {
+      priv->successful_posthandshake_op = TRUE;
+      return nread;
+    }
+
+  return -1;
 }
 
 static gssize
@@ -1883,7 +1888,11 @@ g_tls_connection_base_read_message (GTlsConnectionBase  *tls,
   } while (status == G_TLS_CONNECTION_BASE_REHANDSHAKE);
 
   if (status == G_TLS_CONNECTION_BASE_OK)
-    return nread;
+    {
+      priv->successful_posthandshake_op = TRUE;
+      return nread;
+    }
+
   return -1;
 }
 
@@ -1896,12 +1905,11 @@ g_tls_connection_base_receive_messages (GDatagramBased  *datagram_based,
                                         GCancellable    *cancellable,
                                         GError         **error)
 {
-  GTlsConnectionBase *tls;
+  GTlsConnectionBase *tls = G_TLS_CONNECTION_BASE (datagram_based);
+  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
   guint i;
   GError *child_error = NULL;
 
-  tls = G_TLS_CONNECTION_BASE (datagram_based);
-
   if (flags != G_SOCKET_MSG_NONE)
     {
       g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
@@ -1961,6 +1969,7 @@ g_tls_connection_base_receive_messages (GDatagramBased  *datagram_based,
       return -1;
     }
 
+  priv->successful_posthandshake_op = TRUE;
   return i;
 }
 
@@ -1972,6 +1981,7 @@ g_tls_connection_base_write (GTlsConnectionBase  *tls,
                              GCancellable        *cancellable,
                              GError             **error)
 {
+  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
   GTlsConnectionBaseStatus status;
   gssize nwrote;
 
@@ -1989,9 +1999,12 @@ g_tls_connection_base_write (GTlsConnectionBase  *tls,
   while (status == G_TLS_CONNECTION_BASE_REHANDSHAKE);
 
   if (status == G_TLS_CONNECTION_BASE_OK)
-    return nwrote;
-  else
-    return -1;
+    {
+      priv->successful_posthandshake_op = TRUE;
+      return nwrote;
+    }
+
+  return -1;
 }
 
 static gssize
@@ -2002,6 +2015,7 @@ g_tls_connection_base_write_message (GTlsConnectionBase  *tls,
                                      GCancellable        *cancellable,
                                      GError             **error)
 {
+  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
   GTlsConnectionBaseStatus status;
   gssize nwrote;
 
@@ -2018,7 +2032,11 @@ g_tls_connection_base_write_message (GTlsConnectionBase  *tls,
   } while (status == G_TLS_CONNECTION_BASE_REHANDSHAKE);
 
   if (status == G_TLS_CONNECTION_BASE_OK)
-    return nwrote;
+    {
+      priv->successful_posthandshake_op = TRUE;
+      return nwrote;
+    }
+
   return -1;
 }
 
@@ -2031,12 +2049,11 @@ g_tls_connection_base_send_messages (GDatagramBased  *datagram_based,
                                      GCancellable    *cancellable,
                                      GError         **error)
 {
-  GTlsConnectionBase *tls;
+  GTlsConnectionBase *tls = G_TLS_CONNECTION_BASE (datagram_based);
+  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
   guint i;
   GError *child_error = NULL;
 
-  tls = G_TLS_CONNECTION_BASE (datagram_based);
-
   if (flags != G_SOCKET_MSG_NONE)
     {
       g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
@@ -2084,6 +2101,7 @@ g_tls_connection_base_send_messages (GDatagramBased  *datagram_based,
       return -1;
     }
 
+  priv->successful_posthandshake_op = TRUE;
   return i;
 }
 
@@ -2380,13 +2398,20 @@ 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)
 {
   GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
 
   /* FIXME: Assert this is only used on the handshake thread. */
-  /* FIXME: Assert it's not already requested? Probably. */
 
   priv->missing_requested_client_certificate = TRUE;
 }
@@ -2500,6 +2525,14 @@ 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 98632ad..4361717 100644
--- a/tls/base/gtlsconnection-base.h
+++ b/tls/base/gtlsconnection-base.h
@@ -175,6 +175,8 @@ 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_certificate_error   (GTlsConnectionBase *tls);
 GError                  **g_tls_connection_base_get_read_error          (GTlsConnectionBase *tls);
@@ -197,6 +199,9 @@ 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/gtlsclientconnection-gnutls.c b/tls/gnutls/gtlsclientconnection-gnutls.c
index e51b30d..f1a8921 100644
--- a/tls/gnutls/gtlsclientconnection-gnutls.c
+++ b/tls/gnutls/gtlsclientconnection-gnutls.c
@@ -362,7 +362,9 @@ g_tls_client_connection_gnutls_retrieve_function (gnutls_session_t
           g_tls_certificate_gnutls_copy_free (*pcert, *pcert_length, *pkey);
 
           /* If there is still no client certificate, this connection will
-           * probably fail, but no reason to give up: let's try anyway.
+           * probably fail, but we must not give up yet. The certificate might
+           * be optional, e.g. if the server is using
+           * G_TLS_AUTHENTICATION_REQUESTED, not G_TLS_AUTHENTICATION_REQUIRED.
            */
           g_tls_connection_base_set_missing_requested_client_certificate (tls);
           return 0;
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index 0990006..b504635 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -456,6 +456,21 @@ 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 71379f6..5f13763 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -85,6 +85,7 @@ typedef struct {
   GTlsCertificateFlags accept_flags;
   GError *read_error;
   GError *server_error;
+  GError *expected_client_close_error;
   ServerConnectionReceivedStrategy connection_received_strategy;
   gboolean server_running;
   GTlsCertificate *server_certificate;
@@ -171,6 +172,7 @@ teardown_connection (TestConnection *test, gconstpointer data)
 
   g_clear_error (&test->read_error);
   g_clear_error (&test->server_error);
+  g_clear_error (&test->expected_client_close_error);
 }
 
 static void
@@ -478,7 +480,16 @@ on_client_connection_close_finish (GObject        *object,
   GError *error = NULL;
 
   g_io_stream_close_finish (G_IO_STREAM (object), res, &error);
-  g_assert_no_error (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);
 
   g_main_loop_quit (test->loop);
 }
@@ -1096,6 +1107,8 @@ test_client_auth_failure (TestConnection *test,
   g_signal_connect (test->client_connection, "notify::accepted-cas",
                     G_CALLBACK (on_notify_accepted_cas), &accepted_changed);
 
+  g_set_error_literal (&test->expected_client_close_error, G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED, 
"");
+
   read_test_data_async (test);
   g_main_loop_run (test->loop);
   wait_until_server_finished (test);
@@ -1109,6 +1122,7 @@ test_client_auth_failure (TestConnection *test,
   g_clear_object (&test->server_connection);
   g_clear_error (&test->read_error);
   g_clear_error (&test->server_error);
+  g_clear_error (&test->expected_client_close_error);
 
   /* Now start a new connection to the same server with a valid client cert;
    * this should succeed, and not use the cached failed session from above */
@@ -1283,14 +1297,22 @@ test_client_auth_request_fail (TestConnection *test,
   g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection),
                                                 G_TLS_CERTIFICATE_VALIDATE_ALL);
 
+  g_set_error_literal (&test->expected_client_close_error, G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED, 
"");
+
   read_test_data_async (test);
   g_main_loop_run (test->loop);
   wait_until_server_finished (test);
 
-  /* G_FILE_ERROR_ACCES is the error returned by our mock interaction object
-   * when the GTlsInteraction's certificate request fails.
+#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_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);
@@ -1308,7 +1330,7 @@ test_client_auth_request_none (TestConnection *test,
   g_assert_no_error (error);
   g_assert_nonnull (test->database);
 
-  /* request, but don't provide a client certificate */
+  /* Request, but don't provide, a client certificate */
   connection = start_async_server_and_connect_to_it (test, G_TLS_AUTHENTICATION_REQUESTED);
   test->client_connection = g_tls_client_connection_new (connection, test->identity, &error);
   g_assert_no_error (error);
@@ -1325,6 +1347,10 @@ test_client_auth_request_none (TestConnection *test,
   g_main_loop_run (test->loop);
   wait_until_server_finished (test);
 
+  /* The connection should succeed and everything should work. We only REQUESTED
+   * authentication, in contrast to G_TLS_AUTHENTICATION_REQUIRED where this
+   * should fail.
+   */
   g_assert_no_error (test->read_error);
   g_assert_no_error (test->server_error);
 }
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]