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

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 756fcf7da50dad2d9cf1036e4a0f7cc80c8397e5
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 also means 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, it's very likely that the
    certificate was required. This way, we don't have to change the
    testsuite to expect different errors depending on TLS version, and can
    provide the same consistent errors to API users. The disadvantage is
    that there is a small chance we give the wrong error here, but it's only
    possible if the server requests but does not require a client
    certificate, which should be a miniscule number of servers in practice.
    (But it's not zero, or Cockpit would not have discovered this bug!)
    
    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           | 11 ++++++++--
 tls/base/gtlsconnection-base.h           |  2 ++
 tls/gnutls/gtlsclientconnection-gnutls.c |  4 +++-
 tls/gnutls/gtlsconnection-gnutls.c       | 14 ++++++++++++
 tls/tests/connection.c                   | 37 ++++++++++++++++++++++++++++----
 5 files changed, 61 insertions(+), 7 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index 8bb5f7f..f4c907e 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -1379,7 +1379,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)
@@ -2380,13 +2380,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;
 }
diff --git a/tls/base/gtlsconnection-base.h b/tls/base/gtlsconnection-base.h
index 98632ad..7b0a6b6 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);
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..263d3a8 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -456,6 +456,20 @@ end_gnutls_io (GTlsConnectionGnutls  *gnutls,
       return G_TLS_CONNECTION_BASE_ERROR;
     }
 
+  if (ret == GNUTLS_E_INVALID_SESSION &&
+      g_tls_connection_base_get_missing_requested_client_certificate (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..c21d57b 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,20 @@ 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)
+    {
+      /* Although very rare, it's OK for broken pipe errors to not occur here if
+       * they have already occured earlier during a read. If so, there should be
+       * no error here at all.
+       */
+      if (error || !g_error_matches (test->expected_client_close_error, G_IO_ERROR, G_IO_ERROR_BROKEN_PIPE))
+        g_assert_error (error, test->expected_client_close_error->domain, 
test->expected_client_close_error->code);
+    }
+  else
+    {
+      g_assert_no_error (error);
+    }
 
   g_main_loop_quit (test->loop);
 }
@@ -1096,6 +1111,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_IO_ERROR, G_IO_ERROR_BROKEN_PIPE, "");
+
   read_test_data_async (test);
   g_main_loop_run (test->loop);
   wait_until_server_finished (test);
@@ -1283,14 +1300,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_IO_ERROR, G_IO_ERROR_BROKEN_PIPE, "");
+
   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 +1333,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 +1350,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);
 }