Re: [PATCH] Dynamic SSL Certificate Generation

On 11/16/2010 01:47 AM, Amos Jeffries wrote:
> On 16/11/10 20:27, Tsantilas Christos wrote:
>> This version of the patch addresses the Amos comments and suggestions.
>> Please see below
>>
>>
>> On 11/11/2010 03:11 AM, Amos Jeffries wrote:
>>> On Wed, 10 Nov 2010 17:05:16 +0200, Tsantilas Christos
>>> <chtsanti_at_users.sourceforge.net> wrote:
>>>> Hi all,
>>>>
>>>> This patch implements dynamic SSL certificate generartion in
>>>> Squid.When used with SSL Bump, the feature allows Squid to dynamically
>>>> generate (using a configurable CA certificate) and cache SSL
>>>> certificates for the proxied hosts.
>>>>
>>>> A description for this feature can be found at:
>>>> http://wiki.squid-cache.org/Features/DynamicSslCert
>>>>
>>>> A first version of the patch posted by Alex, some months before:
>>>> http://www.squid-cache.org/mail-archive/squid-dev/201003/0201.html
>>>>
>>>> Some words about the patch:
>>>>
>>>> * ssl related source files moved under the src/ssl directory
>>>>
>>>> * Introduce the TidyPointer class similar to std::auto_ptr, which
>>>> implements a pointer that deletes the object it points to when the
>>>> pointer's owner or context is gone. It is designed to avoid memory
>>>> leaks in the presence of exceptions and processing short cuts.
>>>
>>> So whats different about this new pointer type that the existing
>>> std:auto_ptr, RefCount::Pointer and CbcPointer are all insufficient?
>>>
>>>> From what I've heard of the SSL problems they are all cause by it not
>>> using RefCount on the contexts themselves.
>>>
>>>>
>>>> * Implements ssl context cache to use with generated ssl contexts.
>>>> The Ssl::LocalContextStorage class stores the hostname/ssl context
>>>> pairs
>>>
>>>> for a local listening address/port. The Ssl::GlobalContextStorage class
>>>
>>>> used to store Ssl::LocalContextStorages per local listening address and
>>>> handles squid shutdown/configure/reconfigure
>>>>
>>>> * Ssl::Helper class implements the squid part of the ssl_crtd
>>> helpers.
>>>>
>>>> * The ssl_crtd helper implemented in ssl_crtd.cc and
>>>> certificate_db.* files
>>>>
>>>> * The Ssl::CertificateDb class (certificate_db.* files) implements
>>>> a database of certificates on disk files. It is used by ssl_crtd
>>>> helper to manipulate generated certificates.
>>>>
>>>> * The ssl related files included in the libraries libsslutil.a which
>>>
>>>> contains common classes and functions and the libsquidssl.a which has
>>>> squid related ssl objects and functions
>>>>
>>>> * Use the Ssl namespace for new ssl code
>>>>
>>>>
>>>> Authors: Alex Rousskov, Andrew Balabohin, Christos Tsantilas
>>>> This is a Measurement Factory Project.
>>>
>>> Thanks Christos,
> <snip>
>>
>>> src/Makefile.am:
>>> * why is WIN32_ALL_SOURCE being altered by this patch?
>> It is not altered.
>>
>
> In EXTRA_squid_SOURCES you had/have:
>
> - $(WIN32_ALL_SOURCE) \
>
> Seems like it may be useful at some point but not related to this patch.
>
>
>>> * Instead of adding a new /var/ssl_db which the distros are
>>> guaranteed to patch elsewhere...
>>>
>>> Possibly $(DEFAULT_SWAP_DIR)/ssl_crtd/ (aka
>>> $(localstatedir)/cache/squid/ssl_crtd/)
>>> ** data may be erased at any time
>>> or $(localstatedir)/lib/ssl_crtd/
>>> ** data may be erased but must persist while process running
>>> please see http://www.pathname.com/fhs/pub/fhs-2.3.pdf for the options
>>> and criteria.
>>
>> I put it to $(localstatedir)/lib/.
>> I am not sure it is correct. Maybe the best is under the
>> $(localstatedir)/cache/ but here exist the main squid cache.
>>
>
> Okay.
>
>>> ** possibly also missing<climits> include for other g++ versions and
>>> alternative compilers.
>>> same in other .cc files.
>>
>> Is it needed? We are using "limits.h"
>>
>
> I think not when <limits.h> is used, but when the C++ <limits> is used
> like ProtoPort.cc there have been problems.
>
>
>
> Okay. Looks good. +1

Thank you both for working on the patch! I am glad we can finally commit
this frequently requested feature, despite my reservations about
ssl_crtd code quality. AFAIK, the underlying code has been successfully
deployed, but let's see what the wider testing brings.

Alex.
Received on Wed Nov 17 2010 - 22:03:13 MST