[libsoup] soup-session: fix up TLS/SSL property interactions

[Dan Winship <danw src gnome org>]

commit 0656b1c4d58239d63ee024d40f18d0cb521c43d2
Author: Dan Winship <danw gnome org>
Date:   Tue Apr 10 13:31:45 2012 -0400

    soup-session: fix up TLS/SSL property interactions
    
    SoupSession:ssl-use-system-ca-file was broken, in that setting it to
    either TRUE or FALSE would enable it. Fix that, and also fix up the
    documentation and property notifications around that,
    SoupSession:tls-database, and SoupSession:ssl-ca-file. Add a test to
    tests/ssl-test to verify things.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=673678

 libsoup/soup-session.c |  156 ++++++++++++++++++++++++++++++----------------
 tests/ssl-test.c       |  161 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 263 insertions(+), 54 deletions(-)
---
diff --git a/libsoup/soup-session.c b/libsoup/soup-session.c
index a4f3289..7215b61 100644
--- a/libsoup/soup-session.c
+++ b/libsoup/soup-session.c
@@ -594,10 +594,14 @@ soup_session_class_init (SoupSessionClass *session_class)
 	/**
 	 * SoupSession:ssl-use-system-ca-file:
 	 *
-	 * Setting this to %TRUE overrides #SoupSession:ssl-ca-file
-	 * and #SoupSession:tls-database, and uses the default system
-	 * CA database (which, despite the name, may not actually be a
-	 * file).
+	 * Setting this to %TRUE is equivalent to setting
+	 * #SoupSession:tls-database to the default system CA database.
+	 * (and likewise, setting #SoupSession:tls-database to the
+	 * default database by hand will cause this property to
+	 * become %TRUE).
+	 *
+	 * Setting this to %FALSE (when it was previously %TRUE) will
+	 * clear the #SoupSession:tls-database field.
 	 *
 	 * See #SoupSession:ssl-strict for more information on how
 	 * https certificate validation is handled.
@@ -609,7 +613,7 @@ soup_session_class_init (SoupSessionClass *session_class)
 		g_param_spec_boolean (SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE,
 				      "Use system CA file",
 				      "Use the system certificate database",
-				      TRUE,
+				      FALSE,
 				      G_PARAM_READWRITE));
 	/**
 	 * SOUP_SESSION_TLS_DATABASE:
@@ -621,9 +625,13 @@ soup_session_class_init (SoupSessionClass *session_class)
 	/**
 	 * SoupSession:tls-database:
 	 *
-	 * Overrides #SoupSession:ssl-ca-file and
-	 * #SoupSession:ssl-use-system-ca-file, and uses the provided
-	 * #GTlsDatabase.
+	 * Sets the #GTlsDatabase to use for validating SSL/TLS
+	 * certificates.
+	 *
+	 * Note that setting the #SoupSession:ssl-ca-file or
+	 * #SoupSession:ssl-use-system-ca-file property will cause
+	 * this property to be set to a #GTlsDatabase corresponding to
+	 * the indicated file or system default.
 	 *
 	 * See #SoupSession:ssl-strict for more information on how
 	 * https certificate validation is handled.
@@ -647,12 +655,13 @@ soup_session_class_init (SoupSessionClass *session_class)
 	/**
 	 * SoupSession:ssl-strict:
 	 *
-	 * Normally, if #SoupSession:ssl-ca-file (or
-	 * #SoupSession:tlsdb or #SoupSession:ssl-use-system-ca-file)
-	 * is set, then libsoup will reject any certificate that is
-	 * invalid (ie, expired) or that is not signed by one of the
-	 * given CA certificates, and the #SoupMessage will fail with
-	 * the status %SOUP_STATUS_SSL_FAILED.
+	 * Normally, if #SoupSession:tlsdb is set (including if it was
+	 * set via #SoupSession:ssl-use-system-ca-file or
+	 * #SoupSession:ssl-ca-file), then libsoup will reject any
+	 * certificate that is invalid (ie, expired) or that is not
+	 * signed by one of the given CA certificates, and the
+	 * #SoupMessage will fail with the status
+	 * %SOUP_STATUS_SSL_FAILED.
 	 *
 	 * If you set #SoupSession:ssl-strict to %FALSE, then all
 	 * certificates will be accepted, and you will need to call
@@ -1039,31 +1048,94 @@ accept_languages_from_system (void)
 }
 
 static void
-load_ssl_ca_file (SoupSessionPrivate *priv)
+set_tlsdb (SoupSession *session, GTlsDatabase *tlsdb)
+{
+	SoupSessionPrivate *priv = SOUP_SESSION_GET_PRIVATE (session);
+	GTlsDatabase *system_default;
+
+	if (tlsdb == priv->tlsdb)
+		return;
+
+	g_object_freeze_notify (G_OBJECT (session));
+
+	system_default = g_tls_backend_get_default_database (g_tls_backend_get_default ());
+	if (priv->tlsdb == system_default || tlsdb == system_default) {
+		g_object_notify (G_OBJECT (session), "ssl-use-system-ca-file");
+	}
+	g_object_unref (system_default);
+
+	if (priv->ssl_ca_file) {
+		g_free (priv->ssl_ca_file);
+		priv->ssl_ca_file = NULL;
+		g_object_notify (G_OBJECT (session), "ssl-ca-file");
+	}
+
+	if (priv->tlsdb)
+		g_object_unref (priv->tlsdb);
+	priv->tlsdb = tlsdb;
+	if (priv->tlsdb)
+		g_object_ref (priv->tlsdb);
+
+	g_object_notify (G_OBJECT (session), "tls-database");
+	g_object_thaw_notify (G_OBJECT (session));
+}
+
+static void
+set_use_system_ca_file (SoupSession *session, gboolean use_system_ca_file)
+{
+	SoupSessionPrivate *priv = SOUP_SESSION_GET_PRIVATE (session);
+	GTlsDatabase *system_default;
+
+	system_default = g_tls_backend_get_default_database (g_tls_backend_get_default ());
+
+	if (use_system_ca_file)
+		set_tlsdb (session, system_default);
+	else if (priv->tlsdb == system_default)
+		set_tlsdb (session, NULL);
+
+	g_object_unref (system_default);
+}
+
+static void
+set_ssl_ca_file (SoupSession *session, const char *ssl_ca_file)
 {
+	SoupSessionPrivate *priv = SOUP_SESSION_GET_PRIVATE (session);
+	GTlsDatabase *tlsdb;
 	GError *error = NULL;
 
-	if (g_path_is_absolute (priv->ssl_ca_file)) {
-		priv->tlsdb = g_tls_file_database_new (priv->ssl_ca_file,
-						       &error);
-	} else {
+	if (!g_strcmp0 (priv->ssl_ca_file, ssl_ca_file))
+		return;
+
+	g_object_freeze_notify (G_OBJECT (session));
+
+	if (g_path_is_absolute (ssl_ca_file))
+		tlsdb = g_tls_file_database_new (ssl_ca_file, &error);
+	else {
 		char *path, *cwd;
 
 		cwd = g_get_current_dir ();
-		path = g_build_filename (cwd, priv->ssl_ca_file, NULL);
-		priv->tlsdb = g_tls_file_database_new (path, &error);
+		path = g_build_filename (cwd, ssl_ca_file, NULL);
+		tlsdb = g_tls_file_database_new (path, &error);
 		g_free (path);
 	}
-	if (priv->tlsdb)
-		return;
 
-	if (!g_error_matches (error, G_TLS_ERROR, G_TLS_ERROR_UNAVAILABLE)) {
-		g_warning ("Could not set SSL credentials from '%s': %s",
-			   priv->ssl_ca_file, error->message);
+	if (error) {
+		if (!g_error_matches (error, G_TLS_ERROR, G_TLS_ERROR_UNAVAILABLE)) {
+			g_warning ("Could not set SSL credentials from '%s': %s",
+				   ssl_ca_file, error->message);
 
-		priv->tlsdb = g_tls_file_database_new ("/dev/null", NULL);
+			tlsdb = g_tls_file_database_new ("/dev/null", NULL);
+		}
+		g_error_free (error);
 	}
-	g_error_free (error);
+
+	set_tlsdb (session, tlsdb);
+	g_object_unref (tlsdb);
+
+	priv->ssl_ca_file = g_strdup (ssl_ca_file);
+	g_object_notify (G_OBJECT (session), "ssl-ca-file");
+
+	g_object_thaw_notify (G_OBJECT (session));
 }
 
 /* priv->http_aliases and priv->https_aliases are stored as arrays of
@@ -1130,35 +1202,13 @@ set_property (GObject *object, guint prop_id,
 			g_warning ("Trying to set use-ntlm on session with no auth-manager");
 		break;
 	case PROP_SSL_CA_FILE:
-		if (priv->tlsdb) {
-			g_object_unref (priv->tlsdb);
-			priv->tlsdb = NULL;
-		}
-		g_free (priv->ssl_ca_file);
-
-		priv->ssl_ca_file = g_value_dup_string (value);
-		if (priv->ssl_ca_file)
-			load_ssl_ca_file (priv);
+		set_ssl_ca_file (session, g_value_get_string (value));
 		break;
 	case PROP_SSL_USE_SYSTEM_CA_FILE:
-		if (priv->tlsdb) {
-			g_object_unref (priv->tlsdb);
-			priv->tlsdb = NULL;
-		}
-		g_free (priv->ssl_ca_file);
-		priv->ssl_ca_file = NULL;
-
-		priv->tlsdb = g_tls_backend_get_default_database (g_tls_backend_get_default ());
+		set_use_system_ca_file (session, g_value_get_boolean (value));
 		break;
 	case PROP_TLS_DATABASE:
-		if (priv->tlsdb) {
-			g_object_unref (priv->tlsdb);
-			priv->tlsdb = NULL;
-		}
-		g_free (priv->ssl_ca_file);
-		priv->ssl_ca_file = NULL;
-
-		priv->tlsdb = g_value_dup_object (value);
+		set_tlsdb (session, g_value_get_object (value));
 		break;
 	case PROP_SSL_STRICT:
 		priv->ssl_strict = g_value_get_boolean (value);
diff --git a/tests/ssl-test.c b/tests/ssl-test.c
index 0308254..f999d82 100644
--- a/tests/ssl-test.c
+++ b/tests/ssl-test.c
@@ -122,7 +122,7 @@ do_strict_tests (char *uri)
 {
 	SoupSession *session;
 
-	debug_printf (1, "strict/nonstrict\n");
+	debug_printf (1, "\nstrict/nonstrict\n");
 
 	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
 	debug_printf (1, "  async with CA list\n");
@@ -148,6 +148,164 @@ do_strict_tests (char *uri)
 }
 
 static void
+property_changed (GObject *object, GParamSpec *param, gpointer user_data)
+{
+	gboolean *changed = user_data;
+
+	*changed = TRUE;
+}
+
+static void
+do_session_property_tests (void)
+{
+	gboolean use_system_changed, tlsdb_changed, ca_file_changed;
+	gboolean use_system;
+	GTlsDatabase *tlsdb;
+	char *ca_file;
+	SoupSession *session;
+
+	debug_printf (1, "session properties\n");
+
+	session = soup_session_async_new ();
+	g_signal_connect (session, "notify::ssl-use-system-ca-file",
+			  G_CALLBACK (property_changed), &use_system_changed);
+	g_signal_connect (session, "notify::tls-database",
+			  G_CALLBACK (property_changed), &tlsdb_changed);
+	g_signal_connect (session, "notify::ssl-ca-file",
+			  G_CALLBACK (property_changed), &ca_file_changed);
+
+	g_object_get (G_OBJECT (session),
+		      "ssl-use-system-ca-file", &use_system,
+		      "tls-database", &tlsdb,
+		      "ssl-ca-file", &ca_file,
+		      NULL);
+	if (use_system) {
+		debug_printf (1, "  ssl-use-system-ca-file defaults to TRUE?\n");
+		errors++;
+	}
+	if (tlsdb) {
+		debug_printf (1, "  tls-database set by default?\n");
+		errors++;
+		g_object_unref (tlsdb);
+	}
+	if (ca_file) {
+		debug_printf (1, "  ca-file set by default?\n");
+		errors++;
+		g_free (ca_file);
+	}
+
+	use_system_changed = tlsdb_changed = ca_file_changed = FALSE;
+	g_object_set (G_OBJECT (session),
+		      "ssl-use-system-ca-file", TRUE,
+		      NULL);
+	g_object_get (G_OBJECT (session),
+		      "ssl-use-system-ca-file", &use_system,
+		      "tls-database", &tlsdb,
+		      "ssl-ca-file", &ca_file,
+		      NULL);
+	if (!use_system) {
+		debug_printf (1, "  setting ssl-use-system-ca-file failed\n");
+		errors++;
+	}
+	if (!tlsdb) {
+		debug_printf (1, "  setting ssl-use-system-ca-file didn't set tls-database\n");
+		errors++;
+	} else
+		g_object_unref (tlsdb);
+	if (ca_file) {
+		debug_printf (1, "  setting ssl-use-system-ca-file set ssl-ca-file\n");
+		errors++;
+		g_free (ca_file);
+	}
+	if (!use_system_changed) {
+		debug_printf (1, "  setting ssl-use-system-ca-file didn't emit notify::ssl-use-system-ca-file\n");
+		errors++;
+	}
+	if (!tlsdb_changed) {
+		debug_printf (1, "  setting ssl-use-system-ca-file didn't emit notify::tls-database\n");
+		errors++;
+	}
+	if (ca_file_changed) {
+		debug_printf (1, "  setting ssl-use-system-ca-file emitted notify::ssl-ca-file\n");
+		errors++;
+	}
+
+	use_system_changed = tlsdb_changed = ca_file_changed = FALSE;
+	g_object_set (G_OBJECT (session),
+		      "ssl-ca-file", SRCDIR "/test-cert.pem",
+		      NULL);
+	g_object_get (G_OBJECT (session),
+		      "ssl-use-system-ca-file", &use_system,
+		      "tls-database", &tlsdb,
+		      "ssl-ca-file", &ca_file,
+		      NULL);
+	if (use_system) {
+		debug_printf (1, "  setting ssl-ca-file left ssl-use-system-ca-file set\n");
+		errors++;
+	}
+	if (!tlsdb) {
+		debug_printf (1, "  setting ssl-ca-file didn't set tls-database\n");
+		errors++;
+	} else
+		g_object_unref (tlsdb);
+	if (!ca_file) {
+		debug_printf (1, "  setting ssl-ca-file failed\n");
+		errors++;
+	} else
+		g_free (ca_file);
+	if (!use_system_changed) {
+		debug_printf (1, "  setting ssl-ca-file didn't emit notify::ssl-use-system-ca-file\n");
+		errors++;
+	}
+	if (!tlsdb_changed) {
+		debug_printf (1, "  setting ssl-ca-file didn't emit notify::tls-database\n");
+		errors++;
+	}
+	if (!ca_file_changed) {
+		debug_printf (1, "  setting ssl-ca-file didn't emit notify::ssl-ca-file\n");
+		errors++;
+	}
+
+	use_system_changed = tlsdb_changed = ca_file_changed = FALSE;
+	g_object_set (G_OBJECT (session),
+		      "tls-database", NULL,
+		      NULL);
+	g_object_get (G_OBJECT (session),
+		      "ssl-use-system-ca-file", &use_system,
+		      "tls-database", &tlsdb,
+		      "ssl-ca-file", &ca_file,
+		      NULL);
+	if (use_system) {
+		debug_printf (1, "  setting tls-database NULL left ssl-use-system-ca-file set\n");
+		errors++;
+	}
+	if (tlsdb) {
+		debug_printf (1, "  setting tls-database NULL failed\n");
+		errors++;
+		g_object_unref (tlsdb);
+	}
+	if (ca_file) {
+		debug_printf (1, "  setting tls-database didn't clear ssl-ca-file\n");
+		errors++;
+		g_free (ca_file);
+	}
+	if (use_system_changed) {
+		debug_printf (1, "  setting tls-database emitted notify::ssl-use-system-ca-file\n");
+		errors++;
+	}
+	if (!tlsdb_changed) {
+		debug_printf (1, "  setting tls-database didn't emit notify::tls-database\n");
+		errors++;
+	}
+	if (!ca_file_changed) {
+		debug_printf (1, "  setting tls-database didn't emit notify::ssl-ca-file\n");
+		errors++;
+	}
+
+	soup_test_session_abort_unref (session);
+}
+
+static void
 server_handler (SoupServer        *server,
 		SoupMessage       *msg, 
 		const char        *path,
@@ -175,6 +333,7 @@ main (int argc, char **argv)
 		uri = g_strdup_printf ("https://127.0.0.1:%u/";,
 				       soup_server_get_port (server));
 
+		do_session_property_tests ();
 		do_strict_tests (uri);
 		do_properties_tests (uri);