[glib-networking/mcatanzaro/#92: 2/3] Fix G_TLS_AUTHENTICATION_REQUESTED
- From: Michael Catanzaro <mcatanzaro src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [glib-networking/mcatanzaro/#92: 2/3] Fix G_TLS_AUTHENTICATION_REQUESTED
- Date: Fri, 12 Jul 2019 17:13:28 +0000 (UTC)
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);
}
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]