[glib-networking] Remove IP port pairs from unique session ID when hostname exists

[Marge Bot <marge-bot src gnome org>]

commit 00dae0976a49e2b612b567c7c4d68336505caa80
Author: Goncalo Gomes <goncalo gomes youview com>
Date:   Wed Sep 7 18:26:33 2022 +0100

    Remove IP port pairs from unique session ID when hostname exists
    
    Modern CDNs should be able to resume sessions even if the IP is different
    Hence this commit allows usage of the same session ticket across the
    infrastructure of the CDN, if the servers allow that.
    
    In the case where CDN does not allow that, it will just fail to resume the
    session. Possibly creating new session tickets for the next connections to
    the same hostname.
    
    In the tests we cannot assert that the connection has not been reused as the
    allegedly random port might have been assigned multiple times by the OS
    
    Part-of: <https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/221>

 tls/base/gtlsconnection-base.c             | 83 +++++++++++++++++++++---------
 tls/base/gtlsconnection-base.h             |  5 ++
 tls/gnutls/gtlsclientconnection-gnutls.c   | 15 +++---
 tls/openssl/gtlsclientconnection-openssl.c | 15 +++---
 tls/tests/connection.c                     | 22 +++++---
 5 files changed, 95 insertions(+), 45 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index e8396072..559bf35b 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -232,6 +232,20 @@ g_tls_connection_base_is_dtls (GTlsConnectionBase *tls)
   return priv->base_socket != NULL;
 }
 
+gboolean
+g_tls_connection_base_get_session_resumption (GTlsConnectionBase *tls)
+{
+  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
+  return priv->session_resumption_enabled;
+}
+
+void
+g_tls_connection_base_set_session_resumption (GTlsConnectionBase *tls, gboolean session_resumption_enabled)
+{
+  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
+  priv->session_resumption_enabled = session_resumption_enabled;
+}
+
 static void
 g_tls_connection_base_init (GTlsConnectionBase *tls)
 {
@@ -240,6 +254,22 @@ g_tls_connection_base_init (GTlsConnectionBase *tls)
   priv->need_handshake = TRUE;
   priv->database_is_unset = TRUE;
   priv->is_system_certdb = TRUE;
+
+  /* The testsuite expects handshakes to actually happen. E.g. a test might
+   * check to see that a handshake succeeds and then later check that a new
+   * handshake fails. If we get really unlucky and the same port number is
+   * reused for the server socket between connections, then we'll accidentally
+   * resume the old session and skip certificate verification. Such failures
+   * are difficult to debug because they require running the tests hundreds of
+   * times simultaneously to reproduce (the port number does not get reused
+   * quickly enough if the tests are run sequentially).
+   *
+   * On top of that if using a hostname the session id would be used for all
+   * the connections in the tests.
+   *
+   * This variable allows tests to enable session resumption only when needed
+   * whilst keeping the feature enabled for other uses of the library.
+   */
   priv->session_resumption_enabled = !g_test_initialized ();
 
   g_mutex_init (&priv->verify_certificate_mutex);
@@ -2834,48 +2864,53 @@ g_tls_connection_base_constructed (GObject *object)
           remote_addr = g_socket_connection_get_remote_address (base_conn, NULL);
           if (G_IS_INET_SOCKET_ADDRESS (remote_addr))
             {
+              gchar *cert_hash = NULL;
+              GTlsCertificate *cert = NULL;
+              GTlsConnectionBasePrivate *priv = NULL;
               const gchar *server_hostname = get_server_identity (!g_tls_connection_base_is_dtls (tls) ?
                                                                   
g_tls_client_connection_get_server_identity (G_TLS_CLIENT_CONNECTION (tls)) :
                                                                   
g_dtls_client_connection_get_server_identity (G_DTLS_CLIENT_CONNECTION (tls)));
+              priv = g_tls_connection_base_get_instance_private (tls);
+
+              /* If we have a certificate, make its hash part of the session ID, so
+               * that different connections to the same server can use different
+               * certificates.
+               */
+              g_object_get (G_OBJECT (tls), "certificate", &cert, NULL);
+              if (cert)
+                {
+                  GByteArray *der = NULL;
+                  g_object_get (G_OBJECT (cert), "certificate", &der, NULL);
+                  if (der)
+                    {
+                      cert_hash = g_compute_checksum_for_data (G_CHECKSUM_SHA256, der->data, der->len);
+                      g_byte_array_unref (der);
+                    }
+                  g_object_unref (cert);
+                }
 
               if (server_hostname)
+                {
+                  priv->session_id = g_strdup_printf ("%s/%s", server_hostname,
+                                                      cert_hash ? cert_hash : "");
+                }
+              else
                 {
                   guint port;
                   GInetAddress *iaddr;
-                  GTlsCertificate *cert = NULL;
-                  GTlsConnectionBasePrivate *priv = NULL;
-                  gchar *addrstr = NULL, *cert_hash = NULL;
+                  gchar *addrstr = NULL;
                   GInetSocketAddress *isaddr = G_INET_SOCKET_ADDRESS (remote_addr);
 
-                  priv = g_tls_connection_base_get_instance_private (tls);
                   port = g_inet_socket_address_get_port (isaddr);
                   iaddr = g_inet_socket_address_get_address (isaddr);
                   addrstr = g_inet_address_to_string (iaddr);
 
-                  /* If we have a certificate, make its hash part of the session ID, so
-                   * that different connections to the same server can use different
-                   * certificates.
-                   */
-                  g_object_get (G_OBJECT (tls), "certificate", &cert, NULL);
-                  if (cert)
-                    {
-                      GByteArray *der = NULL;
-                      g_object_get (G_OBJECT (cert), "certificate", &der, NULL);
-                      if (der)
-                        {
-                          cert_hash = g_compute_checksum_for_data (G_CHECKSUM_SHA256, der->data, der->len);
-                          g_byte_array_unref (der);
-                        }
-                      g_object_unref (cert);
-                    }
-
-                  priv->session_id = g_strdup_printf ("%s/%s/%d/%s", addrstr,
-                                                      server_hostname ? server_hostname : "",
+                  priv->session_id = g_strdup_printf ("%s/%d/%s", addrstr,
                                                       port,
                                                       cert_hash ? cert_hash : "");
                   g_free (addrstr);
-                  g_free (cert_hash);
                 }
+              g_free (cert_hash);
             }
           g_object_unref (remote_addr);
         }
diff --git a/tls/base/gtlsconnection-base.h b/tls/base/gtlsconnection-base.h
index 7d98d935..959018cd 100644
--- a/tls/base/gtlsconnection-base.h
+++ b/tls/base/gtlsconnection-base.h
@@ -218,4 +218,9 @@ void                      g_tls_connection_base_handshake_thread_buffer_applicat
 
 gchar                    *g_tls_connection_base_get_session_id          (GTlsConnectionBase  *tls);
 
+gboolean                  g_tls_connection_base_get_session_resumption  (GTlsConnectionBase  *tls);
+
+void                      g_tls_connection_base_set_session_resumption  (GTlsConnectionBase *tls,
+                                                                         gboolean 
session_resumption_enabled);
+
 G_END_DECLS
diff --git a/tls/gnutls/gtlsclientconnection-gnutls.c b/tls/gnutls/gtlsclientconnection-gnutls.c
index 17394d52..b22d7334 100644
--- a/tls/gnutls/gtlsclientconnection-gnutls.c
+++ b/tls/gnutls/gtlsclientconnection-gnutls.c
@@ -57,7 +57,6 @@ struct _GTlsClientConnectionGnutls
   GSocketConnectable *server_identity;
   gboolean use_ssl3;
   gboolean session_reused;
-  gboolean session_resumption_enabled;
 
   /* session_data is either the session ticket that was used to resume this
    * connection, or the most recent session ticket received from the server.
@@ -115,8 +114,6 @@ clear_gnutls_certificate_copy (gnutls_pcert_st  **pcert,
 static void
 g_tls_client_connection_gnutls_init (GTlsClientConnectionGnutls *gnutls)
 {
-    gnutls->session_reused = FALSE;
-    gnutls->session_resumption_enabled = !g_test_initialized ();
 }
 
 static const gchar *
@@ -143,7 +140,9 @@ handshake_thread_session_ticket_received_cb (gnutls_session_t      session,
                                              guint                 incoming,
                                              const gnutls_datum_t *msg)
 {
-  GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (gnutls_session_get_ptr (session));
+  GTlsConnectionBase *tls = gnutls_transport_get_ptr (session);
+  GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (tls);
+
   gnutls_datum_t session_datum;
 
   if (gnutls_session_get_data2 (session, &session_datum) == GNUTLS_E_SUCCESS)
@@ -154,7 +153,7 @@ handshake_thread_session_ticket_received_cb (gnutls_session_t      session,
                                                          (GDestroyNotify)gnutls_free,
                                                          session_datum.data);
 
-      if (gnutls->session_resumption_enabled && gnutls->session_id)
+      if (g_tls_connection_base_get_session_resumption (tls) && gnutls->session_id)
         {
           g_tls_store_session_data (gnutls->session_id,
                                     (gpointer)gnutls->session_data,
@@ -225,6 +224,7 @@ g_tls_client_connection_gnutls_get_property (GObject    *object,
                                              GValue     *value,
                                              GParamSpec *pspec)
 {
+  GTlsConnectionBase *tls = G_TLS_CONNECTION_BASE (object);
   GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (object);
   GList *accepted_cas;
   gint i;
@@ -262,7 +262,7 @@ g_tls_client_connection_gnutls_get_property (GObject    *object,
       break;
 
     case PROP_SESSION_RESUMPTION_ENABLED:
-      g_value_set_boolean (value, gnutls->session_resumption_enabled);
+      g_value_set_boolean (value, g_tls_connection_base_get_session_resumption (tls));
       break;
 
     default:
@@ -276,6 +276,7 @@ g_tls_client_connection_gnutls_set_property (GObject      *object,
                                              const GValue *value,
                                              GParamSpec   *pspec)
 {
+  GTlsConnectionBase *tls = G_TLS_CONNECTION_BASE (object);
   GTlsClientConnectionGnutls *gnutls = G_TLS_CLIENT_CONNECTION_GNUTLS (object);
   const char *hostname;
 
@@ -317,7 +318,7 @@ g_tls_client_connection_gnutls_set_property (GObject      *object,
       break;
 
     case PROP_SESSION_RESUMPTION_ENABLED:
-      gnutls->session_resumption_enabled = g_value_get_boolean (value);
+      g_tls_connection_base_set_session_resumption (tls, g_value_get_boolean (value));
       break;
 
     default:
diff --git a/tls/openssl/gtlsclientconnection-openssl.c b/tls/openssl/gtlsclientconnection-openssl.c
index 1c79aa00..fbf100ba 100644
--- a/tls/openssl/gtlsclientconnection-openssl.c
+++ b/tls/openssl/gtlsclientconnection-openssl.c
@@ -46,7 +46,6 @@ struct _GTlsClientConnectionOpenssl
   GSocketConnectable *server_identity;
   gboolean use_ssl3;
   gboolean session_reused;
-  gboolean session_resumption_enabled;
 
   STACK_OF (X509_NAME) *ca_list;
 
@@ -111,6 +110,7 @@ g_tls_client_connection_openssl_get_property (GObject    *object,
                                              GValue     *value,
                                              GParamSpec *pspec)
 {
+  GTlsConnectionBase *tls = G_TLS_CONNECTION_BASE (object);
   GTlsClientConnectionOpenssl *openssl = G_TLS_CLIENT_CONNECTION_OPENSSL (object);
   GList *accepted_cas;
   gint i;
@@ -161,7 +161,7 @@ g_tls_client_connection_openssl_get_property (GObject    *object,
       break;
 
     case PROP_SESSION_RESUMPTION_ENABLED:
-      g_value_set_boolean (value, openssl->session_resumption_enabled);
+      g_value_set_boolean (value, g_tls_connection_base_get_session_resumption (tls));
       break;
 
     default:
@@ -175,6 +175,7 @@ g_tls_client_connection_openssl_set_property (GObject      *object,
                                              const GValue *value,
                                              GParamSpec   *pspec)
 {
+  GTlsConnectionBase *tls = G_TLS_CONNECTION_BASE (object);
   GTlsClientConnectionOpenssl *openssl = G_TLS_CLIENT_CONNECTION_OPENSSL (object);
 
   switch (prop_id)
@@ -194,7 +195,7 @@ g_tls_client_connection_openssl_set_property (GObject      *object,
       break;
 
     case PROP_SESSION_RESUMPTION_ENABLED:
-      openssl->session_resumption_enabled = g_value_get_boolean (value);
+      g_tls_connection_base_set_session_resumption (tls, g_value_get_boolean (value));
       break;
 
     default:
@@ -306,8 +307,6 @@ g_tls_client_connection_openssl_class_init (GTlsClientConnectionOpensslClass *kl
 static void
 g_tls_client_connection_openssl_init (GTlsClientConnectionOpenssl *openssl)
 {
-  openssl->session_reused = FALSE;
-  openssl->session_resumption_enabled = !g_test_initialized ();
 }
 
 static void
@@ -455,8 +454,12 @@ set_curve_list (GTlsClientConnectionOpenssl *client)
 
 static int g_tls_client_connection_openssl_new_session (SSL *s, SSL_SESSION *sess)
 {
+  GTlsConnectionBase *tls;
   GTlsClientConnectionOpenssl *client = G_TLS_CLIENT_CONNECTION_OPENSSL 
(g_tls_connection_openssl_get_connection_from_ssl (s));
-  if (client->session_resumption_enabled)
+
+  tls = G_TLS_CONNECTION_BASE (client);
+
+  if (g_tls_connection_base_get_session_resumption (tls))
     g_tls_store_session_data (g_tls_connection_base_get_session_id (G_TLS_CONNECTION_BASE (client)),
                               (gpointer)sess,
                               (SessionDup)SSL_SESSION_dup,
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index 1eaca655..b008dcd6 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -43,6 +43,7 @@
 #if defined(G_OS_UNIX)
 #include <dlfcn.h>
 static struct timespec offset;
+static struct timespec session_time_offset;
 #endif
 
 static const gchar *
@@ -115,6 +116,16 @@ setup_connection (TestConnection *test, gconstpointer data)
 #endif
 }
 
+static void
+setup_session_connection (TestConnection *test, gconstpointer data)
+{
+  setup_connection (test, data);
+#if defined(G_OS_UNIX)
+  offset.tv_sec += 11 * 60 + session_time_offset.tv_sec;
+  session_time_offset.tv_sec += 11 * 60;
+#endif
+}
+
 /* Waits about 10 seconds for @var to be NULL/FALSE */
 #define WAIT_UNTIL_UNSET(var)                                \
   if (var)                                                   \
@@ -611,11 +622,6 @@ test_connection_session_resume_ten_minute_expiry (TestConnection *test,
   return;
 #endif
 
-#if defined(G_OS_UNIX)
-  /* Expiry should be 10 min */
-  offset.tv_sec += 11 * 60;
-#endif
-
   test->database = g_tls_file_database_new (tls_test_file_path ("ca-roots.pem"), &error);
   g_assert_no_error (error);
   g_assert_nonnull (test->database);
@@ -647,11 +653,11 @@ test_connection_session_resume_ten_minute_expiry (TestConnection *test,
   g_object_unref (test->client_connection);
   g_clear_object (&test->server_connection);
 
-  /* First connection was not reused */
   g_assert_false (reused);
 
 #if defined(G_OS_UNIX)
   /* Expiry should be 10 min */
+  session_time_offset.tv_sec += 11 * 60;
   offset.tv_sec += 11 * 60;
 #endif
 
@@ -3382,9 +3388,9 @@ main (int   argc,
 #endif
 
   g_test_add ("/tls/" BACKEND "/connection/session/resume_multiple_times", TestConnection, NULL,
-              setup_connection, test_connection_session_resume_multiple_times, teardown_connection);
+              setup_session_connection, test_connection_session_resume_multiple_times, teardown_connection);
   g_test_add ("/tls/" BACKEND "/connection/session/reuse_ten_minute_expiry", TestConnection, NULL,
-              setup_connection, test_connection_session_resume_ten_minute_expiry, teardown_connection);
+              setup_session_connection, test_connection_session_resume_ten_minute_expiry, 
teardown_connection);
   g_test_add ("/tls/" BACKEND "/connection/basic", TestConnection, NULL,
               setup_connection, test_basic_connection, teardown_connection);
   g_test_add ("/tls/" BACKEND "/connection/verified", TestConnection, NULL,