Bug 187102 – [Curl] Embed certificate information into ResourceResponse.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

Including the certificate information for SSL communication into ResourceResponse for better error handling and information providing to the user.

[Basuke Suzuki]

Created attachment 343736 [details]
PATCH

[EWS Watchlist]

Attachment 343736 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/curl/CertificateInfo.h:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
ERROR: Source/WebCore/platform/network/curl/CertificateInfo.h:30:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
ERROR: Source/WebCore/platform/network/curl/ResourceResponse.h:53:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/platform/network/curl/ResourceResponse.h:72:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Created attachment 343741 [details]
Fix style

[youenn fablet]

Comment on attachment 343741 [details]
Fix style

Some comments below

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

> Source/WebCore/platform/network/curl/CertificateInfo.cpp:33
> +CertificateInfo::CertificateInfo(int verificationError, const Vector<String>& certificateChain)

Should probably be Vector<String>&&

> Source/WebCore/platform/network/curl/CertificateInfo.cpp:46
> +        copy.m_certificateChain.append(certificate.isolatedCopy());

crossThreadCopy(...) should work.
Might be able to write it as: return { m_verificationError, crossThreadCopy(m_certificateChain) }

> Source/WebCore/platform/network/curl/CertificateInfo.h:51
> +        return (a.certificateChain() == b.certificateChain());

Could be written as a onliner.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:102
> +    StackOfX509(X509_STORE_CTX* ctx)

explicit?
X509_STORE_CTX* or X509_STORE_CTX&

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:110
> +        sk_X509_pop_free(m_certs, X509_free);

space.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:114
> +    X509* get(unsigned i) { return sk_X509_value(m_certs, i); }

s/get/item/ or maybe s/get/value

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:140
> +        return String(certificateData, length);

Is certificateData always ASCII? Should it be String::fromUTF8 ?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:147
> +std::optional<Vector<String>> CurlSSLVerifier::getPemDataFromCtx(X509_STORE_CTX* ctx)

Should probably be called pemDataFromCtx.
Is there a difference between std::nullopt and an empty vector?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:149
> +    std::optional<Vector<String>> result = Vector<String>();

Would be better to make result a Vector<String>() and do return WTFMove(result) at the end of the method.

> Source/WebCore/platform/network/curl/ResourceResponse.h:53
> +    void setCertificateInfo(const CertificateInfo&);

CertificateInfo&&

> Source/WebCore/platform/network/curl/ResourceResponse.h:54
> +    void setDeprecatedNetworkLoadMetrics(const NetworkLoadMetrics&);

NetworkLoadMetrics&&

[Fujii Hironori]

Comment on attachment 343741 [details]
Fix style

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

I'm not a review. This is just a informal review.

> Source/WebCore/platform/network/curl/CurlRequest.h:91
> +    CertificateInfo getCertificateInfo() const { return m_certificateInfo.isolatedCopy(); }

Why is this getter prefixed with `get`?
Why do you use `isolatedCopy`?

> Source/WebCore/platform/network/curl/CurlRequest.h:198
> +    CertificateInfo m_certificateInfo;

It's weird for me CertificateInfo is added to CurlRequest.
It think CertificateInfo should be added to CurlResponse instead of CurlRequest. Right?

[Basuke Suzuki]

Comment on attachment 343741 [details]
Fix style

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

>> Source/WebCore/platform/network/curl/CertificateInfo.cpp:33
>> +CertificateInfo::CertificateInfo(int verificationError, const Vector<String>& certificateChain)
> 
> Should probably be Vector<String>&&

Ah, okay. Sorry, after a short break, I forgot about that convention.

>> Source/WebCore/platform/network/curl/CertificateInfo.cpp:46
>> +        copy.m_certificateChain.append(certificate.isolatedCopy());
> 
> crossThreadCopy(...) should work.
> Might be able to write it as: return { m_verificationError, crossThreadCopy(m_certificateChain) }

Never heard about this convenient function!

>> Source/WebCore/platform/network/curl/CertificateInfo.h:51
>> +        return (a.certificateChain() == b.certificateChain());
> 
> Could be written as a onliner.

Also this method is used only in operator==(). Move the contents into it.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:110
>> +        sk_X509_pop_free(m_certs, X509_free);
> 
> space.

Ouch.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:114
>> +    X509* get(unsigned i) { return sk_X509_value(m_certs, i); }
> 
> s/get/item/ or maybe s/get/value

item is fine.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:140
>> +        return String(certificateData, length);
> 
> Is certificateData always ASCII? Should it be String::fromUTF8 ?

Right.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:149
>> +    std::optional<Vector<String>> result = Vector<String>();
> 
> Would be better to make result a Vector<String>() and do return WTFMove(result) at the end of the method.

It seems okay.

>> Source/WebCore/platform/network/curl/ResourceResponse.h:53
>> +    void setCertificateInfo(const CertificateInfo&);
> 
> CertificateInfo&&

Got it.

>> Source/WebCore/platform/network/curl/ResourceResponse.h:54
>> +    void setDeprecatedNetworkLoadMetrics(const NetworkLoadMetrics&);
> 
> NetworkLoadMetrics&&

Ditto.

[Fujii Hironori]

Comment on attachment 343741 [details]
Fix style

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

I'm not a review. This is just a informal review.

> Source/WebCore/platform/network/curl/CertificateInfo.h:59
> +inline bool operator==(const CertificateInfo& a, const CertificateInfo& b) { return CertificateInfo::compare(a, b); }

Do you need this? Which code is using this operator==?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:73
> +    if (auto certificates = getPemDataFromCtx(ctx))

You change the logic here. The original code returns 0 if getPemDataFromCtx fails. Is it intentinal?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:74
> +        verifier->m_certificateChain = *certificates;

If you don't use certificates after this, you can move it by using WTFMove.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:86
> +        certList.add(cert);

You convert from Vector<String> to ListHashSet<String> in order to call CurlSSLHandle::canIgnoredHTTPSCertificate.
It seems that canIgnoredHTTPSCertificate can be improved by taking Vector<String>&&.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:117
> +    STACK_OF(X509)* m_certs { };

I think WebKit prefers "{ nullptr }". But you don't need the initializer because m_certs is initialzed in the constructor. You can write "STACK_OF(X509)* m_certs;"

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:132
> +    bool write(X509* cert) { return !!PEM_write_bio_X509(m_bio, cert); }

C++ automatically convert int to bool. You don't need !!.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:144
> +    BIO* m_bio { };

I think WebKit prefers "{ nullptr }". But you don't need the initializer because m_bio is initialzed in the constructor. You can write "BIO* m_bio;"

> Source/WebCore/platform/network/curl/CurlSSLVerifier.h:70
> +    Vector<String> m_certificateChain;

Can you use CertificateInfo instead of m_verificationError and m_certificateChain?

> Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:160
> +CertificateInfo ResourceResponse::platformCertificateInfo() const

You don't need to add platformCertificateInfo because you implement setCertificateInfo. See ResourceResponseBase::includeCertificateInfo.

[Basuke Suzuki]

Comment on attachment 343741 [details]
Fix style

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

>> Source/WebCore/platform/network/curl/CurlRequest.h:91
>> +    CertificateInfo getCertificateInfo() const { return m_certificateInfo.isolatedCopy(); }
> 
> Why is this getter prefixed with `get`?
> Why do you use `isolatedCopy`?

isolatedCopy is used because it basically live in the background thread and information will be gathered from main thread. But if these are moved to CurlResponse, which is more data carrying object, individual isolatedCopy() won't needed. Also rename will be done on that timing. Thanks.

>> Source/WebCore/platform/network/curl/CurlRequest.h:198
>> +    CertificateInfo m_certificateInfo;
> 
> It's weird for me CertificateInfo is added to CurlRequest.
> It think CertificateInfo should be added to CurlResponse instead of CurlRequest. Right?

Humm. Good point. I guess this class has a long history and CurlResponse was introduced lately. Maybe these are left over at that time or added after that. Anyway, your point is completely make sense. I'll consider this tomorrow.

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #6)
> >> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:140
> >> +        return String(certificateData, length);
> > 
> > Is certificateData always ASCII? Should it be String::fromUTF8 ?
> 
> Right.

For example, this web site is using UTF-8 in the cert. Good for testing.
https://www.cybertrust.ne.jp/news/130624.html

[Michael Catanzaro]

Comment on attachment 343741 [details]
Fix style

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

>>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:140
>>> +        return String(certificateData, length);
>> 
>> Is certificateData always ASCII? Should it be String::fromUTF8 ?
> 
> Right.

I don't think String::fromUTF8 is right either. CurlSSLVerifier::getPemDataFromCtx returns a ListHashSet<String>, but I think that is risky because it constructs the String by adding a trailing NULL to the certificate data returned by OpenSSL. But I think the data could contain embedded NULL characters. It should probably be treated as a byte array (unsigned char* or uint8_t*) with a length.

[Basuke Suzuki]

Comment on attachment 343741 [details]
Fix style

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

>> Source/WebCore/platform/network/curl/CertificateInfo.h:59
>> +inline bool operator==(const CertificateInfo& a, const CertificateInfo& b) { return CertificateInfo::compare(a, b); }
> 
> Do you need this? Which code is using this operator==?

It will be used when server trust evaluation is implemented on our very next patch.

>>> Source/WebCore/platform/network/curl/CurlRequest.h:198
>>> +    CertificateInfo m_certificateInfo;
>> 
>> It's weird for me CertificateInfo is added to CurlRequest.
>> It think CertificateInfo should be added to CurlResponse instead of CurlRequest. Right?
> 
> Humm. Good point. I guess this class has a long history and CurlResponse was introduced lately. Maybe these are left over at that time or added after that. Anyway, your point is completely make sense. I'll consider this tomorrow.

It seems this weirdness comes from the mismatch of using the name request in CurlRequest. CurlRequest is actually one network transaction like NetworkDataTask or ResourceHandle except redirection handling. It manages from the beginning to the end. That's why those information exists in CurlRequest. We've designed the curl architecture like that so that it's not easy task for now to move them into CurlResponse but good candidate for future refactoring. Thanks.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:73
>> +    if (auto certificates = getPemDataFromCtx(ctx))
> 
> You change the logic here. The original code returns 0 if getPemDataFromCtx fails. Is it intentinal?

The logic itself is not changed. If the certificates are empty, it returns zero at the end by failing canIgnoredHTTPSCertificate(). We cannot return here to collect other information.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:74
>> +        verifier->m_certificateChain = *certificates;
> 
> If you don't use certificates after this, you can move it by using WTFMove.

It will be changed not to use optional, but always return some result. And it will be moved as you suggest. Thanks.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:86
>> +        certList.add(cert);
> 
> You convert from Vector<String> to ListHashSet<String> in order to call CurlSSLHandle::canIgnoredHTTPSCertificate.
> It seems that canIgnoredHTTPSCertificate can be improved by taking Vector<String>&&.

That's true. I'll take a look on that side.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:132
>> +    bool write(X509* cert) { return !!PEM_write_bio_X509(m_bio, cert); }
> 
> C++ automatically convert int to bool. You don't need !!.

Okay, okay.

>>>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:140
>>>> +        return String(certificateData, length);
>>> 
>>> Is certificateData always ASCII? Should it be String::fromUTF8 ?
>> 
>> Right.
> 
> I don't think String::fromUTF8 is right either. CurlSSLVerifier::getPemDataFromCtx returns a ListHashSet<String>, but I think that is risky because it constructs the String by adding a trailing NULL to the certificate data returned by OpenSSL. But I think the data could contain embedded NULL characters. It should probably be treated as a byte array (unsigned char* or uint8_t*) with a length.

Apple port also treats it as an opaque bytes so that byte array of uint8_t seems appropriate. But I need further research on our code base. https://github.snei.sony.com/WebCore/neko/blob/715c17d961231baf4d6a1bd47e7ce0c841d65f9f/Source/WebCore/platform/network/mac/CertificateInfoMac.mm#L43

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:144
>> +    BIO* m_bio { };
> 
> I think WebKit prefers "{ nullptr }". But you don't need the initializer because m_bio is initialzed in the constructor. You can write "BIO* m_bio;"

I wanna bet placing initializer here is better than consensus of "it must be initialized by the constructor". It won't be executed and the code will be stripped by the optimization. Any drawback by existing this. I understand using { nullptr } though I personally prefer { }.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.h:70
>> +    Vector<String> m_certificateChain;
> 
> Can you use CertificateInfo instead of m_verificationError and m_certificateChain?

I'll consider about that. Thanks.

>> Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:160
>> +CertificateInfo ResourceResponse::platformCertificateInfo() const
> 
> You don't need to add platformCertificateInfo because you implement setCertificateInfo. See ResourceResponseBase::includeCertificateInfo.

Oh, right. Thanks.

[Fujii Hironori]

Comment on attachment 343741 [details]
Fix style

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

>>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:73
>>> +    if (auto certificates = getPemDataFromCtx(ctx))
>> 
>> You change the logic here. The original code returns 0 if getPemDataFromCtx fails. Is it intentinal?
> 
> The logic itself is not changed. If the certificates are empty, it returns zero at the end by failing canIgnoredHTTPSCertificate(). We cannot return here to collect other information.

Umm, I don't understand. Because this code is a security sensitive part, I can't forget.

There are three cases that the certificates will become empty.

1) PEM_write_bio_X509 returns 0.
2) BIO_get_mem_data returns negative.
3) sk_X509_num returns 0.

In the original code:
1) CurlSSLVerifier::certVerifyCallback returns 0 immediately
2) CurlSSLVerifier::certVerifyCallback returns 0 immediately
3) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate

In your change:
1) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
2) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
3) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate

Does canIgnoredHTTPSCertificate always return 0 in that case?

[Basuke Suzuki]

Comment on attachment 343741 [details]
Fix style

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

>>>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:73
>>>> +    if (auto certificates = getPemDataFromCtx(ctx))
>>> 
>>> You change the logic here. The original code returns 0 if getPemDataFromCtx fails. Is it intentinal?
>> 
>> The logic itself is not changed. If the certificates are empty, it returns zero at the end by failing canIgnoredHTTPSCertificate(). We cannot return here to collect other information.
> 
> Umm, I don't understand. Because this code is a security sensitive part, I can't forget.
> 
> There are three cases that the certificates will become empty.
> 
> 1) PEM_write_bio_X509 returns 0.
> 2) BIO_get_mem_data returns negative.
> 3) sk_X509_num returns 0.
> 
> In the original code:
> 1) CurlSSLVerifier::certVerifyCallback returns 0 immediately
> 2) CurlSSLVerifier::certVerifyCallback returns 0 immediately
> 3) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
> 
> In your change:
> 1) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
> 2) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
> 3) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
> 
> Does canIgnoredHTTPSCertificate always return 0 in that case?

Yep. If certificates are empty, they return 0. This flow is error path so that adding extra empty check before function invocation is a kind of cost for normal case. I wanna respect normal case flow.

[Fujii Hironori]

Comment on attachment 343741 [details]
Fix style

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

>>>>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:73
>>>>> +    if (auto certificates = getPemDataFromCtx(ctx))
>>>> 
>>>> You change the logic here. The original code returns 0 if getPemDataFromCtx fails. Is it intentinal?
>>> 
>>> The logic itself is not changed. If the certificates are empty, it returns zero at the end by failing canIgnoredHTTPSCertificate(). We cannot return here to collect other information.
>> 
>> Umm, I don't understand. Because this code is a security sensitive part, I can't forget.
>> 
>> There are three cases that the certificates will become empty.
>> 
>> 1) PEM_write_bio_X509 returns 0.
>> 2) BIO_get_mem_data returns negative.
>> 3) sk_X509_num returns 0.
>> 
>> In the original code:
>> 1) CurlSSLVerifier::certVerifyCallback returns 0 immediately
>> 2) CurlSSLVerifier::certVerifyCallback returns 0 immediately
>> 3) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
>> 
>> In your change:
>> 1) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
>> 2) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
>> 3) CurlSSLVerifier::certVerifyCallback returns the return value of canIgnoredHTTPSCertificate
>> 
>> Does canIgnoredHTTPSCertificate always return 0 in that case?
> 
> Yep. If certificates are empty, they return 0. This flow is error path so that adding extra empty check before function invocation is a kind of cost for normal case. I wanna respect normal case flow.

I got it. Thanks.

[Basuke Suzuki]

Created attachment 344123 [details]
Fix

Fixed reviewed points:
- uses rvalue reference for setter.
- Represent certificate from String -> Vector<uint8_t> opaque data.
- Stop using ListHashSet and switched to use simple Vector instead.

[EWS Watchlist]

Attachment 344123 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/curl/CertificateInfo.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Created attachment 344125 [details]
Style fix

[Fujii Hironori]

Comment on attachment 344125 [details]
Style fix

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

> Source/WebCore/platform/network/curl/CertificateInfo.cpp:46
> +CertificateInfo::Certificate CertificateInfo::makeCertificate(const void* buffer, size_t size)

Why don't you use 'const uint8_t*' for 'buffer'?

> Source/WebCore/platform/network/curl/CertificateInfo.cpp:50
> +    return WTFMove(certificate);

IIUC, this WTFMove is unnecessary because this is a function return.

https://en.cppreference.com/w/cpp/language/move_constructor

> function return: return a; inside a function such as T f(), where a is of type T which has a move constructor.

> Source/WebCore/platform/network/curl/CertificateInfo.h:30
> +#include <wtf/text/WTFString.h>

Do you still use WTFString.h?

> Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:113
> +    m_response.setCertificateInfo(WTFMove(info));

This code look redundant. What about this (if you copy the CertificateInfo)?
> m_response.setCertificateInfo(m_response.setCertificateInfo());

> Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:116
> +    m_response.setDeprecatedNetworkLoadMetrics(WTFMove(metrics));

Ditto.

> Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:170
> +    auto metrics = request.getNetworkLoadMetrics();

Ditto.

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:100
>      m_allowedHosts.set(hostName, certificates);

What about this?
> m_response.setCertificateInfo(hostName, { });

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:-121
> -        value = certificates;

Why did you remove this assignment?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:139
> +        return WTFMove(cert);

You don't need WTFMove.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:163
> +    return WTFMove(result);

Ditto.

[youenn fablet]

Comment on attachment 344125 [details]
Style fix

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

> Source/WebCore/platform/network/curl/CertificateInfo.cpp:50
> +    return WTFMove(certificate);

Probably no need for this WTFMove(), the compiler will do the optimization directly.
It would be nice to write it as a one-liner, but we may probably need some changes in Vector.h

> Source/WebCore/platform/network/curl/CertificateInfo.h:30
> +#include <wtf/text/WTFString.h>

Why do we need WTFString.h?

> Source/WebCore/platform/network/curl/CertificateInfo.h:39
> +    WEBCORE_EXPORT CertificateInfo(int verificationError, Vector<Certificate>&& certificateChain);

Not sure we need certificateChain here.

> Source/WebCore/platform/network/curl/CurlRequest.cpp:357
> +        m_certificateInfo = CertificateInfo(m_sslVerifier->verificationError(), WTFMove(certificateChain));

Instead of exposing verifier chain and error, maybe it should expose something like:
CertificateInfo certificateInfo() const.

That would be useful here and below.

> Source/WebCore/platform/network/curl/CurlRequest.h:92
> +    NetworkLoadMetrics getNetworkLoadMetrics() const { return m_networkLoadMetrics.isolatedCopy(); }

We use getXX() when we pass a T& as out parameter to getXX.
We should probably rename them.

Also, it might be clearer to call isolatedCopy() at the call site of the getters.
Can we do that change in this patch, at least for the newly added getCertificateInfo()?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:41
> +

Two lines.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:99
> +struct StackOfX509 {

It seems strange to use a struct here and below since the constructors and getters are public but not the members.
Maybe they should be changed to 'class'?

[Fujii Hironori]

Comment on attachment 344125 [details]
Style fix

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

>> Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:113
>> +    m_response.setCertificateInfo(WTFMove(info));
> 
> This code look redundant. What about this (if you copy the CertificateInfo)?

Ah, I get the idea.
setCertificateInfo takes only rvalue reference.
setCertificateInfo(CertificateInfo&&)
You can overload it with:
setCertificateInfo(const CertificateInfo&)
But, if it is always copied, there is no need to overload rvalue version.

[Basuke Suzuki]

Comment on attachment 344125 [details]
Style fix

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

>> Source/WebCore/platform/network/curl/CertificateInfo.cpp:46
>> +CertificateInfo::Certificate CertificateInfo::makeCertificate(const void* buffer, size_t size)
> 
> Why don't you use 'const uint8_t*' for 'buffer'?

My bad. You're right. I placed the static_cast to wrong place.

>>> Source/WebCore/platform/network/curl/CertificateInfo.cpp:50
>>> +    return WTFMove(certificate);
>> 
>> IIUC, this WTFMove is unnecessary because this is a function return.
>> 
>> https://en.cppreference.com/w/cpp/language/move_constructor
> 
> Probably no need for this WTFMove(), the compiler will do the optimization directly.
> It would be nice to write it as a one-liner, but we may probably need some changes in Vector.h

Got it. Thanks for clarification. Rvalue reference related things confuse me even lately.

>> Source/WebCore/platform/network/curl/CertificateInfo.h:39
>> +    WEBCORE_EXPORT CertificateInfo(int verificationError, Vector<Certificate>&& certificateChain);
> 
> Not sure we need certificateChain here.

Vector<Certificate> tells enough. Will be removed.

>> Source/WebCore/platform/network/curl/CurlRequest.cpp:357
>> +        m_certificateInfo = CertificateInfo(m_sslVerifier->verificationError(), WTFMove(certificateChain));
> 
> Instead of exposing verifier chain and error, maybe it should expose something like:
> CertificateInfo certificateInfo() const.
> 
> That would be useful here and below.

Exactly.

>> Source/WebCore/platform/network/curl/CurlRequest.h:92
>> +    NetworkLoadMetrics getNetworkLoadMetrics() const { return m_networkLoadMetrics.isolatedCopy(); }
> 
> We use getXX() when we pass a T& as out parameter to getXX.
> We should probably rename them.
> 
> Also, it might be clearer to call isolatedCopy() at the call site of the getters.
> Can we do that change in this patch, at least for the newly added getCertificateInfo()?

Right. I can change both for consistency.

>>> Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:113
>>> +    m_response.setCertificateInfo(WTFMove(info));
>> 
>> This code look redundant. What about this (if you copy the CertificateInfo)?
> 
> Ah, I get the idea.
> setCertificateInfo takes only rvalue reference.
> setCertificateInfo(CertificateInfo&&)
> You can overload it with:
> setCertificateInfo(const CertificateInfo&)
> But, if it is always copied, there is no need to overload rvalue version.

Right. Thanks. Also with Above youenn's advise, isolatedCopy() was moved from implementation to caller. So now code is more clear.

>> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:100
>>      m_allowedHosts.set(hostName, certificates);
> 
> What about this?

This logic is to check the list of certificates passed by the server is identical to previously registered certificate list for the host name. Using ListHashSet is kind of wrong because when the same certificate is stored in the chain more than once, ListHashSet ignores that errors. Say serve sent a chain with certificates of (A, B, B, C). When the exception of the certificate is registered using this chain, it was stored as (A, B, C). Then this configuration will accept any of (A, B, C), (A, B, B, C) or (A, B, C, B, B, A, ...). Those are mis-configured certificate chains and should not treat as same.

ListHasSet is good to be used in client application to prepare certificate chain for users, but engine shouldn't do such care for the chain.

>> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:-121
>> -        value = certificates;
> 
> Why did you remove this assignment?

Because this does nothing. It was assigned and thrown by returning true.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:99
>> +struct StackOfX509 {
> 
> It seems strange to use a struct here and below since the constructors and getters are public but not the members.
> Maybe they should be changed to 'class'?

I don't feel weird by seeing struct here, but no problem to change it to class.

[Fujii Hironori]

Comment on attachment 344125 [details]
Style fix

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

>>> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:-121
>>> -        value = certificates;
>> 
>> Why did you remove this assignment?
> 
> Because this does nothing. It was assigned and thrown by returning true.

I don't think so.
This assignment is the only code to insert a new cert to m_allowedHosts.
setHostAllowsAnyHTTPSCertificate has a following code:

> m_allowedHosts.set(hostName, certificates);

But, this inserts an empty ListHashSet<String>.

[Basuke Suzuki]

Comment on attachment 344125 [details]
Style fix

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

>>>> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:-121
>>>> -        value = certificates;
>>> 
>>> Why did you remove this assignment?
>> 
>> Because this does nothing. It was assigned and thrown by returning true.
> 
> I don't think so.
> This assignment is the only code to insert a new cert to m_allowedHosts.
> setHostAllowsAnyHTTPSCertificate has a following code:

Oh, I see. What a terrible behavior. I cannot guess that behavior from this name. Also I can't understand why it behaves like that.

[Basuke Suzuki]

Created attachment 344164 [details]
Fix

[EWS Watchlist]

Attachment 344164 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/curl/CurlSSLVerifier.h:67:  The parameter name "ctx" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Created attachment 344165 [details]
Fix

[Basuke Suzuki]

Created attachment 344166 [details]
Fix

[Fujii Hironori]

Comment on attachment 344166 [details]
Fix

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

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:100
>      m_allowedHosts.set(hostName, certificates);

Nit: I think you don't need to declare a variable just to insert an empty vector.
m_allowedHosts.set(hostName, { });

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:160
>              break;

You should return an empty vector if PEM_write_bio_X509 fails instead of returning a partial vector.
return { };

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:164
>              break;

Ditto.

[EWS Watchlist]

Comment on attachment 344166 [details]
Fix

Attachment 344166 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8421897

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html

[EWS Watchlist]

Created attachment 344171 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Basuke Suzuki]

Comment on attachment 344166 [details]
Fix

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

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:160
>>              break;
> 
> You should return an empty vector if PEM_write_bio_X509 fails instead of returning a partial vector.
> return { };

Right. I forgot the original intention of introducing RAII is to make this part returning immediately, but by break.

[Basuke Suzuki]

Created attachment 344191 [details]
Fix

[EWS Watchlist]

Comment on attachment 344191 [details]
Fix

Attachment 344191 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8426705

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 344205 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[youenn fablet]

Comment on attachment 344191 [details]
Fix

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

> Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:113
> +    m_response.setDeprecatedNetworkLoadMetrics(request.networkLoadMetrics().isolatedCopy());

This is out of scope of this patch but it is unclear why isolatedCopy() is called here.
m_response is copied from receivedResponse above so is not isolated from receivedResponse.
But we isolate some of m_response fields from request.
Is there a specific rationale here.

> Source/WebCore/platform/network/curl/ResourceResponse.h:3
> + * Copyright (C) 2018 Sony Interactive Entertainment Inc.

You can also do 2017-2018 if that is more accurate.

[Basuke Suzuki]

Created attachment 344213 [details]
Fix

Thanks for r+, Youenn.

isolatedCopy() will be treated in upcoming refactoring patch. Thanks.

[WebKit Commit Bot]

Comment on attachment 344213 [details]
Fix

Clearing flags on attachment: 344213

Committed r233481: <https://trac.webkit.org/changeset/233481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41792846>