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>

5 files changed, 95 insertions, 45 deletions

diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index e839607..559bf35 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 7d98d93..959018c 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 17394d5..b22d733 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 1c79aa0..fbf100b 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 1eaca65..b008dcd 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,