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 _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev