After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 610374 - Provide information about the validity of the certificate a SoupMessage deals with (through a SoupSocket)
Provide information about the validity of the certificate a SoupMessage deals...
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: API
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2010-02-18 16:23 UTC by Gustavo Noronha (kov)
Modified: 2010-02-21 15:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
A new property to allow having a CA file defined without having soup error out on invalid certs (13.49 KB, patch)
2010-02-18 16:24 UTC, Gustavo Noronha (kov)
needs-work Details | Review
A new SoupMessageFlag that reports SoupSocket's findings regarding certificate validity (8.13 KB, patch)
2010-02-18 16:26 UTC, Gustavo Noronha (kov)
committed Details | Review

Description Gustavo Noronha (kov) 2010-02-18 16:23:54 UTC
To provide very basic information regarding the certificates in Epiphany it would be good to have soup let us know whether the certificate it has encoutered while processing a given SoupMessage is valid. This would be a paliative solution before we have proper TLS support built into GIO.
Comment 1 Gustavo Noronha (kov) 2010-02-18 16:24:50 UTC
Created attachment 154147 [details] [review]
A new property to allow having a CA file defined without having soup error out on invalid certs
Comment 2 Gustavo Noronha (kov) 2010-02-18 16:26:44 UTC
Created attachment 154150 [details] [review]
A new SoupMessageFlag that reports SoupSocket's findings regarding certificate validity
Comment 3 Dan Winship 2010-02-20 17:33:15 UTC
Comment on attachment 154147 [details] [review]
A new property to allow having a CA file defined without having soup error out on invalid certs

>+	gboolean     ignore_ssl_cert_errors;

this is kind of an unwieldy name... and also, it ignores the fact that if SOUP_SESSION_SSL_CA_FILE isn't set, then cert errors are ignored regardless.

of course, it's hard to address both of those objections at the same time...

I'd say, just call it "ssl-strict", with the opposite sense of the current flag (ie, the default is %TRUE which means to validate certs, and you have to set it to %FALSE to make it not), and just note in the documentation that it has no effect unless you also set ssl-ca-file.

>+		if (chan->ignore_ssl_cert_errors &&
>+		    (*err)->code == SOUP_SSL_ERROR_CERTIFICATE)
>+			return G_IO_STATUS_NORMAL;

OK, so this *does* cause problems. If you add this to tests/test-utils.c:

diff --git a/tests/test-utils.c b/tests/test-utils.c
index 0b13ca1..d29a406 100644
--- a/tests/test-utils.c
+++ b/tests/test-utils.c
@@ -231,6 +231,11 @@ soup_test_session_new (GType type, ...)
        session = (SoupSession *)g_object_new_valist (type, propname, args);
        va_end (args);
 
+       g_object_set (G_OBJECT (session),
+                     SOUP_SESSION_SSL_CA_FILE, SRCDIR "/test-cert.pem",
+                     SOUP_SESSION_IGNORE_SSL_CERT_ERRORS, TRUE,
+                     NULL);
+
        if (http_debug_level && !logger) {
                SoupLoggerLogLevel level = MIN ((SoupLoggerLogLevel)http_debug_level, SOUP_LOGGER_LOG_BODY);
 


then tests/timeout-test and tests/proxy-test get stuck in infinite loops. This is what I was saying about needing to try again immediately in that case; read_from_network/soup_socket_write assume that 0-length reads/writes only happen under specific circumstances, and "ssl certificate didn't validate" isn't one of them, so now the code is wrong.

You should probably just add the soup_test_session_new() patch above to your patch, so that we're testing these cases in the future.
Comment 4 Dan Winship 2010-02-20 17:39:01 UTC
Comment on attachment 154150 [details] [review]
A new SoupMessageFlag that reports SoupSocket's findings regarding certificate validity

>-	if (chan->type == SOUP_SSL_TYPE_CLIENT && chan->creds->have_ca_file &&
>-	    !verify_certificate (chan->session, chan->hostname, err)) {
>-		if (chan->ignore_ssl_cert_errors &&
>-		    (*err)->code == SOUP_SSL_ERROR_CERTIFICATE)
>+	if (chan->type == SOUP_SSL_TYPE_CLIENT && chan->creds->have_ca_file) {
>+		if (verify_certificate (chan->session, chan->hostname, err) ||
>+		    (chan->ignore_ssl_cert_errors &&
>+		     (*err)->code == SOUP_SSL_ERROR_CERTIFICATE))
> 			return G_IO_STATUS_NORMAL;

does this have any effect?

>+		g_object_get(io->sock,
>+			     "trusted-certificate", &trusted_certificate,

space before paren, and use the #define please (SOUP_SOCKET_TRUSTED_CERTIFICATE)

everythign else looks good. this is ok to commit once the other patch is
Comment 5 Dan Winship 2010-02-21 15:08:58 UTC
I changed the property to "ssl-strict" as described above, fixed up the
problems, and committed it.