Handle SSL errors for SOUP

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The soup network backend doesn't handle SSL errors because it relies on embedders to handle them themselves using the SoupSession directly. This is not possible in WebKit2, so we need to provide a way to allow the UI process to handle SSL errors. Having SSL handling mechanism in WebCore not only beneficts webkit2, but also webkit1, since it will make easier for embedders to handle these errors and will provide a more robust default behaviour.

[Carlos Garcia Campos]

Created attachment 150136 [details]
Patch

Handle SSL errors in the soup backend adding a way to allow the WebKit layer decide on what to do. This will allow us to expose an API to handles SSL errors from the UI process in WebKit2. The behaviour is the same than the current one in both WebKit1 and WebKit2, except for the cases where a subresource has a different https origin and the certificate can't be trusted. In such cases the subresource fails to load now. Now it works the following way:

         - When the main resource receives the response with SSL errors, it asynchronously asks the WebKit layer to check the certificate in a way similar to the policy checker.
         - The default implementation for all ports using the soup backend is to accept the certificate for compatibility. In WebKit2 a message will be send to the UI process, but for now the certificate is always accepted too.
         - If the WebKit layer accepts the certificate, it's stored in the DocumentLoader to check it for subresources.
         - When a subresource receives the response with SSL errors, the certificate is compared to the saved certificate in DocumentLoader, which is considered the trusted certificate. It will be accepted or denied depending on the trusted certificate without asking the WebKit layer.
         - If the certificate is accepted the resource continues loading normally. If it's denied the resource load finishes with a normal SSL error.
         - If the ssl mode is set to strict in the SoupSession (in WebKit2 is always set to FALSE), this mechanism doesn't even start because soup returns and error earlier and the load finishes with a normal SSL error.

[Adam Barth]

The patch touches a bunch of non-SOUP specific code, so it shouldn't have the [SOUP] prefix.

[Adam Barth]

Comment on attachment 150136 [details]
Patch

Historically, we've avoided putting TLS-specific logic into WebKit.  I'm not necessarily opposed to doing that, but if we do it, we should do it in a way that could be used by other ports rather than adding a bunch of #if USE(SOUP) macros in the loader.

[Adam Barth]

I also suspect that ap has opinions on this topic, but he's on vacation.

[Alexey Proskuryakov]

What does Mac WebKit2 do? Are https tests enabled in WebKitTestRunner? I'm reading my bugmail, but don't deeply think or research things while on vacation :)

Mac WebKit1 uses a side channel to communicate with loader, which is arguably not so great for cross-platform API:

    [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:@"localhost"];
    [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:@"127.0.0.1"];

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 150136 [details])
> Historically, we've avoided putting TLS-specific logic into WebKit.  I'm not necessarily opposed to doing that, but if we do it, we should do it in a way that could be used by other ports rather than adding a bunch of #if USE(SOUP) macros in the loader.

Yes, that's not possible in WebKit2, at least not for soup based ports, because the network is in the web process and the soup session is not available in the UI process. Qt simply sends a sync message to the UI process to handle SSL errors, but I prefer to make this fully asyncrhonous, and I don't know what Mac does for WebKit2. I've tried to make the code as port-agnostic as possible, only the certificate object and flags are specific to soup, with the idea of being easily reusable for other ports, but added fisrt as soup only just as a proof of concept. If other ports are interested in this approach I'll rework the patch to make it available for all ports.

[Adam Barth]

I'm not sure you understood my comment.  My concern is that you're introducing SOUP-specific types and code into cross-port code.  Rather than adding #ifdef SOUP to these files, we should create the necessary platform abstractions to do this in a general way.

Before doing that, however, we should understand how apple-mac solves this problem.  It's something of a red flag that you don't understand how they solve this problem.

[Carlos Garcia Campos]

(In reply to comment #7)
> I'm not sure you understood my comment.  My concern is that you're introducing SOUP-specific types and code into cross-port code.  Rather than adding #ifdef SOUP to these files, we should create the necessary platform abstractions to do this in a general way.

Yes, that's what I meant with reworking the patch to make the code available to all ports.

> Before doing that, however, we should understand how apple-mac solves this problem.  It's something of a red flag that you don't understand how they solve this problem.

Sure, I'll check the code and ask apple guys.

(In reply to comment #5)
> What does Mac WebKit2 do? Are https tests enabled in WebKitTestRunner? I'm reading my bugmail, but don't deeply think or research things while on vacation :)

Tests that require HTTPS currently don't work on WebKit2 based ports: See bug 74295

> 
> Mac WebKit1 uses a side channel to communicate with loader, which is arguably not so great for cross-platform API:
> 
>     [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:@"localhost"];
>     [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:@"127.0.0.1"];

[Brady Eidson]

(In reply to comment #9)
> (In reply to comment #5)
> > What does Mac WebKit2 do? Are https tests enabled in WebKitTestRunner? I'm reading my bugmail, but don't deeply think or research things while on vacation :)
> 
> Tests that require HTTPS currently don't work on WebKit2 based ports: See bug 74295

I'll add comments over in bug 74295, but this is not entirely true anymore.

[Brady Eidson]

Maybe it would help everyone clarify the course forward if you would provide a simple test case that shows exactly one problem that needs to be solved.

[Carlos Garcia Campos]

Go to a website with an invalid certificate, like https://www.aeat.com/, for example. Web browsers like chromium or firefox (I don't know what safari does) show a custom error page informing the https certificate can't be trusted, and allowing the user to continue loading the page or cancel the load. In WebKit1 embedders could implement this using the network objects directly (SoupSession in the case of soup based ports), but that's not possible in WebKit2, because the SoupSession object is in the web process. This patch would allow to expose an API to implement this feature.

[Carlos Garcia Campos]

Looking at the code, it seems to me that mac and win ports don't support this use case in WebKit2. The certificate info is serialized as part of ResourceError, so I assume they simply fail to load and the certificate info is sent to the UI process with the ResourceError.

[Alexey Proskuryakov]

Comment on attachment 150136 [details]
Patch

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

> Looking at the code, it seems to me that mac and win ports don't support this use case in WebKit2.

This cannot be true - Safari can deal with untrusted certificates. I don't know all the details, but in the UI, there are options to just continue, or to mark the certificate as trusted for the site permanently. I think that the latter would store the decision in Keychain, and the former calls an NSURLRequest SPI similar to +allowsAnyHTTPSCertificateForHost: to remember the decision in a CFNetwork session, and then the request is retried.

Other ports likely have their solutions too. It might be possible to create a cross-platform abstraction, but I doubt that making an additional client call on every failure would be the right one.

> Source/WebCore/ChangeLog:3
> +        [SOUP] Handle SSL errors

A patch that touches cross-platform code should not have [] prefixes. Especially now that you say that WebKit2 ports need this.

> Source/WebCore/ChangeLog:8
> +        No new tests, this is covered by existing tests.

This explanation doesn't ring true. If this change is covered by existing skipped tests, they should be unskilled in the same patch.

> Source/WebCore/ChangeLog:12
> +        Handle SSL errors in the soup backend adding a way to allow the
> +        WebKit layer decide on what to do. This will allow us to expose an
> +        API to handles SSL errors from the UI process in WebKit2.

Can other network back-ends cannot recover from an SSL failure on a specific connection? If not, exposing this functionality in WebKit API would be undesirable, even for a single port.

There is also the question of how request oriented APIs deal with connection level settings. In my experience, this adds a serious amount of trouble. It's sometimes unavoidable, e.g. for NTLM authentication or keep-alive, but should be minimized when possible.

> Source/WebCore/ChangeLog:20
> +         - When the main resource receives the response with SSL errors,
> +           it asynchronously asks the WebKit layer to check the
> +           certificate in a way similar to the policy checker.

Do you envision a need for UI process to make complicated decisions? If not, a setting should be just sent to WebProcess in advance to minimize complexity and XPC traffic.

> Source/WebCore/ChangeLog:31
> +         - When a subresource receives the response with SSL errors, the
> +           certificate is compared to the saved certificate in
> +           DocumentLoader, which is considered the trusted
> +           certificate. It will be accepted or denied depending on the
> +           trusted certificate without asking the WebKit layer.

While this logic makes sense on the surface, I think that it's without precedent in browsers. Do any other browsers behave like this?

> Source/WebCore/ChangeLog:51
> +        * loader/FrameLoaderClient.h:
> +        (FrameLoaderClient): Add dispatchCheckTlsCertificate().

Style nit: TLS should be capitalized everywhere.

[Alexey Proskuryakov]

I think that a solution for tests is separate from what's needed for browsers. Proposing one for Mac WK2 in bug 90600.

[Carlos Garcia Campos]

(In reply to comment #14)
> (From update of attachment 150136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150136&action=review
> 
> > Looking at the code, it seems to me that mac and win ports don't support this use case in WebKit2.
> 
> This cannot be true - Safari can deal with untrusted certificates. I don't know all the details, but in the UI, there are options to just continue, or to mark the certificate as trusted for the site permanently. I think that the latter would store the decision in Keychain, and the former calls an NSURLRequest SPI similar to +allowsAnyHTTPSCertificateForHost: to remember the decision in a CFNetwork session, and then the request is retried.

Ok, I didn't find code for that in WebKit, I guess they use injected bundle in the app side. I don't have a mac to check what safari does either.

> Other ports likely have their solutions too. It might be possible to create a cross-platform abstraction, but I doubt that making an additional client call on every failure would be the right one.

So, instead of stopping the load until a decision has been made by the WebKit layer, we could use a solution similar to what mac and win seem to do. In case of SSL errors, if the host is not in the list of hosts which any HTTPS certificate should be allowed, we simply finish the load with an error. The error sent to the UI process contains the certificate. The app can handle this error differently and ask the user to decide what to do. If the user decide to ignore SSL errors an continue with the load, the hostname is sent to the web process to be included in the list of hosts that should allow any certificate and a new request is made.

If I'm not wrong again, Qt port sends a sync message to the UI process to decide whether to ignore SSL errors or not. That would block the whole web process until the decision is made by the user, so I prefer to avoid any soluttion that is not asynchronous. 

> > Source/WebCore/ChangeLog:3
> > +        [SOUP] Handle SSL errors
> 
> A patch that touches cross-platform code should not have [] prefixes. Especially now that you say that WebKit2 ports need this.

Ok, I thought this solution might not be valid for other ports.

> > Source/WebCore/ChangeLog:8
> > +        No new tests, this is covered by existing tests.
> 
> This explanation doesn't ring true. If this change is covered by existing skipped tests, they should be unskilled in the same patch.

No, this should not affect skipped tests. 

> > Source/WebCore/ChangeLog:12
> > +        Handle SSL errors in the soup backend adding a way to allow the
> > +        WebKit layer decide on what to do. This will allow us to expose an
> > +        API to handles SSL errors from the UI process in WebKit2.
> 
> Can other network back-ends cannot recover from an SSL failure on a specific connection? If not, exposing this functionality in WebKit API would be undesirable, even for a single port.
> 
> There is also the question of how request oriented APIs deal with connection level settings. In my experience, this adds a serious amount of trouble. It's sometimes unavoidable, e.g. for NTLM authentication or keep-alive, but should be minimized when possible.

I don't know.

> > Source/WebCore/ChangeLog:20
> > +         - When the main resource receives the response with SSL errors,
> > +           it asynchronously asks the WebKit layer to check the
> > +           certificate in a way similar to the policy checker.
> 
> Do you envision a need for UI process to make complicated decisions? If not, a setting should be just sent to WebProcess in advance to minimize complexity and XPC traffic.

No, just allow or reject the certificate for the given website. But this can't be a global setting, because teh decission might depend on the concrete error or the site the user is trying to access.

> > Source/WebCore/ChangeLog:31
> > +         - When a subresource receives the response with SSL errors, the
> > +           certificate is compared to the saved certificate in
> > +           DocumentLoader, which is considered the trusted
> > +           certificate. It will be accepted or denied depending on the
> > +           trusted certificate without asking the WebKit layer.
> 
> While this logic makes sense on the surface, I think that it's without precedent in browsers. Do any other browsers behave like this?

Yes, at least chromium and firefox do that, I don't know how it's done in the code, but they only ask the user to accept/reject the certificate for the main resource. When the user decides to continue, all other subresources are loaded without asking the user. When a subresource has an invalid certificate rom another website it's not loaded without asking the user either.

> > Source/WebCore/ChangeLog:51
> > +        * loader/FrameLoaderClient.h:
> > +        (FrameLoaderClient): Add dispatchCheckTlsCertificate().
> 
> Style nit: TLS should be capitalized everywhere.

Ok.

[Carlos Garcia Campos]

Created attachment 151088 [details]
New patch

This is a new patch following a simpler approach based on what win port does. It doesn't affect WebKit1, because in this case the wk layer should handle the errors using the SoupSession object directly. If SoupSession is set to strict mode, SSL errors will be handled by the SoupSession, otherwhise they will be ignored, so it's exactly the same behaviour. For WebKit2 we are also ignoring SSL errors by default for compatibility, but that will be removed once the UI process can handle those errors. This patch doesn't change any cross-platform code.

[Martin Robinson]

Comment on attachment 151088 [details]
New patch

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

This looks good to me apart from a concern about how you save TLS certifictes (indexed only on URL and not port). I think it would be good for Sergio and Dan to take a look though.

> Source/WebCore/platform/network/soup/ResourceError.h:32
>  #include "ResourceErrorBase.h"
>  
> +#include <wtf/gobject/GRefPtr.h>
> +

Nit: This header could go directly under the first, I think.

> Source/WebCore/platform/network/soup/ResourceError.h:52
> +    ResourceError(const String& domain, int errorCode, const String& failingURL, const String& localizedDescription, unsigned tlsErrors, GTlsCertificate* certificate)
> +        : ResourceErrorBase(domain, errorCode, failingURL, localizedDescription)
> +        , m_tlsErrors(tlsErrors)
> +        , m_certificate(certificate)

m_tlsErrors is uninitialized for other constructors. Maybe it should be initialized to 0 for completeness.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:172
> +static bool ignoreSSLErrors = false;

I like to prefix my global static variables with g. Which, I think, could be useful in this case too.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:431
> +        if (d->m_response.soupMessageTLSErrors() && !ignoreSSLErrors && !allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower())) {

Does host() also include the port number? If I'm not mistaken you want something like the security domain (scheme-host-scheme). The scheme may not be important here, but perhaps it would be better to make this conditional on protocolHostAndPortAreEqual.

I also wonder what happens when you make a request to http://foo.com and then http://foo.com:80.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:432
> +            CertificatesMap::iterator iter = clientCertificates().find(handle->firstRequest().url().host().lower());

I think it makes sense to put this entire check into a helper method called something like messageHasUnignoredTLSError.

[Carlos Garcia Campos]

(In reply to comment #18)
> (From update of attachment 151088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151088&action=review
> 
> This looks good to me apart from a concern about how you save TLS certifictes (indexed only on URL and not port). I think it would be good for Sergio and Dan to take a look though.
> 
> > Source/WebCore/platform/network/soup/ResourceError.h:32
> >  #include "ResourceErrorBase.h"
> >  
> > +#include <wtf/gobject/GRefPtr.h>
> > +
> 
> Nit: This header could go directly under the first, I think.

Yes.

> > Source/WebCore/platform/network/soup/ResourceError.h:52
> > +    ResourceError(const String& domain, int errorCode, const String& failingURL, const String& localizedDescription, unsigned tlsErrors, GTlsCertificate* certificate)
> > +        : ResourceErrorBase(domain, errorCode, failingURL, localizedDescription)
> > +        , m_tlsErrors(tlsErrors)
> > +        , m_certificate(certificate)
> 
> m_tlsErrors is uninitialized for other constructors. Maybe it should be initialized to 0 for completeness.

Right, good catch :-)

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:172
> > +static bool ignoreSSLErrors = false;
> 
> I like to prefix my global static variables with g. Which, I think, could be useful in this case too.

I thought that was only for const globals. 

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:431
> > +        if (d->m_response.soupMessageTLSErrors() && !ignoreSSLErrors && !allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower())) {
> 
> Does host() also include the port number? If I'm not mistaken you want something like the security domain (scheme-host-scheme). The scheme may not be important here, but perhaps it would be better to make this conditional on protocolHostAndPortAreEqual.
> 
> I also wonder what happens when you make a request to http://foo.com and then http://foo.com:80.

hmm, I've followed the same approach than CF, see ResourceHandleCFNet.cpp. Chromium also stores the certs per host using handler->request_url().host() in ssl_policy.cc

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:432
> > +            CertificatesMap::iterator iter = clientCertificates().find(handle->firstRequest().url().host().lower());
> 
> I think it makes sense to put this entire check into a helper method called something like messageHasUnignoredTLSError.

[Martin Robinson]

Comment on attachment 151088 [details]
New patch

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

>>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:431
>>> +        if (d->m_response.soupMessageTLSErrors() && !ignoreSSLErrors && !allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower())) {
>> 
>> Does host() also include the port number? If I'm not mistaken you want something like the security domain (scheme-host-scheme). The scheme may not be important here, but perhaps it would be better to make this conditional on protocolHostAndPortAreEqual.
>> 
>> I also wonder what happens when you make a request to http://foo.com and then http://foo.com:80.
> 
> hmm, I've followed the same approach than CF, see ResourceHandleCFNet.cpp. Chromium also stores the certs per host using handler->request_url().host() in ssl_policy.cc

I'll defer here to someone who knows more than me about TLS, perhaps Dan or Sergio.

[Alexey Proskuryakov]

I don't feel like my concerns have been addressed.

[Carlos Garcia Campos]

(In reply to comment #21)
> I don't feel like my concerns have been addressed.

what exactly? The new patch doesn't stop the current resource load until a decision is made, but it finishes the load with an error. Then, it's up to the app to handle that error differently, whitelist the certificate for the host, and start a new load. The patch doesn't change any corss-platform code, and I think it's does the same than the CF backend.

[Alexey Proskuryakov]

The concern is that this patch adds an entirely different way to do something that can already be done in other ports. Ifdefs are powerful, but as much as possible, we don't want ports to do things in entirely custom ways, because that complicates future work on the code, especially refactoring.

[Carlos Garcia Campos]

Created attachment 151657 [details]
Updated patch to address review comments

[Carlos Garcia Campos]

(In reply to comment #23)
> The concern is that this patch adds an entirely different way to do something that can already be done in other ports. Ifdefs are powerful, but as much as possible, we don't want ports to do things in entirely custom ways, because that complicates future work on the code, especially refactoring.

hmm, are you talking about the first or second patch? In the second patch the only #ifdefed code I added is in ResourceHandle.h, which is full of platform #ifdefs. The new methods I added in the #ifdef block are mostly the same than the methods used by the CF backend. I don't know what CF does internally nor what safari does, but apparently, we are now doing the same than CF, or at least a very similar approach.

[Carlos Garcia Campos]

(In reply to comment #18)
> (From update of attachment 151088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151088&action=review
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:431
> > +        if (d->m_response.soupMessageTLSErrors() && !ignoreSSLErrors && !allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().lower())) {
> 
> Does host() also include the port number? If I'm not mistaken you want something like the security domain (scheme-host-scheme). The scheme may not be important here, but perhaps it would be better to make this conditional on protocolHostAndPortAreEqual.
> 
> I also wonder what happens when you make a request to http://foo.com and then http://foo.com:80.

allowsAnyHTTPSCertificateHosts() won't be exposed in the API, it's mainly used for testing. Mac and win ports use it in DRT and WTR to allow any certificates in localhost and 127.0.0.1. That's what we'll do for WTR too. 

When an invalid certificate will be accepted by the wk layer, setClientCertificate() will be used. In this case, the certificate is stored per host, and it's always checked. So, if https://foo.com is loaded and the certificate is accepted, when loading https://foo.com:80, the certificate will be checked for foo.com. If the certificate is the same, then it will be loaded, otherwise (no certificate or it's invalid and different) the load will finish with an error. Then, again, it's up to the app to handle the error and accept/reject the certificate.

[Carlos Garcia Campos]

> When an invalid certificate will be accepted by the wk layer, setClientCertificate() will be used. In this case, the certificate is stored per host, and it's always checked. So, if https://foo.com is loaded and the certificate is accepted, when loading https://foo.com:80, the certificate will be checked for foo.com. If the certificate is the same, then it will be loaded, otherwise (no certificate or it's invalid and different) the load will finish with an error. Then, again, it's up to the app to handle the error and accept/reject the certificate.

hmm, in this particular case, the previous certificate would be lost, so I think we should store a list of certificates per host, like chromium does.

[Gyuyoung Kim]

Comment on attachment 151657 [details]
Updated patch to address review comments

Attachment 151657 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13215019

[Carlos Garcia Campos]

Created attachment 151687 [details]
Updated patch

Updated patch to allow saving more than one certificate per host. Instead of saving the certificate and compare the whole data to check whether it's equal to another one, I've followed the same approach than chromium, computing and saving the SHA1 of the certificate data. To make it easiert to add and check certificates I've used a simpler helper class. This should also fix the EFL build, adding the new message to LocalizedStrings instead of adding a translatable string directly.

[Carlos Garcia Campos]

Created attachment 151712 [details]
Updated patch

This is just a minor improvement. Dan told me about soup_message_get_https_status(), so I've updated the patch to use it instead of g_obejct_get().

[Carlos Garcia Campos]

Created attachment 152205 [details]
Rebased to current git master

Rebased to current git master because some parts of the patch landed already in another patch, so this is now even simpler.

[Carlos Garcia Campos]

(In reply to comment #31)
> Created an attachment (id=152205) [details]
> Rebased to current git master
> 
> Rebased to current git master because some parts of the patch landed already in another patch, so this is now even simpler.

Does anybody still have objections to land this patch?

[Alexey Proskuryakov]

I do not have objections.

[Martin Robinson]

(In reply to comment #33)
> I do not have objections.

Thanks AP! I think we can go ahead with this patch, though it'd be great to get an opinion from Dan about whether it's important to certificates for host+port or if just the host is enough.

[Carlos Garcia Campos]

(In reply to comment #34)
> (In reply to comment #33)
> > I do not have objections.
> 
> Thanks AP! I think we can go ahead with this patch, though it'd be great to get an opinion from Dan about whether it's important to certificates for host+port or if just the host is enough.

I updated the patch already to allow saving more than one certificate per host to handle such cases.

[Dan Winship]

(In reply to comment #34)
> it'd be great to get an opinion from Dan about whether it's important to certificates for host+port or if just the host is enough.

"whatever other browsers do"

[Martin Robinson]

Comment on attachment 152205 [details]
Rebased to current git master

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

> Source/WebCore/ChangeLog:18
> +        (WebCore::ResourceError::ResourceError): Add new constrcutor for

constrcutor -> constructor

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:164
> +class HostTLSCertificates {

Maybe HostTLSCertificateSet to to make it clear that this is a set?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:166
> +    static const size_t sha1HashSize = 20;

This can be defined in the scope that it's used.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:175
> +    bool check(GTlsCertificate* certificate)

How about "contains" ?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:195
> +        return base64Encode(reinterpret_cast<const char*>(digest.data()), sha1HashSize);

It's a bit surprising that base64Encode does not return a CString.

[Carlos Garcia Campos]

Committed r125258: <http://trac.webkit.org/changeset/125258>