Bug 191646 – [Curl] Add Server Trust Evaluation Support.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

Add Server Trust Evaluation Support. Need to implement UI on MiniBrowser to ask user to trust the server.

[Takashi Komori]

Created attachment 366052 [details]
Patch

[Ross Kirsling]

Comment on attachment 366052 [details]
Patch

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

Just a couple of nitpicks from me.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:327
> +        m_acceptedServerTrustCert.insert(std::make_pair(host, pem));

If you use emplace() then you don't need make_pair. :)

> Tools/MiniBrowser/win/WebKitBrowserWindow.h:77
> +    std::map<std::wstring, std::wstring> m_acceptedServerTrustCert;

I think this can be unordered_map instead, right?

[Fujii Hironori]

Comment on attachment 366052 [details]
Patch

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

> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:80
> +    return ProtectionSpace(url.host().toString(), static_cast<int>(port ? *port : 0), serverType, String(), authenticationScheme, certificateInfo);

"(port ? *port : 0)" can be "port.valueOr(0)". protectionSpaceFromHandle has the same code.

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:423
> +void NetworkDataTaskCurl::restartWithCredential(const AuthenticationChallenge& challenge, const Credential& credential)

It seems that you don't need change the first argument.

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:433
> +    if (!challenge.protectionSpace().certificateInfo().certificateChain().isEmpty())

Is this never be empty in case of ServerTrustEvaluation? 
if (challenge.protectionSpace().authenticationScheme() == ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested)
I feel this is somewhat better.

> Tools/MiniBrowser/win/Common.cpp:288
> +std::wstring replaceWString(std::wstring src, const std::wstring& oldValue, const std::wstring& newValue)

You can name this 'replaceString'. If you will need a std::string version, you can overload replaceString(std::string, const std::string&, const std::string&).

> Tools/MiniBrowser/win/Common.cpp:294
> +    while ((pos = src.find(oldValue, pos)) != std::string::npos) {

Nit: std::string::npos should be std::wstring::npos or src.npos.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:323
> +    if (it != m_acceptedServerTrustCert.end() && it->second == pem)

You check only the first matching. m_acceptedServerTrustCert can be std::set<std::pair<std::wstring,std::wstring>>.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:334
> +std::wstring WebKitBrowserWindow::createPEMString(WKProtectionSpaceRef protectionSpace)

This shouldn't be a WebKitBrowserWindow's method.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:345
> +        pems += std::wstring(reinterpret_cast<const char*>(data), reinterpret_cast<const char*>(data + size));

std::wstring(reinterpret_cast<const char*>(data), size)

> Tools/MiniBrowser/win/WebKitBrowserWindow.h:77
> +    std::map<std::wstring, std::wstring> m_acceptedServerTrustCert;

should be plural if this is just a single cert.
m_acceptedServerTrustCerts

[Fujii Hironori]

Comment on attachment 366052 [details]
Patch

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

>> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:323
>> +    if (it != m_acceptedServerTrustCert.end() && it->second == pem)
> 
> You check only the first matching. m_acceptedServerTrustCert can be std::set<std::pair<std::wstring,std::wstring>>.

Err. I misunderstood. You store only the latest cert for hosts. It makes sense. Ignore my comment.

[Takashi Komori]

Created attachment 366157 [details]
Patch

[Takashi Komori]

(In reply to Ross Kirsling from comment #2)
Thanks for your comment!

> > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:327
> > +        m_acceptedServerTrustCert.insert(std::make_pair(host, pem));
> 
> If you use emplace() then you don't need make_pair. :)

Fixed.

> > Tools/MiniBrowser/win/WebKitBrowserWindow.h:77
> > +    std::map<std::wstring, std::wstring> m_acceptedServerTrustCert;
> 
> I think this can be unordered_map instead, right?

Fixed.

[Takashi Komori]

Thank you for your review!

(In reply to Fujii Hironori from comment #3)
> Comment on attachment 366052 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366052&action=review
> 
> > Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:80
> > +    return ProtectionSpace(url.host().toString(), static_cast<int>(port ? *port : 0), serverType, String(), authenticationScheme, certificateInfo);
> 
> "(port ? *port : 0)" can be "port.valueOr(0)". protectionSpaceFromHandle has
> the same code.

Fixed.

> > Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:423
> > +void NetworkDataTaskCurl::restartWithCredential(const AuthenticationChallenge& challenge, const Credential& credential)
> 
> It seems that you don't need change the first argument.

Reverted to former argument list.

> > Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:433
> > +    if (!challenge.protectionSpace().certificateInfo().certificateChain().isEmpty())
> 
> Is this never be empty in case of ServerTrustEvaluation? 
> if (challenge.protectionSpace().authenticationScheme() ==
> ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested)
> I feel this is somewhat better.

Fixed.

> > Tools/MiniBrowser/win/Common.cpp:288
> > +std::wstring replaceWString(std::wstring src, const std::wstring& oldValue, const std::wstring& newValue)
> 
> You can name this 'replaceString'. If you will need a std::string version,
> you can overload replaceString(std::string, const std::string&, const
> std::string&).

Changed.
 
> > Tools/MiniBrowser/win/Common.cpp:294
> > +    while ((pos = src.find(oldValue, pos)) != std::string::npos) {
> 
> Nit: std::string::npos should be std::wstring::npos or src.npos.

Fixed.

> > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:334
> > +std::wstring WebKitBrowserWindow::createPEMString(WKProtectionSpaceRef protectionSpace)
> 
> This shouldn't be a WebKitBrowserWindow's method.

Moved to Common.cpp

> > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:345
> > +        pems += std::wstring(reinterpret_cast<const char*>(data), reinterpret_cast<const char*>(data + size));
> 
> std::wstring(reinterpret_cast<const char*>(data), size)

Fixed.
 
> > Tools/MiniBrowser/win/WebKitBrowserWindow.h:77
> > +    std::map<std::wstring, std::wstring> m_acceptedServerTrustCert;
> 
> should be plural if this is just a single cert.
> m_acceptedServerTrustCerts

Fixed.

[Fujii Hironori]

Comment on attachment 366157 [details]
Patch

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

> Tools/MiniBrowser/win/Common.h:31
> +#include <WebKit/WebKit2_C.h>

AppleWin port doesn't enable WK2. You shouldn't put WK2-related code in Common.h.

[Takashi Komori]

Created attachment 366258 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #8)
> Comment on attachment 366157 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366157&action=review
> 
> > Tools/MiniBrowser/win/Common.h:31
> > +#include <WebKit/WebKit2_C.h>
> 
> AppleWin port doesn't enable WK2. You shouldn't put WK2-related code in
> Common.h.

Fixed.

[Alex Christensen]

Comment on attachment 366258 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 366258 [details]
Patch

Clearing flags on attachment: 366258

Committed r243654: <https://trac.webkit.org/changeset/243654>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49429836>