Bug 137300 - [SOUP] TLS errors should take precedence over HTTP authentication
Summary: [SOUP] TLS errors should take precedence over HTTP authentication
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2014-10-01 06:24 PDT by Carlos Garcia Campos
Modified: 2015-03-12 13:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.20 KB, patch)
2014-10-01 06:29 PDT, Carlos Garcia Campos
svillar: review+
svillar: commit-queue-
Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2014-10-06 09:09 PDT, Carlos Garcia Campos
svillar: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-10-01 06:24:24 PDT
When not ignoring TLS errors we should fail before showing the HTTP auth dialog in case of invalid certificate. See also https://bugzilla.gnome.org/show_bug.cgi?id=633366
Comment 1 Carlos Garcia Campos 2014-10-01 06:29:12 PDT
Created attachment 239022 [details]
Patch
Comment 2 Michael Catanzaro 2014-10-01 07:51:02 PDT
I'm not sure if your test works.  The server isn't set up to use HTTP authentication; I think you need to add a SoupAuthDomain for "/auth" using soup_server_add_auth_domain().
Comment 3 Carlos Garcia Campos 2014-10-01 08:20:43 PDT
(In reply to comment #2)
> I'm not sure if your test works.  The server isn't set up to use HTTP authentication; I think you need to add a SoupAuthDomain for "/auth" using soup_server_add_auth_domain().

I wrote the test before the patch, the test does the same than the auth tests
Comment 4 Michael Catanzaro 2014-10-01 08:38:14 PDT
OK, I guess the WWW-Authenticate header is sufficient.
Comment 5 Sergio Villar Senin 2014-10-06 01:05:01 PDT
Comment on attachment 239022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239022&action=review

The change looks conceptually good to me. I was wondering if it wouldn't be better to listen to tls-errors notifications from the SoupMessage instead of having to handle these things in different parts of our code...

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-577
> -    if (!response.soupMessageTLSErrors() || gIgnoreSSLErrors)

I guess this deserves a comment explaining that in the case of authentication challenge, the response has not been updated with the SoupMessage contents yet.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:593
> +    handle->client()->didFail(handle, ResourceError::tlsError(d->m_soupRequest.get(), tlsErrors, certificate));

So I guess both soupMessageCertificate() and soupMessageTLSErrors() became unused methods with these changes. Time to remove them?
Comment 6 Carlos Garcia Campos 2014-10-06 02:18:41 PDT
(In reply to comment #5)
> (From update of attachment 239022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239022&action=review
> 
> The change looks conceptually good to me. I was wondering if it wouldn't be better to listen to tls-errors notifications from the SoupMessage instead of having to handle these things in different parts of our code...

Yes, but unfortunately libsoup doesn't emit notify for tls-errors nor flags properties.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-577
> > -    if (!response.soupMessageTLSErrors() || gIgnoreSSLErrors)
> 
> I guess this deserves a comment explaining that in the case of authentication challenge, the response has not been updated with the SoupMessage contents yet.

Ok.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:593
> > +    handle->client()->didFail(handle, ResourceError::tlsError(d->m_soupRequest.get(), tlsErrors, certificate));
> 
> So I guess both soupMessageCertificate() and soupMessageTLSErrors() became unused methods with these changes. Time to remove them?

No, those are used by CertificateInfo when created for a ResourceResponse
Comment 7 Sergio Villar Senin 2014-10-06 03:15:20 PDT
Comment on attachment 239022 [details]
Patch

I'll r+ it now but I'd like to hear Dan's opinion on the matter before landing.
Comment 8 Dan Winship 2014-10-06 06:11:28 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > The change looks conceptually good to me. I was wondering if it wouldn't be better to listen to tls-errors notifications from the SoupMessage instead of having to handle these things in different parts of our code...
> 
> Yes, but unfortunately libsoup doesn't emit notify for tls-errors nor flags properties.

Hm... it's a bug that :flags doesn't get notified when CERTIFICATE_TRUSTED changes. But :tls-errors definitely should be getting notified (and the flags will have already changed at the point when :tls-errors is notified).
Comment 9 Carlos Garcia Campos 2014-10-06 08:59:44 PDT
So, Dan confirmed I was wrong, I was not getting the tls-errors notify signal because I was connecting in request-started and it's too late. But I realized we are connecting to got-headers, that is also emitted before authenticate and tls errors are already set at that point so it looks like a good place to handle TLS errors.
Comment 10 Carlos Garcia Campos 2014-10-06 09:09:54 PDT
Created attachment 239338 [details]
Patch

Updated patch, now TLS errors are handled in a single place.
Comment 11 Sergio Villar Senin 2014-10-07 00:29:51 PDT
Comment on attachment 239338 [details]
Patch

Much better now.
Comment 12 Carlos Garcia Campos 2014-10-07 08:49:19 PDT
Committed r174394: <http://trac.webkit.org/changeset/174394>