Bug 191646

Summary: [Curl] Add Server Trust Evaluation Support.
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, Basuke.Suzuki, commit-queue, don.olmstead, ews-watchlist, galpeter, Hironori.Fujii, ross.kirsling, takashi.komori, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=187679
Bug Depends on: 187679, 191647, 191648    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Basuke Suzuki 2018-11-14 12:34:55 PST
Add Server Trust Evaluation Support. Need to implement UI on MiniBrowser to ask user to trust the server.
Comment 1 Takashi Komori 2019-03-26 22:36:47 PDT
Created attachment 366052 [details]
Patch
Comment 2 Ross Kirsling 2019-03-27 00:22:49 PDT
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?
Comment 3 Fujii Hironori 2019-03-27 01:01:19 PDT
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
Comment 4 Fujii Hironori 2019-03-27 01:49:43 PDT
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.
Comment 5 Takashi Komori 2019-03-28 00:42:00 PDT
Created attachment 366157 [details]
Patch
Comment 6 Takashi Komori 2019-03-28 00:43:14 PDT
(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.
Comment 7 Takashi Komori 2019-03-28 00:48:27 PDT
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.
Comment 8 Fujii Hironori 2019-03-28 01:15:03 PDT
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.
Comment 9 Takashi Komori 2019-03-29 00:37:27 PDT
Created attachment 366258 [details]
Patch
Comment 10 Takashi Komori 2019-03-29 00:38:30 PDT
(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.
Comment 11 Alex Christensen 2019-03-29 11:35:09 PDT
Comment on attachment 366258 [details]
Patch

LGTM
Comment 12 WebKit Commit Bot 2019-03-29 12:03:22 PDT
Comment on attachment 366258 [details]
Patch

Clearing flags on attachment: 366258

Committed r243654: <https://trac.webkit.org/changeset/243654>
Comment 13 WebKit Commit Bot 2019-03-29 12:03:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-03-29 12:04:22 PDT
<rdar://problem/49429836>