Including the certificate information for SSL communication into ResourceResponse for better error handling and information providing to the user.
Created attachment 343736 [details] PATCH
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.
Created attachment 343741 [details] Fix style
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&&
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?
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.
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.
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.
(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
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.
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.
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?
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.
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.
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.
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.
Created attachment 344125 [details] Style fix
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.
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'?
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.
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.
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>.
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.
Created attachment 344164 [details] Fix
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.
Created attachment 344165 [details] Fix
Created attachment 344166 [details] Fix
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.
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
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
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.
Created attachment 344191 [details] Fix
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
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
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.
Created attachment 344213 [details] Fix Thanks for r+, Youenn. isolatedCopy() will be treated in upcoming refactoring patch. Thanks.
Comment on attachment 344213 [details] Fix Clearing flags on attachment: 344213 Committed r233481: <https://trac.webkit.org/changeset/233481>
All reviewed patches have been landed. Closing bug.
<rdar://problem/41792846>