[webkit-reviews] review canceled: [Bug 187102] [Curl] Embed certificate information into ResourceResponse. : [Attachment 344125] Style fix

[bugzilla-daemon at webkit.org]

Basuke Suzuki <Basuke.Suzuki at sony.com> has canceled Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 187102: [Curl] Embed certificate information into ResourceResponse.
https://bugs.webkit.org/show_bug.cgi?id=187102

Attachment 344125: Style fix

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




--- Comment #21 from Basuke Suzuki <Basuke.Suzuki at sony.com> ---
Comment on attachment 344125
  --> https://bugs.webkit.org/attachment.cgi?id=344125
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.