[squid-dev] [PATCH] Fix SSL certificate cache refresh and collision handling.

[Alex Rousskov]

On 07/14/2017 09:18 AM, Christos Tsantilas wrote:
> SslBump was ignoring origin server certificate changes and using the
> previously cached fake certificate (mimicking now-stale properties).

I suggest replacing the above claim with a more accurate one (based on
our off-list discussions):

"""
SslBump was ignoring some origin server certificate changes or
differences, incorrectly using the previously cached fake certificate
(mimicking now-stale properties or properties of a slightly different
certificate).
"""

> +/// \ingroup SslCrtdSslAPI
> +/// \returns certificate database key"
> +std::string &TxtKeyForCertificateProperties(const CertificateProperties &);

An extra trailing '"'.


> +/**
> +  \ingroup ServerProtocolSSLAPI
> +  * Generates a unique key based on CertificateProperties object and store it to key
> + */
> +void UniqueKeyForCertificateProperties(const Ssl::CertificateProperties &certProperties, SBuf &key);

>   - Replace Ssl::CertificateProperties::dbKey() with:
>      * Ssl::TxtKeyForCertificateProperties() in ssl/gadgets.cc for
>        on-disk key generation by the ssl_crtd helper;
>      * Ssl::UniqueKeyForCertificateProperties() in ssl/support.cc for
>        in-memory binary keys generation by the SSL context memory cache.


I think we need to do a better job distinguishing these two functions. I
understand that they make keys (with different sets of features) for two
different databases and that they have to be in different files due to
ssl_crtd linking requirements. I suggest:

* Ssl::OnDiskCertificateDbKey() and Ssl::InRamCertificateDbKey()

or even

* Ssl::DiskDbKey() and Ssl::RamDbKey()


This suggestion also removes the current "unique key" tautology.

I realize that the suggested names do not describe the features of each
key generation method, but neither does the old dbKey() or the new names
in the patch. Those features are too complex to be described in a few
words and, more importantly, are largely irrelevant to the caller.

The above polishing touches can be done during commit.


I cannot track all the low-level changes in the patch, but I agree that
it solves a serious problem, and support the described solution (but I
am biased because I helped design it).


Thank you,

Alex.