[squid-dev] [PATCH] splicing resumed sessions

[Amos Jeffries]

On 11/04/2015 1:49 a.m., Tsantilas Christos wrote:
> I am attaching patch for trunk and squid-3.5
> 

Thank you. Looks pretty good now.

> On 04/09/2015 04:13 PM, Amos Jeffries wrote:
>>
>> * Ssl::Bio::sslFeatures::parseV3Hello()
>>   - similar issues with s/Client Hello/ClientHello/ and "SSL Extension"
>> as above.
> 
> I did it only for the comments added or modified by this patch to avoid
> increasing the size of this patch.
> If required we can do it as separate patch.

Theres still one s/SSL Extension/TLS Extension/ near the end of this method.

>> * Ssl::Bio::sslFeatures::print()
>>   - seems to be lacking display of ALPN received
>>   - missing the format details for sslVersion used elsewhere:
>>       std::hex << std::setw(8) << std::setfill('0')
> 
> 
> I did not fix this. The ALPN is in an encoded form and requires some
> development to correctly print it. We do not have to gain something
> implementing this.
> Also the Ssl::Bio::sslFeatures::print currently is not used. It is here
> for using it for debugging if required.
> 

That means the print() will be incomplete, and needs a TODO added about
the above.

> 
>>
>> * parseMsgHead() documentation about return result >0 is wrong.
>>    - it does not return <0 when the contents of the buffer are a TLS
>> (non-SSL) message.
> 
> This is what it does!
> 

The .h comment says it will return a negative number if the Hello
message "is not SSL". TLS != SSL.

For the case of head[0] == 0x16 the only SSL indicator (SSLv3) is
*exactly* {0x16, 0x03, 0x00}

For TLS versions that final 0x00 byte changes. The method correctly
accepts those and returns the Hello size (N>0) - in contradiction to
what is documented.
The comment should either say "SSLv3 or TLS", or not mention the
protocol name at all.


+1. I dont think this needs another review, once those comments are
added/updated it can merge.


Amos