GNOME.org
 

[glib-networking/mcatanzaro/session-resumption: 17/17] Revert "gnutls: support TLS 1.3 session tickets client-side"

commit 6cf4406257728a63feb0e16a1c1d6bbcda39042e
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Tue Oct 1 18:05:00 2019 -0500

    Revert "gnutls: support TLS 1.3 session tickets client-side"
    
    This reverts commit 1dfb04f1caa587a3000feda8c5eab369ab7f8a14.

 tls/gnutls/gtlsbackend-gnutls.c          |  85 +++++++++++++++--------
 tls/gnutls/gtlsbackend-gnutls.h          |  10 ++-
 tls/gnutls/gtlsclientconnection-gnutls.c | 114 +++++++++++++++----------------
 tls/gnutls/gtlsconnection-gnutls.c       |   6 +-
 tls/gnutls/gtlsconnection-gnutls.h       |   2 +
 5 files changed, 126 insertions(+), 91 deletions(-)
---
diff --git a/tls/gnutls/gtlsbackend-gnutls.c b/tls/gnutls/gtlsbackend-gnutls.c
index bd6863f..d0e5eaa 100644
--- a/tls/gnutls/gtlsbackend-gnutls.c
+++ b/tls/gnutls/gtlsbackend-gnutls.c
@@ -172,14 +172,15 @@ g_tls_backend_gnutls_interface_init (GTlsBackendInterface *iface)
  */
 
 G_LOCK_DEFINE_STATIC (session_cache_lock);
-GHashTable *client_session_cache; /* (owned) GBytes -> (owned) GTlsBackendGnutlsCacheData */
+GHashTable *client_session_cache, *server_session_cache;
 
 #define SESSION_CACHE_MAX_SIZE 50
 #define SESSION_CACHE_MAX_AGE (60 * 60) /* one hour */
 
 typedef struct {
-  GQueue *session_tickets; /* (owned) GBytes */
-  gint64 last_used;
+  GBytes *session_id;
+  GBytes *session_data;
+  time_t  last_used;
 } GTlsBackendGnutlsCacheData;
 
 static void
@@ -188,7 +189,7 @@ session_cache_cleanup (GHashTable *cache)
   GHashTableIter iter;
   gpointer key, value;
   GTlsBackendGnutlsCacheData *cache_data;
-  gint64 expired = g_get_monotonic_time () - SESSION_CACHE_MAX_AGE;
+  time_t expired = time (NULL) - SESSION_CACHE_MAX_AGE;
 
   g_hash_table_iter_init (&iter, cache);
   while (g_hash_table_iter_next (&iter, &key, &value))
@@ -200,51 +201,84 @@ session_cache_cleanup (GHashTable *cache)
 }
 
 static void
-cache_data_free (GTlsBackendGnutlsCacheData *data)
+cache_data_free (gpointer data)
 {
-  g_queue_free_full (data->session_tickets, (GDestroyNotify)g_bytes_unref);
-  g_free (data);
+  GTlsBackendGnutlsCacheData *cache_data = data;
+
+  g_bytes_unref (cache_data->session_id);
+  g_bytes_unref (cache_data->session_data);
+  g_free (cache_data);
 }
 
 static GHashTable *
-get_session_cache (gboolean create)
+get_session_cache (unsigned int type,
+                   gboolean     create)
 {
-  if (!client_session_cache && create)
+  GHashTable **cache_p;
+
+  cache_p = (type == GNUTLS_CLIENT) ? &client_session_cache : &server_session_cache;
+  if (!*cache_p && create)
     {
-      client_session_cache = g_hash_table_new_full (g_bytes_hash, g_bytes_equal,
-                                                    (GDestroyNotify)g_bytes_unref, 
(GDestroyNotify)cache_data_free);
+      *cache_p = g_hash_table_new_full (g_bytes_hash, g_bytes_equal,
+                                        NULL, cache_data_free);
     }
-  return client_session_cache;
+  return *cache_p;
 }
 
 void
-g_tls_backend_gnutls_store_session_data (GBytes *session_id,
-                                         GBytes *session_data)
+g_tls_backend_gnutls_store_session (unsigned int  type,
+                                    GBytes       *session_id,
+                                    GBytes       *session_data)
 {
   GTlsBackendGnutlsCacheData *cache_data;
   GHashTable *cache;
 
   G_LOCK (session_cache_lock);
 
-  cache = get_session_cache (TRUE);
+  cache = get_session_cache (type, TRUE);
   cache_data = g_hash_table_lookup (cache, session_id);
-  if (!cache_data)
+  if (cache_data)
+    {
+      if (!g_bytes_equal (cache_data->session_data, session_data))
+        {
+          g_bytes_unref (cache_data->session_data);
+          cache_data->session_data = g_bytes_ref (session_data);
+        }
+    }
+  else
     {
       if (g_hash_table_size (cache) >= SESSION_CACHE_MAX_SIZE)
         session_cache_cleanup (cache);
 
       cache_data = g_new (GTlsBackendGnutlsCacheData, 1);
-      cache_data->session_tickets = g_queue_new ();
-      g_hash_table_insert (cache, g_bytes_ref (session_id), cache_data);
+      cache_data->session_id = g_bytes_ref (session_id);
+      cache_data->session_data = g_bytes_ref (session_data);
+
+      g_hash_table_insert (cache, cache_data->session_id, cache_data);
     }
-  g_queue_push_tail (cache_data->session_tickets, g_bytes_ref (session_data));
-  cache_data->last_used = g_get_monotonic_time ();
+  cache_data->last_used = time (NULL);
+
+  G_UNLOCK (session_cache_lock);
+}
+
+void
+g_tls_backend_gnutls_remove_session (unsigned int  type,
+                                     GBytes       *session_id)
+{
+  GHashTable *cache;
+
+  G_LOCK (session_cache_lock);
+
+  cache = get_session_cache (type, FALSE);
+  if (cache)
+    g_hash_table_remove (cache, session_id);
 
   G_UNLOCK (session_cache_lock);
 }
 
 GBytes *
-g_tls_backend_gnutls_lookup_session_data (GBytes *session_id)
+g_tls_backend_gnutls_lookup_session (unsigned int  type,
+                                     GBytes       *session_id)
 {
   GTlsBackendGnutlsCacheData *cache_data;
   GBytes *session_data = NULL;
@@ -252,17 +286,14 @@ g_tls_backend_gnutls_lookup_session_data (GBytes *session_id)
 
   G_LOCK (session_cache_lock);
 
-  cache = get_session_cache (FALSE);
+  cache = get_session_cache (type, FALSE);
   if (cache)
     {
       cache_data = g_hash_table_lookup (cache, session_id);
       if (cache_data)
         {
-          /* Note that session tickets should be used only once since TLS 1.3,
-           * so we remove from the queue after retrieval. See RFC 8446 §C.4.
-           */
-          session_data = g_queue_pop_head (cache_data->session_tickets);
-          cache_data->last_used = g_get_monotonic_time ();
+          cache_data->last_used = time (NULL);
+          session_data = g_bytes_ref (cache_data->session_data);
         }
     }
 
diff --git a/tls/gnutls/gtlsbackend-gnutls.h b/tls/gnutls/gtlsbackend-gnutls.h
index fb33361..7b92bf3 100644
--- a/tls/gnutls/gtlsbackend-gnutls.h
+++ b/tls/gnutls/gtlsbackend-gnutls.h
@@ -35,8 +35,12 @@ G_DECLARE_FINAL_TYPE (GTlsBackendGnutls, g_tls_backend_gnutls, G, TLS_BACKEND_GN
 
 void  g_tls_backend_gnutls_register (GIOModule *module);
 
-void    g_tls_backend_gnutls_store_session_data  (GBytes *session_id,
-                                                  GBytes *session_data);
-GBytes *g_tls_backend_gnutls_lookup_session_data (GBytes *session_id);
+void    g_tls_backend_gnutls_store_session  (unsigned int             type,
+                                             GBytes                  *session_id,
+                                             GBytes                  *session_data);
+void    g_tls_backend_gnutls_remove_session (unsigned int             type,
+                                             GBytes                  *session_id);
+GBytes *g_tls_backend_gnutls_lookup_session (unsigned int             type,
+                                             GBytes                  *session_id);
 
 G_END_DECLS
diff --git a/tls/gnutls/gtlsclientconnection-gnutls.c b/tls/gnutls/gtlsclientconnection-gnutls.c
index 8061889..31104f1 100644
--- a/tls/gnutls/gtlsclientconnection-gnutls.c
+++ b/tls/gnutls/gtlsclientconnection-gnutls.c
@@ -52,15 +52,10 @@ struct _GTlsClientConnectionGnutls
   GTlsCertificateFlags validation_flags;
   GSocketConnectable *server_identity;
   gboolean use_ssl3;
+  gboolean session_data_override;
 
-  /* session_data is either the session ticket that was used to resume this
-   * connection, or the most recent session ticket received from the server.
-   * Because session ticket reuse is generally undesirable, it should only be
-   * accessed if session_data_override is set.
-   */
   GBytes *session_id;
   GBytes *session_data;
-  gboolean session_data_override;
 
   GPtrArray *accepted_cas;
   gboolean accepted_cas_changed;
@@ -70,7 +65,7 @@ struct _GTlsClientConnectionGnutls
   gnutls_privkey_t pkey;
 };
 
-static void g_tls_client_connection_gnutls_initable_interface_init (GInitableIface  *iface);
+static void     g_tls_client_connection_gnutls_initable_interface_init (GInitableIface  *iface);
 
 static void g_tls_client_connection_gnutls_client_connection_interface_init (GTlsClientConnectionInterface 
*iface);
 static void g_tls_client_connection_gnutls_dtls_client_connection_interface_init 
(GDtlsClientConnectionInterface *iface);
@@ -142,17 +137,12 @@ g_tls_client_connection_gnutls_compute_session_id (GTlsClientConnectionGnutls *g
   if (g_test_initialized ())
     return;
 
-  /* Create a TLS "session ID." We base it on the IP address since
+  /* Create a TLS session ID. We base it on the IP address since
    * different hosts serving the same hostname/service will probably
    * not share the same session cache. We base it on the
    * server-identity because at least some servers will fail (rather
    * than just failing to resume the session) if we don't.
    * (https://bugs.launchpad.net/bugs/823325)
-   *
-   * Note that our session IDs have no relation to TLS protocol
-   * session IDs, e.g. as provided by gnutls_session_get_id2(). Unlike
-   * our session IDs, actual TLS session IDs can no longer be used for
-   * session resumption.
    */
   g_object_get (G_OBJECT (gnutls), "base-io-stream", &base_conn, NULL);
   if (G_IS_SOCKET_CONNECTION (base_conn))
@@ -201,32 +191,6 @@ g_tls_client_connection_gnutls_compute_session_id (GTlsClientConnectionGnutls *g
   g_clear_object (&base_conn);
 }
 
-static int
-handshake_thread_session_ticket_received_cb (gnutls_session_t      session,
-                                             guint                 htype,
-                                             guint                 when,
-                                             guint                 incoming,
-                                             const gnutls_datum_t *msg)
-{
-  GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (gnutls_session_get_ptr (session));
-  gnutls_datum_t session_datum;
-
-  if (gnutls_session_get_data2 (session, &session_datum) == GNUTLS_E_SUCCESS)
-    {
-      g_clear_pointer (&gnutls->session_data, g_bytes_unref);
-      gnutls->session_data = g_bytes_new_with_free_func (session_datum.data,
-                                                         session_datum.size,
-                                                         (GDestroyNotify)gnutls_free,
-                                                         session_datum.data);
-
-      if (gnutls->session_id)
-        g_tls_backend_gnutls_store_session_data (gnutls->session_id,
-                                                 gnutls->session_data);
-    }
-
-  return 0;
-}
-
 static void
 g_tls_client_connection_gnutls_finalize (GObject *object)
 {
@@ -273,9 +237,6 @@ g_tls_client_connection_gnutls_initable_init (GInitable       *initable,
       g_free (normalized_hostname);
     }
 
-  gnutls_handshake_set_hook_function (session, GNUTLS_HANDSHAKE_NEW_SESSION_TICKET,
-                                      GNUTLS_HOOK_POST, handshake_thread_session_ticket_received_cb);
-
   return TRUE;
 }
 
@@ -446,6 +407,21 @@ g_tls_client_connection_gnutls_handshake_thread_retrieve_function (gnutls_sessio
   return 0;
 }
 
+static void
+g_tls_client_connection_gnutls_clear_session_data (GTlsClientConnectionGnutls *gnutls)
+{
+  gnutls->session_data_override = FALSE;
+  g_clear_pointer (&gnutls->session_data, g_bytes_unref);
+  if (gnutls->session_id)
+    g_tls_backend_gnutls_remove_session (GNUTLS_CLIENT, gnutls->session_id);
+}
+
+static void
+g_tls_client_connection_gnutls_failed (GTlsConnectionGnutls *gnutls)
+{
+  g_tls_client_connection_gnutls_clear_session_data (G_TLS_CLIENT_CONNECTION_GNUTLS (gnutls));
+}
+
 static void
 g_tls_client_connection_gnutls_prepare_handshake (GTlsConnectionBase  *tls,
                                                   gchar              **advertised_protocols)
@@ -454,9 +430,9 @@ g_tls_client_connection_gnutls_prepare_handshake (GTlsConnectionBase  *tls,
 
   g_tls_client_connection_gnutls_compute_session_id (gnutls);
 
+  /* Try to get a cached session */
   if (gnutls->session_data_override)
     {
-      g_assert (gnutls->session_data);
       gnutls_session_set_data (g_tls_connection_gnutls_get_session (G_TLS_CONNECTION_GNUTLS (tls)),
                                g_bytes_get_data (gnutls->session_data, NULL),
                                g_bytes_get_size (gnutls->session_data));
@@ -465,14 +441,14 @@ g_tls_client_connection_gnutls_prepare_handshake (GTlsConnectionBase  *tls,
     {
       GBytes *session_data;
 
-      session_data = g_tls_backend_gnutls_lookup_session_data (gnutls->session_id);
+      session_data = g_tls_backend_gnutls_lookup_session (GNUTLS_CLIENT, gnutls->session_id);
       if (session_data)
         {
           gnutls_session_set_data (g_tls_connection_gnutls_get_session (G_TLS_CONNECTION_GNUTLS (tls)),
                                    g_bytes_get_data (session_data, NULL),
                                    g_bytes_get_size (session_data));
           g_clear_pointer (&gnutls->session_data, g_bytes_unref);
-          gnutls->session_data = g_steal_pointer (&session_data);
+          gnutls->session_data = session_data;
         }
     }
 
@@ -486,6 +462,7 @@ g_tls_client_connection_gnutls_complete_handshake (GTlsConnectionBase  *tls,
                                                    GError             **error)
 {
   GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (tls);
+  int resumed;
 
   G_TLS_CONNECTION_BASE_CLASS (g_tls_client_connection_gnutls_parent_class)->complete_handshake (tls, 
negotiated_protocol, error);
 
@@ -494,6 +471,28 @@ g_tls_client_connection_gnutls_complete_handshake (GTlsConnectionBase  *tls,
    */
   if (gnutls->accepted_cas_changed)
     g_object_notify (G_OBJECT (gnutls), "accepted-cas");
+
+  resumed = gnutls_session_is_resumed (g_tls_connection_gnutls_get_session (G_TLS_CONNECTION_GNUTLS (tls)));
+  if (!resumed)
+    {
+      gnutls_datum_t session_datum;
+
+      g_tls_client_connection_gnutls_clear_session_data (G_TLS_CLIENT_CONNECTION_GNUTLS (tls));
+
+      if (gnutls_session_get_data2 (g_tls_connection_gnutls_get_session (G_TLS_CONNECTION_GNUTLS (tls)),
+                                    &session_datum) == 0)
+        {
+          gnutls->session_data = g_bytes_new_with_free_func (session_datum.data,
+                                                             session_datum.size,
+                                                             (GDestroyNotify)gnutls_free,
+                                                             session_datum.data);
+
+          if (gnutls->session_id)
+            g_tls_backend_gnutls_store_session (GNUTLS_CLIENT,
+                                                gnutls->session_id,
+                                                gnutls->session_data);
+        }
+    }
 }
 
 static void
@@ -503,24 +502,16 @@ g_tls_client_connection_gnutls_copy_session_state (GTlsClientConnection *conn,
   GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (conn);
   GTlsClientConnectionGnutls *gnutls_source = G_TLS_CLIENT_CONNECTION_GNUTLS (source);
 
-  /* Precondition: source has handshaked, conn has not. */
-  g_return_if_fail (!gnutls->session_id);
-  g_return_if_fail (gnutls_source->session_id);
-
-  /* Prefer to use a new session ticket, if possible. */
-  gnutls->session_data = g_tls_backend_gnutls_lookup_session_data (gnutls_source->session_id);
-
-  if (!gnutls->session_data && gnutls_source->session_data)
+  if (gnutls_source->session_data)
     {
-      /* If it's not possible, we'll try to reuse the old ticket, even though
-       * this is a privacy risk since TLS 1.3. Applications should not use this
-       * function unless they need us to try as hard as possible to resume a
-       * session, even at the cost of privacy.
-       */
+      gnutls->session_data_override = TRUE;
       gnutls->session_data = g_bytes_ref (gnutls_source->session_data);
-    }
 
-  gnutls->session_data_override = !!gnutls->session_data;
+      if (gnutls->session_id)
+        g_tls_backend_gnutls_store_session (GNUTLS_CLIENT,
+                                            gnutls->session_id,
+                                            gnutls->session_data);
+    }
 }
 
 static void
@@ -528,6 +519,7 @@ g_tls_client_connection_gnutls_class_init (GTlsClientConnectionGnutlsClass *klas
 {
   GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
   GTlsConnectionBaseClass *base_class = G_TLS_CONNECTION_BASE_CLASS (klass);
+  GTlsConnectionGnutlsClass *gnutls_class = G_TLS_CONNECTION_GNUTLS_CLASS (klass);
 
   gobject_class->get_property = g_tls_client_connection_gnutls_get_property;
   gobject_class->set_property = g_tls_client_connection_gnutls_set_property;
@@ -536,6 +528,8 @@ g_tls_client_connection_gnutls_class_init (GTlsClientConnectionGnutlsClass *klas
   base_class->prepare_handshake  = g_tls_client_connection_gnutls_prepare_handshake;
   base_class->complete_handshake = g_tls_client_connection_gnutls_complete_handshake;
 
+  gnutls_class->failed             = g_tls_client_connection_gnutls_failed;
+
   g_object_class_override_property (gobject_class, PROP_VALIDATION_FLAGS, "validation-flags");
   g_object_class_override_property (gobject_class, PROP_SERVER_IDENTITY, "server-identity");
   g_object_class_override_property (gobject_class, PROP_USE_SSL3, "use-ssl3");
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index 51e8293..8f16523 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -478,7 +478,11 @@ end_gnutls_io (GTlsConnectionGnutls  *gnutls,
 
 #define END_GNUTLS_IO(gnutls, direction, ret, status, errmsg, err)      \
     status = end_gnutls_io (gnutls, direction, ret, err, errmsg);       \
-  } while (status == G_TLS_CONNECTION_BASE_TRY_AGAIN);
+  } while (status == G_TLS_CONNECTION_BASE_TRY_AGAIN);                  \
+                                                                        \
+  if (status == G_TLS_CONNECTION_BASE_ERROR &&                          \
+      G_TLS_CONNECTION_GNUTLS_GET_CLASS (gnutls)-> failed)              \
+    G_TLS_CONNECTION_GNUTLS_GET_CLASS (gnutls)->failed (gnutls);
 
 static void
 set_gnutls_error (GTlsConnectionGnutls *gnutls,
diff --git a/tls/gnutls/gtlsconnection-gnutls.h b/tls/gnutls/gtlsconnection-gnutls.h
index 55ae5ee..db3b726 100644
--- a/tls/gnutls/gtlsconnection-gnutls.h
+++ b/tls/gnutls/gtlsconnection-gnutls.h
@@ -39,6 +39,8 @@ G_DECLARE_DERIVABLE_TYPE (GTlsConnectionGnutls, g_tls_connection_gnutls, G, TLS_
 struct _GTlsConnectionGnutlsClass
 {
   GTlsConnectionBaseClass parent_class;
+
+  void (*failed) (GTlsConnectionGnutls *tls);
 };
 
 gnutls_certificate_credentials_t g_tls_connection_gnutls_get_credentials (GTlsConnectionGnutls *connection);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]