Bug 137300 – [SOUP] TLS errors should take precedence over HTTP authentication

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 239022 [details]
Patch

[Michael Catanzaro]

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().

[Carlos Garcia Campos]

(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

[Michael Catanzaro]

OK, I guess the WWW-Authenticate header is sufficient.

[Sergio Villar Senin]

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?

[Carlos Garcia Campos]

(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

[Sergio Villar Senin]

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.

[Dan Winship]

(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).

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 239338 [details]
Patch

Updated patch, now TLS errors are handled in a single place.

[Sergio Villar Senin]

Comment on attachment 239338 [details]
Patch

Much better now.

[Carlos Garcia Campos]

Committed r174394: <http://trac.webkit.org/changeset/174394>